Re: problems with toast.* reloptions

On Sun, Aug 09, 2026 at 09:37:49AM -0400, Greg Burd wrote:
> Am I misunderstanding this? It seems to me that making autovacuum_enabled
> a ternary and then merging it means a heap table with
> autovacuum_enabled=false and some toast.* option set now stops
> autovacuuming the TOAST table.
>
> [...]
>
> So main enabled=PG_TERNARY_FALSE + toast unset -> toast enabled becomes
> PG_TERNARY_FALSE -> av_enabled is false. Today's all-or-nothing bug
> leaves that TOAST table getting vacuumed. I agree the new behavior
> matches the documented contract, but it is a behavior change for the
> person who disabled autovac on a table they vacuum by hand and never
> thought about the TOAST side. Wraparound is still forced, but ordinary
> dead-tuple bloat on the TOAST relation is now on them. So, maybe a line
> in the commit message and in the CREATE TABLE docs to make that more
> explicit would help people avoid making that mistake in practice?

Eh... I don't see much reason to worry about making relopts work how
they're documented. I mean, that's the whole point of this patch. You
could make roughly the same argument about every other reloption with a
corresponding TOAST setting. From asking around, I get the idea that
setting toast.* relopts is pretty rare, anyway.

Perhaps there's an argument for improving the docs to make this behavior a
little more apparent, but I think we can take care of that separately.

> In merge_autovac_opts() the four offset arrays keyed by "which sentinel
> means unset", is that duplicating knowledge that already lives in the
> relopt tables in reloptions.c?
>
> [...]
>
> Add an AutoVacOpts field, or change a field's default sentinel, and
> forget to update the matching array here, and the merge silently keeps
> the TOAST table's default instead of inheriting, nothing fails to compile
> and no test goes red. Can this be driven off the relopt metadata
> (relopt_parse_elt already knows each option's type and default) instead
> of the hand-maintained offset arrays?

I'm looking into this. Since this is almost certainly a master-only change
at this point, it seems reasonable to spend some more time on making this
stuff less fragile.

> On testing: the coverage doesn't touch the risky code. There's one
> injection-point case, and it's manual VACUUM only, index_cleanup/truncate
> only:
>
> +-- TOAST table inherits main table's resolved values
> +CREATE TABLE vac_tab_toast_inherit(i int, j text) WITH
> + (autovacuum_enabled=false,
> + vacuum_index_cleanup=false,
> + vacuum_truncate=false, toast.vacuum_truncate=true);
> +VACUUM vac_tab_toast_inherit;
> +DROP TABLE vac_tab_toast_inherit;
>
> Nothing exercises the autovacuum decision path, autovacuum_enabled
> inheritance, or any of the numeric AutoVacOpts that merge_autovac_opts()
> actually resolves which is precisely the code I'm worried about above.
> FWIW the pg_stat_get_autovacuum_scores() SRF that 0005 extends looks like
> it could drive a deterministic test of the autovac path (compute the
> decision without spawning a worker), which sidesteps the flakiness worry
> raised upthread.

Will add some more coverage.

> In summary, solid work and I hope it lands. Just a few small issues to
> clean up.

Thanks for reviewing.

--
nathan