Re: problems with toast.* reloptions

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