Index: /main/trunk/greenstone2/perllib/gssql.pm
===================================================================
--- /main/trunk/greenstone2/perllib/gssql.pm	(revision 32590)
+++ /main/trunk/greenstone2/perllib/gssql.pm	(revision 32591)
@@ -52,5 +52,5 @@
 # + 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
+# + 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?
@@ -157,5 +157,5 @@
 #
 # 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 exitin# g 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.
@@ -166,4 +166,5 @@
 #
 # 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."
+#
 #
 sub DESTROY {
@@ -171,25 +172,22 @@
     
     if (${^GLOBAL_PHASE} eq 'DESTRUCT') {
-	
+
 	if ($_dbh_instance) { # database handle still active. Use singleton handle!
-
-	    # rollback code has moved to finish_signal_handler() where it belongs?
-	    
-	    # NOTE: if RaiseError is set on dbi connection, then on any error, perl process will die()
-	    # which will end up calling this DESTROY. If it was a die() that called DESTROY
-	    # then need to rollback the db here. However, if it was not a die() but natural termination
-	    # of the perl process, destroy() will also get called. In that case we don't want to rollback
-	    # but do a commit() to the DB instead.
-	    # Perhaps detecting the difference may be accomplished by checking ref_count:
-	    # - If ref_count not 0 it may require a rollback?
-	    # - If ref_count 0 it may be a natural termination and require a commit? Except that ref_count
-	    # is set back to 0 in finished(), which will do the commit when ref_count becomes 0. So shouldn't
-	    # (have to) do that here.
+	                      # dbh instance being active implies build was cancelled
+
+	    # rollback code has moved to finish_signal_handler() where it belongs
+	    # as rollback() should only happen on cancel/unnatural termination
+	    # vs commit() happening in finished() before disconnect, which is natural termination.
+
 	    
 	    # We're now finally ready to disconnect, as is required for both natural and premature termination
-	    print STDERR "XXXXXXXX Global Destruct: Disconnecting from database\n";
-	    $_dbh_instance->disconnect or warn $_dbh_instance->errstr;
-	    $_dbh_instance = undef;
-	    $ref_count = 0;
+	    # (Though natural termination would have disconnected already)
+	    # We now leave DBI's own destructor to do the disconnection when perl calls its DESTROY()
+	    # We'll just print a message to stop anyone from worrying whether cancelling build
+	    # will ensure disconnection still happens. It happens, but silently.
+	    print STDERR "   Global Destruct Phase: DBI's own destructor will disconnect database\n";
+	    #$_dbh_instance->disconnect or warn $_dbh_instance->errstr;
+	    #$_dbh_instance = undef;
+	    #$ref_count = 0;
 	}
 	return;
@@ -246,5 +244,5 @@
 	}
 	if($params_map->{'autocommit'}) {
-	    print STDERR "   SQL DB CANCEL SUPPORT OFF.\n";
+	    print STDERR "   SQL DB CANCEL SUPPORT OFF.\n" if($params_map->{'verbosity'} > 2);
 	} else {
 	    print STDERR "   SQL DB CANCEL SUPPORT ON.\n";
@@ -259,10 +257,14 @@
     my $db_enc = "utf8mb4" if $params_map->{'db_encoding'} eq "utf8";
 
-    # these are the params for connecting to MySQL
-    my $db_driver = $params_map->{'db_driver'} || "mysql";
-    my $db_user = $params_map->{'db_client_user'} || "root";
+    # Params for connecting to MySQL
+    # These params are ensured default/fallback values by the GS SQL Plugs
+    # so no need to set it here
+    my $db_driver = $params_map->{'db_driver'};
+    my $db_host = $params_map->{'db_host'};
+    my $db_user = $params_map->{'db_client_user'};
+
+    # params that can be undef are db_client_pwd and db_port
     my $db_pwd = $params_map->{'db_client_pwd'}; # even if undef and password was necessary,
                                      # we'll see a sensible error message when connect fails
-    my $db_host = $params_map->{'db_host'} || "127.0.0.1";
         # localhost doesn't work for us, but 127.0.0.1 works
         # https://metacpan.org/pod/DBD::mysql
@@ -355,5 +357,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,
+	# + TODO: 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
@@ -521,5 +523,5 @@
     }
 
-    # TODO Q: commit here, so that future select statements work?
+    # + TODO Q: commit here, so that future select statements work?
     # See https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#Transactions
 }
Index: /main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm
===================================================================
--- /main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm	(revision 32590)
+++ /main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm	(revision 32591)
@@ -43,5 +43,5 @@
 # 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.
+# + 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.
 
@@ -145,4 +145,7 @@
         'desc' => "{GreenstoneSQLPlug.rollbacl_on_cancel}" } ];
 
