problems with toast.* reloptions

While investigating problems caused by vacuum_rel() scribbling on its
VacuumParams argument [0], I noticed some other interesting bugs with the
toast.* reloption code. Note that the documentation for the reloptions has
the following line:

If a table parameter value is set and the equivalent toast. parameter
is not, the TOAST table will use the table's parameter value.

The problems I found are as follows:

* vacuum_rel() does not look up the main relation's reloptions when
processing a TOAST table, which is a problem for manual VACUUMs. The
aforementioned bug [0] causes you to sometimes get the expected behavior
(because the parameters are overridden before recursing to TOAST), but
fixing that bug makes that accidental behavior go away.

* For autovacuum, the main table's reloptions are only used if the TOAST
table has no reloptions set. So, if your relation has
autovacuum_vacuum_threshold and toast.vacuum_index_cleanup set, the main
relation's autovacuum_vacuum_threshold setting won't be used for the
TOAST table.

* Even when the preceding point doesn't apply, autovacuum doesn't use the
main relation's setting for some parameters (e.g., vacuum_truncate).
Instead, it leaves them uninitialized and expects vacuum_rel() to fill
them in. This is a problem because, as mentioned earlier, vacuum_rel()
doesn't consult the main relation's reloptions either.

I think we need to do something like the following to fix this:

* Teach autovacuum to combine the TOAST reloptions with the main relation's
when processing TOAST tables (with the toast.* ones winning if both are
set).

* Teach autovacuum to resolve reloptions for parameters like
vacuum_truncate instead of relying on vacuum_rel() to fill it in.

* Have vacuum_rel() send the main relation's reloptions when recursing to
the TOAST table so that we can combine them there, too.

This doesn't fix VACUUM against a TOAST table directly (e.g., VACUUM
pg_toast.pg_toast_5432), but that might not be too important because
(PROCESS_TOAST TRUE) is the main supported way to vacuum a TOAST table. If
we did want to fix that, though, I think we'd have to teach vacuum_rel() or
the relcache to look up the reloptions for the main relation.

Thoughts?

[0] https://postgr.es/m/flat/CAGRkXqTo%2BaK%3DGTy5pSc-9cy8H2F2TJvcrZ-zXEiNJj93np1UUw%40mail.gmail.com

--
nathan

On Thu, Jun 19, 2025 at 03:20:27PM -0500, Nathan Bossart wrote:
> While investigating problems caused by vacuum_rel() scribbling on its
> VacuumParams argument [0], I noticed some other interesting bugs with the
> toast.* reloption code. Note that the documentation for the reloptions has
> the following line:
>
> If a table parameter value is set and the equivalent toast. parameter
> is not, the TOAST table will use the table's parameter value.
>
> The problems I found are as follows:
>
> * vacuum_rel() does not look up the main relation's reloptions when
> processing a TOAST table, which is a problem for manual VACUUMs. The
> aforementioned bug [0] causes you to sometimes get the expected behavior
> (because the parameters are overridden before recursing to TOAST), but
> fixing that bug makes that accidental behavior go away.

Are you referring to the case of a VACUUM pg_toast.pg_toast_NNN? I'm
not sure that we really need to care about looking up at the parent
relation in this case. It sounds to me that the intention of this
paragraph is for the case where the TOAST table is treated as a
secondary table, not when the TOAST table is directly vacuumed.
Perhaps the wording of the docs should be improved that this does not
happen if vacuuming directly a TOAST table.

> * For autovacuum, the main table's reloptions are only used if the TOAST
> table has no reloptions set. So, if your relation has
> autovacuum_vacuum_threshold and toast.vacuum_index_cleanup set, the main
> relation's autovacuum_vacuum_threshold setting won't be used for the
> TOAST table.

[.. /me double-checks the code .. ]

So we combine the options in do_autovacuum() with the two-pass logic
to gather the relation OIDs, then apply relation_needs_vacanalyze().
That looks like an old issue, that cannot be solved as long as we rely
on the relopts to be an all-or-nothing thing when assigning the
individual values for the TOAST relation. Oops.

> * Even when the preceding point doesn't apply, autovacuum doesn't use the
> main relation's setting for some parameters (e.g., vacuum_truncate).
> Instead, it leaves them uninitialized and expects vacuum_rel() to fill
> them in. This is a problem because, as mentioned earlier, vacuum_rel()
> doesn't consult the main relation's reloptions either.

Relying on vacuum_rel() sounds like a bad idea if we can avoid that,
still perhaps that's OK as long as we don't use a pointer to the
VacuumParams and keep the updates to the value of vacuum_rel() local
inside the routine.

> I think we need to do something like the following to fix this:
>
> * Teach autovacuum to combine the TOAST reloptions with the main relation's
> when processing TOAST tables (with the toast.* ones winning if both are
> set).
>
> * Teach autovacuum to resolve reloptions for parameters like
> vacuum_truncate instead of relying on vacuum_rel() to fill it in.

These two points make sense here, yes.

> * Have vacuum_rel() send the main relation's reloptions when recursing to
> the TOAST table so that we can combine them there, too.

For the case of a manual VACUUM on the main table, where the TOAST
table is treated as a secondary citizen, that makes sense as well,
yes.

> This doesn't fix VACUUM against a TOAST table directly (e.g., VACUUM
> pg_toast.pg_toast_5432), but that might not be too important because
> (PROCESS_TOAST TRUE) is the main supported way to vacuum a TOAST table. If
> we did want to fix that, though, I think we'd have to teach vacuum_rel() or
> the relcache to look up the reloptions for the main relation.

This one does not sound that important to me for the case of manual
VACUUM case directly done on a TOAST table. If you do that, the code
kind of assumes that a TOAST table is actually a "main" relation that
has no TOAST table. That should keep the code simpler, because we
would not need to look at what the parent relation holds when deciding
which options to use in ExecVacuum(). The autovacuum case is
different, as TOAST relations are worked on as their own items rather
than being secondary relations of the main tables.
--
Michael

