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)
