Date: Mon, 10 Aug 2026 11:30:17 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Greg Burd <greg@burd.me>
Cc: solai v <solai.cdac@gmail.com>, Nikita Malakhov <hukutoc@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: problems with toast.* reloptions
Message-ID: <ann8mfCgOh_MYE0T@nathan>
References: <ahWsz2D9quWBgZ7s@nathan>
 <CAF0whucUigX5c2_rAeDNKNXp-L6nJF4Td83opO1i2xBDNNPOvw@mail.gmail.com>
 <CAN-LCVOCk0hcqcu3Fw1LSVYecmQD1J26YNGuc=sybN=ds=WpPg@mail.gmail.com>
 <ah7jzDne6Ptu7fXs@nathan>
 <CAN-LCVPtuuhUyM4QJ81Ma+yU1kyA9GwgX31pGM6ix9L2zWafqQ@mail.gmail.com>
 <aihb_hMloqVDpbtw@nathan>
 <CAF0whuf=D3AncYxfuwK05_TFY+4nK8-teTYtoiu7Zp-pzVAPDQ@mail.gmail.com>
 <anT-1AlQKmXtcfrP@nathan>
 <anYM78YFDXHTA6xE@nathan>
 <896e1dbc-5ca1-4d9d-9f85-ae5a2ccac4f4@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <896e1dbc-5ca1-4d9d-9f85-ae5a2ccac4f4@app.fastmail.com>
Archived-At: <https://www.postgresql.org/message-id/ann8mfCgOh_MYE0T%40nathan>
Precedence: bulk

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