Re: forcing a rebuild of the visibility map - Mailing list pgsql-hackers

On Fri, Jun 17, 2016 at 2:48 PM, Andres Freund <andres@anarazel.de> wrote:
>> From 010e99b403ec733d50c71a7d4ef646b1b446ef07 Mon Sep 17 00:00:00 2001
>> From: Robert Haas <rhaas@postgresql.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