Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation - Mailing list pgsql-hackers

Hi,

On 2023-01-23 19:22:18 -0800, Peter Geoghegan wrote:
> On Mon, Jan 23, 2023 at 6:56 PM Andres Freund <andres@anarazel.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