Re: Allow table AMs to define their own reloptions

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.

cheers

andrew

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