Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation

Hi,

On 2023-01-23 19:22:18 -0800, Peter Geoghegan wrote:
> On Mon, Jan 23, 2023 at 6:56 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Why is there TABLE_ in AUTOVACUUM_TABLE_XID_AGE but not
> > AUTOVACUUM_DEAD_TUPLES? Both are on tables.
>
> Why does vacuum_freeze_table_age contain the word "table", while
> autovacuum_vacuum_scale_factor does not?

I don't know. But that's not really a reason to introduce more oddities.

> To me, "table XID age" is a less awkward term for "relfrozenxid
> advancing", useful in contexts where it's probably more important to
> be understood by non-experts than it is to be unambiguous. Besides,
> relfrozenxid works at the level of the pg_class metadata. Nothing
> whatsoever needs to have changed about the table itself, nor will
> anything necessarily be changed by VACUUM (except the relfrozenxid
> field from pg_class).

I'd just go for "xid age", I don't see a point in adding 'table', particularly
when you don't for dead tuples.

> > What do you think about naming this VacuumTriggerType and adding an
> > VAC_TRIG_MANUAL or such?
>
> But we're not doing anything with it in the context of manual VACUUMs.

It's a member of a struct passed to the routines handling both manual and
interactive vacuum. And we could e.g. eventually start replace
IsAutoVacuumWorkerProcess() checks with it - which aren't e.g. going to work
well if we add parallel index vacuuming support to autovacuum.

> I'd prefer to keep this about what autovacuum.c thought needed to
> happen, at least for as long as manual VACUUMs are something that
> autovacuum.c knows nothing about.

It's an enum defined in a general header, not something in autovacuum.c - so I
don't really buy this.

> > > - bool force_vacuum;
> > > + TransactionId relfrozenxid = classForm->relfrozenxid;
> > > + MultiXactId relminmxid = classForm->relminmxid;
> > > + AutoVacType trigger = AUTOVACUUM_NONE;
> > > + bool tableagevac;
> >
> > Here + below we end up with three booleans that just represent the choices in
> > our fancy new enum. That seems awkward to me.
>
> I don't follow. It's true that "wraparound" is still a synonym of
> "tableagevac" in 0001, but that changes in 0002. And even if you
> assume that 0002 won't get in, I think that it still makes sense to
> structure it in a way that shows that in principle the "wraparound"
> behaviors don't necessarily have to be used whenever "tableagevac" is
> in use.

You have booleans tableagevac, deadtupvac, inserttupvac. Particularly the
latter ones really are just a rephrasing of the trigger:

+ tableagevac = true;
+ *wraparound = false;
+ /* See header comments about trigger precedence */
+ if (TransactionIdIsNormal(relfrozenxid) &&
+ TransactionIdPrecedes(relfrozenxid, xidForceLimit))
+ trigger = AUTOVACUUM_TABLE_XID_AGE;
+ else if (MultiXactIdIsValid(relminmxid) &&
+ MultiXactIdPrecedes(relminmxid, multiForceLimit))
+ trigger = AUTOVACUUM_TABLE_MXID_AGE;
+ else
+ tableagevac = false;
+
+ /* User disabled non-table-age autovacuums in pg_class.reloptions? */
+ if (!av_enabled && !tableagevac)

...

+ deadtupvac = (vactuples > vacthresh);
+ inserttupvac = (vac_ins_base_thresh >= 0 && instuples > vacinsthresh);
+ /* See header comments about trigger precedence */
+ if (!tableagevac)
+ {
+ if (deadtupvac)
+ trigger = AUTOVACUUM_DEAD_TUPLES;
+ else if (inserttupvac)
+ trigger = AUTOVACUUM_INSERTED_TUPLES;
+ }
+
/* Determine if this table needs vacuum or analyze. */
- *dovacuum = force_vacuum || (vactuples > vacthresh) ||
- (vac_ins_base_thresh >= 0 && instuples > vacinsthresh);
+ *dovacuum = (tableagevac || deadtupvac || inserttupvac);

I find this to be awkward code. The booleans are kinda pointless, and the
tableagevac case is hard to follow because trigger is set elsewhere.

I can give reformulating it a go. Need to make some food first.

I suspect that the code would look better if we didn't continue to have
"bool *dovacuum" and the trigger. They're redundant.

> Anything else for 0001? Would be nice to get it committed tomorrow.

Sorry, today was busy with meetings and bashing my head against AIX.

Greetings,

Andres Freund