From 41803d55a2036f9a9602db88169250dd55f9d8cf Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 19 Aug 2017 13:39:38 -0400 Subject: [PATCH] Fix possible core dump in parallel restore when using a TOC list. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 --- src/bin/pg_dump/pg_backup_archiver.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 3b8b950f6d..136445ee59 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -3796,8 +3796,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; @@ -3844,7 +3845,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 @@ -4521,7 +4522,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) @@ -4538,15 +4539,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); -- 2.40.0