On Fri, Jun 20, 2025 at 11:05:37AM +0900, Michael Paquier wrote:
> On Thu, Jun 19, 2025 at 03:20:27PM -0500, Nathan Bossart wrote:
>> * vacuum_rel() does not look up the main relation's reloptions when
>> processing a TOAST table, which is a problem for manual VACUUMs. The
>> aforementioned bug [0] causes you to sometimes get the expected behavior
>> (because the parameters are overridden before recursing to TOAST), but
>> fixing that bug makes that accidental behavior go away.
>
> Are you referring to the case of a VACUUM pg_toast.pg_toast_NNN? I'm
> not sure that we really need to care about looking up at the parent
> relation in this case. It sounds to me that the intention of this
> paragraph is for the case where the TOAST table is treated as a
> secondary table, not when the TOAST table is directly vacuumed.
> Perhaps the wording of the docs should be improved that this does not
> happen if vacuuming directly a TOAST table.

Yeah, I was mainly thinking of a VACUUM command that recurses to the TOAST
table. Of course, it'd be nice to fix VACUUM pg_toast.pg_toast_NNN, too,
but I'm personally not too worried about that use-case.

>> This doesn't fix VACUUM against a TOAST table directly (e.g., VACUUM
>> pg_toast.pg_toast_5432), but that might not be too important because
>> (PROCESS_TOAST TRUE) is the main supported way to vacuum a TOAST table. If
>> we did want to fix that, though, I think we'd have to teach vacuum_rel() or
>> the relcache to look up the reloptions for the main relation.
>
> This one does not sound that important to me for the case of manual
> VACUUM case directly done on a TOAST table. If you do that, the code
> kind of assumes that a TOAST table is actually a "main" relation that
> has no TOAST table. That should keep the code simpler, because we
> would not need to look at what the parent relation holds when deciding
> which options to use in ExecVacuum(). The autovacuum case is
> different, as TOAST relations are worked on as their own items rather
> than being secondary relations of the main tables.

+1

--
nathan

> I think we need to do something like the following to fix this:
>
> * Teach autovacuum to combine the TOAST reloptions with the main relation's
> when processing TOAST tables (with the toast.* ones winning if both are
> set).
>
> * Teach autovacuum to resolve reloptions for parameters like
> vacuum_truncate instead of relying on vacuum_rel() to fill it in.

>> These two points make sense here, yes.

I investigated that this afternoon and identified two potential
implementation approaches:

1) Create functions like resolve_toast_vac_opts() and
resolve_toast_rel_opts(). These would then be used in
table_recheck_autovac(), NeedsAutoVacTableForXidWraparound(), and
do_autovacuum() after the toast table check.
2) When updating a table's relopt, also update the relopt of its
associated TOAST table if it's not already set. Similarly, when
creating a new TOAST table, it would inherit the parent's relopt.

Option 2 seems more reasonable to me, as it avoids requiring customers
to manually resolve these options, when they have different settings
for the parent and TOAST tables."

On Sat, Jun 21, 2025 at 11:45:25PM -0400, shihao zhong wrote:
> 2) When updating a table's relopt, also update the relopt of its
> associated TOAST table if it's not already set. Similarly, when
> creating a new TOAST table, it would inherit the parent's relopt.
>
> Option 2 seems more reasonable to me, as it avoids requiring customers
> to manually resolve these options, when they have different settings
> for the parent and TOAST tables."

I like this one, but since it won't fix existing clusters, it might only be
workable for v19.

--
nathan

On Fri, Jun 20, 2025 at 11:05:37AM +0900, Michael Paquier wrote:
> On Thu, Jun 19, 2025 at 03:20:27PM -0500, Nathan Bossart wrote:
>> I think we need to do something like the following to fix this:
>>
>> * Teach autovacuum to combine the TOAST reloptions with the main relation's
>> when processing TOAST tables (with the toast.* ones winning if both are
>> set).
>>
>> * Teach autovacuum to resolve reloptions for parameters like
>> vacuum_truncate instead of relying on vacuum_rel() to fill it in.
>
> These two points make sense here, yes.
>
>> * Have vacuum_rel() send the main relation's reloptions when recursing to
>> the TOAST table so that we can combine them there, too.
>
> For the case of a manual VACUUM on the main table, where the TOAST
> table is treated as a secondary citizen, that makes sense as well,
> yes.

