Re: problems with toast.* reloptions

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