Index: /main/trunk/greenstone2/perllib/gsmysql.pm
===================================================================
--- /main/trunk/greenstone2/perllib/gsmysql.pm	(revision 32594)
+++ /main/trunk/greenstone2/perllib/gsmysql.pm	(revision 32595)
@@ -41,35 +41,4 @@
 #################
 
-##############################
-
-# TODO Q: If disconnect is automatically called when object destroyed, what does that mean
-# for our file-global handle object, is disconnect only called at end of perl process?
-# Does that mean we don't need to explicitly call disconnect in gsmysql object's destroy during
-# the GLOBAL destruction phase?
-# https://perldoc.perl.org/perlobj.html#Destructors
-
-#+ TODO: add infrastructure for db_port, AutoCommit etc
-# For port, see https://stackoverflow.com/questions/2248665/perl-script-to-connect-to-mysql-server-port-3307
-
-# + TODO: remove unnecessary warn() since PrintError is active
-
-# + TODO: drop table if exists and create table if exists are available in MySQL. Use those cmds
-# instead of always first checking for existence ourselves? Only when subclassing to specific
-# mysql class?
-
-
-# + TODO Q: What on cancelling a build: delete table? But what if it was a rebuild and the rebuild is cancelled (not the original build)?
-# Do we create a copy of the orig database as backup, then start populating current db, and if cancelled, delete current db and RENAME backup table to current? 
-# https://stackoverflow.com/questions/3280006/duplicating-a-mysql-table-indexes-and-data
-# BUT what if the table is HUGE? (Think of a collection with millions of docs.) Huge overhead in copying?
-# The alternative is we just quit on cancel, but then: cancel could leave the table in a partial committed state, with no way of rolling back.
-# Unless they do a full rebuild, which will recreate the table from scratch?
-# SOLUTION-> rollback transaction on error, see https://www.effectiveperlprogramming.com/2010/07/set-custom-dbi-error-handlers/
-# In that case, should set AutoCommit to off on connection, and remember to commit at end.
-
-# + TODO: Consider AutoCommit status (and Autocommit off allowing commit or rollback for GS coll build cancel) later
-
-
-##############################
 
 # singleton connection
@@ -77,20 +46,4 @@
 my $ref_count = 0;
 
-# Need params_map keys:
-# - collection_name
-# - db_encoding (db content encoding) - MySQL can set this at server, db, table levels. For MySQL
-# we set the enc during connect at server level. Not sure whether other DB's support it at the
-# same levels.
-
-# For connection to MySQL, need:
-#  - db_driver, db_client_user, db_client_pwd, db_host, (db_port not used at present)
-# So these will be parameterised, but in a hashmap, for just the connect method.
-
-# Parameterise (one or more methods may use them):
-# - db_name (which is the GS3 sitename, or "greenstone2" for GS2)
-
-# Database access related functions
-# http://g2pc1.bu.edu/~qzpeng/manual/MySQL%20Commands.htm
-# https://www.guru99.com/insert-into.html
 
 # Add signal handlers to cleanup and disconnect from db on sudden termination, incl cancel build
@@ -116,4 +69,8 @@
 	    &gsprintf::gsprintf(STDERR, "{gsmysql.restore_backups_on_build_cancel_msg}\n");
 	    print STDERR "****************************\n";
+
+	    # TODO?
+	    # Since we'll be disconnecting (cancel -> die() -> dbi::DESTROY() -> dbi::disconnect()),
+	    # ensure $sth->finish() called on statement handles if any fetch remnants remain
 	}
     }
@@ -122,4 +79,24 @@
 }
 