Here is a very rough proof-of-concept patch set for this. AFAICT there are
a few options we cannot fix on the back-branches because there is no way to
tell whether it is set or has just picked up the default. On v18 and
newer, we could use isset_offset, but that doesn't exist on older versions.
(I haven't looked closely, but I'm assuming that back-patching isset_offset
isn't an option.)

I would like to explore the "option 2" from upthread [0] for v19. I think
that is a better long-term solution, and it may allow us to remove the
table_toast_map in autovacuum.

[0] https://postgr.es/m/aFl598epAdUrrv0y%40nathan

--
nathan

On Mon, Jun 23, 2025 at 03:59:56PM -0500, Nathan Bossart wrote:
> Here is a very rough proof-of-concept patch set for this. AFAICT there are
> a few options we cannot fix on the back-branches because there is no way to
> tell whether it is set or has just picked up the default. On v18 and
> newer, we could use isset_offset, but that doesn't exist on older versions.
> (I haven't looked closely, but I'm assuming that back-patching isset_offset
> isn't an option.)

Hmm. I am wondering if we need to be aggressive about this set of
changes at all in the back branches. It's been broken for a long time
without anybody really complaining about the fact that reloptions
being set or not influenced the outcome in the context of autovacuum,
so perhaps there is a good argument for keeping all that in v19. My
conservative 2c.

> I would like to explore the "option 2" from upthread [0] for v19. I think
> that is a better long-term solution, and it may allow us to remove the
> table_toast_map in autovacuum.
>
> [0] https://postgr.es/m/aFl598epAdUrrv0y%40nathan

It would be nice to have some tests here to check the state of the
options used? My best guess would be a DEBUG1 entry combined with a
scan of the logs generated and an aggressive autovacuum worker
spawn to check that the options generated are what we expect for the
relations autovacuum picks up.
--
Michael

On Tue, Jun 24, 2025 at 02:10:55PM +0900, Michael Paquier wrote:
> On Mon, Jun 23, 2025 at 03:59:56PM -0500, Nathan Bossart wrote:
>> Here is a very rough proof-of-concept patch set for this. AFAICT there are
>> a few options we cannot fix on the back-branches because there is no way to
>> tell whether it is set or has just picked up the default. On v18 and
>> newer, we could use isset_offset, but that doesn't exist on older versions.
>> (I haven't looked closely, but I'm assuming that back-patching isset_offset
>> isn't an option.)
>
> Hmm. I am wondering if we need to be aggressive about this set of
> changes at all in the back branches. It's been broken for a long time
> without anybody really complaining about the fact that reloptions
> being set or not influenced the outcome in the context of autovacuum,
> so perhaps there is a good argument for keeping all that in v19. My
> conservative 2c.

Yeah, I'm tempted to even ask how folks feel about removing the toast.*
reloptions. Maybe there's some simple cases that work well enough, but
AFAICT any moderately-complicated setup basically doesn't work at all. In
any case, writing out this patch set has got me on the fix-on-HEAD-only
bandwagon.

>> I would like to explore the "option 2" from upthread [0] for v19. I think
>> that is a better long-term solution, and it may allow us to remove the
>> table_toast_map in autovacuum.
>
> It would be nice to have some tests here to check the state of the
> options used? My best guess would be a DEBUG1 entry combined with a
> scan of the logs generated and an aggressive autovacuum worker
> spawn to check that the options generated are what we expect for the
> relations autovacuum picks up.

Eh... I agree that's probably how we'd have to test it with the existing
tools, but it sure sounds like a recipe for a flaky test.

--
nathan

On Mon, Jun 23, 2025 at 10:59:51AM -0500, Nathan Bossart wrote:
> On Sat, Jun 21, 2025 at 11:45:25PM -0400, shihao zhong wrote:
>> 2) When updating a table's relopt, also update the relopt of its
>> associated TOAST table if it's not already set. Similarly, when
>> creating a new TOAST table, it would inherit the parent's relopt.
>>
>> Option 2 seems more reasonable to me, as it avoids requiring customers
>> to manually resolve these options, when they have different settings
>> for the parent and TOAST tables."
>
> I like this one, but since it won't fix existing clusters, it might only be
> workable for v19.

Actually, I think there's a problem with this approach. If we set the
reloption for both the main relation and the TOAST table, then we won't
know what to do for RESET. Take the following examples:

ALTER TABLE test SET (vacuum_truncate = false);
ALTER TABLE test RESET (vacuum_truncate);

ALTER TABLE test SET (vacuum_truncate = false);
ALTER TABLE test SET (toast.vacuum_truncate = false);
ALTER TABLE test RESET (vacuum_truncate);

After executing the commands in the first stanza, you'd expect the
vacuum_truncate reloption to be unset for both the main relation and its
TOAST table. After the second one, you'd expect it to be set for only the
TOAST table. But unless there's some way to know the source of the TOAST
table's reloption, we can't know which behavior is correct at RESET time.

--
nathan

>> Actually, I think there's a problem with this approach...

You're right. I forgot we can reset the table options. While we could
use a placeholder and resolve it on-demand, that seems like too much
work.

On Wed, Jul 2, 2025 at 10:44 AM shihao zhong <zhong950419(at)gmail(dot)com> wrote:

> >> Actually, I think there's a problem with this approach...
>
> You're right. I forgot we can reset the table options. While we could
> use a placeholder and resolve it on-demand, that seems like too much
> work.
>
>
Hi all,

I started a conversation about TOAST table vacuum truncation parameter
inheritance [1] and was pointed to this thread which I totally missed. I
recently came across the issue where `vacuum_truncate` on TOAST tables
wasn't being inherited even though the parent table had the setting, and
the documentation mentioned that it should work [2]. I see that there are a
lot of good conversations here already.

I'm curious to hear what folks think about the approach where we implement
the inheritance logic directly in `vacuum_rel()` when `params.truncate ==
VACOPTVALUE_UNSPECIFIED`. This should address the point raised upthread
about `vacuum_rel()` not looking up the main relation's reloptions when
processing a TOAST and also what I discovered in [1]. For instance it
could be something like:

1. For TOAST tables: When no explicit `toast.vacuum_truncate` is set, scan
`pg_class.reltoastrelid` to find the parent table and inherit its
`vacuum_truncate` setting.
2. For manual VACUUM: Pass the final truncate decision from main table to
TOAST table in the `toast_vacuum_params`
3. For autovacuum: The inheritance happens naturally since autovacuum calls
`vacuum_rel()` for each relation independently

This basically teaches `vacuum_rel()` to consult the main relation's
reloptions for TOAST tables, which was the core issue identified upthread.
It handles both manual VACUUM and autovacuum scenarios in one place,
without changing function signatures (which helps with potential
backporting).

The execution-layer fix should also help us avoid the parameter
contamination issues that were fixed in commit 661643dedad9 (I believe?),
since we're doing the lookup fresh each time `vacuum_rel()` is called on a
TOAST table.

I realize backporting might not be preferred given this issue has existed
for a long time. However, that is precisely why I feel like it might be
worth patching it because, at least per the docs, my understanding was that
if `vacuum_truncate` is set on the parent table then it applies to the
TOAST as well and I wonder if there are others in the same boat as well.

Would something like this focused approach (just teaching `vacuum_rel()` to
look up parent reloptions for TOAST tables) could be a reasonable fix that
complements the broader reloptions work discussed upthread for v19 and also
backportable to v13 onwards?

I am happy to help with this and split out the changes as it makes sense as
well.

Thanks
Shayon

[1]
https://www.postgresql.org/message-id/CANqtF-pLHBDdNM-DifXQqVq%2BVWA3DTYOx4%2B3m2%2BTFJ1zDOZETg%40mail.gmail.com

[2]
https://www.postgresql.org/docs/current/sql-createtable.html#RELOPTION-VACUUM-TRUNCATE

On Tue, Jun 24, 2025 at 11:38:41AM -0500, Nathan Bossart wrote:
> Yeah, I'm tempted to even ask how folks feel about removing the toast.*
> reloptions. Maybe there's some simple cases that work well enough, but
> AFAICT any moderately-complicated setup basically doesn't work at all. In
> any case, writing out this patch set has got me on the fix-on-HEAD-only
> bandwagon.

For the sake of discussion, I wrote a patch for this. I suggested removing
toast.* relopts to some colleagues recently, and nobody was aware of much
(if any) use in the field. So maybe this isn't totally
out-of-the-question...

--
nathan

On Fri, May 01, 2026 at 04:41:12PM -0500, Nathan Bossart wrote:
> On Tue, Jun 24, 2025 at 11:38:41AM -0500, Nathan Bossart wrote:
>> Yeah, I'm tempted to even ask how folks feel about removing the toast.*
>> reloptions. Maybe there's some simple cases that work well enough, but
>> AFAICT any moderately-complicated setup basically doesn't work at all. In
>> any case, writing out this patch set has got me on the fix-on-HEAD-only
>> bandwagon.
>
> For the sake of discussion, I wrote a patch for this. I suggested removing
> toast.* relopts to some colleagues recently, and nobody was aware of much
> (if any) use in the field. So maybe this isn't totally
> out-of-the-question...

Apparently I forgot to test it with injection points enabled. Here's a new
patch.

--
nathan

On Mon, May 04, 2026 at 10:41:15AM -0500, Nathan Bossart wrote:
> Apparently I forgot to test it with injection points enabled. Here's a new
> patch.

v4 fixes a small bug in pg_upgrade's new check query.

--
nathan

Hi,

I tested the TOAST reloptions behavior locally on current HEAD and was
able to reproduce the inheritance and RESET scenarios discussed in the
thread.
From my testing, explicit toast.reloptions behave independently from
parent reloptions. After setting both parent and TOAST vacuum_truncate
values, RESET(vacuum_truncate) on the parent table cleared only the
parent reloption while the explicit TOAST reloption remained intact.
I also tried applying the v1-0004 patch in a clean worktree, but most
hunks in vacuum.c and autovacuum.c no longer apply cleanly against
current HEAD due to code drift. From inspecting the rejected hunks, it
looks like the patch approach was to dynamically combine parent and
TOAST reloptions during VACUUM/autovacuum execution instead of copying
inherited reloptions directly into the TOAST relation.

Regards,
solai

On Tue, May 26, 2026 at 11:25:39AM +0530, solai v wrote:
> I tested the TOAST reloptions behavior locally on current HEAD and was
> able to reproduce the inheritance and RESET scenarios discussed in the
> thread.
> From my testing, explicit toast.reloptions behave independently from
> parent reloptions. After setting both parent and TOAST vacuum_truncate
> values, RESET(vacuum_truncate) on the parent table cleared only the
> parent reloption while the explicit TOAST reloption remained intact.
> I also tried applying the v1-0004 patch in a clean worktree, but most
> hunks in vacuum.c and autovacuum.c no longer apply cleanly against
> current HEAD due to code drift. From inspecting the rejected hunks, it
> looks like the patch approach was to dynamically combine parent and
> TOAST reloptions during VACUUM/autovacuum execution instead of copying
> inherited reloptions directly into the TOAST relation.

I humbly encourage you to read the rest of the thread. In particular, I'm
curious whether anyone would object to removing the TOAST reloptions.

--
nathan

Hi,
> I humbly encourage you to read the rest of the thread. In particular, I'm
> curious whether anyone would object to removing the TOAST reloptions.
I tested the remove-toast-reloptions.patch on current HEAD.
The patch applied and built cleanly .I verified that TOAST reloptions
are no longer accepted:
ALTER TABLE toat_test SET (toast.vacuum_truncate=false):
ALTER TABLE toast_test SET(toast.autovacuum_enabled = false);
Both returned:
ERROR:unrecognized parameter namespace "toast"
Regular reloptions still work as expected:
ALTER TABLE toast_test SET (vacuum_truncate=false);
The patch behaved as expected in my testing

Regards
solai

Hi hackers!

I strongly object to removing TOAST reloptions.
As anyone here knows, TOAST tables are subject to bloating, and in my
opinion
we'll eventually come to multi TOAST tables per relation.
I've tested such an approach some time ago, it does not create any problems
and provides additional possibilities like ability to vacuum separate TOAST
only partly influencing overall work with parent relation.

On Tue, Jun 2, 2026 at 12:42 PM solai v <solai(dot)cdac(at)gmail(dot)com> wrote:

> Hi,
> > I humbly encourage you to read the rest of the thread. In particular,
> I'm
> > curious whether anyone would object to removing the TOAST reloptions.
> I tested the remove-toast-reloptions.patch on current HEAD.
> The patch applied and built cleanly .I verified that TOAST reloptions
> are no longer accepted:
> ALTER TABLE toat_test SET (toast.vacuum_truncate=false):
> ALTER TABLE toast_test SET(toast.autovacuum_enabled = false);
> Both returned:
> ERROR:unrecognized parameter namespace "toast"
> Regular reloptions still work as expected:
> ALTER TABLE toast_test SET (vacuum_truncate=false);
> The patch behaved as expected in my testing
>
> Regards
> solai
>
>
>

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

On Tue, Jun 02, 2026 at 03:31:40PM +0300, Nikita Malakhov wrote:
> I strongly object to removing TOAST reloptions.

FWIW I am working on a patch to actually fix the bugs instead of removing
the TOAST relopts. Will hopefully have something to share in the near
future.

--
nathan

Hi,

Nathan, sorry if there is any misunderstanding, I answered Solai V
against removing TOAST reloptions. Glad you're working on this subject.
Cheers!

On Tue, Jun 2, 2026 at 5:08 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:

> On Tue, Jun 02, 2026 at 03:31:40PM +0300, Nikita Malakhov wrote:
> > I strongly object to removing TOAST reloptions.
>
> FWIW I am working on a patch to actually fix the bugs instead of removing
> the TOAST relopts. Will hopefully have something to share in the near
> future.
>
> --
> nathan
>

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

Okay, here is a new patch set that aims to actually fix the issues, not
just remove the TOAST reloptions. I followed roughly the approach I
originally suggested in my first post: autovacuum merges the relopts, and
VACUUM passes them to the TOAST table when recursing. As previously
mentioned, vacuuming a TOAST table directly isn't fixed, but I think that's
okay. Our main supported way to VACUUM a TOAST table is to use "VACUUM
(PROCESS_MAIN false) main_table".

Something else this patch makes worse is that we remain oblivious to
concurrent storage parameter changes on the main table. That is, if
someone changes a relopt during a long-running vacuum on the main table,
we might use a stale relopt value when we process the TOAST table. To fix
that, I suspect we'd need to do more lookups, which I was hoping to avoid.
But this doesn't seem like a pressing issue, and AFAICT this stuff has been
broken for a very long time, so IMHO it's not worth the additional effort.

--
nathan

Hi,

I tested the v5 patch series on the current HEAD.
The patches applied and built successfully. I verified that TOAST
reloptions are still accepted and stored correctly. I also tested
parent and TOAST reloption combinations, RESET behavior, and VACUUM
processing.
For example, with:
ALTER TABLE toast_test2 SET (vacuum_truncate = false);
ALTER TABLE toast_test2 SET (toast.vacuum_truncate = true);
resetting the parent reloption preserved the explicit TOAST reloption
as expected.
I also verified that VACUUM (PROCESS_MAIN false) correctly processes
only the associated TOAST table.
I did not observe any issues during testing.

Regards,
solai

On Wed, Jun 10, 2026 at 05:11:36PM +0530, solai v wrote:
> I did not observe any issues during testing.

Thanks for looking. Here is a new patch set with some small fixes.

--
nathan

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.

--
nathan

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

Hi,

I tested the v7 patch series on the current HEAD. The patches applied
cleanly, and both the build and make check-world passed.
I also tested the TOAST and autovacuum reloptions, including
vacuum_truncate, autovacuum_enabled, and the numeric autovacuum
options. The parent/TOAST settings and RESET behavior worked as
expected.
VACUUM (VERBOSE) also completed successfully for both the main and TOAST tables.

Overall, the patch worked as expected in my testing.
Regards,
solai

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

Hi,

I looked these patches while looking at potential cases for injection point
conditional strings [0], which could allow for better autovacuum testing,
particularly for the case being discussed here.

As far as the patches go, No comments on 0001-0004, just two nits on 0005.

1/ Instead of setting toast_vacuum_params.* inside each branch, it reads
cleaner to set toast_vacuum_params.* after params.* is done, and this
way you only set toast_vacuum_params.*once. For example, in
index_cleanup:

@@ -2223,15 +2223,12 @@ vacuum_rel(Oid relid, RangeVar *relation,
VacuumParams params,
case STDRD_OPTION_VACUUM_INDEX_CLEANUP_ON:
params.index_cleanup = VACOPTVALUE_ENABLED;
- toast_vacuum_params.main_index_cleanup = VACOPTVALUE_ENABLED;
break;
...
}
+
+ /* A TOAST table inherits the main relation's value. */
+ toast_vacuum_params.main_index_cleanup = params.index_cleanup;
}

The same applies to main_max_eager_freeze_failure_rate and main_truncate.

2/ A couple of comment tweaks in merge_autovac_opts()

```
@@ -3791,8 +3791,11 @@ pg_stat_get_autovacuum_scores(PG_FUNCTION_ARGS)
* The arrays below cover only the options that can be set on a TOAST table,
* grouped by the value that means "unset". Both of those facts come from
* reloptions.c, so they must be kept in sync with the entries here.
+ * Analyze options are intentionally excluded because TOAST tables are never
+ * analyzed, and autovacuum_parallel_workers is excluded because it can't be
+ * set on a TOAST table.
*
- * NB: This function destructively modifies toast_opts!
+ * NB: This modifies toast_opts in place.
*/
static StdRdOptions *
merge_autovac_opts(StdRdOptions *toast_opts, StdRdOptions *main_opts)
@@ -3821,7 +3824,7 @@ merge_autovac_opts(StdRdOptions *toast_opts,
StdRdOptions *main_opts)
offsetof(AutoVacOpts, log_vacuum_min_duration),
};

- /* float fields */
+ /* float fields whose unset sentinel is -1.0 */
static const int float_offsets[] = {
offsetof(AutoVacOpts, vacuum_cost_delay),
offsetof(AutoVacOpts, vacuum_scale_factor),
```

[0] https://www.postgresql.org/message-id/CAA5RZ0sfRTST8RQaoVXfEnVRchRtAP_YhFFUVcm6dC6v1TAChw%40mail.gmail.com

--
Sami Imseih
Amazon Web Services (AWS)

On Mon, Aug 10, 2026 at 11:30:17AM -0500, Nathan Bossart wrote:
> On Sun, Aug 09, 2026 at 09:37:49AM -0400, Greg Burd wrote:
>> 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.

Here is a new patch set with an attempt at the above. The new 0005
contains the log_autovacuum_min_duration default change along with an
assertion that all TOAST storage parameters have unsettable defaults. 0006
is just some prerequisite refactoring for 0007. And 0007 is the fix with
the merge done based on the main reloption list instead of new arrays.

>> 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.

Done.

--
nathan

On Tue, Aug 11, 2026 at 09:53:53AM -0500, Sami Imseih wrote:
> I looked these patches while looking at potential cases for injection point
> conditional strings [0], which could allow for better autovacuum testing,
> particularly for the case being discussed here.

Thanks for reviewing.

> 1/ Instead of setting toast_vacuum_params.* inside each branch, it reads
> cleaner to set toast_vacuum_params.* after params.* is done, and this
> way you only set toast_vacuum_params.*once. For example, in
> index_cleanup:

Done in v8 [0].

> 2/ A couple of comment tweaks in merge_autovac_opts()

I don't think these changes apply to v8, except for perhaps the "NB" one,
which I expanded a bit.

[0] https://postgr.es/m/anuXd35G8WLTUaBR%40nathan

--
nathan

On Tue, Aug 11, 2026 at 04:43:19PM -0500, Nathan Bossart wrote:
> Here is a new patch set with an attempt at the above. The new 0005
> contains the log_autovacuum_min_duration default change along with an
> assertion that all TOAST storage parameters have unsettable defaults. 0006
> is just some prerequisite refactoring for 0007. And 0007 is the fix with
> the merge done based on the main reloption list instead of new arrays.

After sleeping on this, I realized that we can simplify things a bit by
using merge_toast_reloptions() for manual VACUUM, too. I've added a new
0007 for that. The autovacuum fix now lives in 0008.

--
nathan

> After sleeping on this, I realized that we can simplify things a bit by
> using merge_toast_reloptions() for manual VACUUM, too. I've added a new
> 0007 for that. The autovacuum fix now lives in 0008.

Makes sense to me. A few comments:

1/ 0007 adds merge_toast_reloptions(), and for each option it calls
find_reloption() to get the type and default. find_reloption() scans the entire
relOpts[]. The options don't change, so it's just repeating the same work
every time, which also includes strcmp to match the options by name.

find_reloption() already runs initialize_reloptions(), so could
initialize_reloptions()
also build a small table of the options a toast table inherits from
its main table.
merge_toast_reloptions() then just iterates that table and no longer needs to
call find_reloption() in the scan.

This would help most when pg_stat_autovacuum_scores calls
merge_toast_reloptions()
for every toast table in its list, which occurs in 0008.

2/ There are three spots that repeat the work for a toast table,
extractRelOptions() followed by a table_toast_map lookup
and a merge_toast_reloptions().
For example, when do_autovacuum() performs the pass on the toast
tables:

```
/*
- * fetch reloptions -- if this toast table does not
have them, try the
- * main rel
+ * fetch reloptions -- merge any unset options from the main rel
*/
relopts = (StdRdOptions *) extractRelOptions(tuple,
pg_class_desc, NULL);
if (relopts)
free_relopts = true;
- else
- {
- av_relation *hentry;
- bool found;
-
- hentry = hash_search(table_toast_map, &relid,
HASH_FIND, &found);
- if (found)
- relopts = &hentry->ar_reloptions;
- }
+ hentry = hash_search(table_toast_map, &relid,
HASH_FIND, &found);
+ if (found)
+ relopts = merge_toast_reloptions(relopts,
&hentry->ar_reloptions);
```

as well as table_recheck_autovac() and pg_stat_get_autovacuum_scores().

The job here is to find the "effective" toast options, and it might be
worthwhile
to turn this into a single helper for clarity.

--
Sami Imseih
Amazon Web Services (AWS)

On Wed, Aug 12, 2026 at 01:33:20PM -0500, Sami Imseih wrote:
> 1/ 0007 adds merge_toast_reloptions(), and for each option it calls
> find_reloption() to get the type and default. find_reloption() scans the
> entire relOpts[]. The options don't change, so it's just repeating the
> same work every time, which also includes strcmp to match the options by
> name.
>
> find_reloption() already runs initialize_reloptions(), so could
> initialize_reloptions() also build a small table of the options a toast
> table inherits from its main table. merge_toast_reloptions() then just
> iterates that table and no longer needs to call find_reloption() in the
> scan.
>
> This would help most when pg_stat_autovacuum_scores calls
> merge_toast_reloptions() for every toast table in its list, which occurs
> in 0008.

I considered this, but AFAICT that extra work actually pales in comparison
to the sequential scan of pg_class.

> 2/ There are three spots that repeat the work for a toast table,
> extractRelOptions() followed by a table_toast_map lookup and a
> merge_toast_reloptions(). For example, when do_autovacuum() performs the
> pass on the toast tables:
>
> [...]
>
> as well as table_recheck_autovac() and pg_stat_get_autovacuum_scores().
>
> The job here is to find the "effective" toast options, and it might be
> worthwhile to turn this into a single helper for clarity.

Done in v10.

--
nathan

Here's a v11 in which I've tried to fix the compiler warning that cfbot is
complaining about. No other changes.

--
nathan

On Wed, Aug 12, 2026 at 07:41:51PM -0500, Nathan Bossart wrote:
> Here's a v11 in which I've tried to fix the compiler warning that cfbot is
> complaining about. No other changes.

I have put my eyes on the v11 series.

Not much to offer about 0001. extract_autovac_opts() dates back from
2009, where the per-table autovacuum reloptions have been added by
Alvaro in 834a6da4f72d. I thought that this was newer than that.

+ av_enabled = (avopts ? avopts->enabled != PG_TERNARY_FALSE : true);

This bit was in 0002. Making the unset ternary state the same as
enabled should work.

Not much to say about 0003 and CLEANUP_NOT_SET matching to a
VACOPTVALUE_AUTO.

- if (!found)
- {
- /* hash_search already filled in the key */
- hentry->ar_relid = relid;
- hentry->ar_hasrelopts = false;
- if (relopts != NULL)
- {

In 0004, I was wondering if this makes the code weaker on some
aspects, because we are switching from a logic where we always had
an entry in the mapping hashtable for a main relation with a TOAST
table to a logic where a NULL entry could mean either:
- Main relation has no TOAST table.
- Main relation has a TOAST table but no reloptions to inherit from.
Before that the difference was made with ar_hasrelopts being set or
not. I cannot think of anything on top of my mind, but I'm also
wondering if it could be better to always have an entry if a main
relation has a TOAST table, just keep the ar_reloptions to NULL and
rely on that to decide if there are options to inherit, acting as a
replacement of ar_hasrelopts.

- -1, -1, INT_MAX
+ -2, -1, INT_MAX

In 0005, this one is log_autovacuum_min_duration. I'd wish for a
cleaner way to mark that than what looks like to me a default_val, but
well.. That's not new.

Nothing to say about 0006, I saw the link with 0007.

+ * When vacuuming a TOAST table, its main table's storage parameters, for
+ * the TOAST table to inherit anything it doesn't set itself. NULL if the
+ * main table has none, or if this isn't a TOAST table.

In 0007, that may be just me but I am having a hard time parsing that,
especially the " to inherit anything it doesn't set itself". Okay,
this means that this is only set when dealing with a TOAST table, to
track the reloptions of its parent relation.

+ if (rel->rd_options)
+ memcpy(&relopts_copy, rel->rd_options, sizeof(StdRdOptions));
[...]
+ * NB: This destructively modifies toast_opts, and what it returns may be
+ * either argument, so the caller must know which of the two it owns.

Hmm. I am not really cool with this as an API contract. That can
bite. That's not re-entrant, to begin with, and on top of that this
function returns the merged result. It would be saner to create a
copy, and return the copy as a result, copy that we do anyway before
the sole caller of the function with a memcpy(). :)

+ /* if we're a TOAST table, look up our parent's relopts, too */
+ if (classForm->relkind == RELKIND_TOASTVALUE)
+ hentry = hash_search(toast_map, &classForm->oid, HASH_FIND, NULL);
+ *main_opts = hentry ? &hentry->ar_reloptions : NULL;
+
+ /* return the merged reloptions */
+ return merge_toast_reloptions(relopts, *main_opts);

Hmm. We have three callers of get_effective_relopts(), and some paths
can call it for a main relation, meaning that the
merge_toast_reloptions() makes little sense because there is nothing
to merge. Should this enforce a check so as we try to merge
reloptions only when dealing with a toast relation, or should the
callers for that by themselves based on the classForm->relkind?

In 0008, some tests would be nice for the autovacuum case, at least.
That would mean a TAP test to check a bit what do_autovacuum() does,
and now the SQL test in injection_points only looks after
pg_stat_get_autovacuum_scores(). I am honestly puzzled by the reason
why this is added inside injection_points at all. There is no
dependency to a point, and no new information with the NOTICE
messages. A better location would fit better the purpose of the score
test.
--
Michael

On Thu, Aug 13, 2026 at 03:28:23PM +0900, Michael Paquier wrote:
> I have put my eyes on the v11 series.

Appreciate the review.

> Not much to offer about 0001. extract_autovac_opts() dates back from
> 2009, where the per-table autovacuum reloptions have been added by
> Alvaro in 834a6da4f72d. I thought that this was newer than that.
>
> + av_enabled = (avopts ? avopts->enabled != PG_TERNARY_FALSE : true);
>
> This bit was in 0002. Making the unset ternary state the same as
> enabled should work.
>
> Not much to say about 0003 and CLEANUP_NOT_SET matching to a
> VACOPTVALUE_AUTO.

I'll plan on committing these soon to get them out of the way.

> - if (!found)
> - {
> - /* hash_search already filled in the key */
> - hentry->ar_relid = relid;
> - hentry->ar_hasrelopts = false;
> - if (relopts != NULL)
> - {
>
> In 0004, I was wondering if this makes the code weaker on some
> aspects, because we are switching from a logic where we always had
> an entry in the mapping hashtable for a main relation with a TOAST
> table to a logic where a NULL entry could mean either:
> - Main relation has no TOAST table.
> - Main relation has a TOAST table but no reloptions to inherit from.
> Before that the difference was made with ar_hasrelopts being set or
> not. I cannot think of anything on top of my mind, but I'm also
> wondering if it could be better to always have an entry if a main
> relation has a TOAST table, just keep the ar_reloptions to NULL and
> rely on that to decide if there are options to inherit, acting as a
> replacement of ar_hasrelopts.

I personally don't see much point in tracking additional information we
don't need. We can already tell if the table in question is a TOAST table,
so a missing entry in the hash table means that we didn't find any main
table relopts for it. *shrug*

>
> + * When vacuuming a TOAST table, its main table's storage parameters, for
> + * the TOAST table to inherit anything it doesn't set itself. NULL if the
> + * main table has none, or if this isn't a TOAST table.
>
> In 0007, that may be just me but I am having a hard time parsing that,
> especially the " to inherit anything it doesn't set itself". Okay,
> this means that this is only set when dealing with a TOAST table, to
> track the reloptions of its parent relation.

Reworded to the following in v12:

When vacuuming a TOAST table, this holds the main table's storage
parameters (or NULL if it doesn't have any). If a reloption is unset
on the TOAST table but _is_ set on the main table, we use the main
table's setting.

> + if (rel->rd_options)
> + memcpy(&relopts_copy, rel->rd_options, sizeof(StdRdOptions));
> [...]
> + * NB: This destructively modifies toast_opts, and what it returns may be
> + * either argument, so the caller must know which of the two it owns.
>
> Hmm. I am not really cool with this as an API contract. That can
> bite. That's not re-entrant, to begin with, and on top of that this
> function returns the merged result. It would be saner to create a
> copy, and return the copy as a result, copy that we do anyway before
> the sole caller of the function with a memcpy(). :)

Done in v12.

> + /* if we're a TOAST table, look up our parent's relopts, too */
> + if (classForm->relkind == RELKIND_TOASTVALUE)
> + hentry = hash_search(toast_map, &classForm->oid, HASH_FIND, NULL);
> + *main_opts = hentry ? &hentry->ar_reloptions : NULL;
> +
> + /* return the merged reloptions */
> + return merge_toast_reloptions(relopts, *main_opts);
>
> Hmm. We have three callers of get_effective_relopts(), and some paths
> can call it for a main relation, meaning that the
> merge_toast_reloptions() makes little sense because there is nothing
> to merge. Should this enforce a check so as we try to merge
> reloptions only when dealing with a toast relation, or should the
> callers for that by themselves based on the classForm->relkind?

It enforces that already. The relkind check in the function ensures that
main_opts is always NULL for non-TOAST relations, and
merge_toast_reloptions() always returns the first argument when the second
is NULL. I do think this could be called out a bit better, which I've
tried to do in v12.

> In 0008, some tests would be nice for the autovacuum case, at least.
> That would mean a TAP test to check a bit what do_autovacuum() does,
> and now the SQL test in injection_points only looks after
> pg_stat_get_autovacuum_scores(). I am honestly puzzled by the reason
> why this is added inside injection_points at all. There is no
> dependency to a point, and no new information with the NOTICE
> messages. A better location would fit better the purpose of the score
> test.

I only put it there because 0007 added a similar test, and 0007 and 0008
used to be one patch. In v12, I've tried my hand at a TAP test.

--
nathan

On Thu, Aug 13, 2026 at 11:41:59AM -0500, Nathan Bossart wrote:
> I'll plan on committing these soon to get them out of the way.

I've committed 0001-0003. Barring more feedback, I'm hoping to commit the
rest soon. Here is a rebased patch set.

--
nathan

On Thu, Aug 13, 2026 at 11:41:59AM -0500, Nathan Bossart wrote:
> On Thu, Aug 13, 2026 at 03:28:23PM +0900, Michael Paquier wrote:
>> In 0004, I was wondering if this makes the code weaker on some
>> aspects, because we are switching from a logic where we always had
>> an entry in the mapping hashtable for a main relation with a TOAST
>> table to a logic where a NULL entry could mean either:

Looking at the five remaining patches in v13, replying to the message
where v12 was posted.

> I personally don't see much point in tracking additional information we
> don't need. We can already tell if the table in question is a TOAST table,
> so a missing entry in the hash table means that we didn't find any main
> table relopts for it. *shrug*

Hmm. Okay. Fine by me at the end.

>> + if (rel->rd_options)
>> + memcpy(&relopts_copy, rel->rd_options, sizeof(StdRdOptions));
>> [...]
>> + * NB: This destructively modifies toast_opts, and what it returns may be
>> + * either argument, so the caller must know which of the two it owns.
>>
>> Hmm. I am not really cool with this as an API contract. That can
>> bite. That's not re-entrant, to begin with, and on top of that this
>> function returns the merged result. It would be saner to create a
>> copy, and return the copy as a result, copy that we do anyway before
>> the sole caller of the function with a memcpy(). :)
>
> Done in v12.

The API contract in v13-0004 looks much better to me now. No more
overwrites of the inputs. It's almost like you could add some const
markers.

>> Hmm. We have three callers of get_effective_relopts(), and some paths
>> can call it for a main relation, meaning that the
>> merge_toast_reloptions() makes little sense because there is nothing
>> to merge. Should this enforce a check so as we try to merge
>> reloptions only when dealing with a toast relation, or should the
>> callers for that by themselves based on the classForm->relkind?
>
> It enforces that already. The relkind check in the function ensures that
> main_opts is always NULL for non-TOAST relations, and
> merge_toast_reloptions() always returns the first argument when the second
> is NULL. I do think this could be called out a bit better, which I've
> tried to do in v12.

At the end of the day, get_effective_relopts() acts as a thin wrapper
of extractRelOptions(), merging two existing code patterns and
re-using the same pattern for the scoring. Perhaps "effective" is the
term that troubles me here, while having merge_toast_reloptions().
You need the merge_*() for the vacuum part, but I'm also wondering if
this could not be reworked with less routines overall. I don't have a
clean idea on top of my mind now, and that does not count as an
objection. This gives an impression of being slightly
overcomplicated.

>> In 0008, some tests would be nice for the autovacuum case, at least.
>> That would mean a TAP test to check a bit what do_autovacuum() does,
>> and now the SQL test in injection_points only looks after
>> pg_stat_get_autovacuum_scores(). I am honestly puzzled by the reason
>> why this is added inside injection_points at all. There is no
>> dependency to a point, and no new information with the NOTICE
>> messages. A better location would fit better the purpose of the score
>> test.
>
> I only put it there because 0007 added a similar test, and 0007 and 0008
> used to be one patch. In v12, I've tried my hand at a TAP test.

The test looks pretty nice here.
--
Michael

I've committed everything but the last two patches. I've attached a
rebased patch set.

On Wed, Aug 19, 2026 at 04:16:40PM +0900, Michael Paquier wrote:
> The API contract in v13-0004 looks much better to me now. No more
> overwrites of the inputs. It's almost like you could add some const
> markers.

Done in v14.

> At the end of the day, get_effective_relopts() acts as a thin wrapper
> of extractRelOptions(), merging two existing code patterns and
> re-using the same pattern for the scoring. Perhaps "effective" is the
> term that troubles me here, while having merge_toast_reloptions().
> You need the merge_*() for the vacuum part, but I'm also wondering if
> this could not be reworked with less routines overall. I don't have a
> clean idea on top of my mind now, and that does not count as an
> objection. This gives an impression of being slightly
> overcomplicated.

I haven't thought of anything better.

> The test looks pretty nice here.

Thanks for reviewing!

--
nathan

Committed.

--
nathan