Re: forcing a rebuild of the visibility map

Hi,

On 2016-06-15 23:06:37 -0400, Robert Haas wrote:
> After having been scared out of my mind by the discovery of
> longstanding breakage in heap_update[1], it occurred to me that this
> is an excellent example of a case in which the option for which Andres
> was agitating - specifically forcing VACUUM to visit ever single page
> in the heap - would be useful. [...] . Being able
> to simply force every page to be visited is better. Patch for that
> attached. I decided to call the option disable_page_skipping rather
> than even_frozen_pages because it should also force visiting pages
> that are all-visible but not all-frozen.
>
> However, I also think that the approach described above - providing a
> way to excise VM forks specifically - is useful.

I think both make sense.

> From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
> From: Robert Haas <rhaas(at)postgresql(dot)org>
> Date: Wed, 15 Jun 2016 22:52:58 -0400
> Subject: [PATCH 2/2] Add VACUUM (DISABLE_PAGE_SKIPPING) for emergencies.

> Furthermore,
> + except when performing an aggressive vacuum, some pages may be skipped
> + in order to avoid waiting for other sessions to finish using them.

Isn't that still the case? We'll not wait for a cleanup lock and such,
if not necessary?

> /* non-export function prototypes */
> -static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> - Relation *Irel, int nindexes, bool aggressive);
> +static void lazy_scan_heap(Relation onerel, int options,
> + LVRelStats *vacrelstats, Relation *Irel, int nindexes,
> + bool aggressive);

Generally I think it's better to pass bitmaks arguments as an unsigned
integer. But since other related routines already use int...

> /*
> - * We request an aggressive scan if either the table's frozen Xid is now
> + * We request an aggressive scan if the table's frozen Xid is now
> * older than or equal to the requested Xid full-table scan limit; or if
> * the table's minimum MultiXactId is older than or equal to the requested
> - * mxid full-table scan limit.
> + * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
> */
> aggressive = TransactionIdPrecedesOrEquals(onerel->rd_rel->relfrozenxid,
> xidFullScanLimit);
> aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
> mxactFullScanLimit);
> + if (options & VACOPT_DISABLE_PAGE_SKIPPING)
> + aggressive = true;

I'm inclined to convert the agressive |= to an if, it looks a bit
jarring if consecutive assignments use differing forms.

> static void
> -lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> +lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
> Relation *Irel, int nindexes, bool aggressive)
> {
> BlockNumber nblocks,
> @@ -542,25 +545,28 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> * the last page. This is worth avoiding mainly because such a lock must
> * be replayed on any hot standby, where it can be disruptive.
> */
> - for (next_unskippable_block = 0;
> - next_unskippable_block < nblocks;
> - next_unskippable_block++)
> + next_unskippable_block = 0;
> + if ((options & VACOPT_DISABLE_PAGE_SKIPPING) == 0)
> {
> - uint8 vmstatus;
> -
> - vmstatus = visibilitymap_get_status(onerel, next_unskippable_block,
> - &vmbuffer);
> - if (aggressive)
> + while (next_unskippable_block < nblocks)
> {
> - if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
> - break;
> - }
> - else
> - {
> - if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
> - break;
> + uint8 vmstatus;
> +
> + vmstatus = visibilitymap_get_status(onerel, next_unskippable_block,
> + &vmbuffer);
> + if (aggressive)
> + {
> + if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0)
> + break;
> + }
> + else
> + {
> + if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0)
> + break;
> + }
> + vacuum_delay_point();
> + next_unskippable_block++;
> }
> - vacuum_delay_point();
> }
>
> if (next_unskippable_block >= SKIP_PAGES_THRESHOLD)
> @@ -594,26 +600,29 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
> if (blkno == next_unskippable_block)
> {
> /* Time to advance next_unskippable_block */
> - for (next_unskippable_block++;
> - next_unskippable_block < nblocks;
> - next_unskippable_block++)
> + next_unskippable_block++;
> + if ((options & VACOPT_DISABLE_PAGE_SKIPPING) == 0)
> {
> - uint8 vmskipflags;
> -
> - vmskipflags = visibilitymap_get_status(onerel,
> - next_unskippable_block,
> - &vmbuffer);
> - if (aggressive)
> + while (next_unskippable_block < nblocks)
> {
> - if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
> - break;
> - }
> - else
> - {
> - if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0)
> - break;
> + uint8 vmskipflags;
> +
> + vmskipflags = visibilitymap_get_status(onerel,
> + next_unskippable_block,
> + &vmbuffer);
> + if (aggressive)
> + {
> + if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0)
> + break;
> + }
> + else
> + {
> + if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0)
> + break;
> + }
> + vacuum_delay_point();
> + next_unskippable_block++;
> }
> - vacuum_delay_point();
> }

