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
