Re: problems with toast.* reloptions

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