Re: forcing a rebuild of the visibility map

On Fri, Jun 17, 2016 at 2:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> 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?

That's just there to explain what behavior gets changed when you
specify DISABLE_PAGE_SKIPPING.

>> /* 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...

Many years ago, I was told by Tom Lane that project standard was int.
My gut was to use unsigned too, but I've got better things to do than
argue about it - and project style is a legitimate argument in any
case.

>> /*
>> - * 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.

I thought about that, but I like it better the way I have it.

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

That's not a defect in this patch.

>> + | 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.

Neither is that, although the indentation there looks fine to me.

>> /*
>> + * 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.

I thought about that too, but it seemed like it was just bloating the
core server for no real reason. It's not like contrib is off in space
someplace.

>> + /* 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.

Yeah, but if you want to blow away a bunch of visibility map forks at
one go, you're not going to appreciate this function collecting all of
those locks at the same time. This is also why VACUUM starts a
separate transaction for each table.

> 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.

I think it is a very dubious idea to grant access to these functions
to anyone other than the superuser, but if you want to add an owner
check, go for it!

>> +/* 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...

Gosh, I hope we go for getting rid of some of these forks instead of
adding more, but yeah, that could possibly happen. I don't think it's
a big cause for concern, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company