From ecac0e2b9e8e8e78d771b20fe441e95bb02db2fa Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Fri, 20 Jun 2014 11:06:42 +0200 Subject: [PATCH] Do all-visible handling in lazy_vacuum_page() outside its critical section. Since fdf9e21196a lazy_vacuum_page() rechecks the all-visible status of pages in the second pass over the heap. It does so inside a critical section, but both visibilitymap_test() and heap_page_is_all_visible() perform operations that should not happen inside one. The former potentially performs IO and both potentially do memory allocations. To fix, simply move all the all-visible handling outside the critical section. Doing so means that the PD_ALL_VISIBLE on the page won't be included in the full page image of the HEAP2_CLEAN record anymore. But that's fine, the flag will be set by the HEAP2_VISIBLE logged later. Backpatch to 9.3 where the problem was introduced. The bug only came to light due to the assertion added in 4a170ee9 and isn't likely to cause problems in production scenarios. The worst outcome is a avoidable PANIC restart. This also gets rid of the difference in the order of operations between master and standby mentioned in 2a8e1ac5. Per reports from David Leverton and Keith Fiske in bug #10533. --- src/backend/commands/vacuumlazy.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index b4abeed5ac..5d6d031b48 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -1213,13 +1213,6 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, PageRepairFragmentation(page); - /* - * Now that we have removed the dead tuples from the page, once again - * check if the page has become all-visible. - */ - if (heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid)) - PageSetAllVisible(page); - /* * Mark buffer dirty before we write WAL. */ @@ -1237,6 +1230,23 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, PageSetLSN(page, recptr); } + /* + * End critical section, so we safely can do visibility tests (which + * possibly need to perform IO and allocate memory!). If we crash now the + * page (including the corresponding vm bit) might not be marked all + * visible, but that's fine. A later vacuum will fix that. + */ + END_CRIT_SECTION(); + + /* + * Now that we have removed the dead tuples from the page, once again + * check if the page has become all-visible. The page is already marked + * dirty, exclusively locked, and, if needed, a full page image has been + * emitted in the log_heap_clean() above. + */ + if (heap_page_is_all_visible(onerel, buffer, &visibility_cutoff_xid)) + PageSetAllVisible(page); + /* * All the changes to the heap page have been done. If the all-visible * flag is now set, also set the VM bit. @@ -1249,8 +1259,6 @@ lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer, visibility_cutoff_xid); } - END_CRIT_SECTION(); - return tupindex; }