+
+# Need to pass in to constructor for creating member variables:
+# - collection_name
+# - verbosity
+
+# For connection to MySQL, need:
+#  - db_driver, db_client_user, db_client_pwd, db_host, (db_port not used at present)
+# So these will be parameterised, but in a hashmap, for just the connect() method.
+
+# Parameterise (one or more methods may use them):
+# - db_name (which is the GS3 sitename, or "greenstone2" for GS2)
+
+# Don't need to parameterise the db_encoding (db content encoding)
+# This is for now an internal variable, as the Greenstone db contents are always going to be utf8
+# reflecting how their doc.xml counterparts should only contain utf8.
+# 
+# - MySQL can set the desired db_encoding at server, db, table levels.
+# Not sure whether other DBs support it at the same levels.
+# For MySQL we set the enc during connect at server level.
+#
 sub new
 {  
@@ -128,6 +105,4 @@
     my ($params_map) = @_;
     
-    # library_url: to be specified on the cmdline if not using a GS-included web server
-    # the GSDL_LIBRARY_URL env var is useful when running cmdline buildcol.pl in the linux package manager versions of GS3
     
     # https://stackoverflow.com/questions/7083453/copying-a-hashref-in-perl 
@@ -162,6 +137,9 @@
 # For more on when destroy is called, see https://www.perlmonks.org/?node_id=1020920
 #
+# However, database is automatically disconnected on DBI DESTROY method called by perl on
+# a perl process' termination:
+#
 # https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#disconnect
-# "Disconnects the database from the database handle. disconnect is typically only used before exiting the program. The handle is of little use after disconnecting.
+# 'Disconnects the database from the database handle. disconnect is typically only used before exiting the program. The handle is of little use after disconnecting.
 #
 # The transaction behaviour of the disconnect method is, sadly, undefined. Some database systems (such as Oracle and Ingres) will automatically commit any outstanding changes, but others (such as Informix) will rollback any outstanding changes. Applications not using AutoCommit should explicitly call commit or rollback before calling disconnect.
@@ -171,5 +149,5 @@
 # Generally, if you want your changes to be committed or rolled back when you disconnect, then you should explicitly call "commit" or "rollback" before disconnecting.
 #
-# If you disconnect from a database while you still have active statement handles (e.g., SELECT statement handles that may have more data to fetch), you will get a warning. The warning may indicate that a fetch loop terminated early, perhaps due to an uncaught error. To avoid the warning call the finish method on the active handles."
+# If you disconnect from a database while you still have active statement handles (e.g., SELECT statement handles that may have more data to fetch), you will get a warning. The warning may indicate that a fetch loop terminated early, perhaps due to an uncaught error. To avoid the warning call the finish method on the active handles.'
 #
 #
@@ -193,4 +171,6 @@
 	    # will ensure disconnection still happens. It happens, but silently.
 	    print STDERR "   Global Destruct Phase: DBI's own destructor will disconnect database\n";
+
+	    # When we did the disconnection manually on premature termination:
 	    #$_dbh_instance->disconnect or warn $_dbh_instance->errstr;
 	    #$_dbh_instance = undef;
@@ -211,5 +191,4 @@
 ################### BASIC DB OPERATIONS ##################
 
-# THE NEW DB FUNCTIONS
 # NOTE: FULLTEXT is a reserved keyword in (My)SQL. So we can't name a table or any of its columns "fulltext".
 # https://dev.mysql.com/doc/refman/5.5/en/keywords.html
@@ -239,5 +218,4 @@
 
 # SINGLETON METHOD #
-# TODO: where should the defaults for these params be, here or in GS-SQLPlugin/Plugout?
 sub _get_connection_instance
 {
@@ -247,5 +225,5 @@
     
     return $_dbh_instance if($_dbh_instance);
-    # or make the connection
+    # or else make the connection, as happens below
 
 
@@ -300,4 +278,6 @@
     # More: https://www.oreilly.com/library/view/programming-the-perl/1565926994/re44.html
     my $autocommit = (defined $params_map->{'autocommit'}) ? $params_map->{'autocommit'} : 1;
+
+    # Useful: https://www.effectiveperlprogramming.com/2010/07/set-custom-dbi-error-handlers/
     
     my $dbh = DBI->connect("$connect_str", $db_user, $db_pwd,
@@ -307,5 +287,7 @@
 			       RaiseError => 0, # off by default, but being explicit
 			       AutoCommit => $autocommit,
-			       mysql_enable_utf8mb4 => 1 # tells MySQL to use UTF-8 for communication and tells DBD::mysql to decode the data, see https://stackoverflow.com/questions/46727362/perl-mysql-utf8mb4-issue-possible-bug 
+			       mysql_enable_utf8mb4 => 1 # tells MySQL to use (4 byte) UTF-8 for
+				   # communication and tells DBD::mysql to use it to decode the data, 
+				   # see https://stackoverflow.com/questions/46727362/perl-mysql-utf8mb4-issue-possible-bug 
 			   });
 
@@ -328,5 +310,5 @@
     # To set up the db for utf8mb4, therefore, 
     # the MySQL server needs to be configured for that char encoding by running the server as:
-    # mysql-5.7.23-linux-glibc2.12-x86_64/bin>./mysqld_safe --datadir=/Scratch/ak19/mysql/data --character_set_server=utf8mb4
+    # mysql/mysql-5.7.23-linux-glibc2.12-x86_64/bin>./mysqld_safe --datadir=/PATHTO/mysql/data --character_set_server=utf8mb4
     # AND when connecting to the server, we can can either set mysql_enable_utf8mb4 => 1
     # as a connection option
@@ -337,8 +319,6 @@
     # is no more objectionable. It has the advantage of cutting out the 2 extra lines of doing
     # set NAMES '<enc>' and $dbh->{mysql_enable_utf8mb4} = 1 here.
-    # These lines may be preferred if more db_driver options are to be supported in future:
-    # then a separate method called set_db_encoding($enc) can work out what db_driver we're using
-    # and if mysql and enc=utfy, then it can do the following whereas it will issue other do stmts
-    # for other db_drivers, see https://www.perlmonks.org/?node_id=259456:
+    # These lines may be preferred if more db_driver options are to be supported in future?
+    # (see https://www.perlmonks.org/?node_id=259456)
     
     #my $stmt = "set NAMES '" . $db_enc . "'";
@@ -364,5 +344,5 @@
     if($ref_count == 0) { # Only commit transaction when we're about to actually disconnect, not before
 	
-	# + TODO: If AutoCommit was off, meaning transactions were on/enabled,
+	# If AutoCommit was off, meaning transactions were on/enabled,
 	# then here is where we commit our one long transaction.
 	# https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#commit
@@ -383,6 +363,5 @@
 # Call this method on die(), so that you're sure the perl process has disconnected from SQL db
 # Disconnect from db - https://metacpan.org/pod/DBI#disconnect
-# TODO: make sure to have committed or rolled back before disconnect
-# and that you've call finish() on statement handles if any fetch remnants remain
+# Make sure to have committed or rolled back before disconnect
 sub _force_disconnect_from_db {
     my $self= shift (@_);
@@ -403,5 +382,5 @@
 
 
-# Load the designated database, i.e. 'use <dbname>;'.
+# Loads the designated database, i.e. 'use <dbname>;'.
 # If the database doesn't yet exist, creates it and loads it.
 # (Don't create the collection's tables yet, though)
@@ -590,6 +569,6 @@
 # 'The q{...} style quoting used in this example avoids clashing with quotes that may be used in the SQL statement. Use the double-quote like qq{...} operator if you want to interpolate variables into the string. See "Quote and Quote-like Operators" in perlop for more details.'
 #
-# This method uses lazy loading to prepare the SQL insert stmt once for a table and store it,
-# then execute the (stored) statement each time it's needed for that table.
+# Each insert method uses lazy loading to prepare the SQL insert stmts once for a table and
+# store it, then executes the (stored) statement each time it's needed for that table.
 sub insert_row_into_metadata_table {
     my $self = shift (@_);
@@ -599,5 +578,5 @@
     
     my $tablename = $self->get_metadata_table_name();
-    my $sth = $dbh->prepare_cached(qq{INSERT INTO $tablename (did, sid, metaname, metavalue) VALUES (?, ?, ?, ?)});# || warn("Could not prepare insert statement for metadata table\n");
+    my $sth = $dbh->prepare_cached(qq{INSERT INTO $tablename (did, sid, metaname, metavalue) VALUES (?, ?, ?, ?)});
 
     # Now we're ready to execute the command, unless we're only debugging
@@ -626,5 +605,5 @@
     
     my $tablename = $self->get_fulltext_table_name();
-    my $sth = $dbh->prepare_cached(qq{INSERT INTO $tablename (did, sid, fulltxt) VALUES (?, ?, ?)});# || warn("Could not prepare insert statement for fulltxt table\n");
+    my $sth = $dbh->prepare_cached(qq{INSERT INTO $tablename (did, sid, fulltxt) VALUES (?, ?, ?)});
     
     # Now we're ready to execute the command, unless we're only debugging
@@ -651,7 +630,6 @@
 #   https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#selectall_arrayref
 #
-# Returns the statement handle that prepared and executed
+# Returns the resulting records of preparing and executing
 # a "SELECT * FROM <COLL>_metadata WHERE did = $oid" SQL statement.
-# Caller can call fetchrow_array() on returned statement handle, $sth
 # Have to use prepare() and execute() instead of do() since do() does
 # not allow for fetching result set thereafter:
@@ -682,7 +660,6 @@
 
 # See select_from_metatable_matching_docid() above.
-# Returns the statement handle that prepared and executed
-# a "SELECT * FROM <COLL>_metadata WHERE did = $oid" SQL statement.
-# Caller can call fetchrow_array() on returned statement handle, $sth
+# Returns the resulting records from preparing and executing
+# a "SELECT * FROM <COLL>_fulltxt WHERE did = $oid" SQL statement.
 sub select_from_texttable_matching_docid {
     my $self= shift (@_);
@@ -720,5 +697,5 @@
 }
 
-# delete all records in metatable with specified docid
+# delete all records in fulltxt table with the specified docid
 sub delete_recs_from_texttable_with_docid {
     my $self= shift (@_);
@@ -758,5 +735,5 @@
 
 # Attempt to make sure the name parameter (for db or table name) is acceptable syntax
-# for the db in question, e.g. for mysql. For example, (My)SQL doesn't like tables or
+# for the db in question, e.g. for mysql. For example, MySQL doesn't like tables or
 # databases with '-' (hyphens) in their names
 sub sanitize_name {
@@ -768,9 +745,9 @@
 
 
+# MySQL has non-standard command to CREATE TABLE IF NOT EXISTS and DROP TABLE IF EXISTS, using that.
+# See https://www.perlmonks.org/bare/?node=DBI%20Recipes
+#    The page further has a table_exists function that could work with proper comparison
+# Couldn't get the first solution at https://www.perlmonks.org/bare/?node_id=500050 to work though
 # I can get my version of table_exists to work, but it's not so ideal
-# Interesting that MySQL has non-standard command to CREATE TABLE IF NOT EXISTS and DROP TABLE IF EXISTS, 
-# see https://www.perlmonks.org/bare/?node=DBI%20Recipes
-#    The page further has a table_exists function that could work with proper comparison
-# TODO Q: Couldn't get the first solution at https://www.perlmonks.org/bare/?node_id=500050 to work though
 sub table_exists {
     my $self = shift (@_);
Index: /main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm
===================================================================
--- /main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm	(revision 32594)
+++ /main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm	(revision 32595)
@@ -38,76 +38,8 @@
 
 
-# TODO:
-# - Run TODOs here, in Plugout and in gsmysql.pm by Dr Bainbridge.
-# - Have not yet tested writing out just meta or just fulltxt to sql db and reading just that 
-# back in from the sql db while the remainder is to be read back in from the docsql .xml files.
-
-# + TODO: Add public instructions on using this plugin and its plugout: start with installing mysql binary, changing pwd, running the server (and the client against it for checking: basic cmds like create and drop). Then discuss db name, table names (per coll), db cols and col types, and how the plugout and plugin work.
-# Discuss the plugin/plugout parameters.
-
-# TODO, test on windows and mac.
-# Note: if parsing fails (e.g. using wrong plugout like GS XML plugout, which chokes on args intended for SQL plugout) then SQL plugin init would have already been called and done connection, but disconnect would not have been done because SQL plugin disconnect would not have been called upon parse failure.
-
-# DONE:
-# + TODO: For on cancel, add a SIGTERM handler or so to call end()
-# or to explicitly call gs_sql->close_connection if $gs_sql def
-#
-# + TODO: Incremental delete can't work until GSSQLPlugout has implemented build_mode = incremental
-# (instead of tossing away db on every build)
-# + Ask about docsql naming convention adopted to identify OID. Better way?
-# collection names -> table names: it seems hyphens not allowed. Changed to underscores.
-# + Startup parameters (except removeold/build_mode)
-# + how do we detect we're to do removeold during plugout in import.pl phase
-# + incremental building: where do we need to add code to delete rows from our sql table after
-# incrementally importing a coll with fewer docs (for instance)? What about deleted/modified meta?
-# + Ask if I can assume that all SQL dbs (not just MySQL) will preserve the order of inserted nodes
-# (sections) which in this case had made it easy to reconstruct the doc_obj in memory in the correct order.
-# YES: Otherwise for later db types (drivers), can set order by primary key column and then order by did column
-# + NOTTODO: when db is not running GLI is paralyzed -> can we set timeout on DBI connection attempt?
-#   NOT A PROBLEM: Tested to find DBI connection attempt fails immediately when MySQL server not 
-# running. The GLI "paralyzing" incident last time was not because of the gs sql connection code,
-# but because my computer was freezing on-and-off.
-# + "Courier" demo documents in lucene-sql collection: character (degree symbol) not preserved in title. Is this because we encode in utf8 when putting into db and reading back in?
-# Test doc with meta and text like macron in Maori text.
-# + TODO Q: During import, the GS SQL Plugin is called before the GS SQL Plugout with undesirable side
-# effect that if the db doesn't exist, gsmysql::use_db() fails, as it won't create db.
-#   This got fixed when GSSQLPlugin stopped connecting on init().
-#
-#
-#+ TODO: deal with incremental vs removeold. If docs removed from import folder, then import step
-# won't delete it from archives but buildcol step will. Need to implement this with this database plugin or wherever the actual flow is.
-#
-# + TODO Q: is "reindex" = del from db + add to db?
-# - is this okay for reindexing, or will it need to modify existing values (update table)
-# - if it's okay, what does reindex need to accomplish (and how) if the OID changes because hash id produced is different?
-# - delete is accomplished in GS SQL Plugin, during buildcol.pl. When should reindexing take place?
-# during SQL plugout/import.pl or during plugin? If adding is done by GSSQLPlugout, does it need to
-# be reimplemented in GSSQLPlugin to support the adding portion of reindexing.
-#
-# INCREMENTAL REBUILDING IMPLEMENTED CORRECTLY AND WORKS:
-# Overriding plugins' remove_all() method covered removeold.
-# Overriding plugins' remove_one() method is all I needed to do for reindex and deletion
-# (incremental and non-incremental) to work.
-# but doing all this needed an overhaul of gsmysql.pm and its use by the GS SQL plugin and plugout.
-# - needed to correct plugin.pm::remove_some() to process all files
-# - and needed to correct GreenstoneSQLPlugin::close_document() to setOID() after all
-# All incremental import and buildcol worked after that:
-# - deleting files and running incr-import and incr-buildcol (= "incr delete"),
-# - deleting files and running incr-import and buildcol (="non-incr delete")
-# - modifying meta and doing an incr rebuild
-# - modifying fulltext and doing an incr rebuild
-# - renaming a file forces a reindex: doc is removed from db and added back in, due to remove_one()
-# - tested CSV file: adding some records, changing some records
-#    + CSVPlugin test (collection csvsql)
-#    + MetadataCSVPlugin test (modified collection sqltest to have metadata.csv refer to the
-#      filenames of sqltest's documents)
-#    + shared image test (collection shareimg): if 2 html files reference the same image, the docs
-#      are indeed both reindexed if the image is modified (e.g. I replaced the image with another
-#      of the same name) which in the GS SQL plugin/plugout case is that the 2 docs are deleted
-#      and added in again.
 
 ########################################################################################
 
-# GreenstoneSQLPlugin inherits from GreenstoneXMLPlugin so that it if meta or fulltext
+# GreenstoneSQLPlugin inherits from GreenstoneXMLPlugin so that if meta or fulltext
 # is still written out to doc.xml (docsql .xml), that will be processed as usual,
 # whereas GreenstoneSQLPlugin will process all the rest (full text and/or meta, whichever
@@ -145,5 +77,5 @@
         'desc' => "{GreenstoneSQLPlug.rollbacl_on_cancel}" } ];
 
-# TODO: If subclassing gsmysql for other supporting databases and if they have different required
+# NOTE: If subclassing gsmysql for other supporting databases and if they have different required
 # connection parameters, we can check how WordPlugin, upon detecting Word is installed,
 # dynamically loads Word specific configuration options.
@@ -225,30 +157,16 @@
 }
 
-# GS SQL Plugin::init() (and deinit()) is called by import.pl and also by buildcol.pl
-# This means it connects and deconnects during import.pl as well. This is okay
-# as removeold, which should drop the collection tables, happens during the import phase,
-# calling GreenstoneSQLPlugin::and therefore also requires a db connection.
-# + TODO: Eventually can try moving get_gssql_instance into gsmysql.pm? That way both GS SQL Plugin
-# and Plugout would be using one connection during import.pl phase when both plugs exist.
-
 # Call init() not begin() because there can be multiple plugin passes and begin() called for
 # each pass (one for doc level and another for section level indexing), whereas init() should
 # be called before any and all passes.
 # This way, we can connect to the SQL database once per buildcol run.
+# Although now it doesn't matter, since gsmysql.pm uses the get_instance pattern to return a
+# singleton db connection, regardless of the number of gsmysql objects instantiated and
+# the number of connect() calls made on them.
 sub init {
     my ($self) = shift (@_);
-    ##print STDERR "@@@@@@@@@@ INIT CALLED\n";
     
     $self->SUPER::init(@_); # super (GreenstoneXMLPlugin) will not yet be trying to read from doc.xml (docsql .xml) files in init().
 
-    ####################
-#    print "@@@ SITE NAME: ". $self->{'site'} . "\n" if defined $self->{'site'};
-#    print "@@@ COLL NAME: ". $ENV{'GSDLCOLLECTION'} . "\n";
-
-#    print STDERR "@@@@ db_pwd: " . $self->{'db_client_pwd'} . "\n";
-#    print STDERR "@@@@ user: " . $self->{'db_client_user'} . "\n";
-#    print STDERR "@@@@ db_host: " . $self->{'db_host'} . "\n";
-#    print STDERR "@@@@ db_driver: " . $self->{'db_driver'} . "\n";
-    ####################
 
     # create gsmysql object.
@@ -287,5 +205,4 @@
 	# PrintError would already have displayed the warning message on load fail
 	# And on die() perl will call gsmysql destroy which will ensure a disconnect() from db
-	#$gs_sql->force_disconnect_from_db();
 	die("Could not use db $db_name. Can't proceed.\n");
     }
@@ -297,13 +214,13 @@
 
 
-# This method also runs on import.pl if gs_sql has a value. But we just want to run it on buildcol
+# This method also runs on import.pl if gs_sql has a value.
 # Call deinit() not end() because there can be multiple plugin passes:
 # one for doc level and another for section level indexing
 # and deinit() should be called before all passes
 # This way, we can close the SQL database once per buildcol run.
+# Again, this doesn't matter because we gsmysql the ensures the connection
+# is a singleton connection instance, which connects once and disconnects once per perl process.
 sub deinit {
     my ($self) = shift (@_);
-    
-    ##print STDERR "@@@@@@@@@@ GreenstoneSQLPlugin::DEINIT CALLED\n";
     
     if($self->{'gs_sql'}) {
@@ -360,7 +277,8 @@
 # but if a plugin does any stuff outside of creating doc obj, then it may need
 # to clear something.
-# remove the doc(s) denoted by oids from GS SQL db
+# In the case of GreenstoneSQL plugs: Remove the doc(s) denoted by oids from GS SQL db.
 # This takes care of incremental deletes (docs marked D by ArchivesInfPlugin when building
-# incrementally) as well as cases of "Non-icremental Delete", see ArchivesInfPlugin.pm
+# incrementally) as well as cases of "Non-icremental Delete", see ArchivesInfPlugin.pm.
+# As well as cases involving reindexing, which are implemented here as delete followed by add.
 sub remove_one {
     my $self = shift (@_);
@@ -379,7 +297,4 @@
     
     my $gs_sql = $self->{'gs_sql'} || return 0; # couldn't make the connection or no db etc
-
-    print STDERR "*****************************\nAsked to remove_one oid\n***********************\n";
-    print STDERR "Num oids: " . scalar (@$oids) . "\n";
     
     my $proc_mode = $self->{'process_mode'};
@@ -431,5 +346,5 @@
 # Since all passes need the doc_obj, all are read in from docsql + SQL db into the docobj in memory
 
-# We should only ever get here during the buildcol.pl phase
+# We only ever get here or do any parsing of the docsql.xml file during the buildcol.pl phase.
 # At the end of superclass GreenstoneXMLPlugin.pm's close_document() method,
 # the doc_obj in memory is processed (indexed) and then made undef.
@@ -438,6 +353,4 @@
     my $self = shift(@_);
 
-    ##print STDERR "XXXXXXXXX in SQLPlugin::close_doc()\n";
-    
     my $gs_sql = $self->{'gs_sql'};
     
@@ -468,8 +381,6 @@
 	
 	print $outhandle "----------SQL DB contains meta-----------\n" if $self->{'verbosity'} > 2;
-	# https://www.effectiveperlprogramming.com/2010/07/set-custom-dbi-error-handlers/
 
 	foreach my $row (@$records) {
-	    #print $outhandle "row: @$row\n";
 	    my ($primary_key, $did, $sid, $metaname, $metaval) = @$row;
 	    
@@ -480,8 +391,6 @@
 		if $self->{'verbosity'} > 2;
 	    
-	    # + TODO:  we accessed the db in utf8 mode, so, we can call doc_obj->add_utf8_meta directly:
-	    #$doc_obj->add_utf8_metadata($sid, $metaname, &docprint::unescape_text($metaval));
-	    
-	    # data stored unescaped in db: escaping only for html/xml files, not for txt files or db
+	    # We're only dealing with utf8 data where docobj is concerned
+	    # Data stored unescaped in db: escaping only for html/xml files, not for txt files or db
 	    $doc_obj->add_utf8_metadata($sid, $metaname, $metaval);
 	}
@@ -510,9 +419,7 @@
 	    print $outhandle "### did: $did, sid: |$sid|, fulltext: <TXT>\n"
 		if $self->{'verbosity'} > 2;
-	    
-	    # TODO - pass by ref?
-	    # + TODO: we accessed the db in utf8 mode, so, we can call doc_obj->add_utf8_text directly:
-	    # data stored unescaped in db: escaping is only for html/xml files, not for txt files or db
-	    #my $textref = &docprint::unescape_textref(\$text);
+
+	    # We're only dealing with utf8 data where docobj is concerned
+	    # Data stored unescaped in db: escaping is only for html/xml files, not for txt files or db
 	    $doc_obj->add_utf8_textref($sid, \$text);
 	}	
Index: /main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm
===================================================================
--- /main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm	(revision 32594)
+++ /main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm	(revision 32595)
@@ -39,25 +39,11 @@
 
 
-# + TODO: SIGTERM rollback and disconnect?
-# + TODO Q: what about verbosity for debugging, instead of current situation of printing out upon debug set at the expense of writing to db
-#+ TODO Q: introduced site param to plugins and plugouts. Did I do it right? And should they have hiddengli = "yes". No longer a param
-# !!!! Did I do the pass by ref in docprint's escape and unescape textref functions correctly, and how they're called here?
-#   Any more optimisation I can do around this?
-
-# this plugout does not output the metadata and/or fulltxt xml to a file,
-# but outputs rows into a mysql table for metadata and/or a table for fulltxt
+# This plugout does not output the metadata and/or fulltxt xml to a file,
+# but outputs rows into a MySQL db table for metadata and/or a db table for fulltxt
+
 sub BEGIN {
     @GreenstoneSQLPlugout::ISA = ('GreenstoneXMLPlugout');
 }
 
-# + NOTTODO: die() statements need to be replaced with premature_termination
-# which should ensure the GreenstoneXMLPlugin (group)'s stuff is closed and cleaned up SOMEHOW
-# It's fine: the die() stmts all take place before setting up the super class' begin
-
-# + TODO Q: about build_mode: how to detect removeold. Now handled by 
-#   GreenstoneSQLPlugout::remove_all(), which is inherited from a base plugin.
-# + TODO: deal with -removeold and everything? Or type out instructions for user
-
-# + TODO Q: what is "group" in GreenstoneXMLPlugout?
 
 my $process_mode_list =
@@ -141,5 +127,4 @@
         return bless $self, $class;
     }
-    #print STDERR "***** GreenstoneSQLPlugout process mode = \"", $self->{'process_mode'}, "\"\n";
     
     return bless $self, $class;
@@ -151,17 +136,4 @@
     my $self= shift (@_);
     
-    # The saveas.options
-    #print STDERR "@@@@ PLUGOUT db_pwd: " . $self->{'db_client_pwd'} . "\n";
-    #print STDERR "@@@@ user: " . $self->{'db_client_user'} . "\n";
-    #print STDERR "@@@@ db_host: " . $self->{'db_host'} . "\n";
-    #print STDERR "@@@@ db_driver: " . $self->{'db_driver'} . "\n";
-    #print STDERR "@@@@ proc_mode: " . $self->{'process_mode'} . "\n";
-
-    ############ LOAD NECESSARY OPTIONS ###########
-    #print "@@@ plugout SITE NAME: ". $self->{'site'} . "\n" if defined $self->{'site'};
-    #print STDERR "########## COLLECTION: ". $ENV{'GSDLCOLLECTION'}."\n";
-
-    print STDERR "***** GreenstoneSQLPlugout process mode = \"", $self->{'process_mode'}, "\"\n";
-
     my $db_params = {
 	'collection_name' => $ENV{'GSDLCOLLECTION'},
@@ -192,6 +164,4 @@
     }
 
-    #die("@@@@ TEST. Connected successfully. Testing gsmysql::destructor.\n"); # WORKS
-    
     my $db_name = $self->{'site'} || "greenstone2"; # one database per GS3 site, for GS2 the db is called greenstone2
     my $proc_mode = $self->{'process_mode'};
@@ -201,9 +171,7 @@
     
     if($success && $proc_mode ne "text_only") {
-	##print STDERR "@@@@ Ensuring meta table exists\n";
 	$success = $gs_sql->ensure_meta_table_exists();
     }    
     if($success && $proc_mode ne "meta_only") {
-	##print STDERR "@@@@ Ensuring fulltxt table exists\n";
 	$success = $gs_sql->ensure_fulltxt_table_exists();
     }
@@ -213,5 +181,4 @@
 	# PrintError would already have displayed the warning message on load fail
 	# And on die() perl will call gsmysql destroy which will ensure a disconnect() from db
-	#$gs_sql->force_disconnect_from_db(); # disconnect_from_db() will issue a warning on error
 	die("Could not use db $db_name and/or prepare its tables. Can't proceed.\n");
     }
@@ -221,7 +188,7 @@
     
     
-    # if setting up to work with sql db failed, we'd have terminated and wouldn't come up to here:
-    # won't bother preparing GreenstoneXMLPlugout by calling superclass' begin()
-    # finally, can call begin on super - important as doc.xml is opened as a group etc
+    # If setting up connection to sql db failed, we'd have terminated and wouldn't come up to here
+    # and wouldn't have bothered preparing GreenstoneXMLPlugout by calling superclass' begin().
+    # Finally, can call begin on super - important as doc.xml is opened as a group etc
     
     $self->SUPER::begin(@_);
@@ -261,6 +228,5 @@
 }
  
-# + X TODO: check arc-inf.db for whether each entry is to be deleted/indexed/reindexed/been indexed?
-# That's only for indexing, not for this step which only generates the content in archives dir
+# saveas() only generates the content in archives dir and in the SQL database
 sub saveas {
     my $self = shift (@_);
@@ -276,7 +242,4 @@
     $self->{'debug_outhandle'} = $docxml_outhandler if ($self->{'debug'}); # STDOUT if debug
 
-    # TODO: also set debugging in begin()? Then stmts creating db and tables also sent to debug out and not executed
-   
-    
     # 2. overriding saving behaviour to do what the superclass does (writing out doc.xml files,
     # under new name of docsql.xml, with breadcrumbs pointing to sql db) PLUS saving to sql db    
@@ -308,6 +271,6 @@
     
     
-    # database connection is closed in end() method
-    # so we don't open and close over and over for each doc during a single build
+    # database connection is closed once, in end() method
+    # We're not opening and closing over and over for each doc during a single build
 }
 
@@ -324,6 +287,4 @@
 }
 
-# Perl: Reading or Writing to Another Program
-# https://nnc3.com/mags/Perl3/cookbook/ch16_05.htm
 sub recursive_write_meta_and_text {
     my $self = shift (@_);
@@ -345,10 +306,7 @@
 	foreach my $data (@{$section_ptr->{'metadata'}}) {
 	    my $meta_name = $data->[0];
-	    # TODO: does it need to be stored escaped, as it requires unescaping when read back in
-	    # from db (unlike for reading back in from doc.xml)
 
 	    # Treat db like a text file instead of an html/xml file: don't need to escape text
-	    # going into it
-	    #my $escaped_meta_value = &docprint::escape_text($data->[1]);
+	    # going into it, unlike with doc(sql).xml
 	    my $meta_value = $data->[1];
 	    
@@ -359,4 +317,5 @@
 	    # filling in the values
 	    # OR if debugging, then it will print the SQL insert statement but not execute it
+	    # (a behaviour following what the GS XML Plugout superclass does on debug)
 	    
 	    $gs_sql->insert_row_into_metadata_table($doc_oid, $section_name, $meta_name, $meta_value, $self->{'debug'});
@@ -368,7 +327,7 @@
 
 	# See above, no need to html-escape for db
-	my $section_text = $section_ptr->{'text'}; #&docprint::escape_textref(\$section_ptr->{'text'});
+	my $section_text = $section_ptr->{'text'};
 	
-	# fulltxt column can be SQL NULL. undef value gets written out as NULL:
+	# fulltxt column can be SQL NULL. undef value for $section_text gets written out as NULL:
 	# https://stackoverflow.com/questions/12708633/which-one-represents-null-undef-or-empty-string
 	# The following will do the SQL insertion
