Re: Support EXCEPT for ALL SEQUENCES publications

On Wed, 8 Jul 2026 at 07:56, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for v19-0001 (no comments for v19-0002)
>
> //////////
> Patch 0001
> //////////
>
> NOTE: The following review comments are all coming from the same PoV
> that actually there can be multiple simultaneous EXCEPT clauses (e.g.
> FOR ALL TABLES EXCEPT, and FOR ALL SEQUENCES EXCEPT), so it is not
> quite correct to just refer to the singular "the EXCEPT clause" --
> should use plural, or remove the ambiguity from the text.
>
> ======
> src/backend/catalog/pg_publication.c
>
> GetAllTablesPublications:
>
> 1.
> - * For a FOR ALL TABLES publication, the returned list excludes
> tables mentioned
> - * in the EXCEPT clause.
> + * For a FOR ALL TABLES publication, the returned list excludes tables
> + * specified in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
> + * it excludes sequences specified in the EXCEPT clause.
>
> SUGGESTION
> For a FOR ALL TABLES publication, the returned list excludes tables
> specified in the EXCEPT (TABLE ...) clause. For a FOR ALL SEQUENCES
> publication, it excludes sequences specified in the EXCEPT (SEQUENCE
> ...) clause.
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> getPublications:
>
> 2.
> - * Get the list of tables for publications specified in the EXCEPT
> - * TABLE clause.
> + * Get the list of tables and sequences specified in the publication's
> + * EXCEPT clause.
>
> You cannot exclude tables and sequences from the same EXCEPT, so that
> should say "EXCEPT clauses" (plural). But, it may be simpler to just
> reword it.
>
> SUGGESTION
> Get the list of tables and sequences explicitly excluded from the publication.
>
> ======
> src/bin/psql/describe.c
>
> describeOneTableDetails:
>
> 3.
> + /* Print publications whose EXCEPT clause contains this sequence */
> + if (pset.sversion >= 200000)
>
> This is similar. "whose EXCEPT clause" implies there is only one, but
> in practice there can be multiple EXCEPT clauses: (FOR ALL TABLES
> EXCEPT + FOR ALL SEQUENCES EXCEPT).
>
> SUGGESTION
> Print publications that explicitly exclude this sequence.
>
> ~~~
>
> 4.
> + /* Get sequences in the EXCEPT clause for this publication */
> + printfPQExpBuffer(&buf, "/* %s */\n",
> + _("Get sequences excluded by this publication"));
>
> This is similar. "the EXCEPT clause" implies there is only one, but
> there may be multiple. I saw that you were mimicking the existing
> comment, so those should be the same.
>
> SUGGESTION #1 (redundant, because the next string already says what
> the code is doing)
> Just remove both comments.
>
> SUGGESTION #2 (clarify what clauses you are talking about)
> /* Get tables in the EXCEPT (TABLE ...) clause for this publication */
> and
> /* Get sequences in the EXCEPT (SEQUENCE ...) clause for this publication */
>

> ======
> src/include/nodes/parsenodes.h
>
> 5.
> +typedef struct PublicationRelation
> {
> NodeTag type;
> - RangeVar *relation; /* publication relation */
> - Node *whereClause; /* qualifications */
> + RangeVar *relation; /* publication table/sequence */
> + Node *whereClause; /* qualifications for publication table */
> List *columns; /* List of columns in a publication table */
> bool except; /* True if listed in the EXCEPT clause */
> -} PublicationTable;
> +} PublicationRelation;
>
> Same. /True if listed in the EXCEPT clause/True if listed in an EXCEPT clause/
>
> ~~~
>
> 6.
> PUBLICATIONOBJ_EXCEPT_TABLE, /* A table in the EXCEPT clause */
> + PUBLICATIONOBJ_EXCEPT_SEQUENCE, /* A sequence in the EXCEPT clause */
>
> /A table in the EXCEPT clause/A table in an EXCEPT clause/
> /A sequence in the EXCEPT clause/A sequence in an EXCEPT clause/
>
> ~~~
>
> 7.
> - List *except_tables; /* tables specified in the EXCEPT clause */
> + List *except_relations; /* depending on the 'pubobjtype', this is
> + * a list of either tables or sequences
> + * specified in the EXCEPT clause */
> ParseLoc location; /* token location, or -1 if unknown */
> } PublicationAllObjSpec;
>
> /specified in the EXCEPT clause/specified in an EXCEPT clause/
>
> ======
> src/test/regress/sql/publication.sql
>
> 8.
> +-- Check that the sequence description shows the publications where
> it is listed
> +-- in the EXCEPT clause
>
> /the EXCEPT clause/an EXCEPT clause/
>
> ~~~
>
> 9.
> +-- Test combination of ALL SEQUENCES and ALL TABLES with EXCEPT clause
>
> /with EXCEPT clause/, each with an EXCEPT clause/
>
>
> ======
> src/test/subscription/t/037_except.pl
>
> 10.
> +is($result, '200|t',
> + 'check replication of a sequence in the EXCEPT clause of one
> publication but included by another'
> +);
>
> (same x2)
>
> SUGGESTION
> check replication of a sequence excluded by one publication but
> included by another
>
I have addressed the comments and attached the updated v20 patch.

Thanks

Shlok Kyal