Index: main/trunk/greenstone2/perllib/gssql.pm
===================================================================
--- main/trunk/greenstone2/perllib/gssql.pm	(revision 32582)
+++ main/trunk/greenstone2/perllib/gssql.pm	(revision 32583)
@@ -2,5 +2,5 @@
 #
 # gssql.pm -- DBI for SQL related utility functions used by
-# GreenstoneSQLPlugout and hereafter by GreenstoneSQLPlugin too.
+# GreenstoneSQLPlugout and GreenstoneSQLPlugin too.
 # A component of the Greenstone digital library software
 # from the New Zealand Digital Library Project at the 
@@ -61,5 +61,5 @@
 
 # Parameterise (one or more methods may use them):
-# - db_name (which is the GS3 sitename)
+# - db_name (which is the GS3 sitename, or "greenstone2" for GS2)
 
 
@@ -83,5 +83,4 @@
 	}
     }
-
     
     die "Caught a $sig signal $!"; # die() will always call destructor (sub DESTROY)
@@ -125,5 +124,5 @@
 # "Itâs common to call die when handling SIGINT and SIGTERM. die is useful because it will ensure that Perl stops correctly: for example Perl will execute a destructor method if present when die is called, but the destructor method will not be called if a SIGINT or SIGTERM is received and no signal handler calls die."
 #
-# https://perldoc.perl.org/perlobj.html#Destructors
+# Useful: https://perldoc.perl.org/perlobj.html#Destructors
 #
 # https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#disconnect
@@ -145,17 +144,18 @@
 	if ($_dbh_instance) { # database handle still active. Use singleton handle!
 
-	    # THIS CODE HAS MOVED TO finish_signal_handler() WHERE IT BELONGS
-	    # If autocommit wasn't set, then this is a cancel operation.
-	    # If we've not disconnected from the sql db yet and if we've not committed
-	    # transactions yet, then cancel means we do a rollback here
-
-	    # if($_dbh_instance->{AutoCommit} == 0) {
-		
-	    # 	$_dbh_instance->rollback(); # will warn on failure, nothing more we can/want to do,
-            #     # don't do a die() here: possibility of infinite loop and we still want to disconnect
-	    # }
-
-	    # Either way, we're now finally ready to disconnect as is required for premature
-	    # termination too
+	    # 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.
+	    
+	    # 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;
@@ -165,4 +165,5 @@
 	return;
     }
+
 }
 
@@ -229,7 +230,7 @@
 	}
 	if($params_map->{'autocommit'}) {
-	    print STDERR "   SQL DB UNDO SUPPORT OFF.\n";
+	    print STDERR "   SQL DB CANCEL SUPPORT OFF.\n";
 	} else {
-	    print STDERR "   SQL DB UNDO SUPPORT ON.\n";
+	    print STDERR "   SQL DB CANCEL SUPPORT ON.\n";
 	}
     }
@@ -326,25 +327,6 @@
 
 # Will disconnect if this instance of gssql holds the last reference to the db connection
+# If disconnecting and autocommit is off, then this will commit before disconnecting
 sub finished {
-    my $self= shift (@_);
-
-    # 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
-    my $rc = 1;    
-    
-    $ref_count--;
-    if($ref_count == 0) {
-	# Only commit transaction when we're about to disconnect, not before
-	# If autocommit was on, then we'd have committed after every db operation, so nothing to do
-	$rc = $self->do_commit_if_on();
-	
-	$self->force_disconnect_from_db();
-    }
-
-    return $rc;
-}
-
-sub do_commit_if_on {
     my $self= shift (@_);
     my $dbh = $self->{'db_handle'};
@@ -352,15 +334,24 @@
     my $rc = 1; # return code: everything went fine, regardless of whether we needed to commit
                 # (AutoCommit on or off)
-
-    # https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#commit
-    if($dbh->{AutoCommit} == 0) {
-	print STDERR "   Committing transaction to SQL database now.\n" if $self->{'verbosity'};
-	$rc = $dbh->commit() or warn("SQL DB COMMIT FAILED: " . $dbh->errstr); # important problem
-	                                                        # worth embellishing error message
-    }
-    # If autocommit was on, then we'd have committed after every db operation, so nothing to do
+    
+    $ref_count--;
+    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,
+	# then here is where we commit our one long transaction.
+	# https://metacpan.org/pod/release/TIMB/DBI-1.634_50/DBI.pm#commit
+	if($dbh->{AutoCommit} == 0) {
+	    print STDERR "   Committing transaction to SQL database now.\n" if $self->{'verbosity'};
+	    $rc = $dbh->commit() or warn("SQL DB COMMIT FAILED: " . $dbh->errstr); # important problem
+	    # worth embellishing error message
+	}
+	# else if autocommit was on, then we'd have committed after every db operation, so nothing to do
+	
+	$self->_force_disconnect_from_db();
+    }
 
     return $rc;
 }
