From c931c07124d4a582c7346942fa168e21e66a608f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 11 Feb 2008 19:14:30 +0000 Subject: [PATCH] Repair VACUUM FULL bug introduced by HOT patch: the original way of calculating a page's initial free space was fine, and should not have been "improved" by letting PageGetHeapFreeSpace do it. VACUUM FULL is going to reclaim LP_DEAD line pointers later, so there is no need for a guard against the page being too full of line pointers, and having one risks rejecting pages that are perfectly good move destinations. This also exposed a second bug, which is that the empty_end_pages logic assumed that any page with no live tuples would get entered into the fraged_pages list automatically (by virtue of having more free space than the threshold in the do_frag calculation). This assumption certainly seems risky when a low fillfactor has been chosen, and even without tunable fillfactor I think it could conceivably fail on a page with many unused line pointers. So fix the code to force do_frag true when notup is true, and patch this part of the fix all the way back. Per report from Tomas Szepe. --- src/backend/commands/vacuum.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 6b2b70dd1a..20374f4cd6 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -13,7 +13,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.363 2008/01/03 21:23:15 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.364 2008/02/11 19:14:30 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1659,12 +1659,18 @@ scan_heap(VRelStats *vacrelstats, Relation onerel, free_space += vacpage->free; /* - * Add the page to fraged_pages if it has a useful amount of free - * space. "Useful" means enough for a minimal-sized tuple. But we - * don't know that accurately near the start of the relation, so add - * pages unconditionally if they have >= BLCKSZ/10 free space. + * Add the page to vacuum_pages if it requires reaping, and add it to + * fraged_pages if it has a useful amount of free space. "Useful" + * means enough for a minimal-sized tuple. But we don't know that + * accurately near the start of the relation, so add pages + * unconditionally if they have >= BLCKSZ/10 free space. Also + * forcibly add pages with no live tuples, to avoid confusing the + * empty_end_pages logic. (In the presence of unreasonably small + * fillfactor, it seems possible that such pages might not pass + * the free-space test, but they had better be in the list anyway.) */ - do_frag = (vacpage->free >= min_tlen || vacpage->free >= BLCKSZ / 10); + do_frag = (vacpage->free >= min_tlen || vacpage->free >= BLCKSZ / 10 || + notup); if (do_reap || do_frag) { @@ -1679,6 +1685,7 @@ scan_heap(VRelStats *vacrelstats, Relation onerel, /* * Include the page in empty_end_pages if it will be empty after * vacuuming; this is to keep us from using it as a move destination. + * Note that such pages are guaranteed to be in fraged_pages. */ if (notup) { @@ -3725,7 +3732,19 @@ enough_space(VacPage vacpage, Size len) static Size PageGetFreeSpaceWithFillFactor(Relation relation, Page page) { - Size freespace = PageGetHeapFreeSpace(page); + /* + * It is correct to use PageGetExactFreeSpace() here, *not* + * PageGetHeapFreeSpace(). This is because (a) we do our own, exact + * accounting for whether line pointers must be added, and (b) we will + * recycle any LP_DEAD line pointers before starting to add rows to a + * page, but that may not have happened yet at the time this function is + * applied to a page, which means PageGetHeapFreeSpace()'s protection + * against too many line pointers on a page could fire incorrectly. We do + * not need that protection here: since VACUUM FULL always recycles all + * dead line pointers first, it'd be physically impossible to insert more + * than MaxHeapTuplesPerPage tuples anyway. + */ + Size freespace = PageGetExactFreeSpace(page); Size targetfree; targetfree = RelationGetTargetPageFreeSpace(relation,