Re: Allow table AMs to define their own reloptions

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.

cheers

andrew

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