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
