Re: Allow table AMs to define their own reloptions - Mailing list pgsql-hackers

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