+
 
 # Call this method on die(), so that you're sure the perl process has disconnected from SQL db
@@ -368,5 +359,5 @@
 # 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
-sub force_disconnect_from_db {
+sub _force_disconnect_from_db {
     my $self= shift (@_);
 
Index: main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm
===================================================================
--- main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm	(revision 32582)
+++ main/trunk/greenstone2/perllib/plugins/GreenstoneSQLPlugin.pm	(revision 32583)
@@ -50,4 +50,7 @@
 
 # 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)
@@ -126,5 +129,4 @@
 
     return q^(?i)docsql(-\d+)?\.xml$^; # regex based on this method in GreenstoneXMLPlugin
-    #return q^(?i)docsql(-.+)?\.xml$^; # no longer storing the OID embedded in docsql .xml filename
 }
 
@@ -191,6 +193,5 @@
 
 
-# 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
+###### Methods called during buildcol and import #######
 
 sub new {
@@ -217,5 +218,104 @@
 }
 
-###### Called during import.pl
+# 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 gssql.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.
+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_name'} . "\n" if defined $self->{'site_name'};
+#    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 gssql object.
+    # collection name will be used for naming tables (site name will be used for naming database)
+    my $gs_sql = new gssql({
+	'collection_name' => $ENV{'GSDLCOLLECTION'},
+	'verbosity' => $self->{'verbosity'} || 0
+			   });
+    
+    # if autocommit is set, there's no rollback support
+    my $autocommit = ($self->{'rollback_on_cancel'} eq "false") ? 1 : 0;
+
+    # try connecting to the mysql db, die if that fails
+    if(!$gs_sql->connect_to_db({
+	'db_driver' => $self->{'db_driver'},
+	'db_client_user' => $self->{'db_client_user'},
+	'db_client_pwd' => $self->{'db_client_pwd'},
+	'db_host' => $self->{'db_host'},
+	'autocommit' => $autocommit
+			       })
+	)
+    {
+	# This is fatal for the plugout, let's terminate here
+	# PrintError would already have displayed the warning message on connection fail	
+	die("Could not connect to db. Can't proceed.\n");
+    }
+    
+    my $db_name = $self->{'site_name'} || "greenstone2"; # one database per GS3 site, for GS2 the db is called greenstone2
+
+    # Attempt to use the db, create it if it doesn't exist (but don't create the tables yet)
+    # Bail if we can't use the database
+    if(!$gs_sql->use_db($db_name)) {
+	
+	# This is fatal for the plugout, let's terminate here after disconnecting again
+	# PrintError would already have displayed the warning message on load fail
+	# And on die() perl will call gssql 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");
+    }
+    
+    
+    # store db handle now that we're connected
+    $self->{'gs_sql'} = $gs_sql;    
+}
+
+
+# This method also runs on import.pl if gs_sql has a value. But we just want to run it on buildcol
+# 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.
+sub deinit {
+    my ($self) = shift (@_);
+    
+    ##print STDERR "@@@@@@@@@@ GreenstoneSQLPlugin::DEINIT CALLED\n";
+    
+    if($self->{'gs_sql'}) {
+
+	# Important to call finished():
+	# it will disconnect from db if this is the last gssql instance,
+	# and it will commit to db before disconnecting if rollbback_on_cancel turned on
+	$self->{'gs_sql'}->finished();
+
+	# Clear gs_sql (setting key to undef has a different meaning from deleting:
+	# undef makes key still exist but its value is unded whereas delete deletes the key)
+	# So all future use has to make the connection again
+	delete $self->{'gs_sql'};
+    }
+
+    $self->SUPER::deinit(@_);
+}
+
+
+
+###### Methods only called during import.pl #####
 
 # This is called once if removeold is set with import.pl. Most plugins will do
@@ -246,9 +346,4 @@
     }
 
-    # UNNECESSARY
-    # The removeold related DB transaction (deleting collection tables) is complete
-    # Don't let GS SQL PlugIN interfere with GS SQL PlugOUT's database transactions
-    # during import.pl hereafter. Finish up.
-    #$gs_sql->do_commit_if_on();
 }
 
@@ -294,5 +389,5 @@
 }
 
-#### Called during buildcol
+##### Methods called only during buildcol #####
 
 sub xml_start_tag {
@@ -417,100 +512,4 @@
 }
 
