Refactored backup manifest update by centralizing the code.

README.md

@@ -472,9 +472,9 @@ The `general` section defines settings that are shared between multiple operatio
 Set the buffer size used for copy, compress, and uncompress functions.  A maximum of 3 buffers will be in use at a time per thread.  An additional maximum of 256K per thread may be used for zlib buffers.
 ```
 required: n
-default: 1048576
+default: 4194304
-allow: 4096 - 8388608
+allow: 16384 - 8388608
-example: buffer-size=16384
+example: buffer-size=32768
 ```
 
 ##### `compress` key
@@ -698,7 +698,7 @@ example: db-path=/data/db
 
 ## Release Notes
 
-### v0.60: **************TBD***************
+### v0.60: better version support and WAL improvements
 
 * Pushing duplicate WAL now generates an error.  This worked before only if checksums were disabled.
 
@@ -706,6 +706,8 @@ example: db-path=/data/db
 
 * Regression tests working back to PostgreSQL 8.3.
 
+* Improved threading model by starting threads early and terminating them late.
+
 ### v0.50: restore and much more
 
 * Added restore functionality.

bin/pg_backrest.pl

@@ -83,6 +83,8 @@ sub safe_exit
 {
     my $iExitCode = shift;
 
+    &log(ERROR, "safe exit called, terminating threads");
+
     my $iTotal = threadGroupDestroy();
     remoteDestroy();
 

doc/doc.xml

@@ -449,8 +449,8 @@ Run a <id>full</id> backup on the <id>db</id> stanza.  <param>--type</param> can
 					<config-key id="buffer-size">
 						<text>Set the buffer size used for copy, compress, and uncompress functions.  A maximum of 3 buffers will be in use at a time per thread.  An additional maximum of 256K per thread may be used for zlib buffers.</text>
 
-						<allow>4096 - 8388608</allow>
+						<allow>16384 - 8388608</allow>
-						<example>16384</example>
+						<example>32768</example>
 					</config-key>
 
 					<!-- CONFIG - BACKUP SECTION - COMPRESS -->
@@ -656,7 +656,7 @@ Run a <id>full</id> backup on the <id>db</id> stanza.  <param>--type</param> can
 
 	<release title="Release Notes">
 		<release-version-list>
-			<release-version version="0.60" title="**************TBD***************">
+			<release-version version="0.60" title="better version support and WAL improvements">
 				<release-feature-bullet-list>
 					<release-feature>
 						<text>Pushing duplicate WAL now generates an error.  This worked before only if checksums were disabled.</text>
@@ -667,6 +667,9 @@ Run a <id>full</id> backup on the <id>db</id> stanza.  <param>--type</param> can
 					<release-feature>
 						<text>Regression tests working back to <postgres/> 8.3.</text>
 					</release-feature>
+					<release-feature>
+						<text>Improved threading model by starting threads early and terminating them late.</text>
+					</release-feature>
 				</release-feature-bullet-list>
 			</release-version>
 

lib/BackRest/Backup.pm

@@ -473,23 +473,8 @@ sub backup_file
                                $$oFileCopy{checksum}, $$oFileCopy{checksum_only},
                                $$oFileCopy{size}, $lSizeTotal, $lSizeCurrent);
 
-                # If copy was successful store the checksum and size
+                backupManifestUpdate($oBackupManifest, $$oFileCopy{file_section}, $$oFileCopy{file},
-                if ($bCopied)
+                                     $bCopied, $lCopySize, $strCopyChecksum);
-                {
-                    $oBackupManifest->set($$oFileCopy{file_section}, $$oFileCopy{file},
-                                          MANIFEST_SUBKEY_SIZE, $lCopySize + 0);
-
-                    if ($lCopySize > 0)
-                    {
-                        $oBackupManifest->set($$oFileCopy{file_section}, $$oFileCopy{file},
-                                              MANIFEST_SUBKEY_CHECKSUM, $strCopyChecksum);
-                    }
-                }
-                # Else the file was removed during backup so remove from manifest
-                else
-                {
-                    $oBackupManifest->remove($$oFileCopy{file_section}, $$oFileCopy{file});
-                }
             }
         }
     }
