On Thu, Aug 20, 2026 at 11:40 AM Bingshuai Li <[email protected]> wrote: > > Hi Peter, > > Thanks for the quick v3 — the doc comments and the merged coexistence > sentences all look good. > > One point from my review seems to have been dropped, though. The merged > comment in is_table_publication() now reads: > > /* > * FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES. > */ > > That states the grammar fact, but not why skipping the > pg_publication_rel scan is safe: a FOR ALL TABLES publication can still > have EXCEPT rows there, and returning false is correct precisely because > those are the only rows it can have. Would you consider keeping one more > sentence to that effect, e.g. > > /* > * FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES. > * A FOR ALL TABLES publication can have only EXCEPT entries in > * pg_publication_rel, so it never counts as a table publication. > */ >
The name of the function is "is_table_publication". The purpose is in the name. Just return T/F if there is "FOR TABLE" clause for the publication; nothing more. e.g., if the function was called "is_table_in_pg_publication_rel", then I would want to explain about EXCEPT. Since "FOR ALL TABLES" is already mutually exclusive from "FOR TABLE" I didn't see any need to explain about "FOR ALL TABLES EXCEPT". Was your review AI-based? It appeared overly concerned about this detail. Anyway, I am marking this as "ready for committer" so a committer can decide on this point. ====== Kind Regards, Peter Smith. Fujitsu Australia
