Fix possible core dump in parallel restore when using a TOC list.

Commit 3eb9a5e7c unintentionally introduced an ordering dependency
into restore_toc_entries_prefork().  The existing coding of
reduce_dependencies() contains a check to skip moving a TOC entry
to the ready_list if it wasn't initially in the pending_list.
This used to suffice to prevent reduce_dependencies() from trying to
move anything into the ready_list during restore_toc_entries_prefork(),
because the pending_list stayed empty throughout that phase; but it no
longer does.  The problem doesn't manifest unless the TOC has been
reordered by SortTocFromFile, which is how I missed it in testing.

To fix, just add a test for ready_list == NULL, converting the call
with NULL from a poor man's sanity check into an explicit command
not to touch TOC items' list membership.  Clarify some of the comments
around this; in particular, note the primary purpose of the check for
pending_list membership, which is to ensure that we can't try to restore
the same item twice, in case a TOC list forces it to be restored before
its dependency count goes to zero.

Per report from Fabrízio de Royes Mello.  Back-patch to 9.3, like the
previous commit.

Discussion: https://postgr.es/m/CAFcNs+pjuv0JL_x4+=71TPUPjdLHOXA4YfT32myj_OrrZb4ohA@mail.gmail.com

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 4cfb71c0133ad57a071d6d3ea3b78f6cd9f0d413..8ae7515c9cdc7a54fb9c82d3a77accd3dff22c39 100644 (file)
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3851,8 +3851,9 @@ restore_toc_entries_prefork(ArchiveHandle *AH, TocEntry *pending_list)
         *
         * Note: as of 9.2, it should be guaranteed that all PRE_DATA items appear
         * before DATA items, and all DATA items before POST_DATA items.  That is
-        * not certain to be true in older archives, though, so this loop is coded
-        * to not assume it.
+        * not certain to be true in older archives, though, and in any case use
+        * of a list file would destroy that ordering (cf. SortTocFromFile).  So
+        * this loop cannot assume that it holds.
         */
        AH->restorePass = RESTORE_PASS_MAIN;
        skipped_some = false;
@@ -3899,7 +3900,7 @@ restore_toc_entries_prefork(ArchiveHandle *AH, TocEntry *pending_list)
 
                        (void) restore_toc_entry(AH, next_work_item, false);
 
-                       /* there should be no touch of ready_list here, so pass NULL */
+                       /* Reduce dependencies, but don't move anything to ready_list */
                        reduce_dependencies(AH, next_work_item, NULL);
                }
                else
@@ -4545,7 +4546,7 @@ identify_locking_dependencies(ArchiveHandle *AH, TocEntry *te)
 /*
  * Remove the specified TOC entry from the depCounts of items that depend on
  * it, thereby possibly making them ready-to-run.  Any pending item that
- * becomes ready should be moved to the ready list.
+ * becomes ready should be moved to the ready_list, if that's provided.
  */
 static void
 reduce_dependencies(ArchiveHandle *AH, TocEntry *te, TocEntry *ready_list)
@@ -4562,15 +4563,19 @@ reduce_dependencies(ArchiveHandle *AH, TocEntry *te, TocEntry *ready_list)
                otherte->depCount--;
 
                /*
-                * It's ready if it has no remaining dependencies and it belongs in
-                * the current restore pass.  However, don't move it if it has not yet
-                * been put into the pending list.
+                * It's ready if it has no remaining dependencies, and it belongs in
+                * the current restore pass, and it is currently a member of the
+                * pending list (that check is needed to prevent double restore in
+                * some cases where a list-file forces out-of-order restoring).
+                * However, if ready_list == NULL then caller doesn't want any list
+                * memberships changed.
                 */
                if (otherte->depCount == 0 &&
                        _tocEntryRestorePass(otherte) == AH->restorePass &&
-                       otherte->par_prev != NULL)
+                       otherte->par_prev != NULL &&
+                       ready_list != NULL)
                {
-                       /* It must be in the pending list, so remove it ... */
+                       /* Remove it from pending list ... */
                        par_list_remove(otherte);
                        /* ... and add to ready_list */
                        par_list_append(ready_list, otherte);