+# TODO: If subclassing gssql 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.
 my $arguments =
     [ { 'name' => "process_exp",
@@ -161,5 +164,5 @@
 	'type' => "enum",
 	'list' => $rollback_on_cancel_list,
-	'deft' => "false", # TODO Q: what's the better default? If "true", any memory concerns?
+	'deft' => "false", # better default than true
 	'reqd' => "no",
 	'hiddengli' => "no"},
@@ -178,9 +181,9 @@
 	'type' => "string",
 	'deft' => "",
-	'reqd' => "no"}, # pwd required? NO.
+	'reqd' => "no"}, # pwd not required: can create mysql accounts without pwd
       { 'name' => "db_host", 
 	'desc' => "{GreenstoneSQLPlug.db_host}",
 	'type' => "string",
-	'deft' => "127.0.0.1",
+	'deft' => "127.0.0.1", # NOTE: make this int? No default for port, since it's not a required connection param
 	'reqd' => "yes"},
       { 'name' => "db_port", 
@@ -226,5 +229,5 @@
 # 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 gssql.pm? That way both GS SQL Plugin
+# + TODO: Eventually can try moving get_gssql_instance into gssql.pm? That way both GS SQL Plugin
 # and Plugout would be using one connection during import.pl phase when both plugs exist.
 
@@ -421,5 +424,10 @@
 }
 
-# TODO Q: Why are there 4 passes when we're only indexing at doc and section level (2 passes)? What's the dummy pass, why is there a pass for infodb?
+# There are multiple passes processing the document (see buildcol's mode parameter description):
+# - compressing the text which may be a dummy pass for lucene/solr, wherein they still want the
+# docobj for different purposes,
+# - the pass(es) for indexing, e.g. doc/didx and section/sidx level passes
+# - and an infodb pass for processing the classifiers. This pass too needs the docobj
+# 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
@@ -472,6 +480,9 @@
 		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));
+	    # + 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
+	    $doc_obj->add_utf8_metadata($sid, $metaname, $metaval);
 	}
 	print $outhandle "----------FIN READING DOC's META FROM SQL DB------------\n"
@@ -501,7 +512,8 @@
 	    
 	    # TODO - pass by ref?
-	    # TODO: we accessed the db in utf8 mode, so, we can call doc_obj->add_utf8_text directly:
-	    my $textref = &docprint::unescape_textref(\$text);
-	    $doc_obj->add_utf8_text($sid, $$textref);
+	    # + 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);
+	    $doc_obj->add_utf8_textref($sid, \$text);
 	}	
 	print $outhandle "----------FIN READING DOC's TXT FROM SQL DB------------\n"
Index: /main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm
===================================================================
--- /main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm	(revision 32590)
+++ /main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm	(revision 32591)
@@ -41,6 +41,6 @@
 # + 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
-#X 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?
+#+ 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?
 
@@ -88,5 +88,5 @@
       'type' => "enum",
       'list' => $rollback_on_cancel_list,
-      'deft' => "false", # TODO Q: what's the better default? If "true", any memory concerns?
+      'deft' => "false", # better default than true
       'reqd' => "no",
       'hiddengli' => "no"},
@@ -105,9 +105,9 @@
       'type' => "string",
       'deft' => "",
-      'reqd' => "no"}, # pwd required? NO.
+      'reqd' => "no"}, # pwd not required: can create mysql accounts without pwd
     { 'name' => "db_host", 
       'desc' => "{GreenstoneSQLPlug.db_host}",
       'type' => "string",
-      'deft' => "127.0.0.1",
+      'deft' => "127.0.0.1", # localhost doesn't work for us, but 127.0.0.1 works. See gsmysql.pm
       'reqd' => "yes"},
     { 'name' => "db_port", 
@@ -347,5 +347,9 @@
 	    # 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)
-	    my $escaped_meta_value = &docprint::escape_text($data->[1]);
+
+	    # 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]);
+	    my $meta_value = $data->[1];
 	    
 	    # Write out the current section's meta to collection db's METADATA table	    
@@ -356,5 +360,5 @@
 	    # OR if debugging, then it will print the SQL insert statement but not execute it
 	    
-	    $gs_sql->insert_row_into_metadata_table($doc_oid, $section_name, $meta_name, $escaped_meta_value, $self->{'debug'});
+	    $gs_sql->insert_row_into_metadata_table($doc_oid, $section_name, $meta_name, $meta_value, $self->{'debug'});
 	}
     }
@@ -362,6 +366,7 @@
     
     if($proc_mode eq "all" || $proc_mode eq "text_only" ) {
-    
-	my $section_textref = &docprint::escape_textref(\$section_ptr->{'text'});
+
+	# See above, no need to html-escape for db
+	my $section_text = $section_ptr->{'text'}; #&docprint::escape_textref(\$section_ptr->{'text'});
 	
 	# fulltxt column can be SQL NULL. undef value gets written out as NULL:
@@ -369,5 +374,5 @@
 	# The following will do the SQL insertion
 	# or if debug, the following will print the SQL insert stmt without executing it
-	$gs_sql->insert_row_into_fulltxt_table($doc_oid, $section_name, $section_textref, $self->{'debug'});
+	$gs_sql->insert_row_into_fulltxt_table($doc_oid, $section_name, \$section_text, $self->{'debug'});
     
     }