-#### Called during buildcol and import
-
-# 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 gssql.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.
-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_name'} . "\n" if defined $self->{'site_name'};
-#    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 gssql object.
-    # collection name will be used for naming tables (site name will be used for naming database)
-    my $gs_sql = new gssql({
-	'collection_name' => $ENV{'GSDLCOLLECTION'},
-	'verbosity' => $self->{'verbosity'} || 0
-			   });
-    
-    # if autocommit is set, there's no rollback support
-    my $autocommit = ($self->{'rollback_on_cancel'} eq "false") ? 1 : 0;
-
-    # try connecting to the mysql db, die if that fails
-    if(!$gs_sql->connect_to_db({
-	'db_driver' => $self->{'db_driver'},
-	'db_client_user' => $self->{'db_client_user'},
-	'db_client_pwd' => $self->{'db_client_pwd'},
-	'db_host' => $self->{'db_host'},
-	'autocommit' => $autocommit
-			       })
-	)
-    {
-	# This is fatal for the plugout, let's terminate here
-	# PrintError would already have displayed the warning message on connection fail	
-	die("Could not connect to db. Can't proceed.\n");
-    }
-    
-    my $db_name = $self->{'site_name'} || "greenstone2"; # one database per GS3 site, for GS2 the db is called greenstone2
-
-    # Attempt to use the db, create it if it doesn't exist (but don't create the tables yet)
-    # Bail if we can't use the database
-    if(!$gs_sql->use_db($db_name)) {
-	
-	# This is fatal for the plugout, let's terminate here after disconnecting again
-	# PrintError would already have displayed the warning message on load fail
-	$gs_sql->force_disconnect_from_db();
-	die("Could not use db $db_name. Can't proceed.\n");
-    }
-    
-    
-    # store db handle now that we're connected
-    $self->{'gs_sql'} = $gs_sql;    
-}
-
-
-# This method also runs on import.pl if gs_sql has a value. But we just want to run it on buildcol
-# 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.
-sub deinit {
-    my ($self) = shift (@_);
-    
-    ##print STDERR "@@@@@@@@@@ GreenstoneSQLPlugin::DEINIT CALLED\n";
-    
-    if($self->{'gs_sql'}) { # only want to work with sql db if buildcol.pl, gs_sql won't have
-	# a value except during buildcol, so when processor =~ m/buildproc$/.
-	$self->{'gs_sql'}->finished();
-
-	# Clear gs_sql (setting key to undef has a different meaning from deleting:
-	# undef makes key still exist but its value is unded whereas delete deletes the key)
-	# So all future use has to make the connection again
-	delete $self->{'gs_sql'};
-    }
-
-    $self->SUPER::deinit(@_);
-}
-
-
-
-
+
+1;
Index: main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm
===================================================================
--- main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm	(revision 32582)
+++ main/trunk/greenstone2/perllib/plugouts/GreenstoneSQLPlugout.pm	(revision 32583)
@@ -39,8 +39,6 @@
 
 
-# This entire class is called only during import.pl
-
-# 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: 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_name param to plugins and plugouts. Did I do it right? And should they have hiddengli = "yes"
 # Did I do the pass by ref in docprint's escape and unescape textref functions correctly, and how they're called here?
@@ -121,4 +119,8 @@
 		'args'     => $arguments };
 
+##### This entire class is called only during import.pl #####
+
+##### Overridden methods #####
+
 sub new {
     my ($class) = shift (@_);
@@ -205,5 +207,6 @@
 	# This is fatal for the plugout, let's terminate here after disconnecting again
 	# PrintError would already have displayed the warning message on load fail
-	$gs_sql->force_disconnect_from_db(); # disconnect_from_db() will issue a warning on error
+	# And on die() perl will call gssql 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");
     }
@@ -227,6 +230,9 @@
     # do the superclass stuff first, as any sql db failures should not prevent superclass cleanup
     $self->SUPER::end(@_);    
-    
-    $self->{'gs_sql'}->finished(); # will disconnect from db if last instance
+
+    # Important to call finished():
+    # it will disconnect from db if this is the last gssql instance,
+    # and it will commit to db before disconnecting if rollbback_on_cancel turned on
+    $self->{'gs_sql'}->finished();
     delete $self->{'gs_sql'}; # key gs_sql no longer exists, not just the value being undef
 }
@@ -295,6 +301,5 @@
 
     
-    # 3. post save out
-    #$self->SUPER::post_saveas(@_);
+    # 3. post save out 
     $self->SUPER::post_saveas($doc_obj, $doc_dir, $docxml_outhandler, $output_file);
     
@@ -304,4 +309,5 @@
 }
 
+##### New methods, not inherited  #####
 
 # write meta and/or text PER DOC out to DB