This code really makes me unhappy, everytime I see it.

> + | IDENT
> + {
> + if (strcmp($1, "disable_page_skipping") == 0)
> + $$ = VACOPT_DISABLE_PAGE_SKIPPING;
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("unrecognized VACUUM option \"%s\"", $1),
> + parser_errposition(@1)));
> + }
> ;

If we could get rid of that indentation behaviour by pgindent, I'd be
eclectic.

> /*
> + * Remove the visibility map fork for a relation. If there turn out to be
> + * any bugs in the visibility map code that require rebuilding the VM, this
> + * provides users with a way to do it that is cleaner than shutting down the
> + * server and removing files by hand.
> + *
> + * This is a cut-down version of RelationTruncate.
> + */
> +Datum
> +pg_truncate_visibility_map(PG_FUNCTION_ARGS)
> +{
> + Oid relid = PG_GETARG_OID(0);
> + Relation rel;
> +
> + rel = relation_open(relid, AccessExclusiveLock);
> +
> + if (rel->rd_rel->relkind != RELKIND_RELATION &&
> + rel->rd_rel->relkind != RELKIND_MATVIEW &&
> + rel->rd_rel->relkind != RELKIND_TOASTVALUE)
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("\"%s\" is not a table, materialized view, or TOAST table",
> + RelationGetRelationName(rel))));
> +
> + RelationOpenSmgr(rel);
> + rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
> +
> + visibilitymap_truncate(rel, 0);
> +
> + if (RelationNeedsWAL(rel))
> + {
> + xl_smgr_truncate xlrec;
> +
> + xlrec.blkno = 0;
> + xlrec.rnode = rel->rd_node;
> + xlrec.flags = SMGR_TRUNCATE_VM;
> +
> + XLogBeginInsert();
> + XLogRegisterData((char *) &xlrec, sizeof(xlrec));
> +
> + XLogInsert(RM_SMGR_ID, XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
> + }

Having an XLogInsert() in contrib makes me more than a bit squeamish. I
think it'd be fair bit better to have that section of code in
visibilitymap.c, and then call that from the extension.

> + /* Don't keep the relation locked any longer than necessary! */
> + relation_close(rel, AccessExclusiveLock);

Don't think that's a good idea. We use transactional semantics for a
good reason, and the function returns afterwards anyway.

I'm also thinking that we should perform owner-checks in these
functions. Sure, by default they're superuser only. But there's
legitimate reason to grant execute to other users, but that shouldn't
automatically mean they can do everything. That's probably done best in
a separate commit tho.

> +/* flags for xl_smgr_truncate */
> +#define SMGR_TRUNCATE_HEAP 0x0001
> +#define SMGR_TRUNCATE_VM 0x0002
> +#define SMGR_TRUNCATE_FSM 0x0004
> +#define SMGR_TRUNCATE_ALL \
> + (SMGR_TRUNCATE_HEAP|SMGR_TRUNCATE_VM|SMGR_TRUNCATE_FSM)
> +

Hm. I wonder if somebody will forget to update this the next time a new
fork is introduced...

- Andres