Feedback-ID: i675e48f3:Fastmail
MIME-Version: 1.0
Date: Sun, 09 Aug 2026 09:37:49 -0400
From: "Greg Burd" <greg@burd.me>
To: "Nathan Bossart" <nathandbossart@gmail.com>,
 "solai v" <solai.cdac@gmail.com>
Cc: "Nikita Malakhov" <hukutoc@gmail.com>,
 "Michael Paquier" <michael@paquier.xyz>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Message-Id: <896e1dbc-5ca1-4d9d-9f85-ae5a2ccac4f4@app.fastmail.com>
In-Reply-To: <anYM78YFDXHTA6xE@nathan>
References: <afuzQuX17qpVugnY@nathan>
 <CAF0whuenaSkNNVXhk9207Q_fR57SLYbi_dV-p9Mer8=FjM7iwg@mail.gmail.com>
 <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>
Subject: Re: problems with toast.* reloptions
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/896e1dbc-5ca1-4d9d-9f85-ae5a2ccac4f4%40app.fastmail.com>
Precedence: bulk


On Fri, Aug 7, 2026, at 12:50 PM, Nathan Bossart wrote:
> On Thu, Aug 06, 2026 at 04:38:28PM -0500, Nathan Bossart wrote:
>> Thanks for looking.  Here is a new patch set with some small fixes.
>
> Sorry for the noise.  I found some other small bugs and found another small
> cleanup opportunity.

Hey Nathan,

First, thanks for taking on this subtle and confusing aspect of heap/TOAST/reloptions interactions.  I agree with your diagnosis and your approach and I think this would be a solid step in the right direction.  I applied your v7 patch set and ran the test world, works as advertised.

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.
  
In v7-0002 the option moves from bool to ternary:
  
    --- a/src/include/utils/rel.h
    +++ b/src/include/utils/rel.h
    @@ typedef struct AutoVacOpts
    - bool    enabled;
    + pg_ternary  enabled;
  
    --- a/src/backend/postmaster/autovacuum.c
    +++ b/src/backend/postmaster/autovacuum.c
    @@ relation_needs_vacanalyze
    - av_enabled = (avopts ? avopts->enabled : true);
    + av_enabled = (avopts ? avopts->enabled != PG_TERNARY_FALSE : true);
  
and in v7-0005 the merge fills an unset TOAST value from the main table:
  
    + /* ternary fields */
    + static const int ternary_offsets[] = {
    +   offsetof(AutoVacOpts, enabled),
    + };
    ...
    + for (int i = 0; i < lengthof(ternary_offsets); i++)
    + {
    +   pg_ternary *toast_opt;
    +   pg_ternary *main_opt;
    +
    +   toast_opt = (pg_ternary *) ((char *) toast_avopts + ternary_offsets[i]);
    +   main_opt = (pg_ternary *) ((char *) main_avopts + ternary_offsets[i]);
    +
    +   if (*toast_opt == PG_TERNARY_UNSET)
    +     *toast_opt = *main_opt;
    + }
  
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?
  
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?
  
    + /* integer fields whose unset sentinel is -1 */
    + static const int int_offsets_1[] = {
    +   offsetof(AutoVacOpts, vacuum_threshold),
    +   offsetof(AutoVacOpts, vacuum_cost_limit),
    +   offsetof(AutoVacOpts, freeze_min_age),
    +   offsetof(AutoVacOpts, freeze_max_age),
    +   offsetof(AutoVacOpts, freeze_table_age),
    +   offsetof(AutoVacOpts, multixact_freeze_min_age),
    +   offsetof(AutoVacOpts, multixact_freeze_max_age),
    +   offsetof(AutoVacOpts, multixact_freeze_table_age),
    + };
    +
    + /* integer fields whose unset sentinel is -2 */
    + static const int int_offsets_2[] = {
    +   offsetof(AutoVacOpts, vacuum_max_threshold),
    +   offsetof(AutoVacOpts, vacuum_ins_threshold),
    +   offsetof(AutoVacOpts, log_vacuum_min_duration),
    + };
  
Those -1/-2 sentinels are the very defaults declared over in reloptions.c this same patch even moves one of them from -1 to -2 there:
  
    --- a/src/backend/access/common/reloptions.c
    +++ b/src/backend/access/common/reloptions.c
    @@ static relopt_int intRelOpts[] =
          RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
          ShareUpdateExclusiveLock
        },
    -   -1, -1, INT_MAX
    +   -2, -1, INT_MAX
  
so the "which sentinel means unset for field X" fact now lives in two places, kept in agreement only by the NB comment added in rel.h:
  
    + * NB: When adding a new member, be sure to update merge_autovac_opts() and/or
    + * table_recheck_autovac() as necessary!
  
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?
  
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.

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

best.

-greg


> -- 
> nathan
>
> Attachments:
> * v7-0001-Remove-extract_autovac_opts.patch
> * v7-0002-Make-autovacuum_enabled-a-ternary-reloption.patch
> * v7-0003-Add-an-unset-value-for-vacuum_index_cleanup.patch
> * v7-0004-Simplify-autovacuum-s-TOAST-to-main-relation-relo.patch
> * v7-0005-Fix-VACUUM-and-autovacuum-handling-of-TOAST-stora.patch