@@ -512,63 +497,15 @@ sub backup_file
         # Complete thread queues
         threadGroupComplete();
 
-        # Read the messages that we passed back from the threads.  These should be two types:
+        # Read the messages that are passed back from the backup threads
-        # 1) remove - files that were skipped because they were removed from the database during backup
+        while (my $oMessage = $oResultQueue->dequeue_nb())
-        # 2) checksum - file checksums calculated by the threads
-        while (my $strMessage = $oResultQueue->dequeue_nb())
         {
-            &log(TRACE, "message received in master queue: ${strMessage}");
+            &log(TRACE, "message received in master queue: section = $$oMessage{file_section}, file = $$oMessage{file}" .
+                        ", copied = $$oMessage{copied}"); #, size = $$oMessage{size}, checksum = " .
+#                        (defined($$oMessage{checksum}) ? $$oMessage{checksum} : '[undef]'));
 
-            # Split the message.  Currently using | as the split character.  Not ideal, but it will do for now.
+            backupManifestUpdate($oBackupManifest, $$oMessage{file_section}, $$oMessage{file},
-            my @strSplit = split(/\|/, $strMessage);
+                                 $$oMessage{copied}, $$oMessage{size}, $$oMessage{checksum});
-
-            my $strCommand = $strSplit[0];      # Command to execute on a file
-            my $strFileSection = $strSplit[1];  # File section where the file is located
-            my $strFile = $strSplit[2];         # The file to act on
-
-            # These three parts are required
-            if (!defined($strCommand) || !defined($strFileSection) || !defined($strFile))
-            {
-                confess &log(ASSERT, 'thread messages must have strCommand, strFileSection and strFile defined');
-            }
-
-            &log (DEBUG, "command = ${strCommand}, file_section = ${strFileSection}, file = ${strFile}");
-
-            # If command is 'remove' then mark the skipped file in the manifest
-            if ($strCommand eq 'remove')
-            {
-                $oBackupManifest->remove($strFileSection, $strFile);
-
-                &log (INFO, "removed file ${strFileSection}:${strFile} from the manifest (it was removed by db during backup)");
-            }
-            # If command is 'checksum' then record the checksum in the manifest
-            elsif ($strCommand eq 'checksum')
-            {
-                my $strChecksum = $strSplit[3]; # File checksum calculated by the thread
-                my $lFileSize = $strSplit[4];   # File size calculated by the thread
-
-                # Checksum must be defined
-                if (!defined($strChecksum))
-                {
-                    confess &log(ASSERT, 'thread checksum messages must have strChecksum defined');
-                }
-
-                # Checksum must be defined
-                if (!defined($lFileSize))
-                {
-                    confess &log(ASSERT, 'thread checksum messages must have lFileSize defined');
-                }
-
-                $oBackupManifest->set($strFileSection, $strFile, MANIFEST_SUBKEY_SIZE, $lFileSize + 0);
-
-                if ($lFileSize > 0)
-                {
-                    $oBackupManifest->set($strFileSection, $strFile, MANIFEST_SUBKEY_CHECKSUM, $strChecksum);
-                }
-
-                # Log the checksum
-                &log (DEBUG, "write checksum ${strFileSection}:${strFile} into manifest: ${strChecksum} (${lFileSize})");
-            }
         }
     }
 }

lib/BackRest/BackupFile.pm

@@ -22,7 +22,7 @@ use BackRest::Manifest;
 use BackRest::File;
 
 ####################################################################################################################################
-# backupThread
+# backupFile
 ####################################################################################################################################
 sub backupFile
 {
@@ -73,19 +73,6 @@ sub backupFile
             # If file is missing assume the database removed it (else corruption and nothing we can do!)
             &log(INFO, "skipped file removed by database: " . $strSourceFile);
 
-            # # Remove file from the manifest
-            # if ($bMulti)
-            # {
-            #     # Write a message into the master queue to have the file removed from the manifest
-            #     $oMasterQueue[$iThreadIdx]->enqueue("remove|$oFileCopyMap{$strFile}{file_section}|".
-            #                                         "$oFileCopyMap{$strFile}{file}");
-            # }
-            # else
-            # {
-            #     # remove it directly
-            #     $oBackupManifest->remove($oFileCopyMap{$strFile}{file_section}, $oFileCopyMap{$strFile}{file});
-            # }
-
             return false, $lSizeCurrent, undef, undef;
         }
     }
