Re: Allow table AMs to define their own reloptions

On 2026-10-10 Sa 7:29 AM, Andrew Dunstan wrote:
>
>
> On 2026-10-02 Fr 5:48 PM, Andrew Dunstan wrote:
>>
>>
>> On 2026-10-01 Th 10:57 AM, Álvaro Herrera wrote:
>>> On 2026-Sep-30, Andrew Dunstan wrote:
>>>
>>>> diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
>>>> index ea3f2a6be99..6dfc6e8026c 100644
>>>> --- a/src/include/access/tableam.h
>>>> +++ b/src/include/access/tableam.h
>>>> @@ -17,6 +17,7 @@
>>>> #ifndef TABLEAM_H
>>>> #define TABLEAM_H
>>>>
>>>> +#include "access/amapi.h"
>>>> #include "access/relscan.h"
>>>> #include "access/sdir.h"
>>>> #include "access/xact.h"
>>> I don't love this. I have a bunch of patches queued to remove includes
>>> from other includes to reduce cross-header contamination. This kind of
>>> change makes it impossible to remove the cross inclusion here and is
>>> more or less a step backwards. (It's not _too_ bad because tableam.h is
>>> not as widely used
>>>
>>> Is there a better way to have a function definition that can be used in
>>> both amapi.h and tableam.h without this cross-header inclusion?
>>
>>
>> Yes, there's no need for it. All the other TableAmRoutine members are
>> declared as bare function pointers, so amoptions now is too:
>>
>>      bytea      *(*amoptions) (Datum reloptions, bool validate);
>>
>> and the include is gone. The type is identical to amoptions_function,
>> so nothing else changes.
>>
>>> Other comments on verbiage, just passing by:
>>>
>>> I'm not much in love with the LLM-written comments TBH -- I don't think
>>> phrases like "owns the option set entirely" are valuable, for example.
>>
>>
>> You're absolutely right! :-)
>>
>> I should have been much more careful. I hope the attached is more to
>> your liking.
>>
>>
>>> Also, the comment just above the ATValidateAccessMethodOptions() call in
>>> ATController is redundant: it would be enough to say "validate options
>>> as needed", and then have the comment atop ATValidateAccessMethodOptions()
>>> itself carry the explanation of what we do and why.
>>
>>
>> Done
>>
>>
>>> In tableam.sgml, I'm not sure it makes much sense to state "The callback
>>> has the same signature as the corresponding index AM callback". Why not
>>> just say what the signature is without directing the user to read a
>>> reference page that's not relevant to the topic of table AMs? I think
>>> the first mention of reloptions in that page should be <firstterm>.
>>>
>>> It also talks about validating and throwing ereport(ERROR) but it
>>> doesn't say in so many words what must happen or not happen on each
>>> possible value of 'validate'. It could be clearer.
>>
>>
>> I've rewritten that section. It's now called "Table Access Method
>> Options", since a lot of what goes under the user-facing name "storage
>> parameters" isn't about storage, and refers to CREATE TABLE for that.
>> It uses <firstterm> for reloptions, gives the signature, and says what
>> the callback must do when validate is true (error on unrecognized or
>> invalid values, and never substitute something else for them) and when
>> it's false (ignore invalid entries without error).
>>
>>> There's also "raises an error rather than silently dropping the value".
>>> I mean, why not say "rather than launching an ICBM"? Why not just
>>> "raises an error, period"?
>>
>>
>> Done
>>
>>
>>> If I were the user of such an AM, I would not be sure how to interpret
>>> the phrase "validate that the option read with SELECT reloptions FROM
>>> pg_class are the ones that will be used". What does that mean exactly?
>>> Should it say "examine" rather than "validate"?
>>
>>
>> That sentence is gone. What it was trying to say is now stated
>> directly: the callback must not accept an invalid value and use
>> something else in its place, since the stored parameters would then
>> not be the ones in effect.
>>
>>
>> I wrote:
>>
>>> On 2026-09-30 We 5:22 PM, Zsolt Parragi wrote:
>>>> pg_dump has the option "--no-table-access-method". Should that somehow
>>>> interact with this feature? As currently that can result in non
>>>> restorable dumps.
>>>>
>>>
>>> Good catch.
>>>
>>> Worse, the CREATE TABLE fails, so the table and its data are lost,
>>> with both pg_dump and pg_restore --no-table-access-method.
>>>
>>> We can't just move the options to a later ALTER TABLE, because some
>>> standard ones (toast_value_type) only matter at creation time. So my
>>> plan, as a second patch:
>>>
>>> - A function pg_reloption_is_standard(name), true if the option is
>>>   registered for RELOPT_KIND_HEAP (including ones an AM inherits via
>>>   add_reloption_to_kind()).
>>>
>>> - For non-heap tables and matviews, pg_dump keeps standard options in
>>>   the CREATE, and puts AM-specific ones in a separate ALTER TABLE ...
>>>   SET (...) TOC entry, which --no-table-access-method omits or skips.
>>>
>>> - TAP tests using dummy_table_am.
>>>
>>> That requires AMs to inherit standard option names rather than
>>> redefine them, and to have their own options work when set by ALTER
>>> TABLE on an empty table, since every restore would apply them that
>>> way. I'd document both in tableam.sgml. The second one isn't ideal,
>>> but I don't see a simpler way.
>>
>> patch 2 implements this.
>>
>
> Here's a v11.
>
>
> Changes since v10, all in 0001:
>  - Whether rd_options holds StdRdOptions is now decided once, where
>    RelationParseRelOptions() picks the parser, and cached in a new
>    rd_stdoptions field.  The two checks can no longer disagree, and the
>    per-tuple heap insert path is back to a single inline test, same as
>    master, fixing a small performance hit.
>  - New RelationGetTableAmOptions() helper replaces two copies of the
>    parser-choice test.
>
>
>

Here's a v12. Changes are cosmetic to make it pgindent clean.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com