From: Tom Lane Date: Thu, 2 Dec 2004 19:28:49 +0000 (+0000) Subject: Disallow the combination VACUUM FULL FREEZE for safety's sake, for the X-Git-Tag: REL8_0_0RC1~34 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=e9c03c3b1b6d14e48fa1cfbb5831a9a03a0c0ab7;p=postgresql Disallow the combination VACUUM FULL FREEZE for safety's sake, for the reasons I outlined in pghackers a few days ago. Also, undo someone's overly optimistic decision to reduce tuple state checks from if (...) elog() to Asserts. If I trusted this code more, I might think it was a good idea to disable these checks in production installations. But I don't. --- diff --git a/doc/src/sgml/manage-ag.sgml b/doc/src/sgml/manage-ag.sgml index 0237e026b1..5f01b66ac7 100644 --- a/doc/src/sgml/manage-ag.sgml +++ b/doc/src/sgml/manage-ag.sgml @@ -1,5 +1,5 @@ @@ -240,8 +240,7 @@ createdb -T template0 dbname After preparing a template database, or making any changes to one, - it is a good idea to perform - VACUUM FREEZE or VACUUM FULL FREEZE in that + it is a good idea to perform VACUUM FREEZE in that database. If this is done when there are no other open transactions in the same database, then it is guaranteed that all rows in the database are frozen and will not be subject to transaction diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 930ce7e3ba..e020c9a9ac 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -1,5 +1,5 @@ @@ -20,8 +20,8 @@ PostgreSQL documentation -VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ table ] -VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ table [ (column [, ...] ) ] ] +VACUUM [ FULL | FREEZE ] [ VERBOSE ] [ table ] +VACUUM [ FULL | FREEZE ] [ VERBOSE ] ANALYZE [ table [ (column [, ...] ) ] ] diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 941e0d36e6..93694e4960 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.296 2004/12/01 19:00:41 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.297 2004/12/02 19:28:49 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -265,6 +265,27 @@ vacuum(VacuumStmt *vacstmt) else in_outer_xact = IsInTransactionChain((void *) vacstmt); + /* + * Disallow the combination VACUUM FULL FREEZE; although it would mostly + * work, VACUUM FULL's ability to move tuples around means that it is + * injecting its own XID into tuple visibility checks. We'd have to + * guarantee that every moved tuple is properly marked XMIN_COMMITTED or + * XMIN_INVALID before the end of the operation. There are corner cases + * where this does not happen, and getting rid of them all seems hard + * (not to mention fragile to maintain). On the whole it's not worth it + * compared to telling people to use two operations. See pgsql-hackers + * discussion of 27-Nov-2004, and comments below for update_hint_bits(). + * + * Note: this is enforced here, and not in the grammar, since (a) we can + * give a better error message, and (b) we might want to allow it again + * someday. + */ + if (vacstmt->vacuum && vacstmt->full && vacstmt->freeze) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("VACUUM FULL FREEZE is not supported"), + errhint("Use VACUUM FULL, then VACUUM FREEZE."))); + /* * Send info about dead objects to the statistics collector */ @@ -1346,8 +1367,7 @@ scan_heap(VRelStats *vacrelstats, Relation onerel, do_shrinking = false; break; default: - /* unexpected HeapTupleSatisfiesVacuum result */ - Assert(false); + elog(ERROR, "unexpected HeapTupleSatisfiesVacuum result"); break; } @@ -1530,9 +1550,7 @@ repair_frag(VRelStats *vacrelstats, Relation onerel, VacPageList vacuum_pages, VacPageList fraged_pages, int nindexes, Relation *Irel) { -#ifdef USE_ASSERT_CHECKING TransactionId myXID = GetCurrentTransactionId(); -#endif Buffer dst_buffer = InvalidBuffer; BlockNumber nblocks, blkno; @@ -1702,36 +1720,30 @@ repair_frag(VRelStats *vacrelstats, Relation onerel, */ if (!(tuple.t_data->t_infomask & HEAP_XMIN_COMMITTED)) { + if (tuple.t_data->t_infomask & HEAP_MOVED_IN) + elog(ERROR, "HEAP_MOVED_IN was not expected"); + if (!(tuple.t_data->t_infomask & HEAP_MOVED_OFF)) + elog(ERROR, "HEAP_MOVED_OFF was expected"); + /* - * There cannot be another concurrently running VACUUM. If - * the tuple had been moved in by a previous VACUUM, the - * visibility check would have set XMIN_COMMITTED. If the - * tuple had been moved in by the currently running - * VACUUM, the loop would have been terminated. We had - * elog(ERROR, ...) here, but as we are testing for a - * can't-happen condition, Assert() seems more - * appropriate. + * MOVED_OFF by another VACUUM would have caused the + * visibility check to set XMIN_COMMITTED or XMIN_INVALID. */ - Assert(!(tuple.t_data->t_infomask & HEAP_MOVED_IN)); + if (HeapTupleHeaderGetXvac(tuple.t_data) != myXID) + elog(ERROR, "invalid XVAC in tuple header"); /* * If this (chain) tuple is moved by me already then I * have to check is it in vacpage or not - i.e. is it * moved while cleaning this page or some previous one. */ - Assert(tuple.t_data->t_infomask & HEAP_MOVED_OFF); - - /* - * MOVED_OFF by another VACUUM would have caused the - * visibility check to set XMIN_COMMITTED or XMIN_INVALID. - */ - Assert(HeapTupleHeaderGetXvac(tuple.t_data) == myXID); /* Can't we Assert(keep_tuples > 0) here? */ if (keep_tuples == 0) continue; - if (chain_tuple_moved) /* some chains was moved while */ - { /* cleaning this page */ + if (chain_tuple_moved) + { + /* some chains were moved while cleaning this page */ Assert(vacpage->offsets_free > 0); for (i = 0; i < vacpage->offsets_free; i++) { @@ -2133,15 +2145,19 @@ repair_frag(VRelStats *vacrelstats, Relation onerel, continue; /* - * * See comments in the walk-along-page loop above, why - * we * have Asserts here instead of if (...) elog(ERROR). + * See comments in the walk-along-page loop above about + * why only MOVED_OFF tuples should be found here. */ - Assert(!(htup->t_infomask & HEAP_MOVED_IN)); - Assert(htup->t_infomask & HEAP_MOVED_OFF); - Assert(HeapTupleHeaderGetXvac(htup) == myXID); + if (htup->t_infomask & HEAP_MOVED_IN) + elog(ERROR, "HEAP_MOVED_IN was not expected"); + if (!(htup->t_infomask & HEAP_MOVED_OFF)) + elog(ERROR, "HEAP_MOVED_OFF was expected"); + if (HeapTupleHeaderGetXvac(htup) != myXID) + elog(ERROR, "invalid XVAC in tuple header"); + if (chain_tuple_moved) { - /* some chains was moved while cleaning this page */ + /* some chains were moved while cleaning this page */ Assert(vacpage->offsets_free > 0); for (i = 0; i < vacpage->offsets_free; i++) { @@ -2294,7 +2310,13 @@ repair_frag(VRelStats *vacrelstats, Relation onerel, vacrelstats->rel_tuples, keep_tuples); } - /* clean moved tuples from last page in Nvacpagelist list */ + /* + * Clean moved-off tuples from last page in Nvacpagelist list. + * + * We need only do this in this one page, because higher-numbered + * pages are going to be truncated from the relation entirely. + * But see comments for update_hint_bits(). + */ if (vacpage->blkno == (blkno - 1) && vacpage->offsets_free > 0) { @@ -2324,16 +2346,18 @@ repair_frag(VRelStats *vacrelstats, Relation onerel, continue; /* - * * See comments in the walk-along-page loop above, why - * we * have Asserts here instead of if (...) elog(ERROR). + * See comments in the walk-along-page loop above about + * why only MOVED_OFF tuples should be found here. */ - Assert(!(htup->t_infomask & HEAP_MOVED_IN)); - Assert(htup->t_infomask & HEAP_MOVED_OFF); - Assert(HeapTupleHeaderGetXvac(htup) == myXID); + if (htup->t_infomask & HEAP_MOVED_IN) + elog(ERROR, "HEAP_MOVED_IN was not expected"); + if (!(htup->t_infomask & HEAP_MOVED_OFF)) + elog(ERROR, "HEAP_MOVED_OFF was expected"); + if (HeapTupleHeaderGetXvac(htup) != myXID) + elog(ERROR, "invalid XVAC in tuple header"); itemid->lp_flags &= ~LP_USED; num_tuples++; - } Assert(vacpage->offsets_free == num_tuples); @@ -2644,20 +2668,36 @@ move_plain_tuple(Relation rel, /* * update_hint_bits() -- update hint bits in destination pages * - * Scan all the pages that we moved tuples onto and update tuple - * status bits. This is not really necessary, but will save time for - * future transactions examining these tuples. + * Scan all the pages that we moved tuples onto and update tuple status bits. + * This is normally not really necessary, but it will save time for future + * transactions examining these tuples. + * + * This pass guarantees that all HEAP_MOVED_IN tuples are marked as + * XMIN_COMMITTED, so that future tqual tests won't need to check their XVAC. + * + * BUT NOTICE that this code fails to clear HEAP_MOVED_OFF tuples from + * pages that were move source pages but not move dest pages. The bulk + * of the move source pages will be physically truncated from the relation, + * and the last page remaining in the rel will be fixed separately in + * repair_frag(), so the only cases where a MOVED_OFF tuple won't get its + * hint bits updated are tuples that are moved as part of a chain and were + * on pages that were not either move destinations nor at the end of the rel. + * To completely ensure that no MOVED_OFF tuples remain unmarked, we'd have + * to remember and revisit those pages too. * - * XXX NOTICE that this code fails to clear HEAP_MOVED_OFF tuples from - * pages that were move source pages but not move dest pages. One - * also wonders whether it wouldn't be better to skip this step and - * let the tuple status updates happen someplace that's not holding an - * exclusive lock on the relation. + * Because of this omission, VACUUM FULL FREEZE is not a safe combination; + * it's possible that the VACUUM's own XID remains exposed as something that + * tqual tests would need to check. + * + * For the non-freeze case, one wonders whether it wouldn't be better to skip + * this work entirely, and let the tuple status updates happen someplace + * that's not holding an exclusive lock on the relation. */ static void update_hint_bits(Relation rel, VacPageList fraged_pages, int num_fraged_pages, BlockNumber last_move_dest_block, int num_moved) { + TransactionId myXID = GetCurrentTransactionId(); int checked_moved = 0; int i; VacPage *curpage; @@ -2696,12 +2736,13 @@ update_hint_bits(Relation rel, VacPageList fraged_pages, int num_fraged_pages, continue; /* - * See comments in the walk-along-page loop above, why we have - * Asserts here instead of if (...) elog(ERROR). The - * difference here is that we may see MOVED_IN. + * Here we may see either MOVED_OFF or MOVED_IN tuples. */ - Assert(htup->t_infomask & HEAP_MOVED); - Assert(HeapTupleHeaderGetXvac(htup) == GetCurrentTransactionId()); + if (!(htup->t_infomask & HEAP_MOVED)) + elog(ERROR, "HEAP_MOVED_OFF/HEAP_MOVED_IN was expected"); + if (HeapTupleHeaderGetXvac(htup) != myXID) + elog(ERROR, "invalid XVAC in tuple header"); + if (htup->t_infomask & HEAP_MOVED_IN) { htup->t_infomask |= HEAP_XMIN_COMMITTED;