@@ -96,22 +83,6 @@ sub backupFile
     # Generate checksum for file if configured
     if ($lCopySize != 0)
     {
-        # # Store checksum in the manifest
-        # if ($bMulti)
-        # {
-        #     # Write the checksum message into the master queue
-        #     $oMasterQueue[$iThreadIdx]->enqueue("checksum|$oFileCopyMap{$strFile}{file_section}|" .
-        #                                         "$oFileCopyMap{$strFile}{file}|${strCopyChecksum}|${lCopySize}");
-        # }
-        # else
-        # {
-        #     # Write it directly
-        #     $oBackupManifest->set($oFileCopyMap{$strFile}{file_section}, $oFileCopyMap{$strFile}{file},
-        #                           MANIFEST_SUBKEY_CHECKSUM, $strCopyChecksum);
-        #     $oBackupManifest->set($oFileCopyMap{$strFile}{file_section}, $oFileCopyMap{$strFile}{file},
-        #                           MANIFEST_SUBKEY_SIZE, $lCopySize + 0);
-        # }
-
         # Output information about the file to be checksummed
         if (!defined($strLog))
         {
@@ -130,4 +101,35 @@ sub backupFile
 
 our @EXPORT = qw(backupFile);
 
+####################################################################################################################################
+# backupManifestUpdate
+####################################################################################################################################
+sub backupManifestUpdate
+{
+    my $oManifest = shift;
+    my $strSection = shift;
+    my $strFile = shift;
+    my $bCopied = shift;
+    my $lSize = shift;
+    my $strChecksum = shift;
+
+    # If copy was successful store the checksum and size
+    if ($bCopied)
+    {
+        $oManifest->set($strSection, $strFile, MANIFEST_SUBKEY_SIZE, $lSize + 0);
+
+        if ($lSize > 0)
+        {
+            $oManifest->set($strSection, $strFile, MANIFEST_SUBKEY_CHECKSUM, $strChecksum);
+        }
+    }
+    # Else the file was removed during backup so remove from manifest
+    else
+    {
+        $oManifest->remove($strSection, $strFile);
+    }
+}
+
+push @EXPORT, qw(backupManifestUpdate);
+
 1;

lib/BackRest/ThreadGroup.pm

@@ -120,28 +120,20 @@ sub threadGroupThread
                 }
                 elsif ($$oCommand{function} eq 'backup')
                 {
-                    my $bCopied;            # Was the file copied?
+                    # Result hash that can be passed back to the master process
-                    my $lCopySize;          # Size reported by copy
+                    my $oResult = {};
-                    my $strCopyChecksum;    # Checksum reported by copy
 
                     # Backup the file
-                    ($bCopied, $lSizeCurrent, $lCopySize, $strCopyChecksum) =
+                    ($$oResult{copied}, $lSizeCurrent, $$oResult{size}, $$oResult{checksum}) =
                         backupFile($oFile, $$oMessage{db_file}, $$oMessage{backup_file}, $$oCommand{param}{compress},
                                    $$oMessage{checksum}, $$oMessage{checksum_only},
                                    $$oMessage{size}, $$oCommand{param}{size_total}, $lSizeCurrent);
 
-                    # If copy was successful store the checksum and size
+                    # Send a message to update the manifest
-                    if ($bCopied)
+                    $$oResult{file_section} = $$oMessage{file_section};
-                    {
+                    $$oResult{file} = $$oMessage{file};
-                        $$oCommand{param}{result_queue}->enqueue("checksum|$$oMessage{file_section}|" .
+
-                                                                 "$$oMessage{file}|${strCopyChecksum}|${lCopySize}");
+                    $$oCommand{param}{result_queue}->enqueue($oResult);
-                    }
-                    # Else the file was removed during backup so remove from manifest
-                    else
-                    {
-                        $$oCommand{param}{result_queue}->enqueue("remove|$$oMessage{file_section}|".
-                                                                 "$$oMessage{file}");
-                    }
                 }
                 else
                 {