DKIM2-Signature: i=1; m=1; t=1790873203; d=kurilemu.de;
	mf=PGFsdmhlcnJlQGt1cmlsZW11LmRlPg==;
	rt=PHBnc3FsLWhhY2tlcnNAbGlzdHMucG9zdGdyZXNxbC5vcmc+;
	s=fm3:rsa-sha256:q/BXNYwk1Zkx0fj8M3Y43NtX2HJrlJKPPhIX7Ek4sGvH6EL
	EzlS7gk2JrcQbh385wGPZfODg5kPy7HRuKBlSmME3M7NsXAf4WMW9Jxw+WsEqe8+
	7AzxpYPdKW8BHELkCfCThTLUgxh5Avydg2FPOy7Flq4UK2wZBXCACF+dD5Jtj9Ao
	3bru7Zeb9U0fXN/xTqbTST5vXLu7vCQzQS500o6UwkKT/knliEc1/TT0ASVNvGG4
	3kfG/oIuBR311LGlRjzwul2m2utUK+wIYRM8QqXG2IlDq5F1FgYhL8+zYQQS6xbf
	2r+qmPUSBjj6aZmYvqnA6USYucuI2lAulfPTLjw==;
Message-Instance: m=1;
 h=sha256:y/LVutwUK87ovo2mZUVQqycrMDf0VJS0w3j2iZHpNSc=:Q4bGr6cjZWAwnNHkBXkzZyJ2sfcdxJnvCRGQE0clAwI=;
Feedback-ID: ie3de48e3:Fastmail
Date: Thu, 1 Oct 2026 16:57:42 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Andrew Dunstan <andrew@dunslane.net>
Cc: Ajit Awekar <ajitpostgres@gmail.com>,
	Aleksander Alekseev <aleksander@tigerdata.com>,
 pgsql-hackers@lists.postgresql.org,
	Junwang Zhao <zhjwpku@gmail.com>, Zsolt Parragi <zsolt.parragi@percona.com>,
	Rafia Sabih <rafia.pghackers@gmail.com>,
 Julien Tachoires <julien@tachoires.me>
Subject: Re: Allow table AMs to define their own reloptions
Message-ID: <ar5l4TRlCebwEp5s@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a15db9d5-3ebb-4e6a-80e9-c900aad638a0@dunslane.net>
Archived-At: 
 <https://www.postgresql.org/message-id/ar5l4TRlCebwEp5s%40alvherre.pgsql>
Precedence: bulk

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?


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.
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.

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.

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"?

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"?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)