On Wed, Jul 22, 2026 at 11:35 AM Peter Smith <[email protected]> wrote: > I've added a CF entry. > [1] https://commitfest.postgresql.org/patch/7042/
Hi, I've reviewed v2 of this patch. Summary: the simplification is correct and I like it; one behavioral observation from corruption testing and a few comment nits below. Tested on master (36f7330b8b2): applies cleanly (two hunks in pg_publication.c land with a 17-line offset, no fuzz), assert-enabled build with zero warnings, core regression 245/245, subscription TAP 589/589. Correctness of dropping the prexcept check: I verified the invariant this relies on ― pg_publication_rel holds EXCEPT rows iff puballtables is set: - the grammar is strictly disjunctive (FOR ALL TABLES/SEQUENCES take no object list together with FOR TABLE / FOR TABLES IN SCHEMA), - CheckAlterPublication() rejects adding tables/schemas whenever either flag is set, - the only transition that clears puballtables (SET ALL SEQUENCES on an all-tables publication) reconciles the EXCEPT rows in the same command that flips the flag, - GetIncludedPublicationRelations()/GetExcludedPublicationTables() already Assert exactly this invariant. So returning HeapTupleIsValid(tup) without inspecting prexcept is sound. It may also be worth stating in the commit message that the early returns are reachable from exactly one call site: CheckAlterPublication() (publicationcmds.c:1588). In AlterPublicationTables() the flagged case errors out earlier, and AlterPublicationOwner_internal() already guards on form->puballtables || form->puballsequences before calling. One behavioral difference I verified end-to-end (assert builds of master with and without the patch, corrupting pg_publication_rel directly): if an inclusion row ever exists on a FOR ALL TABLES publication ― a state the invariant rules out, reachable only through catalog corruption or the pre-existing check-then-lock window in AlterPublication() (CheckAlterPublication() runs before LockDatabaseObject()) ― the old code's first-tuple probe can be poisoned by it. When the stray row is the scan's first tuple (equal prpubid keys come out in heap order), legitimate DDL like ALTER PUBLICATION ... SET ALL TABLES EXCEPT / SET ALL SEQUENCES fails with "does not support ALL TABLES/SEQUENCES operations" ― a misleading error, since the publication already IS all-tables ― while the stray row stays put. The new code short-circuits on the flag, so the DDL proceeds; the stray row remains in both versions, i.e. neither heals the state. So I don't see this as a point against the patch (arguably the new behavior is the more robust one), but it does make the invariant truly load-bearing: I'd suggest keeping a condensed version of the deleted comment in is_table_publication(), e.g. "A FOR ALL TABLES publication can have only EXCEPT entries in pg_publication_rel, so it never counts as a table publication." Framing nit for the commit message: the puballsequences clause added in v2 is a consistency/optimization change, not a correctness fix ― with puballsequences set, the scans were guaranteed to find nothing, so the old code already returned false. Saying so plainly would be more accurate than implying v1 had a bug here. Comment nits: - the new block comments have a double space after '*'; project style is a single space (pgindent won't reflow it); - the doc comment "(e.g., FOR TABLE)" should be "i.e." ― FOR TABLE is exactly the case, not an example; alternatively keep the old contract sentence that EXCEPT-only publications return false; - the two stacked coexistence sentences could be merged, and the puballtables half could mention the EXCEPT-row rationale. No new tests needed IMO: publication.sql and 037_except.pl already exercise both the early-return and the scan-then-error paths. I also don't think a benchmark is warranted ― the saving is ~2 catalog index scans on a DDL path; the honest justification is simplification, so the commit message maybe shouldn't lead with "more efficient". Process note: this patch currently has two CommitFest entries, #6705 and #7042, both "Needs review" in PG20-2. Could you close one of them so review activity lands in a single place? With the invariant comment retained, this gets my +1. Regards, Bingshuai Li
