Re: Support EXCEPT for ALL SEQUENCES publications

Hi hackers,

Thread [1] introduced support for the EXCEPT clause for publications
defined with ALL TABLES. To extend this functionality, as discussed in
[2], this patch series adds support for the EXCEPT clause for
publications defined with ALL SEQUENCES.

The series consists of the following patches:

0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
This allows excluding specific sequences when using CREATE PUBLICATION
... FOR ALL SEQUENCES.
Example:
CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);

0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
Examples:
ALTER PUBLICATION pub1 SET ALL SEQUENCES;
This clears any existing sequence exclusions.

ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
This replaces the exclusion list with the specified sequences.

Sequences listed in the EXCEPT clause are excluded from the
publication and are not replicated to the subscriber.

This brings sequence publication behavior in line with the existing
support for FOR ALL TABLES ... EXCEPT.

[1]: https://www.postgresql.org/message-id/CALDaNm3%3DJrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh%3DtamA%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1JWz=+-zEhQRszrGEbOzYKReXtsS9hGycWubHV8v2U-QA@mail.gmail.com

Thanks,
Shlok Kyal

On Fri, 10 Apr 2026 at 11:39, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> Hi hackers,
>
> Thread [1] introduced support for the EXCEPT clause for publications
> defined with ALL TABLES. To extend this functionality, as discussed in
> [2], this patch series adds support for the EXCEPT clause for
> publications defined with ALL SEQUENCES.
>
> The series consists of the following patches:
>
> 0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
> This allows excluding specific sequences when using CREATE PUBLICATION
> ... FOR ALL SEQUENCES.
> Example:
> CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
>
> 0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
> This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
> Examples:
> ALTER PUBLICATION pub1 SET ALL SEQUENCES;
> This clears any existing sequence exclusions.
>
> ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> This replaces the exclusion list with the specified sequences.
>
> Sequences listed in the EXCEPT clause are excluded from the
> publication and are not replicated to the subscriber.

+1 for this feature.

Currently, FOR ALL SEQUENCES is effectively an all-or-nothing choice.
In practice, there are often specific sequences, such as temporary or
locally managed ones, that do not need to be replicated. With this
change, those can now be excluded cleanly.

Regards,
Vignesh

On Fri, Apr 10, 2026 at 11:59 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 10 Apr 2026 at 11:39, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > Hi hackers,
> >
> > Thread [1] introduced support for the EXCEPT clause for publications
> > defined with ALL TABLES. To extend this functionality, as discussed in
> > [2], this patch series adds support for the EXCEPT clause for
> > publications defined with ALL SEQUENCES.
> >
> > The series consists of the following patches:
> >
> > 0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
> > This allows excluding specific sequences when using CREATE PUBLICATION
> > ... FOR ALL SEQUENCES.
> > Example:
> > CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> >
> > 0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
> > This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
> > Examples:
> > ALTER PUBLICATION pub1 SET ALL SEQUENCES;
> > This clears any existing sequence exclusions.
> >
> > ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> > This replaces the exclusion list with the specified sequences.
> >
> > Sequences listed in the EXCEPT clause are excluded from the
> > publication and are not replicated to the subscriber.
>
> +1 for this feature.
>
> Currently, FOR ALL SEQUENCES is effectively an all-or-nothing choice.
> In practice, there are often specific sequences, such as temporary or
> locally managed ones, that do not need to be replicated. With this
> change, those can now be excluded cleanly.
>

+1 on the idea. The use cases will be such temproary or short
duration sequences (created for temp purpose but persisted ones) which
are node specific and may not require replication to replica node.

I have not finished full review yet, but few comments on 001:

1)
postgres=# create unlogged sequence seq4;
CREATE SEQUENCE
postgres=# create publication pub9 for all sequences EXCEPT ( SEQUENCE seq4);
ERROR: cannot specify relation "seq4" in the publication EXCEPT clause
DETAIL: This operation is not supported for unlogged tables.

Detail need to be corrected. Same for temporary sequence.

2)
/*
* Add or remove table to/from publication.
*/
AlterPublicationTables

This function is now called even to add sequence in except-clause.
Shall we change the name and comment?

3)
CreatePublication():
+ if (stmt->for_all_sequences && exceptseqs != NIL)
+ {
+ List *rels;

+ /* Process EXCEPT sequence list */
+ rels = OpenRelationList(exceptseqs);
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
+ CloseRelationList(rels);
+ }
if (stmt->for_all_tables)

We can have a blank line in between.

4)
Below comment in getPublications() need update to incoporate sequence
in full comment.
* Get the list of tables for publications specified in the EXCEPT
* TABLE clause.
*
* Although individual table entries in EXCEPT list could be stored in
...

thanks
Shveta

On Fri, 10 Apr 2026 at 11:39, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> Hi hackers,
>
> Thread [1] introduced support for the EXCEPT clause for publications
> defined with ALL TABLES. To extend this functionality, as discussed in
> [2], this patch series adds support for the EXCEPT clause for
> publications defined with ALL SEQUENCES.
>
> The series consists of the following patches:
>
> 0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
> This allows excluding specific sequences when using CREATE PUBLICATION
> ... FOR ALL SEQUENCES.
> Example:
> CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
>
> 0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
> This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
> Examples:
> ALTER PUBLICATION pub1 SET ALL SEQUENCES;
> This clears any existing sequence exclusions.
>
> ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> This replaces the exclusion list with the specified sequences.
>
> Sequences listed in the EXCEPT clause are excluded from the
> publication and are not replicated to the subscriber.
>
> This brings sequence publication behavior in line with the existing
> support for FOR ALL TABLES ... EXCEPT.

Few comments on first patch:
1) There are few warnings while building:
gram.y: warning: 1 nonterminal useless in grammar [-Wother]
gram.y: warning: 2 rules useless in grammar [-Wother]
gram.y:14038.1-12: warning: nonterminal useless in grammar:
opt_sequence [-Wother]
14038 | opt_sequence: SEQUENCE
| ^~~~~~~~~~~~
preproc.y: warning: 1 nonterminal useless in grammar [-Wother]
preproc.y: warning: 2 rules useless in grammar [-Wother]
preproc.y:5589.2-13: warning: nonterminal useless in grammar:
opt_sequence [-Wother]
5589 | opt_sequence:
| ^~~~~~~~~~~~

2) The renaming of table to relation can be moved to a separate patch
and kept as 0001 patch:
2.a) pubtable to pubrelation
- $$->pubtable =
makeNode(PublicationTable);
- $$->pubtable->relation
= makeRangeVar(NULL, $1, @1);
- $$->pubtable->columns = $2;
- $$->pubtable->whereClause = $3;
+ $$->pubrelation =
makeNode(PublicationRelation);
+
$$->pubrelation->relation = makeRangeVar(NULL, $1, @1);
+ $$->pubrelation->columns = $2;
+
$$->pubrelation->whereClause = $3;

2.b) except_tables to except_relations
- $$->except_tables = $3;
+ $$->except_relations = $3;

similarly there may be few others

3) In case of except table we have used "Except tables:", we can
change it to "Except sequences:" to keep it consistent:
+ if (!addFooterToPublicationDesc(&buf,
_("Except Sequences:"),
+
true, &cont))
+ goto error_return;

Also it is agreed at [1] to use lower case for the second one.

4) add one example in doc for except (sequence)

5) Tab completion for "create publication pub1 for all sequences
except (" is missing:

+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
"ALL", "SEQUENCES", "EXCEPT"))
+ COMPLETE_WITH("( SEQUENCE");
+ else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
"ALL", "SEQUENCES", "EXCEPT", "(", "SEQUENCE"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);

[1] - https://www.postgresql.org/message-id/flat/CAHut%2BPt3t_tCYwDStkj5fG4Z%3DYXrHvPBA7iGdh745QipC5zKeg%40mail.gmail.com

Regards,
Vignesh

On Fri, 10 Apr 2026 at 16:31, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 10 Apr 2026 at 11:39, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > Hi hackers,
> >
> > Thread [1] introduced support for the EXCEPT clause for publications
> > defined with ALL TABLES. To extend this functionality, as discussed in
> > [2], this patch series adds support for the EXCEPT clause for
> > publications defined with ALL SEQUENCES.
> >
> > The series consists of the following patches:
> >
> > 0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
> > This allows excluding specific sequences when using CREATE PUBLICATION
> > ... FOR ALL SEQUENCES.
> > Example:
> > CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> >
> > 0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
> > This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
> > Examples:
> > ALTER PUBLICATION pub1 SET ALL SEQUENCES;
> > This clears any existing sequence exclusions.
> >
> > ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> > This replaces the exclusion list with the specified sequences.
> >
> > Sequences listed in the EXCEPT clause are excluded from the
> > publication and are not replicated to the subscriber.
> >
> > This brings sequence publication behavior in line with the existing
> > support for FOR ALL TABLES ... EXCEPT.
>
> Few comments on first patch:
> 1) There are few warnings while building:
> gram.y: warning: 1 nonterminal useless in grammar [-Wother]
> gram.y: warning: 2 rules useless in grammar [-Wother]
> gram.y:14038.1-12: warning: nonterminal useless in grammar:
> opt_sequence [-Wother]
> 14038 | opt_sequence: SEQUENCE
> | ^~~~~~~~~~~~
> preproc.y: warning: 1 nonterminal useless in grammar [-Wother]
> preproc.y: warning: 2 rules useless in grammar [-Wother]
> preproc.y:5589.2-13: warning: nonterminal useless in grammar:
> opt_sequence [-Wother]
> 5589 | opt_sequence:
> | ^~~~~~~~~~~~
>
> 2) The renaming of table to relation can be moved to a separate patch
> and kept as 0001 patch:
> 2.a) pubtable to pubrelation
> - $$->pubtable =
> makeNode(PublicationTable);
> - $$->pubtable->relation
> = makeRangeVar(NULL, $1, @1);
> - $$->pubtable->columns = $2;
> - $$->pubtable->whereClause = $3;
> + $$->pubrelation =
> makeNode(PublicationRelation);
> +
> $$->pubrelation->relation = makeRangeVar(NULL, $1, @1);
> + $$->pubrelation->columns = $2;
> +
> $$->pubrelation->whereClause = $3;
>
> 2.b) except_tables to except_relations
> - $$->except_tables = $3;
> + $$->except_relations = $3;
>
> similarly there may be few others
>
> 3) In case of except table we have used "Except tables:", we can
> change it to "Except sequences:" to keep it consistent:
> + if (!addFooterToPublicationDesc(&buf,
> _("Except Sequences:"),
> +
> true, &cont))
> + goto error_return;
>
> Also it is agreed at [1] to use lower case for the second one.
>
> 4) add one example in doc for except (sequence)
>
> 5) Tab completion for "create publication pub1 for all sequences
> except (" is missing:
>
> + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
> "ALL", "SEQUENCES", "EXCEPT"))
> + COMPLETE_WITH("( SEQUENCE");
> + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
> "ALL", "SEQUENCES", "EXCEPT", "(", "SEQUENCE"))
> + COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);
>
> [1] - https://www.postgresql.org/message-id/flat/CAHut%2BPt3t_tCYwDStkj5fG4Z%3DYXrHvPBA7iGdh745QipC5zKeg%40mail.gmail.com
>
Hi Vignesh and Shveta,

Thanks for reviewing the patches.
I have addressed the comments and attached the updated patch.

This also addressed the comments shared by Shveta in [1].
[1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com

Thanks,
Shlok Kyal

On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, 10 Apr 2026 at 16:31, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Fri, 10 Apr 2026 at 11:39, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > Hi hackers,
> > >
> > > Thread [1] introduced support for the EXCEPT clause for publications
> > > defined with ALL TABLES. To extend this functionality, as discussed in
> > > [2], this patch series adds support for the EXCEPT clause for
> > > publications defined with ALL SEQUENCES.
> > >
> > > The series consists of the following patches:
> > >
> > > 0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
> > > This allows excluding specific sequences when using CREATE PUBLICATION
> > > ... FOR ALL SEQUENCES.
> > > Example:
> > > CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> > >
> > > 0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
> > > This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
> > > Examples:
> > > ALTER PUBLICATION pub1 SET ALL SEQUENCES;
> > > This clears any existing sequence exclusions.
> > >
> > > ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> > > This replaces the exclusion list with the specified sequences.
> > >
> > > Sequences listed in the EXCEPT clause are excluded from the
> > > publication and are not replicated to the subscriber.
> > >
> > > This brings sequence publication behavior in line with the existing
> > > support for FOR ALL TABLES ... EXCEPT.
> >
> > Few comments on first patch:
> > 1) There are few warnings while building:
> > gram.y: warning: 1 nonterminal useless in grammar [-Wother]
> > gram.y: warning: 2 rules useless in grammar [-Wother]
> > gram.y:14038.1-12: warning: nonterminal useless in grammar:
> > opt_sequence [-Wother]
> > 14038 | opt_sequence: SEQUENCE
> > | ^~~~~~~~~~~~
> > preproc.y: warning: 1 nonterminal useless in grammar [-Wother]
> > preproc.y: warning: 2 rules useless in grammar [-Wother]
> > preproc.y:5589.2-13: warning: nonterminal useless in grammar:
> > opt_sequence [-Wother]
> > 5589 | opt_sequence:
> > | ^~~~~~~~~~~~
> >
> > 2) The renaming of table to relation can be moved to a separate patch
> > and kept as 0001 patch:
> > 2.a) pubtable to pubrelation
> > - $$->pubtable =
> > makeNode(PublicationTable);
> > - $$->pubtable->relation
> > = makeRangeVar(NULL, $1, @1);
> > - $$->pubtable->columns = $2;
> > - $$->pubtable->whereClause = $3;
> > + $$->pubrelation =
> > makeNode(PublicationRelation);
> > +
> > $$->pubrelation->relation = makeRangeVar(NULL, $1, @1);
> > + $$->pubrelation->columns = $2;
> > +
> > $$->pubrelation->whereClause = $3;
> >
> > 2.b) except_tables to except_relations
> > - $$->except_tables = $3;
> > + $$->except_relations = $3;
> >
> > similarly there may be few others
> >
> > 3) In case of except table we have used "Except tables:", we can
> > change it to "Except sequences:" to keep it consistent:
> > + if (!addFooterToPublicationDesc(&buf,
> > _("Except Sequences:"),
> > +
> > true, &cont))
> > + goto error_return;
> >
> > Also it is agreed at [1] to use lower case for the second one.
> >
> > 4) add one example in doc for except (sequence)
> >
> > 5) Tab completion for "create publication pub1 for all sequences
> > except (" is missing:
> >
> > + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
> > "ALL", "SEQUENCES", "EXCEPT"))
> > + COMPLETE_WITH("( SEQUENCE");
> > + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
> > "ALL", "SEQUENCES", "EXCEPT", "(", "SEQUENCE"))
> > + COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);
> >
> > [1] - https://www.postgresql.org/message-id/flat/CAHut%2BPt3t_tCYwDStkj5fG4Z%3DYXrHvPBA7iGdh745QipC5zKeg%40mail.gmail.com
> >
> Hi Vignesh and Shveta,
>
> Thanks for reviewing the patches.
> I have addressed the comments and attached the updated patch.

FYI, this patch no longer applies cleanly after commit
49ce41810faca2722424b3d8fabda79bf4902339, which also includes changes
in pg_dump.
git am v2-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLIC.patch
Applying: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
error: patch failed: src/bin/pg_dump/pg_dump.c:4721
error: src/bin/pg_dump/pg_dump.c: patch does not apply

You can handle this in the next version. I will use an older version
to review currently.

Regards,
Vignesh

On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Vignesh and Shveta,
>
> Thanks for reviewing the patches.
> I have addressed the comments and attached the updated patch.
>
> This also addressed the comments shared by Shveta in [1].
> [1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com
>

Thanks Shlok. The patch does not apply to latest head. Would you be
able ot rebase and post?

thanks
Shveta

On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> Hi Vignesh and Shveta,
>
> Thanks for reviewing the patches.
> I have addressed the comments and attached the updated patch.

Few comments for the first patch:
1) I felt these also could be moved to first patch:
- * Gets list of table oids that were specified in the EXCEPT clause for a
+ * Gets list of relation oids that were specified in the EXCEPT clause for a

2) Similarly here too:
- * The returned tables are locked in ShareUpdateExclusiveLock mode in order to
- * add them to a publication.
+ * The returned relations are locked in ShareUpdateExclusiveLock mode in order
+ * to add them to a publication.

3) Similarly here too:
/*
- * Add listed tables to the publication.
+ * Add listed relations to the publication.
*/

4) Similarly here too:
- /* Must be owner of the table or superuser. */
+ /* Must be owner of the relation or superuser. */

5) Ordering issue here, PublicationRelation should be after PublicationRelKind:
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ea95e7984bc..ff20501223c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2461,7 +2461,7 @@ PublicationPartOpt
PublicationRelInfo
PublicationRelKind
PublicationSchemaInfo
-PublicationTable
+PublicationRelation

Regards,
Vignesh

Hi Shlok.

Before this patch goes too far, I had a fundamental question.

I understand that sequences and tables are closely related; sequences
are just like a single-row table, but they have a RELKIND_SEQUENCE.

There is a lot of shared code internally, so I guess it is tempting to
specify both the published sequences and tables together in the System
Catalog 'pg_publication_rels'.

I'm just wondering whether that is really the best way to go?

Currently, pg_publication_rels has only tables. So they might be:
* only included tables -- from a publication using "FOR TABLE ..."
* only excluded tables -- from a publication using "FOR ALL TABLES
EXCEPT (TABLE ...)"

Because included/excluded tables cannot co-exist, we can easily know
the type of the CREATE/ALTER PUBLICATION command and the type of
'pg_publication_rels' content without digging deeper.

~~~

But introducing sequences introduces complexity. Now, AFAICT, we
cannot know what each row of 'pg_publication_rels' means without
inspecting the relation type of that row. e.g. now we have lots of
possible combinations like.

pg_publication_rels has:
* only included tables.
* only excluded tables.
* only excluded sequences.
* excluded tables and excluded sequences.
* included tables and excluded sequences.

Furthermore, there will be yet more combinations one day if the
individual "FOR SEQUENCE ..." syntax is implemented.

pg_publication_rels has:
* only included sequences
* included sequences and included tables
* included sequences and excluded tables

IIUC, it means that the SQL everywhere now requires joins and relkind
checks to identify the type.

~~

Furthermore, AFAICT, the 'pg_publication_rels' attributes 'prattrs'
and 'prquals' don't even have meaning for sequences. That's another
reason why it feels a bit like a square peg was jammed into a round
hole just because 'pg_publication_rels' was conveniently available.

~~

SUMMARY

Given:
* Option 1 = use 'pg_publication_rels' for both tables and sequences.
* Option 2 = use 'pg_publication_rels' just for tables and use a new
'pg_publication_seq' just for sequences.

I'm not convinced that the chosen way (Option 1) is better. Can you
explain why it is?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Vignesh and Shveta,
>
> Thanks for reviewing the patches.
> I have addressed the comments and attached the updated patch.
>
> This also addressed the comments shared by Shveta in [1].
> [1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com
>

Please find few comments on 001 and 002:

v-001:
1)
- List *except_tables; /* tables specified in the EXCEPT clause */
+ List *except_relations; /* relations specified in the EXCEPT
+ * clause */
Since we have not changed the comments for anything else in patch001,
we can keep this comment too same as old and changeit in 002.

v-002:
2)
pg_publication_rel's prrelid doc says:
Reference to table
We shall change it now to 'Reference to table or sequence'

3)
In doc, do we eed to change pg_publication_rel's prqual too? IMO, it
is not applicable to sequence and thus we can change 'relation' to
'table' in explanation..

4)
Marks the publication as one that synchronizes changes for all sequences
- in the database, including sequences created in the future.
+ in the database, including sequences created in the future. Sequences
+ listed in <literal>EXCEPT</literal> clause are excluded from the
+ publication.

I think we should place it the end of second paragraph rather than at
the end of first. How about something liek this:

Marks the publication as one that synchronizes changes for all
sequences in the database, including sequences created in the future.

Only persistent sequences are included in the publication. Temporary
sequences and unlogged sequences are excluded from the publication.
Sequences listed in EXCEPT clause are also excluded from the
publication.

5)
+ In such a case, a table or partition or sequence that is included in one
+ publication but excluded (explicitly or implicitly) by the
+ <literal>EXCEPT</literal> clause of another is considered included for
+ replication.

'a table or partition or sequence' can be changed to 'a table,
partition, or sequence'

6)
In existign doc, shall we give example of publication creation for
both tables and sequences, each having its except list? This is
important to show that EXCEPT to be given with individual ALL OBJ. We
can cahnge last example of doc file to make this. This one:
'Create a publication that publishes all sequences for
synchronization, and all changes in all tables except users and
departments:'

7)
getPublications:
+ * Get the list of tables/sequences for publications specified in the
+ * EXCEPT clause.

We can have both tables and sequences in single publication. We should change

'tables/sequences' --> tables and sequences

8)
In describePublications(),

We had:
if (!puballtables)
else
* Get tables in the EXCEPT clause for this publication */

now we have added:
if (puballsequences)
/* Get sequences in the EXCEPT clause for this publication */

Since now we can hit this function for 'all-seq' pub too, shall we
change if-block's condition to:

if (!puballtables && !puballsequences)

and else-block to

else if (puballtables)

otherwise all-seq case will unnecessary enter these blocks and will
exceute the logic

Please review other functions too in pg_dump to see if we need such
conditions altering.

9)
+# Check the initial data on subscriber
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT last_value, is_called FROM seq1");
+is($result, '200|t', 'sequences in EXCEPT list is excluded');
+
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT last_value, is_called FROM seq2");
+is($result, '200|t', 'initial test data replicated for seq2');

Since both are replicated now because of conflicting EXCEPT in
multi-pub, shall we change
comment in 'is(..)'?

thanks
Shveta

On Mon, Apr 13, 2026 at 9:52 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > Hi Vignesh and Shveta,
> >
> > Thanks for reviewing the patches.
> > I have addressed the comments and attached the updated patch.
>
> Few comments for the first patch:
> 1) I felt these also could be moved to first patch:
> - * Gets list of table oids that were specified in the EXCEPT clause for a
> + * Gets list of relation oids that were specified in the EXCEPT clause for a
>

I noticed all these, but since we are actually fetching sequences in
patch002,. I thought changing comments made more sense in patch002 as
done by Shlok currently. But I don't have a strong opinion here, so
fine with this too.

> 2) Similarly here too:
> - * The returned tables are locked in ShareUpdateExclusiveLock mode in order to
> - * add them to a publication.
> + * The returned relations are locked in ShareUpdateExclusiveLock mode in order
> + * to add them to a publication.
>
> 3) Similarly here too:
> /*
> - * Add listed tables to the publication.
> + * Add listed relations to the publication.
> */
>
> 4) Similarly here too:
> - /* Must be owner of the table or superuser. */
> + /* Must be owner of the relation or superuser. */
>
> 5) Ordering issue here, PublicationRelation should be after PublicationRelKind:
> diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
> index ea95e7984bc..ff20501223c 100644
> --- a/src/tools/pgindent/typedefs.list
> +++ b/src/tools/pgindent/typedefs.list
> @@ -2461,7 +2461,7 @@ PublicationPartOpt
> PublicationRelInfo
> PublicationRelKind
> PublicationSchemaInfo
> -PublicationTable
> +PublicationRelation
>
> Regards,
> Vignesh
>
>

On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Vignesh and Shveta,
>
> Thanks for reviewing the patches.
> I have addressed the comments and attached the updated patch.

Few comments for the second patch patch:
1) This documentation can handle "CREATE PUBLICATION
all_sequences_except FOR ALL SEQUENCES EXCEPT (SEQUENCE seq1, SEQUENCE
seq2);" but cannot handle "CREATE PUBLICATION all_sequences_except FOR
ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
", should we document similar to how it is done for except table:
+ ALL SEQUENCES [ EXCEPT ( <replaceable
class="parameter">except_sequence_object</replaceable> [, ... ] ) ]

<phrase>and <replaceable
class="parameter">table_and_columns</replaceable> is:</phrase>

@@ -46,6 +46,10 @@ CREATE PUBLICATION <replaceable
class="parameter">name</replaceable>
<phrase>and <replaceable class="parameter">table_object</replaceable>
is:</phrase>

[ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
+
+<phrase>and <replaceable
class="parameter">except_sequence_object</replaceable> is:</phrase>
+
+ SEQUENCE <replaceable
class="parameter">sequence_name</replaceable> [, ... ]

2) There is no comment when run with --echo-hidden, we can include the
following:
printfPQExpBuffer(&buf, "/* %s */\n",
_("Get publications that exclude
this sequence"));

3) "Except Publications" should be "Except publications"
+ if (tuples > 0)
+ {
+ printfPQExpBuffer(&tmpbuf,
_("Except Publications:"));

Check except table which handles similar:
+ /* Print publications where the sequence is in the
EXCEPT clause */
+ if (pset.sversion >= 190000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT pubname\n"
+ "FROM
pg_catalog.pg_publication p\n"
+ "JOIN
pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
+ "WHERE
pr.prrelid = '%s' AND pr.prexcept\n"
+ "ORDER BY 1;", oid);

4) Add one test for a publication having both EXCEPT TABLE AND EXCEPT SEQUENCE:
+ 'CREATE PUBLICATION pub11' => {
+ create_order => 92,
+ create_sql =>
+ 'CREATE PUBLICATION pub11 FOR ALL SEQUENCES EXCEPT
(SEQUENCE public.test_table_col1_seq);',
+ regexp => qr/^
+ \QCREATE PUBLICATION pub11 FOR ALL SEQUENCES
EXCEPT (SEQUENCE public.test_table_col1_seq) WITH (publish = 'insert,
update, delete, truncate');\E
+ /xm,
+ like => { %full_runs, section_post_data => 1, },
+ },

5) Similarly add one here too:
+-- Test ALL SEQUENCES with EXCEPT clause
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION regress_pub_forallsequences3 FOR ALL SEQUENCES
EXCEPT (SEQUENCE regress_pub_seq0, pub_test.regress_pub_seq1, SEQUENCE
regress_pub_seq2);
+\dRp+ regress_pub_forallsequences3
+-- Check that the sequence description shows the publications where
it is listed
+-- in the EXCEPT clause
+\d+ regress_pub_seq0

Regards,
Vignesh

Hi Shlok.

Here are a couple of review comments about the documentation (patch v2-0002).

======
doc/src/sgml/logical-replication.sgml

(29.1. Publication #)

1a.
- <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>
- clause.
+ <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
+ clause. When a publication is created with <literal>FOR ALL
SEQUENCES</literal>,
+ a sequence or set of sequences can be explicitly excluded from publication
+ using the
+ <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>

SUGGESTION
Similarly, when a publication is created with <literal>FOR ALL
SEQUENCES</literal>, a sequence or set of sequences can be explicitly
excluded from publication.

~~

1b.
Last year, I had suggested there should be an entirely separate
chapter in Logical Replication" for describing "Excluding objects from
the Publication". In the past, that idea was rejected because there
was only "EXCEPT tables". But now there is a growing list, so the idea
of having a separate chapter is becoming increasingly relevant. IMO, a
new chapter will be a good common place to describe everything, with
examples as necessary. It can help reduce some clutter from the CREATE
PUBLICATION page,

Then, this patch text (1a) could say something more like: "Specific
tables or sequences can be excluded from the publication. See XXX for
details."

======
doc/src/sgml/ref/create_publication.sgml

(EXCEPT)

2.
- This clause specifies a list of tables to be excluded from the
- publication.
+ This clause specifies a list of tables for <literal>ALL TABLES</literal>
+ publication or a list of sequences for <literal>ALL SEQUENCES</literal>
+ to be excluded from the publication.

SUGGESTION:
This clause specifies the tables or sequences to exclude from an
<literal>ALL TABLES</literal> or <literal>ALL SEQUENCES</literal>
publication.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Apr 13, 2026 at 9:58 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok.
>
> Before this patch goes too far, I had a fundamental question.
>
> I understand that sequences and tables are closely related; sequences
> are just like a single-row table, but they have a RELKIND_SEQUENCE.
>
> There is a lot of shared code internally, so I guess it is tempting to
> specify both the published sequences and tables together in the System
> Catalog 'pg_publication_rels'.
>
> I'm just wondering whether that is really the best way to go?
>
> Currently, pg_publication_rels has only tables. So they might be:
> * only included tables -- from a publication using "FOR TABLE ..."
> * only excluded tables -- from a publication using "FOR ALL TABLES
> EXCEPT (TABLE ...)"
>
> Because included/excluded tables cannot co-exist, we can easily know
> the type of the CREATE/ALTER PUBLICATION command and the type of
> 'pg_publication_rels' content without digging deeper.
>
> ~~~
>
> But introducing sequences introduces complexity. Now, AFAICT, we
> cannot know what each row of 'pg_publication_rels' means without
> inspecting the relation type of that row. e.g. now we have lots of
> possible combinations like.
>
> pg_publication_rels has:
> * only included tables.
> * only excluded tables.
> * only excluded sequences.
> * excluded tables and excluded sequences.
> * included tables and excluded sequences.
>
> Furthermore, there will be yet more combinations one day if the
> individual "FOR SEQUENCE ..." syntax is implemented.
>
> pg_publication_rels has:
> * only included sequences
> * included sequences and included tables
> * included sequences and excluded tables
>
> IIUC, it means that the SQL everywhere now requires joins and relkind
> checks to identify the type.
>
> ~~
>
> Furthermore, AFAICT, the 'pg_publication_rels' attributes 'prattrs'
> and 'prquals' don't even have meaning for sequences. That's another
> reason why it feels a bit like a square peg was jammed into a round
> hole just because 'pg_publication_rels' was conveniently available.
>
> ~~
>
> SUMMARY
>
> Given:
> * Option 1 = use 'pg_publication_rels' for both tables and sequences.
> * Option 2 = use 'pg_publication_rels' just for tables and use a new
> 'pg_publication_seq' just for sequences.
>
> I'm not convinced that the chosen way (Option 1) is better. Can you
> explain why it is?
>

pg_publication_seq seems like an idea worth considering. After giving
it some thought, we already have a view named
pg_publication_sequences, and adding another catalog with a similar
name may introduce confusion. Apart from that, here are my thoughts:

1)
pg_publication_rels already represents 'relations', and thus I feel
sequences already fit naturally into this.

2)
I agree that relkind checks may be required, but IMO these are
typically cheap. In C code paths we already rely heavily on
cache-lookups such as get_rel_relkind, and thus pg_class.relkind
access is well-optimized. This is already used widely across the
codebase, so IMO, extending it here does not introduce meaningful
overhead.

3)
Looking at the code paths accessing pg_publication_rels, they broadly
fall into 4 categories:

a) Publication DDL (CREATE/ALTER/DROP PUBLICATION)
b) Tools and queries (e.g., pg_publication_tables view, pg_dump)
c) Logical decoding (pgoutput paths like get_rel_sync_entry,
pgoutput_column_list_init, pgoutput_row_filter_init)
d) DML paths (INSERT/UPDATE) to check publication membership and RI
applicability (CheckCmdReplicaIdentity)

The first two are infrequent operations. For the logical decoding
path, relation information is cached in RelationSyncCache, so relkind
checks are not repeatedly performed in the hot path. For INSERT/UPDATE
paths, PublicationDesc (rd_pubdesc) is built once per relation and is
cached and reused, so pg_publication_rels is not accessed heavily here
too.

~~

Considering all the above, I feel we should be good with Option 1.
But, I would like to see what others think on this.

thanks
Shveta

On Mon, 13 Apr 2026 at 15:46, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Mon, Apr 13, 2026 at 9:58 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Hi Shlok.
> >
> > Before this patch goes too far, I had a fundamental question.
> >
> > I understand that sequences and tables are closely related; sequences
> > are just like a single-row table, but they have a RELKIND_SEQUENCE.
> >
> > There is a lot of shared code internally, so I guess it is tempting to
> > specify both the published sequences and tables together in the System
> > Catalog 'pg_publication_rels'.
> >
> > I'm just wondering whether that is really the best way to go?
> >
> > Currently, pg_publication_rels has only tables. So they might be:
> > * only included tables -- from a publication using "FOR TABLE ..."
> > * only excluded tables -- from a publication using "FOR ALL TABLES
> > EXCEPT (TABLE ...)"
> >
> > Because included/excluded tables cannot co-exist, we can easily know
> > the type of the CREATE/ALTER PUBLICATION command and the type of
> > 'pg_publication_rels' content without digging deeper.
> >
> > ~~~
> >
> > But introducing sequences introduces complexity. Now, AFAICT, we
> > cannot know what each row of 'pg_publication_rels' means without
> > inspecting the relation type of that row. e.g. now we have lots of
> > possible combinations like.
> >
> > pg_publication_rels has:
> > * only included tables.
> > * only excluded tables.
> > * only excluded sequences.
> > * excluded tables and excluded sequences.
> > * included tables and excluded sequences.
> >
> > Furthermore, there will be yet more combinations one day if the
> > individual "FOR SEQUENCE ..." syntax is implemented.
> >
> > pg_publication_rels has:
> > * only included sequences
> > * included sequences and included tables
> > * included sequences and excluded tables
> >
> > IIUC, it means that the SQL everywhere now requires joins and relkind
> > checks to identify the type.
> >
> > ~~
> >
> > Furthermore, AFAICT, the 'pg_publication_rels' attributes 'prattrs'
> > and 'prquals' don't even have meaning for sequences. That's another
> > reason why it feels a bit like a square peg was jammed into a round
> > hole just because 'pg_publication_rels' was conveniently available.
> >
> > ~~
> >
> > SUMMARY
> >
> > Given:
> > * Option 1 = use 'pg_publication_rels' for both tables and sequences.
> > * Option 2 = use 'pg_publication_rels' just for tables and use a new
> > 'pg_publication_seq' just for sequences.
> >
> > I'm not convinced that the chosen way (Option 1) is better. Can you
> > explain why it is?
> >
>
> pg_publication_seq seems like an idea worth considering. After giving
> it some thought, we already have a view named
> pg_publication_sequences, and adding another catalog with a similar
> name may introduce confusion. Apart from that, here are my thoughts:
>
> 1)
> pg_publication_rels already represents 'relations', and thus I feel
> sequences already fit naturally into this.
>
> 2)
> I agree that relkind checks may be required, but IMO these are
> typically cheap. In C code paths we already rely heavily on
> cache-lookups such as get_rel_relkind, and thus pg_class.relkind
> access is well-optimized. This is already used widely across the
> codebase, so IMO, extending it here does not introduce meaningful
> overhead.
>
> 3)
> Looking at the code paths accessing pg_publication_rels, they broadly
> fall into 4 categories:
>
> a) Publication DDL (CREATE/ALTER/DROP PUBLICATION)
> b) Tools and queries (e.g., pg_publication_tables view, pg_dump)
> c) Logical decoding (pgoutput paths like get_rel_sync_entry,
> pgoutput_column_list_init, pgoutput_row_filter_init)
> d) DML paths (INSERT/UPDATE) to check publication membership and RI
> applicability (CheckCmdReplicaIdentity)
>
> The first two are infrequent operations. For the logical decoding
> path, relation information is cached in RelationSyncCache, so relkind
> checks are not repeatedly performed in the hot path. For INSERT/UPDATE
> paths, PublicationDesc (rd_pubdesc) is built once per relation and is
> cached and reused, so pg_publication_rels is not accessed heavily here
> too.
>
> ~~
>
> Considering all the above, I feel we should be good with Option 1.
> But, I would like to see what others think on this.

+1 for option 1 and checking relkind for the same reason mentioned by
shveta above. It seems like a reasonable tradeoff compared to
introducing and maintaining a separate pg_publication_seq system
table.

Regards,
Vignesh

On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, 10 Apr 2026 at 16:31, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Fri, 10 Apr 2026 at 11:39, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > Hi hackers,
> > >
> > > Thread [1] introduced support for the EXCEPT clause for publications
> > > defined with ALL TABLES. To extend this functionality, as discussed in
> > > [2], this patch series adds support for the EXCEPT clause for
> > > publications defined with ALL SEQUENCES.
> > >
> > > The series consists of the following patches:
> > >
> > > 0001: Support EXCEPT for ALL SEQUENCES in CREATE PUBLICATION
> > > This allows excluding specific sequences when using CREATE PUBLICATION
> > > ... FOR ALL SEQUENCES.
> > > Example:
> > > CREATE PUBLICATION pub1 FOR ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> > >
> > > 0002: Support EXCEPT for ALL SEQUENCES in ALTER PUBLICATION
> > > This extends ALTER PUBLICATION to manage exclusions for ALL SEQUENCES.
> > > Examples:
> > > ALTER PUBLICATION pub1 SET ALL SEQUENCES;
> > > This clears any existing sequence exclusions.
> > >
> > > ALTER PUBLICATION pub1 SET ALL SEQUENCES EXCEPT (SEQUENCE s1, s2);
> > > This replaces the exclusion list with the specified sequences.
> > >
> > > Sequences listed in the EXCEPT clause are excluded from the
> > > publication and are not replicated to the subscriber.
> > >
> > > This brings sequence publication behavior in line with the existing
> > > support for FOR ALL TABLES ... EXCEPT.
> >
> > Few comments on first patch:
> > 1) There are few warnings while building:
> > gram.y: warning: 1 nonterminal useless in grammar [-Wother]
> > gram.y: warning: 2 rules useless in grammar [-Wother]
> > gram.y:14038.1-12: warning: nonterminal useless in grammar:
> > opt_sequence [-Wother]
> > 14038 | opt_sequence: SEQUENCE
> > | ^~~~~~~~~~~~
> > preproc.y: warning: 1 nonterminal useless in grammar [-Wother]
> > preproc.y: warning: 2 rules useless in grammar [-Wother]
> > preproc.y:5589.2-13: warning: nonterminal useless in grammar:
> > opt_sequence [-Wother]
> > 5589 | opt_sequence:
> > | ^~~~~~~~~~~~
> >
> > 2) The renaming of table to relation can be moved to a separate patch
> > and kept as 0001 patch:
> > 2.a) pubtable to pubrelation
> > - $$->pubtable =
> > makeNode(PublicationTable);
> > - $$->pubtable->relation
> > = makeRangeVar(NULL, $1, @1);
> > - $$->pubtable->columns = $2;
> > - $$->pubtable->whereClause = $3;
> > + $$->pubrelation =
> > makeNode(PublicationRelation);
> > +
> > $$->pubrelation->relation = makeRangeVar(NULL, $1, @1);
> > + $$->pubrelation->columns = $2;
> > +
> > $$->pubrelation->whereClause = $3;
> >
> > 2.b) except_tables to except_relations
> > - $$->except_tables = $3;
> > + $$->except_relations = $3;
> >
> > similarly there may be few others
> >
> > 3) In case of except table we have used "Except tables:", we can
> > change it to "Except sequences:" to keep it consistent:
> > + if (!addFooterToPublicationDesc(&buf,
> > _("Except Sequences:"),
> > +
> > true, &cont))
> > + goto error_return;
> >
> > Also it is agreed at [1] to use lower case for the second one.
> >
> > 4) add one example in doc for except (sequence)
> >
> > 5) Tab completion for "create publication pub1 for all sequences
> > except (" is missing:
> >
> > + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
> > "ALL", "SEQUENCES", "EXCEPT"))
> > + COMPLETE_WITH("( SEQUENCE");
> > + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR",
> > "ALL", "SEQUENCES", "EXCEPT", "(", "SEQUENCE"))
> > + COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);
> >
> > [1] - https://www.postgresql.org/message-id/flat/CAHut%2BPt3t_tCYwDStkj5fG4Z%3DYXrHvPBA7iGdh745QipC5zKeg%40mail.gmail.com
> >
> Hi Vignesh and Shveta,
>
> Thanks for reviewing the patches.
> I have addressed the comments and attached the updated patch.

One issue with the alter publication patch:
Tab completion lists tables instead of sequences here:
postgres=# alter publication pub1 set all sequences EXCEPT ( SEQUENCE
information_schema. public. t1 t2
tbl1 tbl2

Here Query_for_list_of_tables should be Query_for_list_of_sequences:
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET",
"ALL", "SEQUENCES", "EXCEPT", "(", "SEQUENCE"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET",
"ALL", "SEQUENCES", "EXCEPT", "(", "SEQUENCE", MatchAnyN) &&
ends_with(prev_wd, ','))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);

Regards,
Vignesh

On Mon, Apr 13, 2026 at 10:48 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > Hi Vignesh and Shveta,
> >
> > Thanks for reviewing the patches.
> > I have addressed the comments and attached the updated patch.
> >
> > This also addressed the comments shared by Shveta in [1].
> > [1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com
> >
>
> Please find few comments on 001 and 002:
>
>
> v-001:
> 1)
> - List *except_tables; /* tables specified in the EXCEPT clause */
> + List *except_relations; /* relations specified in the EXCEPT
> + * clause */
> Since we have not changed the comments for anything else in patch001,
> we can keep this comment too same as old and changeit in 002.
>
>
> v-002:
> 2)
> pg_publication_rel's prrelid doc says:
> Reference to table
> We shall change it now to 'Reference to table or sequence'
>
> 3)
> In doc, do we eed to change pg_publication_rel's prqual too? IMO, it
> is not applicable to sequence and thus we can change 'relation' to
> 'table' in explanation..
>
> 4)
> Marks the publication as one that synchronizes changes for all sequences
> - in the database, including sequences created in the future.
> + in the database, including sequences created in the future. Sequences
> + listed in <literal>EXCEPT</literal> clause are excluded from the
> + publication.
>
> I think we should place it the end of second paragraph rather than at
> the end of first. How about something liek this:
>
> Marks the publication as one that synchronizes changes for all
> sequences in the database, including sequences created in the future.
>
> Only persistent sequences are included in the publication. Temporary
> sequences and unlogged sequences are excluded from the publication.
> Sequences listed in EXCEPT clause are also excluded from the
> publication.
>
> 5)
> + In such a case, a table or partition or sequence that is included in one
> + publication but excluded (explicitly or implicitly) by the
> + <literal>EXCEPT</literal> clause of another is considered included for
> + replication.
>
> 'a table or partition or sequence' can be changed to 'a table,
> partition, or sequence'
>
> 6)
> In existign doc, shall we give example of publication creation for
> both tables and sequences, each having its except list? This is
> important to show that EXCEPT to be given with individual ALL OBJ. We
> can cahnge last example of doc file to make this. This one:
> 'Create a publication that publishes all sequences for
> synchronization, and all changes in all tables except users and
> departments:'
>
> 7)
> getPublications:
> + * Get the list of tables/sequences for publications specified in the
> + * EXCEPT clause.
>
> We can have both tables and sequences in single publication. We should change
>
> 'tables/sequences' --> tables and sequences
>
> 8)
> In describePublications(),
>
> We had:
> if (!puballtables)
> else
> * Get tables in the EXCEPT clause for this publication */
>
> now we have added:
> if (puballsequences)
> /* Get sequences in the EXCEPT clause for this publication */
>
> Since now we can hit this function for 'all-seq' pub too, shall we
> change if-block's condition to:
>
> if (!puballtables && !puballsequences)
>
> and else-block to
>
> else if (puballtables)
>
> otherwise all-seq case will unnecessary enter these blocks and will
> exceute the logic
>
> Please review other functions too in pg_dump to see if we need such
> conditions altering.
>
>
> 9)
> +# Check the initial data on subscriber
> +$result = $node_subscriber->safe_psql('postgres',
> + "SELECT last_value, is_called FROM seq1");
> +is($result, '200|t', 'sequences in EXCEPT list is excluded');
> +
> +$result = $node_subscriber->safe_psql('postgres',
> + "SELECT last_value, is_called FROM seq2");
> +is($result, '200|t', 'initial test data replicated for seq2');
>
> Since both are replicated now because of conflicting EXCEPT in
> multi-pub, shall we change
> comment in 'is(..)'?
>

For v-003, one trivial thing:

Shall we change the name of AlterPublicationTables() as well? It now
deals in both tables and sequences.

thanks
Shveta

On Mon, 13 Apr 2026 at 10:51, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Mon, Apr 13, 2026 at 9:52 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > > Hi Vignesh and Shveta,
> > >
> > > Thanks for reviewing the patches.
> > > I have addressed the comments and attached the updated patch.
> >
> > Few comments for the first patch:
> > 1) I felt these also could be moved to first patch:
> > - * Gets list of table oids that were specified in the EXCEPT clause for a
> > + * Gets list of relation oids that were specified in the EXCEPT clause for a
> >
>
> I noticed all these, but since we are actually fetching sequences in
> patch002,. I thought changing comments made more sense in patch002 as
> done by Shlok currently. But I don't have a strong opinion here, so
> fine with this too.
>
I agree with Shveta and had the same thought while splitting the
patch. Since we start fetching sequences in patch 0002, it makes more
sense to keep the comment changes there. I will keep the changes in
patch 0002.

> > 2) Similarly here too:
> > - * The returned tables are locked in ShareUpdateExclusiveLock mode in order to
> > - * add them to a publication.
> > + * The returned relations are locked in ShareUpdateExclusiveLock mode in order
> > + * to add them to a publication.
> >
> > 3) Similarly here too:
> > /*
> > - * Add listed tables to the publication.
> > + * Add listed relations to the publication.
> > */
> >
> > 4) Similarly here too:
> > - /* Must be owner of the table or superuser. */
> > + /* Must be owner of the relation or superuser. */
> >
> > 5) Ordering issue here, PublicationRelation should be after PublicationRelKind:
> > diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
> > index ea95e7984bc..ff20501223c 100644
> > --- a/src/tools/pgindent/typedefs.list
> > +++ b/src/tools/pgindent/typedefs.list
> > @@ -2461,7 +2461,7 @@ PublicationPartOpt
> > PublicationRelInfo
> > PublicationRelKind
> > PublicationSchemaInfo
> > -PublicationTable
> > +PublicationRelation
> >
> > Regards,
> > Vignesh
> >
> >

On Mon, 13 Apr 2026 at 16:21, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 13 Apr 2026 at 15:46, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Mon, Apr 13, 2026 at 9:58 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > Hi Shlok.
> > >
> > > Before this patch goes too far, I had a fundamental question.
> > >
> > > I understand that sequences and tables are closely related; sequences
> > > are just like a single-row table, but they have a RELKIND_SEQUENCE.
> > >
> > > There is a lot of shared code internally, so I guess it is tempting to
> > > specify both the published sequences and tables together in the System
> > > Catalog 'pg_publication_rels'.
> > >
> > > I'm just wondering whether that is really the best way to go?
> > >
> > > Currently, pg_publication_rels has only tables. So they might be:
> > > * only included tables -- from a publication using "FOR TABLE ..."
> > > * only excluded tables -- from a publication using "FOR ALL TABLES
> > > EXCEPT (TABLE ...)"
> > >
> > > Because included/excluded tables cannot co-exist, we can easily know
> > > the type of the CREATE/ALTER PUBLICATION command and the type of
> > > 'pg_publication_rels' content without digging deeper.
> > >
> > > ~~~
> > >
> > > But introducing sequences introduces complexity. Now, AFAICT, we
> > > cannot know what each row of 'pg_publication_rels' means without
> > > inspecting the relation type of that row. e.g. now we have lots of
> > > possible combinations like.
> > >
> > > pg_publication_rels has:
> > > * only included tables.
> > > * only excluded tables.
> > > * only excluded sequences.
> > > * excluded tables and excluded sequences.
> > > * included tables and excluded sequences.
> > >
> > > Furthermore, there will be yet more combinations one day if the
> > > individual "FOR SEQUENCE ..." syntax is implemented.
> > >
> > > pg_publication_rels has:
> > > * only included sequences
> > > * included sequences and included tables
> > > * included sequences and excluded tables
> > >
> > > IIUC, it means that the SQL everywhere now requires joins and relkind
> > > checks to identify the type.
> > >
> > > ~~
> > >
> > > Furthermore, AFAICT, the 'pg_publication_rels' attributes 'prattrs'
> > > and 'prquals' don't even have meaning for sequences. That's another
> > > reason why it feels a bit like a square peg was jammed into a round
> > > hole just because 'pg_publication_rels' was conveniently available.
> > >
> > > ~~
> > >
> > > SUMMARY
> > >
> > > Given:
> > > * Option 1 = use 'pg_publication_rels' for both tables and sequences.
> > > * Option 2 = use 'pg_publication_rels' just for tables and use a new
> > > 'pg_publication_seq' just for sequences.
> > >
> > > I'm not convinced that the chosen way (Option 1) is better. Can you
> > > explain why it is?
> > >
> >
> > pg_publication_seq seems like an idea worth considering. After giving
> > it some thought, we already have a view named
> > pg_publication_sequences, and adding another catalog with a similar
> > name may introduce confusion. Apart from that, here are my thoughts:
> >
> > 1)
> > pg_publication_rels already represents 'relations', and thus I feel
> > sequences already fit naturally into this.
> >
> > 2)
> > I agree that relkind checks may be required, but IMO these are
> > typically cheap. In C code paths we already rely heavily on
> > cache-lookups such as get_rel_relkind, and thus pg_class.relkind
> > access is well-optimized. This is already used widely across the
> > codebase, so IMO, extending it here does not introduce meaningful
> > overhead.
> >
> > 3)
> > Looking at the code paths accessing pg_publication_rels, they broadly
> > fall into 4 categories:
> >
> > a) Publication DDL (CREATE/ALTER/DROP PUBLICATION)
> > b) Tools and queries (e.g., pg_publication_tables view, pg_dump)
> > c) Logical decoding (pgoutput paths like get_rel_sync_entry,
> > pgoutput_column_list_init, pgoutput_row_filter_init)
> > d) DML paths (INSERT/UPDATE) to check publication membership and RI
> > applicability (CheckCmdReplicaIdentity)
> >
> > The first two are infrequent operations. For the logical decoding
> > path, relation information is cached in RelationSyncCache, so relkind
> > checks are not repeatedly performed in the hot path. For INSERT/UPDATE
> > paths, PublicationDesc (rd_pubdesc) is built once per relation and is
> > cached and reused, so pg_publication_rels is not accessed heavily here
> > too.
> >
> > ~~
> >
> > Considering all the above, I feel we should be good with Option 1.
> > But, I would like to see what others think on this.
>
> +1 for option 1 and checking relkind for the same reason mentioned by
> shveta above. It seems like a reasonable tradeoff compared to
> introducing and maintaining a separate pg_publication_seq system
> table.
>
Thanks Peter, Shveta, and Vignesh for the analysis.

I agree with the points shared by Shveta and Vignesh and have verified
them. Based on the reasoning, I think Option 1 is better. I will
retain this design and use the pg_publication_rel to store sequences.

Thanks,
Shlok Kyal

On Tue, 14 Apr 2026 at 15:12, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Mon, Apr 13, 2026 at 10:48 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > Hi Vignesh and Shveta,
> > >
> > > Thanks for reviewing the patches.
> > > I have addressed the comments and attached the updated patch.
> > >
> > > This also addressed the comments shared by Shveta in [1].
> > > [1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com
> > >
> >
> > Please find few comments on 001 and 002:
> >
> >
> > v-001:
> > 1)
> > - List *except_tables; /* tables specified in the EXCEPT clause */
> > + List *except_relations; /* relations specified in the EXCEPT
> > + * clause */
> > Since we have not changed the comments for anything else in patch001,
> > we can keep this comment too same as old and changeit in 002.
> >
> >
> > v-002:
> > 2)
> > pg_publication_rel's prrelid doc says:
> > Reference to table
> > We shall change it now to 'Reference to table or sequence'
> >
> > 3)
> > In doc, do we eed to change pg_publication_rel's prqual too? IMO, it
> > is not applicable to sequence and thus we can change 'relation' to
> > 'table' in explanation..
> >
> > 4)
> > Marks the publication as one that synchronizes changes for all sequences
> > - in the database, including sequences created in the future.
> > + in the database, including sequences created in the future. Sequences
> > + listed in <literal>EXCEPT</literal> clause are excluded from the
> > + publication.
> >
> > I think we should place it the end of second paragraph rather than at
> > the end of first. How about something liek this:
> >
> > Marks the publication as one that synchronizes changes for all
> > sequences in the database, including sequences created in the future.
> >
> > Only persistent sequences are included in the publication. Temporary
> > sequences and unlogged sequences are excluded from the publication.
> > Sequences listed in EXCEPT clause are also excluded from the
> > publication.
> >
> > 5)
> > + In such a case, a table or partition or sequence that is included in one
> > + publication but excluded (explicitly or implicitly) by the
> > + <literal>EXCEPT</literal> clause of another is considered included for
> > + replication.
> >
> > 'a table or partition or sequence' can be changed to 'a table,
> > partition, or sequence'
> >
> > 6)
> > In existign doc, shall we give example of publication creation for
> > both tables and sequences, each having its except list? This is
> > important to show that EXCEPT to be given with individual ALL OBJ. We
> > can cahnge last example of doc file to make this. This one:
> > 'Create a publication that publishes all sequences for
> > synchronization, and all changes in all tables except users and
> > departments:'
> >
> > 7)
> > getPublications:
> > + * Get the list of tables/sequences for publications specified in the
> > + * EXCEPT clause.
> >
> > We can have both tables and sequences in single publication. We should change
> >
> > 'tables/sequences' --> tables and sequences
> >
> > 8)
> > In describePublications(),
> >
> > We had:
> > if (!puballtables)
> > else
> > * Get tables in the EXCEPT clause for this publication */
> >
> > now we have added:
> > if (puballsequences)
> > /* Get sequences in the EXCEPT clause for this publication */
> >
> > Since now we can hit this function for 'all-seq' pub too, shall we
> > change if-block's condition to:
> >
> > if (!puballtables && !puballsequences)
> >
> > and else-block to
> >
> > else if (puballtables)
> >
> > otherwise all-seq case will unnecessary enter these blocks and will
> > exceute the logic
> >
> > Please review other functions too in pg_dump to see if we need such
> > conditions altering.
> >
> >
> > 9)
> > +# Check the initial data on subscriber
> > +$result = $node_subscriber->safe_psql('postgres',
> > + "SELECT last_value, is_called FROM seq1");
> > +is($result, '200|t', 'sequences in EXCEPT list is excluded');
> > +
> > +$result = $node_subscriber->safe_psql('postgres',
> > + "SELECT last_value, is_called FROM seq2");
> > +is($result, '200|t', 'initial test data replicated for seq2');
> >
> > Since both are replicated now because of conflicting EXCEPT in
> > multi-pub, shall we change
> > comment in 'is(..)'?
> >
>
>
> For v-003, one trivial thing:
>
> Shall we change the name of AlterPublicationTables() as well? It now
> deals in both tables and sequences.
>
Thanks for reviewing the patch. I agree that we should change the name
here. Modified the patch.

I have also addressed the remaining comments by you and Vignesh in [1], [2], [3]
Attached the updated version.

[1]: https://www.postgresql.org/message-id/CALDaNm2pGi3FAkN+x+10nqFKNHOUdMwEgqt_TtjLbvz04F3Ktg@mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAJpy0uBB4N8KOrHchdgprVi2Ws1+gTcEr+bC2A_ziAHOcZcTqA@mail.gmail.com
[3]: https://www.postgresql.org/message-id/CALDaNm1BRQ1s9na_gwLwN3BYER9be+4QNn4V8sDjiMUvao28Jg@mail.gmail.com

Thanks,
Shlok Kyal

On Mon, 13 Apr 2026 at 11:23, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Sun, 12 Apr 2026 at 00:32, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > Hi Vignesh and Shveta,
> >
> > Thanks for reviewing the patches.
> > I have addressed the comments and attached the updated patch.
>
> Few comments for the second patch patch:
> 1) This documentation can handle "CREATE PUBLICATION
> all_sequences_except FOR ALL SEQUENCES EXCEPT (SEQUENCE seq1, SEQUENCE
> seq2);" but cannot handle "CREATE PUBLICATION all_sequences_except FOR
> ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
> ", should we document similar to how it is done for except table:
> + ALL SEQUENCES [ EXCEPT ( <replaceable
> class="parameter">except_sequence_object</replaceable> [, ... ] ) ]
>
> <phrase>and <replaceable
> class="parameter">table_and_columns</replaceable> is:</phrase>
>
> @@ -46,6 +46,10 @@ CREATE PUBLICATION <replaceable
> class="parameter">name</replaceable>
> <phrase>and <replaceable class="parameter">table_object</replaceable>
> is:</phrase>
>
> [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
> +
> +<phrase>and <replaceable
> class="parameter">except_sequence_object</replaceable> is:</phrase>
> +
> + SEQUENCE <replaceable
> class="parameter">sequence_name</replaceable> [, ... ]

I checked the syntax and think that it is correct.

The syntax is as follows:
ALL SEQUENCES [ EXCEPT ( <replaceable
class="parameter">except_sequence_object</replaceable> [, ... ] ) ]
.
.
<phrase>and <replaceable
class="parameter">except_sequence_object</replaceable> is:</phrase>

SEQUENCE <replaceable class="parameter">sequence_name</replaceable> [, ... ]

The [,..] mentioned after "sequence_name" will handle the case:
CREATE PUBLICATION all_sequences_except FOR ALL SEQUENCES EXCEPT
(SEQUENCE seq1, seq2)
and the [,..] mentioned after "except_sequence_object" will handle the case:
CREATE PUBLICATION all_sequences_except FOR ALL SEQUENCES EXCEPT
(SEQUENCE seq1, SEQUENCE seq2);

And the syntax added is similar to the existing syntax for ALL TABLES
(EXCEPT TABLE .. ) case.
So, I think the syntax is correct. Am I missing something?

>
> 2) There is no comment when run with --echo-hidden, we can include the
> following:
> printfPQExpBuffer(&buf, "/* %s */\n",
> _("Get publications that exclude
> this sequence"));
>
> 3) "Except Publications" should be "Except publications"
> + if (tuples > 0)
> + {
> + printfPQExpBuffer(&tmpbuf,
> _("Except Publications:"));
>
> Check except table which handles similar:
> + /* Print publications where the sequence is in the
> EXCEPT clause */
> + if (pset.sversion >= 190000)
> + {
> + printfPQExpBuffer(&buf,
> + "SELECT pubname\n"
> + "FROM
> pg_catalog.pg_publication p\n"
> + "JOIN
> pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
> + "WHERE
> pr.prrelid = '%s' AND pr.prexcept\n"
> + "ORDER BY 1;", oid);
>
>
> 4) Add one test for a publication having both EXCEPT TABLE AND EXCEPT SEQUENCE:
> + 'CREATE PUBLICATION pub11' => {
> + create_order => 92,
> + create_sql =>
> + 'CREATE PUBLICATION pub11 FOR ALL SEQUENCES EXCEPT
> (SEQUENCE public.test_table_col1_seq);',
> + regexp => qr/^
> + \QCREATE PUBLICATION pub11 FOR ALL SEQUENCES
> EXCEPT (SEQUENCE public.test_table_col1_seq) WITH (publish = 'insert,
> update, delete, truncate');\E
> + /xm,
> + like => { %full_runs, section_post_data => 1, },
> + },
>
> 5) Similarly add one here too:
> +-- Test ALL SEQUENCES with EXCEPT clause
> +SET client_min_messages = 'ERROR';
> +CREATE PUBLICATION regress_pub_forallsequences3 FOR ALL SEQUENCES
> EXCEPT (SEQUENCE regress_pub_seq0, pub_test.regress_pub_seq1, SEQUENCE
> regress_pub_seq2);
> +\dRp+ regress_pub_forallsequences3
> +-- Check that the sequence description shows the publications where
> it is listed
> +-- in the EXCEPT clause
> +\d+ regress_pub_seq0
>

I have addressed the remaining comments in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEU_Yq9ZJ2n5Sqa7RoHze0TD0RGxLQQgV1F6Jm2AROEh8g%40mail.gmail.com

Thanks,
Shlok Kyal

On Mon, 13 Apr 2026 at 13:17, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok.
>
> Here are a couple of review comments about the documentation (patch v2-0002).
>
> ======
> doc/src/sgml/logical-replication.sgml
>
> (29.1. Publication #)
>
> 1a.
> - <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>
> - clause.
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
> + clause. When a publication is created with <literal>FOR ALL
> SEQUENCES</literal>,
> + a sequence or set of sequences can be explicitly excluded from publication
> + using the
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
>
> SUGGESTION
> Similarly, when a publication is created with <literal>FOR ALL
> SEQUENCES</literal>, a sequence or set of sequences can be explicitly
> excluded from publication.
>
> ~~
>
> 1b.
> Last year, I had suggested there should be an entirely separate
> chapter in Logical Replication" for describing "Excluding objects from
> the Publication". In the past, that idea was rejected because there
> was only "EXCEPT tables". But now there is a growing list, so the idea
> of having a separate chapter is becoming increasingly relevant. IMO, a
> new chapter will be a good common place to describe everything, with
> examples as necessary. It can help reduce some clutter from the CREATE
> PUBLICATION page,
>
> Then, this patch text (1a) could say something more like: "Specific
> tables or sequences can be excluded from the publication. See XXX for
> details."
>
The documentation changes introduced by these patches are small and
don’t add much complexity, so to me a separate section doesn’t seem
necessary at this point. I’d be interested in hearing others’ views on
whether we should create a new chapter.

> ======
> doc/src/sgml/ref/create_publication.sgml
>
> (EXCEPT)
>
> 2.
> - This clause specifies a list of tables to be excluded from the
> - publication.
> + This clause specifies a list of tables for <literal>ALL TABLES</literal>
> + publication or a list of sequences for <literal>ALL SEQUENCES</literal>
> + to be excluded from the publication.
>
> SUGGESTION:
> This clause specifies the tables or sequences to exclude from an
> <literal>ALL TABLES</literal> or <literal>ALL SEQUENCES</literal>
> publication.
>
I have addressed the remaining comments in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEU_Yq9ZJ2n5Sqa7RoHze0TD0RGxLQQgV1F6Jm2AROEh8g%40mail.gmail.com

Thanks,
Shlok Kyal

On Tue, 14 Apr 2026 at 18:12, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Tue, 14 Apr 2026 at 15:12, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Mon, Apr 13, 2026 at 10:48 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > > >
> > > > Hi Vignesh and Shveta,
> > > >
> > > > Thanks for reviewing the patches.
> > > > I have addressed the comments and attached the updated patch.
> > > >
> > > > This also addressed the comments shared by Shveta in [1].
> > > > [1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com
> > > >
> > >
> > > Please find few comments on 001 and 002:
> > >
> > >
> > > v-001:
> > > 1)
> > > - List *except_tables; /* tables specified in the EXCEPT clause */
> > > + List *except_relations; /* relations specified in the EXCEPT
> > > + * clause */
> > > Since we have not changed the comments for anything else in patch001,
> > > we can keep this comment too same as old and changeit in 002.
> > >
> > >
> > > v-002:
> > > 2)
> > > pg_publication_rel's prrelid doc says:
> > > Reference to table
> > > We shall change it now to 'Reference to table or sequence'
> > >
> > > 3)
> > > In doc, do we eed to change pg_publication_rel's prqual too? IMO, it
> > > is not applicable to sequence and thus we can change 'relation' to
> > > 'table' in explanation..
> > >
> > > 4)
> > > Marks the publication as one that synchronizes changes for all sequences
> > > - in the database, including sequences created in the future.
> > > + in the database, including sequences created in the future. Sequences
> > > + listed in <literal>EXCEPT</literal> clause are excluded from the
> > > + publication.
> > >
> > > I think we should place it the end of second paragraph rather than at
> > > the end of first. How about something liek this:
> > >
> > > Marks the publication as one that synchronizes changes for all
> > > sequences in the database, including sequences created in the future.
> > >
> > > Only persistent sequences are included in the publication. Temporary
> > > sequences and unlogged sequences are excluded from the publication.
> > > Sequences listed in EXCEPT clause are also excluded from the
> > > publication.
> > >
> > > 5)
> > > + In such a case, a table or partition or sequence that is included in one
> > > + publication but excluded (explicitly or implicitly) by the
> > > + <literal>EXCEPT</literal> clause of another is considered included for
> > > + replication.
> > >
> > > 'a table or partition or sequence' can be changed to 'a table,
> > > partition, or sequence'
> > >
> > > 6)
> > > In existign doc, shall we give example of publication creation for
> > > both tables and sequences, each having its except list? This is
> > > important to show that EXCEPT to be given with individual ALL OBJ. We
> > > can cahnge last example of doc file to make this. This one:
> > > 'Create a publication that publishes all sequences for
> > > synchronization, and all changes in all tables except users and
> > > departments:'
> > >
> > > 7)
> > > getPublications:
> > > + * Get the list of tables/sequences for publications specified in the
> > > + * EXCEPT clause.
> > >
> > > We can have both tables and sequences in single publication. We should change
> > >
> > > 'tables/sequences' --> tables and sequences
> > >
> > > 8)
> > > In describePublications(),
> > >
> > > We had:
> > > if (!puballtables)
> > > else
> > > * Get tables in the EXCEPT clause for this publication */
> > >
> > > now we have added:
> > > if (puballsequences)
> > > /* Get sequences in the EXCEPT clause for this publication */
> > >
> > > Since now we can hit this function for 'all-seq' pub too, shall we
> > > change if-block's condition to:
> > >
> > > if (!puballtables && !puballsequences)
> > >
> > > and else-block to
> > >
> > > else if (puballtables)
> > >
> > > otherwise all-seq case will unnecessary enter these blocks and will
> > > exceute the logic
> > >
> > > Please review other functions too in pg_dump to see if we need such
> > > conditions altering.
> > >
> > >
> > > 9)
> > > +# Check the initial data on subscriber
> > > +$result = $node_subscriber->safe_psql('postgres',
> > > + "SELECT last_value, is_called FROM seq1");
> > > +is($result, '200|t', 'sequences in EXCEPT list is excluded');
> > > +
> > > +$result = $node_subscriber->safe_psql('postgres',
> > > + "SELECT last_value, is_called FROM seq2");
> > > +is($result, '200|t', 'initial test data replicated for seq2');
> > >
> > > Since both are replicated now because of conflicting EXCEPT in
> > > multi-pub, shall we change
> > > comment in 'is(..)'?
> > >
> >
> >
> > For v-003, one trivial thing:
> >
> > Shall we change the name of AlterPublicationTables() as well? It now
> > deals in both tables and sequences.
> >
> Thanks for reviewing the patch. I agree that we should change the name
> here. Modified the patch.
>
> I have also addressed the remaining comments by you and Vignesh in [1], [2], [3]
> Attached the updated version.
>
> [1]: https://www.postgresql.org/message-id/CALDaNm2pGi3FAkN+x+10nqFKNHOUdMwEgqt_TtjLbvz04F3Ktg@mail.gmail.com
> [2]: https://www.postgresql.org/message-id/CAJpy0uBB4N8KOrHchdgprVi2Ws1+gTcEr+bC2A_ziAHOcZcTqA@mail.gmail.com
> [3]: https://www.postgresql.org/message-id/CALDaNm1BRQ1s9na_gwLwN3BYER9be+4QNn4V8sDjiMUvao28Jg@mail.gmail.com

I noticed that CFbot is failling.
The test case in this patch needs to be updated due to changes
introduced by the recent commit 2e1d4fd.
I have made the change and attached the updated patch. I have also
made some cosmetic changes.

Thanks,
Shlok Kyal

On Thu, 7 May 2026 at 10:35, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Tue, 14 Apr 2026 at 18:12, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Tue, 14 Apr 2026 at 15:12, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Apr 13, 2026 at 10:48 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > >
> > > > On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > > > >
> > > > > Hi Vignesh and Shveta,
> > > > >
> > > > > Thanks for reviewing the patches.
> > > > > I have addressed the comments and attached the updated patch.
> > > > >
> > > > > This also addressed the comments shared by Shveta in [1].
> > > > > [1]: https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g@mail.gmail.com
> > > > >
> > > >
> > > > Please find few comments on 001 and 002:
> > > >
> > > >
> > > > v-001:
> > > > 1)
> > > > - List *except_tables; /* tables specified in the EXCEPT clause */
> > > > + List *except_relations; /* relations specified in the EXCEPT
> > > > + * clause */
> > > > Since we have not changed the comments for anything else in patch001,
> > > > we can keep this comment too same as old and changeit in 002.
> > > >
> > > >
> > > > v-002:
> > > > 2)
> > > > pg_publication_rel's prrelid doc says:
> > > > Reference to table
> > > > We shall change it now to 'Reference to table or sequence'
> > > >
> > > > 3)
> > > > In doc, do we eed to change pg_publication_rel's prqual too? IMO, it
> > > > is not applicable to sequence and thus we can change 'relation' to
> > > > 'table' in explanation..
> > > >
> > > > 4)
> > > > Marks the publication as one that synchronizes changes for all sequences
> > > > - in the database, including sequences created in the future.
> > > > + in the database, including sequences created in the future. Sequences
> > > > + listed in <literal>EXCEPT</literal> clause are excluded from the
> > > > + publication.
> > > >
> > > > I think we should place it the end of second paragraph rather than at
> > > > the end of first. How about something liek this:
> > > >
> > > > Marks the publication as one that synchronizes changes for all
> > > > sequences in the database, including sequences created in the future.
> > > >
> > > > Only persistent sequences are included in the publication. Temporary
> > > > sequences and unlogged sequences are excluded from the publication.
> > > > Sequences listed in EXCEPT clause are also excluded from the
> > > > publication.
> > > >
> > > > 5)
> > > > + In such a case, a table or partition or sequence that is included in one
> > > > + publication but excluded (explicitly or implicitly) by the
> > > > + <literal>EXCEPT</literal> clause of another is considered included for
> > > > + replication.
> > > >
> > > > 'a table or partition or sequence' can be changed to 'a table,
> > > > partition, or sequence'
> > > >
> > > > 6)
> > > > In existign doc, shall we give example of publication creation for
> > > > both tables and sequences, each having its except list? This is
> > > > important to show that EXCEPT to be given with individual ALL OBJ. We
> > > > can cahnge last example of doc file to make this. This one:
> > > > 'Create a publication that publishes all sequences for
> > > > synchronization, and all changes in all tables except users and
> > > > departments:'
> > > >
> > > > 7)
> > > > getPublications:
> > > > + * Get the list of tables/sequences for publications specified in the
> > > > + * EXCEPT clause.
> > > >
> > > > We can have both tables and sequences in single publication. We should change
> > > >
> > > > 'tables/sequences' --> tables and sequences
> > > >
> > > > 8)
> > > > In describePublications(),
> > > >
> > > > We had:
> > > > if (!puballtables)
> > > > else
> > > > * Get tables in the EXCEPT clause for this publication */
> > > >
> > > > now we have added:
> > > > if (puballsequences)
> > > > /* Get sequences in the EXCEPT clause for this publication */
> > > >
> > > > Since now we can hit this function for 'all-seq' pub too, shall we
> > > > change if-block's condition to:
> > > >
> > > > if (!puballtables && !puballsequences)
> > > >
> > > > and else-block to
> > > >
> > > > else if (puballtables)
> > > >
> > > > otherwise all-seq case will unnecessary enter these blocks and will
> > > > exceute the logic
> > > >
> > > > Please review other functions too in pg_dump to see if we need such
> > > > conditions altering.
> > > >
> > > >
> > > > 9)
> > > > +# Check the initial data on subscriber
> > > > +$result = $node_subscriber->safe_psql('postgres',
> > > > + "SELECT last_value, is_called FROM seq1");
> > > > +is($result, '200|t', 'sequences in EXCEPT list is excluded');
> > > > +
> > > > +$result = $node_subscriber->safe_psql('postgres',
> > > > + "SELECT last_value, is_called FROM seq2");
> > > > +is($result, '200|t', 'initial test data replicated for seq2');
> > > >
> > > > Since both are replicated now because of conflicting EXCEPT in
> > > > multi-pub, shall we change
> > > > comment in 'is(..)'?
> > > >
> > >
> > >
> > > For v-003, one trivial thing:
> > >
> > > Shall we change the name of AlterPublicationTables() as well? It now
> > > deals in both tables and sequences.
> > >
> > Thanks for reviewing the patch. I agree that we should change the name
> > here. Modified the patch.
> >
> > I have also addressed the remaining comments by you and Vignesh in [1], [2], [3]
> > Attached the updated version.
> >
> > [1]: https://www.postgresql.org/message-id/CALDaNm2pGi3FAkN+x+10nqFKNHOUdMwEgqt_TtjLbvz04F3Ktg@mail.gmail.com
> > [2]: https://www.postgresql.org/message-id/CAJpy0uBB4N8KOrHchdgprVi2Ws1+gTcEr+bC2A_ziAHOcZcTqA@mail.gmail.com
> > [3]: https://www.postgresql.org/message-id/CALDaNm1BRQ1s9na_gwLwN3BYER9be+4QNn4V8sDjiMUvao28Jg@mail.gmail.com
>
> I noticed that CFbot is failling.
> The test case in this patch needs to be updated due to changes
> introduced by the recent commit 2e1d4fd.
> I have made the change and attached the updated patch. I have also
> made some cosmetic changes.
>
The patch needed a rebase after commit a49b9cf.
Attached the rebased patches.

Thanks,
Shlok Kyal

Hi Shlok -

I wonder if it might be worth reconsidering the split between patches
0001 and 0002. My understanding is that the only reason for splitting
is to make review easier, but it had the opposite effect for me.

The difficulty is that 0001 contains renaming of functions and
variables whose purpose only becomes clear in 0002. Without any
context, the changes in 0001 are hard to evaluate on their own merits
— anybody reading patch 0001 is left guessing what is coming rather
than seeing the full picture.

It also won't be saving much overall patch size, since AFAICT most of
the same locations are touched again in 0002 anyway.

There is also the matter of identifier renames (patch 0001) being
separated from their associated comment updates (0002). Keeping those
together would reduce the risk of anything being inadvertently missed.

I think a combined 0001/0002 patch would be easier to follow, since
reviewers could see each change alongside its reason.

======
Kind Regards
Peter Smith.
Fujitsu Australia

Hi Shlok -

Here are some minor review comments for patch v5-0002.

======
doc/src/sgml/catalogs.sgml

1.
<para>
- True if the table is excluded from the publication. See
- <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>.
+ True if the table or the sequence is excluded from the publication. See
+ <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>.

/or the sequence/or sequence/

======
src/backend/catalog/pg_publication.c

check_publication_add_relation:

2.
* Check if relation can be in given publication and throws appropriate
* error if not.

~

Too terse. There are missing words.

SUGGESTION
Check if the target relation is allowed to be specified in the given
publication and throw an error if not.

~~~

3.
if (pri->except)
{
relname = RelationGetQualifiedRelationName(targetrel);
if (pubrelkind == RELKIND_SEQUENCE)
errormsg = gettext_noop("cannot specify \"%s\" in the publication
EXCEPT (SEQUENCE) clause");
else
errormsg = gettext_noop("cannot specify \"%s\" in the publication
EXCEPT (TABLE) clause");
}
else
{
relname = RelationGetRelationName(targetrel);
errormsg = gettext_noop("cannot add relation \"%s\" to publication");
}

~

Wondering why sometimes the error `relname` is fully-qualified and
sometimes it is not. Isn't it better to always be qualified?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, 25 May 2026 at 13:08, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok -
>
> Here are some minor review comments for patch v5-0002.
>
> ======
> doc/src/sgml/catalogs.sgml
>
> 1.
> <para>
> - True if the table is excluded from the publication. See
> - <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>.
> + True if the table or the sequence is excluded from the publication. See
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>.
>
> /or the sequence/or sequence/
>
> ======
> src/backend/catalog/pg_publication.c
>
> check_publication_add_relation:
>
> 2.
> * Check if relation can be in given publication and throws appropriate
> * error if not.
>
> ~
>
> Too terse. There are missing words.
>
> SUGGESTION
> Check if the target relation is allowed to be specified in the given
> publication and throw an error if not.
>
> ~~~
>
> 3.
> if (pri->except)
> {
> relname = RelationGetQualifiedRelationName(targetrel);
> if (pubrelkind == RELKIND_SEQUENCE)
> errormsg = gettext_noop("cannot specify \"%s\" in the publication
> EXCEPT (SEQUENCE) clause");
> else
> errormsg = gettext_noop("cannot specify \"%s\" in the publication
> EXCEPT (TABLE) clause");
> }
> else
> {
> relname = RelationGetRelationName(targetrel);
> errormsg = gettext_noop("cannot add relation \"%s\" to publication");
> }
>
> ~
>
> Wondering why sometimes the error `relname` is fully-qualified and
> sometimes it is not. Isn't it better to always be qualified?
>
This change was made as part of thread [1]. And by reading the thread
I understood that we made 'relname' fully-qualified for the EXCEPT
case.
For the non-EXCEPT case the patch is still in discussion.
I think making `relname` fully-qualified for non-EXCEPT is not related
to this patch and should be discussed in thread [1].

I have addressed the remaining comments in the v6 patches. I have also
merged the patches 0001 and 0002.

[1]: https://www.postgresql.org/message-id/CAA4eK1LvV6ex8n1UV_HZ%2Bs77y%2B5wOpbCns-0rF95Gu3EF0SPNA%40mail.gmail.com

Thanks,
Shlok Kyal

Hi Shlok,

No more comments for v6-0001.

Below are some review comments for v6-0002.

======
doc/src/sgml/ref/alter_publication.sgml

1.
<literal>SET ALL TABLES</literal> can be used to update the tables specified
in the <literal>EXCEPT</literal> clause of a
- <literal>FOR ALL TABLES</literal> publication. If <literal>EXCEPT</literal>
- is specified with a list of tables, the existing exclusion list is replaced
- with the specified tables. If <literal>EXCEPT</literal> is omitted, the
- existing exclusion list is cleared. The <literal>SET</literal> clause, when
- used with a publication defined with <literal>FOR TABLE</literal> or
+ <literal>FOR ALL TABLES</literal> publication and
+ <literal>SET ALL SEQUENCES</literal> can be used to update the sequences
+ specified in the <literal>EXCEPT</literal> clause of a
+ <literal>FOR ALL SEQUENCES</literal> publication. If
+ <literal>EXCEPT</literal> is specified with a list of tables or sequences,
+ the existing exclusion list is replaced with the specified tables or
+ sequences. If <literal>EXCEPT</literal> is omitted, the existing exclusion
+ list is cleared. The <literal>SET</literal> clause, when used with a
+ publication defined with <literal>FOR TABLE</literal> or
<literal>FOR TABLES IN SCHEMA</literal>, replaces the list of tables/schemas
in the publication with the specified list; the existing tables or schemas
that were present in the publication will be removed.

TBH, I think this part of the description is repetitive and
unnecessarily difficult to read. OTOH, fixing it is probably outside
the scope of this patch. Perhaps we can revisit and address all this
properly after both your EXCEPT changes and Nisha's EXCEPT [1] changes
get combined?

~

Examples:

2.
There are many other small examples, so should you also add one also
for this syntax?

======
src/backend/catalog/pg_publication.c

3.
static List *
get_publication_relations(Oid pubid, PublicationPartOpt pub_partopt,
- bool except_flag)
+ bool except_flag, char pubrelkind)

IIUC there should be a sanity Assert in this function to check that
`pubrelkind` can only be either RELKIND_RELATION or RELKIND_SEQUENCE.

~~~

GetIncludedPublicationRelations:

4.
/*
* Gets list of relation oids that are associated with a publication.
*
* This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQUENCES
* should use GetAllPublicationRelations().
*/
List *
GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
{
Assert(!GetPublication(pubid)->alltables);

return get_publication_relations(pubid, pub_partopt, false, RELKIND_RELATION);
}

The comment does not match the code/assert. Shouldn't it also do
assert !allsequences?

~~~

GetAllPublicationRelations:

5.
exceptlist = GetExcludedPublicationRelations(pubid, pubviaroot ?
PUBLICATION_PART_ROOT :
- PUBLICATION_PART_LEAF);
+ PUBLICATION_PART_LEAF,
+ relkind);

IIUC, that `relkind` should be renamed to pubrelkind.

======
src/backend/commands/publicationcmds.c

get_delete_rels:

6.
+get_delete_rels(Oid pubid, List *rels, List *oldrelids, List **delrels)

Add some function comments to describe these parameters.

I also wondered if it would be better to return `delrels` from this
function instead of void. Let the caller accumulate them.

~~~

7.
+ foreach(newlc, rels)
+ {
+ PublicationRelInfo *newpubrel;
+ Oid newrelid;
+ Bitmapset *newcolumns = NULL;
+
+ newpubrel = (PublicationRelInfo *) lfirst(newlc);
+ newrelid = RelationGetRelid(newpubrel->relation);
+
+ /*
+ * Validate the column list. If the column list or WHERE clause
+ * changes, then the validation done here will be duplicated
+ * inside PublicationAddRelations(). The validation is cheap
+ * enough that that seems harmless.
+ */
+ newcolumns = pub_collist_validate(newpubrel->relation,
+ newpubrel->columns);
+
+ /*
+ * Check if any of the new set of relations matches with the
+ * existing relations in the publication. Additionally, if the
+ * relation has an associated WHERE clause, check the WHERE
+ * expressions also match. Same for the column list. Drop the
+ * rest.
+ */

IIUC, that comment "Check if any of the new..." is really describing
the purpose of this entire loop. IMO, this comment would be better put
atop the "foreach(newlc, rels)".

~

8.
+ if (newrelid == oldrelid)
+ {
+ if (equal(oldrelwhereclause, newpubrel->whereClause) &&
+ bms_equal(oldcolumns, newcolumns))
+ {
+ found = true;
+ break;
+ }
+ }
+ }

Consider writing that in a simpler way.

SUGGESTION
found = (newrelid == oldrelid) &&
equal(oldrelwhereclause, newpubrel->whereClause) &&
bms_equal(oldcolumns, newcolumns);

if (found)
break;

~~~

9.
+ /*
+ * Add the non-matched relations to a list so that they can be
+ * dropped.
+ */
+ if (!found)
+ {
+ oldrel = palloc_object(PublicationRelInfo);
+ oldrel->whereClause = NULL;
+ oldrel->columns = NIL;
+ oldrel->except = false;
+ oldrel->relation = table_open(oldrelid,
+ ShareUpdateExclusiveLock);
+ *delrels = lappend(*delrels, oldrel);
+ }

9a.
There seems to be some implicit knowledge that the later DROP is only
going to care about the relation and the other whereClause/columns/etc
will have nothing to do with the dropping. Maybe the comment needs to
say something abouit that?

~

9b.
Maybe palloc0_object can be used instead of explicitly setting
everything else you don't care about to NULL/NIL/false?

~~~

AlterPublicationRelations:

10.
/*
* Nothing to do if no objects, except in SET: for that it is quite
* possible that user has not specified any tables in which case we need
* to remove all the existing tables.
*/
if (!tables && stmt->action != AP_SetObjects)
return;

~

The above code comment seems stale because it is not saying anything
about sequences, and I think it ought to be.

~~~

11.
+ CloseRelationList(seqs);
CloseRelationList(rels);

Everywhere else rels came before seqs. So maybe reverse these to match.

~~~

PublicationDropRelations:

12.
* Remove listed tables from the publication.
*/
static void
-PublicationDropTables(Oid pubid, List *rels, bool missing_ok)
+PublicationDropRelations(Oid pubid, List *rels, bool missing_ok)

The function comment should mention sequences as well.

======
[1] https://www.postgresql.org/message-id/flat/CAHut%2BPvBjw8JJOksjJsCN%2BU4Lda0vWAQTYaYy7ucuMMr8stj0w%40mail.gmail.com#0f1c9caea31ce84b161ec776fe0994b4

Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 27 May 2026 at 09:08, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok,
>
> No more comments for v6-0001.
>
> Below are some review comments for v6-0002.
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> <literal>SET ALL TABLES</literal> can be used to update the tables specified
> in the <literal>EXCEPT</literal> clause of a
> - <literal>FOR ALL TABLES</literal> publication. If <literal>EXCEPT</literal>
> - is specified with a list of tables, the existing exclusion list is replaced
> - with the specified tables. If <literal>EXCEPT</literal> is omitted, the
> - existing exclusion list is cleared. The <literal>SET</literal> clause, when
> - used with a publication defined with <literal>FOR TABLE</literal> or
> + <literal>FOR ALL TABLES</literal> publication and
> + <literal>SET ALL SEQUENCES</literal> can be used to update the sequences
> + specified in the <literal>EXCEPT</literal> clause of a
> + <literal>FOR ALL SEQUENCES</literal> publication. If
> + <literal>EXCEPT</literal> is specified with a list of tables or sequences,
> + the existing exclusion list is replaced with the specified tables or
> + sequences. If <literal>EXCEPT</literal> is omitted, the existing exclusion
> + list is cleared. The <literal>SET</literal> clause, when used with a
> + publication defined with <literal>FOR TABLE</literal> or
> <literal>FOR TABLES IN SCHEMA</literal>, replaces the list of tables/schemas
> in the publication with the specified list; the existing tables or schemas
> that were present in the publication will be removed.
>
> TBH, I think this part of the description is repetitive and
> unnecessarily difficult to read. OTOH, fixing it is probably outside
> the scope of this patch. Perhaps we can revisit and address all this
> properly after both your EXCEPT changes and Nisha's EXCEPT [1] changes
> get combined?
>
I agree.

> ~
>
> Examples:
>
> 2.
> There are many other small examples, so should you also add one also
> for this syntax?
>
> ======
> src/backend/catalog/pg_publication.c
>
> 3.
> static List *
> get_publication_relations(Oid pubid, PublicationPartOpt pub_partopt,
> - bool except_flag)
> + bool except_flag, char pubrelkind)
>
> IIUC there should be a sanity Assert in this function to check that
> `pubrelkind` can only be either RELKIND_RELATION or RELKIND_SEQUENCE.
>
> ~~~
>
> GetIncludedPublicationRelations:
>
> 4.
> /*
> * Gets list of relation oids that are associated with a publication.
> *
> * This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQUENCES
> * should use GetAllPublicationRelations().
> */
> List *
> GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
> {
> Assert(!GetPublication(pubid)->alltables);
>
> return get_publication_relations(pubid, pub_partopt, false, RELKIND_RELATION);
> }
>
> The comment does not match the code/assert. Shouldn't it also do
> assert !allsequences?
>
This function can be called for ALL SEQUENCES publication. For example
'pg_get_publication_tables', 'InvalidatePubRelSyncCache', etc can call
this function for all sequence publications, but in this case the list
returned is empty. So, there is no overall impact.
So, we should not add an 'assert !allsequences'.

This has been already discussed here [1].
> ~~~
>
> GetAllPublicationRelations:
>
> 5.
> exceptlist = GetExcludedPublicationRelations(pubid, pubviaroot ?
> PUBLICATION_PART_ROOT :
> - PUBLICATION_PART_LEAF);
> + PUBLICATION_PART_LEAF,
> + relkind);
>
> IIUC, that `relkind` should be renamed to pubrelkind.
>
> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 6.
> +get_delete_rels(Oid pubid, List *rels, List *oldrelids, List **delrels)
>
> Add some function comments to describe these parameters.
>
> I also wondered if it would be better to return `delrels` from this
> function instead of void. Let the caller accumulate them.
>
> ~~~
>
> 7.
> + foreach(newlc, rels)
> + {
> + PublicationRelInfo *newpubrel;
> + Oid newrelid;
> + Bitmapset *newcolumns = NULL;
> +
> + newpubrel = (PublicationRelInfo *) lfirst(newlc);
> + newrelid = RelationGetRelid(newpubrel->relation);
> +
> + /*
> + * Validate the column list. If the column list or WHERE clause
> + * changes, then the validation done here will be duplicated
> + * inside PublicationAddRelations(). The validation is cheap
> + * enough that that seems harmless.
> + */
> + newcolumns = pub_collist_validate(newpubrel->relation,
> + newpubrel->columns);
> +
> + /*
> + * Check if any of the new set of relations matches with the
> + * existing relations in the publication. Additionally, if the
> + * relation has an associated WHERE clause, check the WHERE
> + * expressions also match. Same for the column list. Drop the
> + * rest.
> + */
>
> IIUC, that comment "Check if any of the new..." is really describing
> the purpose of this entire loop. IMO, this comment would be better put
> atop the "foreach(newlc, rels)".
>
> ~
>
> 8.
> + if (newrelid == oldrelid)
> + {
> + if (equal(oldrelwhereclause, newpubrel->whereClause) &&
> + bms_equal(oldcolumns, newcolumns))
> + {
> + found = true;
> + break;
> + }
> + }
> + }
>
> Consider writing that in a simpler way.
>
> SUGGESTION
> found = (newrelid == oldrelid) &&
> equal(oldrelwhereclause, newpubrel->whereClause) &&
> bms_equal(oldcolumns, newcolumns);
>
> if (found)
> break;
>
> ~~~
>
> 9.
> + /*
> + * Add the non-matched relations to a list so that they can be
> + * dropped.
> + */
> + if (!found)
> + {
> + oldrel = palloc_object(PublicationRelInfo);
> + oldrel->whereClause = NULL;
> + oldrel->columns = NIL;
> + oldrel->except = false;
> + oldrel->relation = table_open(oldrelid,
> + ShareUpdateExclusiveLock);
> + *delrels = lappend(*delrels, oldrel);
> + }
>
> 9a.
> There seems to be some implicit knowledge that the later DROP is only
> going to care about the relation and the other whereClause/columns/etc
> will have nothing to do with the dropping. Maybe the comment needs to
> say something abouit that?
>
> ~
>
> 9b.
> Maybe palloc0_object can be used instead of explicitly setting
> everything else you don't care about to NULL/NIL/false?
>
> ~~~
>
> AlterPublicationRelations:
>
> 10.
> /*
> * Nothing to do if no objects, except in SET: for that it is quite
> * possible that user has not specified any tables in which case we need
> * to remove all the existing tables.
> */
> if (!tables && stmt->action != AP_SetObjects)
> return;
>
> ~
>
> The above code comment seems stale because it is not saying anything
> about sequences, and I think it ought to be.
>
> ~~~
>
> 11.
> + CloseRelationList(seqs);
> CloseRelationList(rels);
>
> Everywhere else rels came before seqs. So maybe reverse these to match.
>
> ~~~
>
> PublicationDropRelations:
>
> 12.
> * Remove listed tables from the publication.
> */
> static void
> -PublicationDropTables(Oid pubid, List *rels, bool missing_ok)
> +PublicationDropRelations(Oid pubid, List *rels, bool missing_ok)
>
> The function comment should mention sequences as well.
>
> ======
> [1] https://www.postgresql.org/message-id/flat/CAHut%2BPvBjw8JJOksjJsCN%2BU4Lda0vWAQTYaYy7ucuMMr8stj0w%40mail.gmail.com#0f1c9caea31ce84b161ec776fe0994b4

I have addressed the remaining comments as well and attached the v7 patch.

[1]: https://www.postgresql.org/message-id/CAA4eK1+rnjBOvkiQC2r4LuTwuje653iVPPAXcmJZXPpKvsNbOQ@mail.gmail.com

Thanks,
Shlok Kyal

Hello!

+ if (footers[0] == NULL)
+ footers[0] = pg_strdup(tmpbuf.data);
+ else if (footers[1] == NULL)
+ footers[1] = pg_strdup(tmpbuf.data);
+ else
+ footers[2] = pg_strdup(tmpbuf.data);
+ resetPQExpBuffer(&tmpbuf);

Shouldn't the matching free calls also updated, this now leaks footers[2]?

+ "\n AND NOT EXISTS (\n"
+ " SELECT 1\n"
+ " FROM pg_catalog.pg_publication_rel pr\n"
+ " WHERE pr.prpubid = p.oid AND\n"
+ " pr.prrelid = '%s')"

Isn't a pr.prexcept check missing from this? It is included in other queries.

- /* EXCEPT clause is not supported with ALTER PUBLICATION */
- Assert(exceptseqs == NIL);
-

Would it be worth it to keep a more restricted assert, saying that
EXCEPT clause is only supported for ALTER PUBLICATION ... SET?

Hi Zsolt,
Thanks for reviewing the patch.

On Thu, 28 May 2026 at 03:22, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> Hello!
>
> + if (footers[0] == NULL)
> + footers[0] = pg_strdup(tmpbuf.data);
> + else if (footers[1] == NULL)
> + footers[1] = pg_strdup(tmpbuf.data);
> + else
> + footers[2] = pg_strdup(tmpbuf.data);
> + resetPQExpBuffer(&tmpbuf);
>
> Shouldn't the matching free calls also updated, this now leaks footers[2]?
>
Yes, correct. Made the change for the same.

> + "\n AND NOT EXISTS (\n"
> + " SELECT 1\n"
> + " FROM pg_catalog.pg_publication_rel pr\n"
> + " WHERE pr.prpubid = p.oid AND\n"
> + " pr.prrelid = '%s')"
>
> Isn't a pr.prexcept check missing from this? It is included in other queries.
>
pg_publication_rel has an entry for table/sequence if we specifically
include/exclude it in a publication.
So, we can say, pg_publication_rel contains an entry for a
Table/Sequence for a ALL TABLES/ ALL SEQUENCE publication, only if it
is specified in the EXCEPT clause.
So, the above query will give the expected results even if we donot
use the ' pr.prexcept' flag.

For the case: _("Get publications that publish this table"))
Here as well we use a similar query without prexcept:
"WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
" AND NOT EXISTS (\n"
" SELECT 1\n"
" FROM
pg_catalog.pg_publication_rel pr\n"
" WHERE pr.prpubid = p.oid AND\n"
" (pr.prrelid = '%s' OR
pr.prrelid = pg_catalog.pg_partition_root('%s')))\n"
"ORDER BY 1;",

> - /* EXCEPT clause is not supported with ALTER PUBLICATION */
> - Assert(exceptseqs == NIL);
> -
>
> Would it be worth it to keep a more restricted assert, saying that
> EXCEPT clause is only supported for ALTER PUBLICATION ... SET?
>
>
Added the Assert in 0002 patch.

I have addressed the comments and attached the v8 patch.

Thanks,
Shlok Kyal

Hi Shlok,

Some review comments for patch v8-0002.

======
doc/src/sgml/ref/alter_publication.sgml

Examples.

1.
Everywhere in these example descriptions where you say "EXCEPT" or
"ALL SEQUENCES" etc, those should all be using SGML <literal> markup.

~~~

2.
+ <para>
+ Replace the sequence list in the publication's EXCEPT clause:
+<programlisting>
+ALTER PUBLICATION mypublication SET ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
+</programlisting></para>
+
+ <para>
+ Reset the publication to be a ALL SEQUENCES publication with no excluded
+ sequences:
+<programlisting>
+ALTER PUBLICATION mypublication SET ALL SEQUENCES;
+</programlisting></para>

2a.
Too wordy.

/in the publication's EXCEPT clause/in the EXCEPT clause/

~

2b.
The 1st example comment saying "Replace the..." doesn't really make
sense because reading from top-to-bottom, this was not even publishing
sequences.

So, I think the "ALTER PUBLICATION mypublication SET ALL SEQUENCES
EXCEPT (SEQUENCE seq1, seq2);" example should come *after* the "ALTER
PUBLICATION mypublication SET ALL SEQUENCES;" example.

~~~

3.
+ <para>
+ Replace the table and sequence list in the publication's EXCEPT clause:
+<programlisting>
+ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users,
departments), ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
</programlisting></para>

Too wordy. Should be plural.

/in the publication's EXCEPT clause:/in the EXCEPT clauses:/

======
src/backend/catalog/pg_publication.c

GetIncludedPublicationRelations:

On Wed, May 27, 2026 at 11:03 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Wed, 27 May 2026 at 09:08, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > GetIncludedPublicationRelations:
> >
> > 4.
> > /*
> > * Gets list of relation oids that are associated with a publication.
> > *
> > * This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQUENCES
> > * should use GetAllPublicationRelations().
> > */
> > List *
> > GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
> > {
> > Assert(!GetPublication(pubid)->alltables);
> >
> > return get_publication_relations(pubid, pub_partopt, false, RELKIND_RELATION);
> > }
> >
> > The comment does not match the code/assert. Shouldn't it also do
> > assert !allsequences?
> >
> This function can be called for ALL SEQUENCES publication. For example
> 'pg_get_publication_tables', 'InvalidatePubRelSyncCache', etc can call
> this function for all sequence publications, but in this case the list
> returned is empty. So, there is no overall impact.
> So, we should not add an 'assert !allsequences'.

Hmm. The reply seems contrary to the function comment that says "This
should only be used FOR TABLE publications", so if not going to add an
Assert then doesn't the function comment need fixing?

======
src/backend/commands/publicationcmds.c

get_delete_rels:

5.
/*
- * Add or remove table to/from publication.
+ * Recreate list of tables/sequences to be dropped from the publication.
+ * To recreate the relation list for the publication, look for existing
+ * relations that do not need to be dropped.
+ *
+ * 'rels' contains the given list of relations, and 'oldrelids' contains
+ * the OIDs of existing relations in the publication identified by 'pubid'.
+ */

Why say "recreate" everywhere? Can it just say "Returns the list of
...." instead of "Recreate ...".

Also, I think this function comment needs to explain in more detail
that this is called during ALTER SET. IIUC, the 'rels' is the reids
you want to end up with in the publication; so those need to be
compared with the 'oldrelids' to find which old ones are not wanted
anymore. Those unwanted ones are what the function returns.

~~~

AlterPublicationRelations:

6.
/*
* Nothing to do if no objects, except in SET: for that it is quite
- * possible that user has not specified any tables in which case we need
- * to remove all the existing tables.
+ * possible that user has not specified any tables or sequences in which
+ * case we need to remove all the existing tables and sequences.
*/

Maybe this can be reworded more simply:

SUGGESTION
/*
* Nothing to do if no objects were specified, unless this is a SET
* command, which may need to remove all existing tables and sequences.
*/

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, 2 Jun 2026 at 05:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok,
>
> Some review comments for patch v8-0002.
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> Examples.
>
> 1.
> Everywhere in these example descriptions where you say "EXCEPT" or
> "ALL SEQUENCES" etc, those should all be using SGML <literal> markup.
>
Fixed

> ~~~
>
> 2.
> + <para>
> + Replace the sequence list in the publication's EXCEPT clause:
> +<programlisting>
> +ALTER PUBLICATION mypublication SET ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
> +</programlisting></para>
> +
> + <para>
> + Reset the publication to be a ALL SEQUENCES publication with no excluded
> + sequences:
> +<programlisting>
> +ALTER PUBLICATION mypublication SET ALL SEQUENCES;
> +</programlisting></para>
>
> 2a.
> Too wordy.
>
> /in the publication's EXCEPT clause/in the EXCEPT clause/
>
In the existing documentation we already have similar documentation:
<para>
Replace the table list in the publication's EXCEPT clause:
<programlisting>
ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users,
departments);
</programlisting></para>

I think we should keep the wording consistent. Thoughts?

> ~
>
> 2b.
> The 1st example comment saying "Replace the..." doesn't really make
> sense because reading from top-to-bottom, this was not even publishing
> sequences.
>
> So, I think the "ALTER PUBLICATION mypublication SET ALL SEQUENCES
> EXCEPT (SEQUENCE seq1, seq2);" example should come *after* the "ALTER
> PUBLICATION mypublication SET ALL SEQUENCES;" example.
>
Fixed

> ~~~
>
> 3.
> + <para>
> + Replace the table and sequence list in the publication's EXCEPT clause:
> +<programlisting>
> +ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users,
> departments), ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
> </programlisting></para>
>
> Too wordy. Should be plural.
>
> /in the publication's EXCEPT clause:/in the EXCEPT clauses:/
>
I have kept the wording the same. Reason same as 2(a).
I have made it plural.

> ======
> src/backend/catalog/pg_publication.c
>
> GetIncludedPublicationRelations:
>
> On Wed, May 27, 2026 at 11:03 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, 27 May 2026 at 09:08, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > GetIncludedPublicationRelations:
> > >
> > > 4.
> > > /*
> > > * Gets list of relation oids that are associated with a publication.
> > > *
> > > * This should only be used FOR TABLE publications, the FOR ALL TABLES/SEQUENCES
> > > * should use GetAllPublicationRelations().
> > > */
> > > List *
> > > GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
> > > {
> > > Assert(!GetPublication(pubid)->alltables);
> > >
> > > return get_publication_relations(pubid, pub_partopt, false, RELKIND_RELATION);
> > > }
> > >
> > > The comment does not match the code/assert. Shouldn't it also do
> > > assert !allsequences?
> > >
> > This function can be called for ALL SEQUENCES publication. For example
> > 'pg_get_publication_tables', 'InvalidatePubRelSyncCache', etc can call
> > this function for all sequence publications, but in this case the list
> > returned is empty. So, there is no overall impact.
> > So, we should not add an 'assert !allsequences'.
>
> Hmm. The reply seems contrary to the function comment that says "This
> should only be used FOR TABLE publications", so if not going to add an
> Assert then doesn't the function comment need fixing?
>
Yes, the function comment is not correct and a patch was already
proposed for the same in [1]. But it is a stale state there.
Since this patch is related to ALL SEQUENCES and EXCEPT, I think we
can update it here. I have updated the comment.

> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 5.
> /*
> - * Add or remove table to/from publication.
> + * Recreate list of tables/sequences to be dropped from the publication.
> + * To recreate the relation list for the publication, look for existing
> + * relations that do not need to be dropped.
> + *
> + * 'rels' contains the given list of relations, and 'oldrelids' contains
> + * the OIDs of existing relations in the publication identified by 'pubid'.
> + */
>
> Why say "recreate" everywhere? Can it just say "Returns the list of
> ...." instead of "Recreate ...".
>
> Also, I think this function comment needs to explain in more detail
> that this is called during ALTER SET. IIUC, the 'rels' is the reids
> you want to end up with in the publication; so those need to be
> compared with the 'oldrelids' to find which old ones are not wanted
> anymore. Those unwanted ones are what the function returns.
>
I have reworded the comment

> ~~~
>
> AlterPublicationRelations:
>
> 6.
> /*
> * Nothing to do if no objects, except in SET: for that it is quite
> - * possible that user has not specified any tables in which case we need
> - * to remove all the existing tables.
> + * possible that user has not specified any tables or sequences in which
> + * case we need to remove all the existing tables and sequences.
> */
>
> Maybe this can be reworded more simply:
>
> SUGGESTION
> /*
> * Nothing to do if no objects were specified, unless this is a SET
> * command, which may need to remove all existing tables and sequences.
> */
Fixed

[1]:https://www.postgresql.org/message-id/CANhcyEUkV-T6cK142w9wfME9nobFHOvn1f4itJLMG-oR4QoPbQ%40mail.gmail.com

Thanks,
Shlok Kyal

Some review comments for v9-0001.

======
src/backend/catalog/pg_publication.c

GetExcludedPublicationTables:

1.
- Assert(GetPublication(pubid)->alltables);
+ Assert(GetPublication(pubid)->alltables ||
+ GetPublication(pubid)->allsequences);

Better to assign to a variable first, instead of calling GetPublication() 2x.

~~~

GetAllTablesPublications:

2.
- * 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
+ * mentioned in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
+ * it excludes sequences mentioned in the EXCEPT clause.

(2 times)

/mentioned/specified/ or
/mentioned/named/

~~~

3.
+ /* EXCEPT filtering applies to tables and sequences */
+ exceptlist = GetExcludedPublicationRelations(pubid, pubviaroot ?
+ PUBLICATION_PART_ROOT :
+ PUBLICATION_PART_LEAF);

I don't think the comment is needed anymore. It was relevant before,
when there was a condition, but now there is no condition.

======
src/backend/parser/gram.y

preprocess_pubobj_list:

4.
- /* relation name or pubtable must be set for this type of object */
- if (!pubobj->name && !pubobj->pubtable)
+ /* relation name or pubrelation must be set for this type of object */
+ if (!pubobj->name && !pubobj->pubrelation)
...
- /* convert it to PublicationTable */
- PublicationTable *pubtable = makeNode(PublicationTable);
+ /* convert it to PublicationRelation */
+ PublicationRelation *pubrelation = makeNode(PublicationRelation);

Since you are changing these comments, you can uppercase them
in-passing for consistency.
/relation name/Relation name/
/convert it/Convert it/

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Some comments for v9-0002.

======
APPLY:

0.
The patch does not apply cleanly:

$ git apply ../patches_misc/v9-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLICA.patch
error: patch failed: doc/src/sgml/ref/alter_publication.sgml:277

======
doc/src/sgml/ref/alter_publication.sgml

1.
+ <para>
+ Reset the publication to be a <literal>ALL SEQUENCES</literal> publication
+ with no excluded sequences:

Personally, I don't see how it is good to have "publication" 2x and
"sequences" 2x in the same sentence.

SUGGESTION
Reset the publication to be <literal>ALL SEQUENCES</literal> with no exclusions:

~

I know you're only following the same pattern as "Reset the
publication to be a FOR ALL TABLES publication with no excluded
tables:". It is good to be consistent, but when the original text is
poor, copying it doesn't make it better.

Things like this fall into a grey-zone because, unless they get fixed
"in-passing", nothing ever changes:
a) I think a patch to only change the original wording would be
rejected because it is too trivial.
b) OTOH, when the original is used as a precedent, the poor wording
just spreads further.

~

Anyway, if your chose not to reword this, then there is a typo /a ALL
SEQUENCES publication/an ALL SEQUENCES publication/

======
src/backend/commands/publicationcmds.c

get_delete_rels:

2.
+ /* look up the cache for the old relmap */

/look up/Look up/

~~~

3.
+ if(found)
+ break;

Missing space after 'if'.

~~~

4.
+ oldrel = palloc0_object (PublicationRelInfo);

Unwanted space after function name.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, 12 Jun 2026 at 10:55, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for v9-0001.
>
> ======
> src/backend/catalog/pg_publication.c
>
> GetExcludedPublicationTables:
>
> 1.
> - Assert(GetPublication(pubid)->alltables);
> + Assert(GetPublication(pubid)->alltables ||
> + GetPublication(pubid)->allsequences);
>
> Better to assign to a variable first, instead of calling GetPublication() 2x.
>
I agree. I've stored the result of GetPublication(pubid) in a local
variable and declared it under USE_ASSERT_CHECKING, since it is only
used by the assertion.

> ~~~
>
> GetAllTablesPublications:
>
> 2.
> - * 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
> + * mentioned in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
> + * it excludes sequences mentioned in the EXCEPT clause.
>
> (2 times)
>
> /mentioned/specified/ or
> /mentioned/named/
>
> ~~~
>
> 3.
> + /* EXCEPT filtering applies to tables and sequences */
> + exceptlist = GetExcludedPublicationRelations(pubid, pubviaroot ?
> + PUBLICATION_PART_ROOT :
> + PUBLICATION_PART_LEAF);
>
> I don't think the comment is needed anymore. It was relevant before,
> when there was a condition, but now there is no condition.
>
> ======
> src/backend/parser/gram.y
>
> preprocess_pubobj_list:
>
> 4.
> - /* relation name or pubtable must be set for this type of object */
> - if (!pubobj->name && !pubobj->pubtable)
> + /* relation name or pubrelation must be set for this type of object */
> + if (!pubobj->name && !pubobj->pubrelation)
> ...
> - /* convert it to PublicationTable */
> - PublicationTable *pubtable = makeNode(PublicationTable);
> + /* convert it to PublicationRelation */
> + PublicationRelation *pubrelation = makeNode(PublicationRelation);
>
> Since you are changing these comments, you can uppercase them
> in-passing for consistency.
> /relation name/Relation name/
> /convert it/Convert it/

I have addressed the remaining comments in the v10 patches.

Thanks,
Shlok Kyal

On Fri, 12 Jun 2026 at 10:59, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some comments for v9-0002.
>
> ======
> APPLY:
>
> 0.
> The patch does not apply cleanly:
>
> $ git apply ../patches_misc/v9-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLICA.patch
> error: patch failed: doc/src/sgml/ref/alter_publication.sgml:277
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> + <para>
> + Reset the publication to be a <literal>ALL SEQUENCES</literal> publication
> + with no excluded sequences:
>
> Personally, I don't see how it is good to have "publication" 2x and
> "sequences" 2x in the same sentence.
>
> SUGGESTION
> Reset the publication to be <literal>ALL SEQUENCES</literal> with no exclusions:
>
> ~
>
> I know you're only following the same pattern as "Reset the
> publication to be a FOR ALL TABLES publication with no excluded
> tables:". It is good to be consistent, but when the original text is
> poor, copying it doesn't make it better.
>
> Things like this fall into a grey-zone because, unless they get fixed
> "in-passing", nothing ever changes:
> a) I think a patch to only change the original wording would be
> rejected because it is too trivial.
> b) OTOH, when the original is used as a precedent, the poor wording
> just spreads further.
>
> ~
>
> Anyway, if your chose not to reword this, then there is a typo /a ALL
> SEQUENCES publication/an ALL SEQUENCES publication/
>
Yes, I wanted the text to be consistent with the existing doc. But
after reading your suggestion, I feel your version is more
appropriate. I have updated the patch to use it.

> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 2.
> + /* look up the cache for the old relmap */
>
> /look up/Look up/
>
> ~~~
>
> 3.
> + if(found)
> + break;
>
> Missing space after 'if'.
>
> ~~~
>
> 4.
> + oldrel = palloc0_object (PublicationRelInfo);
>
> Unwanted space after function name.
>

I have addressed the remaining comments and have attached the updated
v10 patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEVDStqoR3qDSgsv5VEuBMzMX4YpdEfW-7VPX3c5Vf1YJA%40mail.gmail.com

Thanks,
Shlok Kyal

////////////////////
Some review comments for v10-00001.
////////////////////

======
doc/src/sgml/logical-replication.sgml

1.
<xref linkend="logical-replication-sequences"/>. When a publication is
created with <literal>FOR ALL TABLES</literal>, a table or set of tables can
be explicitly excluded from publication using the
- <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>
+ <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
+ clause. Similarly, when a publication is created with
+ <literal>FOR ALL SEQUENCES</literal>, a sequence or set of sequences can be
+ explicitly excluded from publication using the
+ <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
clause.
</para>

Yes, it is valid English to say "excluded from publication" (meaning
won't be published); however, in most other documentation about
EXCEPT, we refer to the PUBLICATION *object*, so it is worded like
"excluded from the publication".

So I think it should be /from publication/from the publication/

(fix in 2 places in this file)

======
src/backend/catalog/pg_publication.c

GetAllTablesPublications:

2.
> - * 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
> + * mentioned in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
> + * it excludes sequences mentioned in the EXCEPT clause.
>
> (2 times)
>
> /mentioned/specified/ or
> /mentioned/named/

Please also fix the 1st "mentioned" in that comment.

======
src/backend/commands/publicationcmds.c

3.
+ /* Process EXCEPT sequence list */
+ if (stmt->for_all_sequences && exceptseqs != NIL)
+ {
+ List *rels = OpenRelationList(exceptseqs);
+
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
+ CloseRelationList(rels);
+ }

if (stmt->for_all_tables)
{
/* Process EXCEPT table list */
- if (exceptrelations != NIL)
+ if (excepttbls != NIL)
{
List *rels;

- rels = OpenTableList(exceptrelations);
- PublicationAddTables(puboid, rels, true, NULL);
- CloseTableList(rels);
+ rels = OpenRelationList(excepttbls);
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
+ CloseRelationList(rels);

I felt that the fragments for "Process EXCEPT sequence list" and
"Process EXCEPT table list" should look exactly the same, but they are
currently formatted slightly differently. Normally, you might not
write the first `if` as nested, but here I think consistency is more
important.

SUGGESTION
if (stmt->for_all_sequences)
{
/* Process EXCEPT sequence list */
if (exceptseqs != NIL)
{
List *rels = OpenRelationList(exceptseqs);

PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
CloseRelationList(rels);
}
}

if (stmt->for_all_tables)
{
/* Process EXCEPT table list */
if (excepttbls != NIL)
{
List *rels = OpenRelationList(excepttbls);

PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
CloseRelationList(rels);
}
...
}

======
src/bin/pg_dump/pg_dump.c

4.
+ {
+ if (relkind == RELKIND_SEQUENCE)
+ simple_ptr_list_append(&pubinfo[i].except_sequences, tbinfo);
+ else
+ simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);
+ }

This code is building the `except_sequences` list in a block guarded
by >= 190000. In fact, EXCEPT SEQUENCES won't exist until PG20. I'm
not sure if this code should have a 20000 check... It may be harmless
as-is if this can never happen for PG19.

======
src/bin/psql/describe.c

describeOneTableDetails:

5.
printfPQExpBuffer(&buf, "/* %s */\n",
_("Get publications containing this sequence"));
- appendPQExpBuffer(&buf, "SELECT pubname FROM pg_catalog.pg_publication p"
+ appendPQExpBuffer(&buf, "SELECT p.pubname FROM pg_catalog.pg_publication p"
"\nWHERE p.puballsequences"
"\n AND pg_catalog.pg_relation_is_publishable('%s')"
+ "\n AND NOT EXISTS (\n"
+ " SELECT 1\n"
+ " FROM pg_catalog.pg_publication_rel pr\n"
+ " WHERE pr.prpubid = p.oid AND\n"
+ " pr.prrelid = '%s')"
"\nORDER BY 1",
- oid);
+ oid, oid);

5a.
IIUC, the "NOT EXISTS" part is for skipping the excluded sequences.
And, you don't say "AND pr.prexcept" because it is redundant since
the only way for a SEQUENCE to be individually in pg_publication_rel
is if it is an excluded sequence.

Is my understanding correct? I think it is too subtle -- it might be
better to say "AND pr.prexcept" even if it is not strictly needed.

~

5b.
Anyway, this code should not com into play for PG19, because excluded
sequences will be a PG20 feature... so I think that whole "AND NOT
EXISTS" part needs another check.

~~~

6.
+ /* Print publications where the sequence is in the EXCEPT clause */
+ if (pset.sversion >= 190000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT p.pubname\n"
+ "FROM pg_catalog.pg_publication p\n"
+ "JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
+ "WHERE pr.prrelid = '%s' AND pr.prexcept\n"
+ "ORDER BY 1;", oid);

This whole part will be a PG20 feature. I think you should add a
"TODO" reminder comment here to flag/remind to modify this to check >=
200000 as soon as the PG19 version is bumped.

~~~

describePublications:

7.
+ if (puballsequences)
+ {
+ if (pset.sversion >= 190000)
+ {
+ /* Get sequences in the EXCEPT clause for this publication */
+ printfPQExpBuffer(&buf, "/* %s */\n",
+ _("Get sequences excluded by this publication"));
+ printfPQExpBuffer(&buf,
+ "SELECT n.nspname || '.' || c.relname\n"
+ "FROM pg_catalog.pg_class c\n"
+ " JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace\n"
+ " JOIN pg_catalog.pg_publication_rel pr ON c.oid = pr.prrelid\n"
+ "WHERE pr.prpubid = '%s' AND pr.prexcept AND c.relkind = 'S'\n"
+ "ORDER BY 1", pubid);
+ if (!addFooterToPublicationDesc(&buf, _("Except sequences:"),
+ true, &cont))
+ goto error_return;
+ }
+ }

Similar to the previous review comment. EXCEPT SEQUENCES is a PG20
feature, not PG19, so this should have a "TODO" comment reminder to
change the version check to 200000 when the PG version is bumped.

======
src/test/subscription/t/037_except.pl

8.
+# Subscribe to multiple publications with different EXCEPT sequence list

/list/lists/

////////////////////
Some review comments for patch v10-0002
////////////////////

======
doc/src/sgml/ref/alter_publication.sgml

Examples:

1.
+ Reset the publication to be <literal>ALL SEQUENCES</literal> with no
+ exclusions:

Thanks for changing the above. "In passing", can you modify the "ALL
TABLES" text to use the same wording? Otherwise, the ALL TABLES text
will never be improved.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, 17 Jun 2026 at 12:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ////////////////////
> Some review comments for v10-00001.
> ////////////////////
>
> ======
> doc/src/sgml/logical-replication.sgml
>
> 1.
> <xref linkend="logical-replication-sequences"/>. When a publication is
> created with <literal>FOR ALL TABLES</literal>, a table or set of tables can
> be explicitly excluded from publication using the
> - <link linkend="sql-createpublication-params-for-except-table"><literal>EXCEPT</literal></link>
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
> + clause. Similarly, when a publication is created with
> + <literal>FOR ALL SEQUENCES</literal>, a sequence or set of sequences can be
> + explicitly excluded from publication using the
> + <link linkend="sql-createpublication-params-for-except"><literal>EXCEPT</literal></link>
> clause.
> </para>
>
> Yes, it is valid English to say "excluded from publication" (meaning
> won't be published); however, in most other documentation about
> EXCEPT, we refer to the PUBLICATION *object*, so it is worded like
> "excluded from the publication".
>
> So I think it should be /from publication/from the publication/
>
> (fix in 2 places in this file)
>
> ======
> src/backend/catalog/pg_publication.c
>
> GetAllTablesPublications:
>
> 2.
> > - * 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
> > + * mentioned in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
> > + * it excludes sequences mentioned in the EXCEPT clause.
> >
> > (2 times)
> >
> > /mentioned/specified/ or
> > /mentioned/named/
>
> Please also fix the 1st "mentioned" in that comment.
>
> ======
> src/backend/commands/publicationcmds.c
>
> 3.
> + /* Process EXCEPT sequence list */
> + if (stmt->for_all_sequences && exceptseqs != NIL)
> + {
> + List *rels = OpenRelationList(exceptseqs);
> +
> + PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
> + CloseRelationList(rels);
> + }
>
> if (stmt->for_all_tables)
> {
> /* Process EXCEPT table list */
> - if (exceptrelations != NIL)
> + if (excepttbls != NIL)
> {
> List *rels;
>
> - rels = OpenTableList(exceptrelations);
> - PublicationAddTables(puboid, rels, true, NULL);
> - CloseTableList(rels);
> + rels = OpenRelationList(excepttbls);
> + PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
> + CloseRelationList(rels);
>
> I felt that the fragments for "Process EXCEPT sequence list" and
> "Process EXCEPT table list" should look exactly the same, but they are
> currently formatted slightly differently. Normally, you might not
> write the first `if` as nested, but here I think consistency is more
> important.
>
> SUGGESTION
> if (stmt->for_all_sequences)
> {
> /* Process EXCEPT sequence list */
> if (exceptseqs != NIL)
> {
> List *rels = OpenRelationList(exceptseqs);
>
> PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
> CloseRelationList(rels);
> }
> }
>
> if (stmt->for_all_tables)
> {
> /* Process EXCEPT table list */
> if (excepttbls != NIL)
> {
> List *rels = OpenRelationList(excepttbls);
>
> PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
> CloseRelationList(rels);
> }
> ...
> }
>
> ======
> src/bin/pg_dump/pg_dump.c
>
> 4.
> + {
> + if (relkind == RELKIND_SEQUENCE)
> + simple_ptr_list_append(&pubinfo[i].except_sequences, tbinfo);
> + else
> + simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);
> + }
>
> This code is building the `except_sequences` list in a block guarded
> by >= 190000. In fact, EXCEPT SEQUENCES won't exist until PG20. I'm
> not sure if this code should have a 20000 check... It may be harmless
> as-is if this can never happen for PG19.
>
Added a TODO to use version check for PG20

> ======
> src/bin/psql/describe.c
>
> describeOneTableDetails:
>
> 5.
> printfPQExpBuffer(&buf, "/* %s */\n",
> _("Get publications containing this sequence"));
> - appendPQExpBuffer(&buf, "SELECT pubname FROM pg_catalog.pg_publication p"
> + appendPQExpBuffer(&buf, "SELECT p.pubname FROM pg_catalog.pg_publication p"
> "\nWHERE p.puballsequences"
> "\n AND pg_catalog.pg_relation_is_publishable('%s')"
> + "\n AND NOT EXISTS (\n"
> + " SELECT 1\n"
> + " FROM pg_catalog.pg_publication_rel pr\n"
> + " WHERE pr.prpubid = p.oid AND\n"
> + " pr.prrelid = '%s')"
> "\nORDER BY 1",
> - oid);
> + oid, oid);
>
> 5a.
> IIUC, the "NOT EXISTS" part is for skipping the excluded sequences.
> And, you don't say "AND pr.prexcept" because it is redundant since
> the only way for a SEQUENCE to be individually in pg_publication_rel
> is if it is an excluded sequence.
>
> Is my understanding correct? I think it is too subtle -- it might be
> better to say "AND pr.prexcept" even if it is not strictly needed.
>
Yes, your understanding is correct.
For ALL SEQUENCES publication, the pg_publication_rel will have the
entry only if the sequence is specified in the EXCEPT list.
I didn't add "AND pr.prexcept" because the corresponding code for ALL
TABLES publications also does not use "AND pr.prexcept".
```
SELECT pubname\n"
" , NULL\n"
" , NULL\n"
"FROM pg_catalog.pg_publication p\n"
"WHERE p.puballtables AND
pg_catalog.pg_relation_is_publishable('%s')\n"
" AND NOT EXISTS (\n"
" SELECT 1\n"
" FROM
pg_catalog.pg_publication_rel pr\n"
" WHERE pr.prpubid = p.oid AND\n"
" (pr.prrelid = '%s' OR
pr.prrelid = pg_catalog.pg_partition_root('%s')))\n"
"ORDER BY 1;",
```
I think it will be consistent with EXCEPT (TABLE .. ) case. So, I
think we should not add "AND pr.prexcept".

> ~
>
> 5b.
> Anyway, this code should not com into play for PG19, because excluded
> sequences will be a PG20 feature... so I think that whole "AND NOT
> EXISTS" part needs another check.
>
Separated and added a TODO to change the version.

> ~~~
>
> 6.
> + /* Print publications where the sequence is in the EXCEPT clause */
> + if (pset.sversion >= 190000)
> + {
> + printfPQExpBuffer(&buf,
> + "SELECT p.pubname\n"
> + "FROM pg_catalog.pg_publication p\n"
> + "JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
> + "WHERE pr.prrelid = '%s' AND pr.prexcept\n"
> + "ORDER BY 1;", oid);
>
> This whole part will be a PG20 feature. I think you should add a
> "TODO" reminder comment here to flag/remind to modify this to check >=
> 200000 as soon as the PG19 version is bumped.
>
Added

> ~~~
>
> describePublications:
>
> 7.
> + if (puballsequences)
> + {
> + if (pset.sversion >= 190000)
> + {
> + /* Get sequences in the EXCEPT clause for this publication */
> + printfPQExpBuffer(&buf, "/* %s */\n",
> + _("Get sequences excluded by this publication"));
> + printfPQExpBuffer(&buf,
> + "SELECT n.nspname || '.' || c.relname\n"
> + "FROM pg_catalog.pg_class c\n"
> + " JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace\n"
> + " JOIN pg_catalog.pg_publication_rel pr ON c.oid = pr.prrelid\n"
> + "WHERE pr.prpubid = '%s' AND pr.prexcept AND c.relkind = 'S'\n"
> + "ORDER BY 1", pubid);
> + if (!addFooterToPublicationDesc(&buf, _("Except sequences:"),
> + true, &cont))
> + goto error_return;
> + }
> + }
>
> Similar to the previous review comment. EXCEPT SEQUENCES is a PG20
> feature, not PG19, so this should have a "TODO" comment reminder to
> change the version check to 200000 when the PG version is bumped.
>
Added a TODO
> ======
> src/test/subscription/t/037_except.pl
>
> 8.
> +# Subscribe to multiple publications with different EXCEPT sequence list
>
> /list/lists/
>
>
> ////////////////////
> Some review comments for patch v10-0002
> ////////////////////
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> Examples:
>
> 1.
> + Reset the publication to be <literal>ALL SEQUENCES</literal> with no
> + exclusions:
>
> Thanks for changing the above. "In passing", can you modify the "ALL
> TABLES" text to use the same wording? Otherwise, the ALL TABLES text
> will never be improved.

I have also addressed the remaining comments and attached the updated v11 patch.

Thanks,
Shlok Kyal

Hi Shlok.

Some review comments for v11-0001 and v11-0002.

//////////
v11-0001

======
doc/src/sgml/ref/create_publication.sgml

(EXCEPT)

1.
The recent commit 77b6dd9 added some more information to say:

---
Once a table is excluded, the exclusion applies to that table
regardless of its name or schema. Renaming the table or moving it to
another schema using <command>ALTER TABLE ... SET SCHEMA</command> does
not remove the exclusion.
---

1a
AFAIK this same rule (the exclusion follows the object regardless of
the schema) is going to apply also for excluding sequences. So, the
docs should be updated to say something similar about behaviour for
sequences.

~

1b.
Maybe you need to add another test case to exclude some sequence, then
alter the sequence's schema, then verify that the moved sequence is
still excluded.

~~~
On Thu, Jun 18, 2026 at 11:29 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Wed, 17 Jun 2026 at 12:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
...
> > ======
> > src/bin/psql/describe.c
> >
> > describeOneTableDetails:
> >
> > 5.
> > printfPQExpBuffer(&buf, "/* %s */\n",
> > _("Get publications containing this sequence"));
> > - appendPQExpBuffer(&buf, "SELECT pubname FROM pg_catalog.pg_publication p"
> > + appendPQExpBuffer(&buf, "SELECT p.pubname FROM pg_catalog.pg_publication p"
> > "\nWHERE p.puballsequences"
> > "\n AND pg_catalog.pg_relation_is_publishable('%s')"
> > + "\n AND NOT EXISTS (\n"
> > + " SELECT 1\n"
> > + " FROM pg_catalog.pg_publication_rel pr\n"
> > + " WHERE pr.prpubid = p.oid AND\n"
> > + " pr.prrelid = '%s')"
> > "\nORDER BY 1",
> > - oid);
> > + oid, oid);
> >
> > 5a.
> > IIUC, the "NOT EXISTS" part is for skipping the excluded sequences.
> > And, you don't say "AND pr.prexcept" because it is redundant since
> > the only way for a SEQUENCE to be individually in pg_publication_rel
> > is if it is an excluded sequence.
> >
> > Is my understanding correct? I think it is too subtle -- it might be
> > better to say "AND pr.prexcept" even if it is not strictly needed.
> >
> Yes, your understanding is correct.
> For ALL SEQUENCES publication, the pg_publication_rel will have the
> entry only if the sequence is specified in the EXCEPT list.
> I didn't add "AND pr.prexcept" because the corresponding code for ALL
> TABLES publications also does not use "AND pr.prexcept".
...
> I think it will be consistent with EXCEPT (TABLE .. ) case. So, I
> think we should not add "AND pr.prexcept".

That’s OK, but I felt if you are going to omit the 'prexcept', then
perhaps there should be a comment to explain (in all places) why it is
ok to do that.

//////////
v11-0002.

======
doc/src/sgml/ref/alter_publication.sgml

1.
...or marks the publication as FOR ALL SEQUENCES or FOR ALL TABLES,
optionally using EXCEPT to exclude specific tables or sequences.

~

That current documentation sentence seems muddled because the
publication types are not in the same as the order of the excluded
types.

IMO, alternatives below are better:
a) ...FOR ALL SEQUENCES or FOR ALL TABLES, optionally using EXCEPT to
exclude specific sequences or tables.
b) ...FOR ALL TABLES or FOR ALL SEQUENCES, optionally using EXCEPT to
exclude specific tables or sequences.

(I preferred b, since everywhere else seems to be ordered
tables/sequences not sequences/tables.).

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Hi Shlok.

FYI, I tested that the exclusion of a SEQUENCE follows the sequence
object around the same as a table excluded from FOR ALL TABLES does.

> 1.
> The recent commit 77b6dd9 added some more information to say:
>
> ---
> Once a table is excluded, the exclusion applies to that table
> regardless of its name or schema. Renaming the table or moving it to
> another schema using <command>ALTER TABLE ... SET SCHEMA</command> does
> not remove the exclusion.
> ---
>
> 1a
> AFAIK this same rule (the exclusion follows the object regardless of
> the schema) is going to apply also for excluding sequences. So, the
> docs should be updated to say something similar about behaviour for
> sequences.
>
> ~
>
> 1b.
> Maybe you need to add another test case to exclude some sequence, then
> alter the sequence's schema, then verify that the moved sequence is
> still excluded.
>

test_pub=# CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT (TABLE
PUBT1), ALL SEQUENCES EXCEPT (SEQUENCE PUBSEQ);
CREATE PUBLICATION

test_pub=# \dRp+ pub1
Publication pub1
Owner | All tables | All sequences | Inserts | Updates | Deletes |
Truncates | Generated columns | Via root | Descri
ption
----------+------------+---------------+---------+---------+---------+-----------+-------------------+----------+-------
------
postgres | t | t | t | t | t |
t | none | f |
Except tables:
"public.pubt1"
Except sequences:
"public.pubseq"

-- Now move the excluded sequence to another schema and see that it is
still excluded

test_pub=# alter sequence pubseq set schema myschema;
ALTER SEQUENCE
test_pub=# \dRp+ pub1
Publication pub1
Owner | All tables | All sequences | Inserts | Updates | Deletes |
Truncates | Generated columns | Via root | Descri
ption
----------+------------+---------------+---------+---------+---------+-----------+-------------------+----------+-------
------
postgres | t | t | t | t | t |
t | none | f |
Except tables:
"public.pubt1"
Except sequences:
"myschema.pubseq"

-- Now rename the excluded sequence and see that it is still excluded

test_pub=# alter sequence myschema.pubseq rename to pubseq2;
ALTER SEQUENCE
test_pub=# \dRp+ pub1
Publication pub1
Owner | All tables | All sequences | Inserts | Updates | Deletes |
Truncates | Generated columns | Via root | Descri
ption
----------+------------+---------------+---------+---------+---------+-----------+-------------------+----------+-------
------
postgres | t | t | t | t | t |
t | none | f |
Except tables:
"public.pubt1"
Except sequences:
"myschema.pubseq2"

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, 22 Jun 2026 at 10:05, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok.
>
> Some review comments for v11-0001 and v11-0002.
>
> //////////
> v11-0001
>
> ======
> doc/src/sgml/ref/create_publication.sgml
>
> (EXCEPT)
>
> 1.
> The recent commit 77b6dd9 added some more information to say:
>
> ---
> Once a table is excluded, the exclusion applies to that table
> regardless of its name or schema. Renaming the table or moving it to
> another schema using <command>ALTER TABLE ... SET SCHEMA</command> does
> not remove the exclusion.
> ---
>
> 1a
> AFAIK this same rule (the exclusion follows the object regardless of
> the schema) is going to apply also for excluding sequences. So, the
> docs should be updated to say something similar about behaviour for
> sequences.
>
Modified
> ~
>
> 1b.
> Maybe you need to add another test case to exclude some sequence, then
> alter the sequence's schema, then verify that the moved sequence is
> still excluded.
>
Added the test

> ~~~
> On Thu, Jun 18, 2026 at 11:29 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, 17 Jun 2026 at 12:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> ...
> > > ======
> > > src/bin/psql/describe.c
> > >
> > > describeOneTableDetails:
> > >
> > > 5.
> > > printfPQExpBuffer(&buf, "/* %s */\n",
> > > _("Get publications containing this sequence"));
> > > - appendPQExpBuffer(&buf, "SELECT pubname FROM pg_catalog.pg_publication p"
> > > + appendPQExpBuffer(&buf, "SELECT p.pubname FROM pg_catalog.pg_publication p"
> > > "\nWHERE p.puballsequences"
> > > "\n AND pg_catalog.pg_relation_is_publishable('%s')"
> > > + "\n AND NOT EXISTS (\n"
> > > + " SELECT 1\n"
> > > + " FROM pg_catalog.pg_publication_rel pr\n"
> > > + " WHERE pr.prpubid = p.oid AND\n"
> > > + " pr.prrelid = '%s')"
> > > "\nORDER BY 1",
> > > - oid);
> > > + oid, oid);
> > >
> > > 5a.
> > > IIUC, the "NOT EXISTS" part is for skipping the excluded sequences.
> > > And, you don't say "AND pr.prexcept" because it is redundant since
> > > the only way for a SEQUENCE to be individually in pg_publication_rel
> > > is if it is an excluded sequence.
> > >
> > > Is my understanding correct? I think it is too subtle -- it might be
> > > better to say "AND pr.prexcept" even if it is not strictly needed.
> > >
> > Yes, your understanding is correct.
> > For ALL SEQUENCES publication, the pg_publication_rel will have the
> > entry only if the sequence is specified in the EXCEPT list.
> > I didn't add "AND pr.prexcept" because the corresponding code for ALL
> > TABLES publications also does not use "AND pr.prexcept".
> ...
> > I think it will be consistent with EXCEPT (TABLE .. ) case. So, I
> > think we should not add "AND pr.prexcept".
>
> That’s OK, but I felt if you are going to omit the 'prexcept', then
> perhaps there should be a comment to explain (in all places) why it is
> ok to do that.
>
Added the comments

> //////////
> v11-0002.
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> ...or marks the publication as FOR ALL SEQUENCES or FOR ALL TABLES,
> optionally using EXCEPT to exclude specific tables or sequences.
>
> ~
>
> That current documentation sentence seems muddled because the
> publication types are not in the same as the order of the excluded
> types.
>
> IMO, alternatives below are better:
> a) ...FOR ALL SEQUENCES or FOR ALL TABLES, optionally using EXCEPT to
> exclude specific sequences or tables.
> b) ...FOR ALL TABLES or FOR ALL SEQUENCES, optionally using EXCEPT to
> exclude specific tables or sequences.
>
> (I preferred b, since everywhere else seems to be ordered
> tables/sequences not sequences/tables.).
>
I also think (b) is better.

I have addressed the comments and attached the updated v12 patch

Thanks,
Shlok Kyal

Hi Shlok,

I applied the latest v11 patch series and tested it on my setup.
I verified the new EXCEPT support for both CREATE PUBLICATION and
ALTER PUBLICATION with FOR ALL SEQUENCES. I tested single and multiple
sequence exclusions, replacing and clearing exclusion lists, and
checked the output using \dRp+.
I also tried a number of edge cases, including non-existent sequences,
temporary sequences, unlogged sequences, duplicate entries in the
EXCEPT list, schema-qualified sequence names, and sequences with the
same name in different schemas. All of these behaved as expected.
In addition, I verified that pg_dump correctly preserves the EXCEPT
(SEQUENCE...)clause and that psql tab completion suggests sequence
names correctly for both CREATE PUBLICATION and ALTER PUBLICATION.
Overall, the patch applied cleanly, built successfully, and all the
test cases I tried worked as expected. I didn't notice any issues
during testing.

Regards,
solai

Hi Shlok.

Some review comments for patches of v12

//////////
Patch v12-0001
//////////

======
src/backend/commands/publicationcmds.c

ObjectsInPublicationToOids:

1.
/*
* Convert the PublicationObjSpecType list into schema oid list and
* PublicationRelation list.
*/
static void
ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
List **rels, List **excepttbls, List **exceptseqs,
List **schemas)

I think this function comment can be made clearer. IIUC, there are 4
possible outputs from this function but the comment currently only
mentions 2. Also, it might be better to rearrange to make the
description use the same order as the output parameters.

SUGGESTION
Convert the PublicationObjSpecType list into PublicationRelation lists
(`rels`, `excepttbls`, `exceptseqs`) and a schema oid list
(`schemas`).

~~~

CreatePublication:

2.
+ if (stmt->for_all_sequences)
+ {
+ /* Process EXCEPT sequence list */
+ if (exceptseqs != NIL)
+ {
+ List *rels = OpenRelationList(exceptseqs);
+
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_SEQUENCE);
+ CloseRelationList(rels);
+ }
+ }

if (stmt->for_all_tables)
{
/* Process EXCEPT table list */
- if (exceptrelations != NIL)
+ if (excepttbls != NIL)
{
List *rels;

- rels = OpenTableList(exceptrelations);
- PublicationAddTables(puboid, rels, true, NULL);
- CloseTableList(rels);
+ rels = OpenRelationList(excepttbls);
+ PublicationAddRelations(puboid, rels, true, NULL, RELKIND_RELATION);
+ CloseRelationList(rels);

Here the 1st `rels` declaration does the assignment at the same time,
but the 2nd one does not. These code fragment are almost identical, so
let's make the declarations/assignments consistent.

~~~

3.
/* EXCEPT clause is not supported with ALTER PUBLICATION */
Assert(exceptseqs == NIL);
The comment is a bit misleading because:
- AFAIK this is only temporary for patch 0001.
- It is only EXCEPT (SEQUENCE ...) that is not supported yet; not
every EXCEPT clause.

SUGGESTION
TODO - The EXCEPT (SEQUENCE ...) clause is not yet supported with
ALTER PUBLICATION

======
src/backend/parser/gram.y

4.
static void
preprocess_pub_all_objtype_list(List *all_objects_list, List **pubobjects,
bool *all_tables, bool *all_sequences,
core_yyscan_t yyscanner)
{
if (!all_objects_list)
return;

*all_tables = false;
*all_sequences = false;

Shouldn't those assignments precede the "if (!all_objects_list)",
otherwise if the booleans are not set before the check you are relying
too much that the caller's makeNode() had set them already false on
entry.

======
src/bin/pg_dump/pg_dump.c

5.
+ /* Include EXCEPT (SEQUENCE) clause if there are except_sequences. */
+ for (SimplePtrListCell *cell = pubinfo->except_sequences.head; cell;
cell = cell->next)
+ {
+ TableInfo *tbinfo = (TableInfo *) cell->ptr;
+
+ if (++n_except == 1)
+ appendPQExpBufferStr(query, " EXCEPT (");
+ else
+ appendPQExpBufferStr(query, ", ");
+ appendPQExpBuffer(query, "SEQUENCE %s", fmtQualifiedDumpable(tbinfo));
+ }

Unlike tables which might have qualifiers 'ONLY' and '*', there are no
additional qualifier needed for sequences, so perhaps it is better to
just generate one excluded list:

e.g.
EXCEPT (SEQUENCE a, b, c, d, e);
instead of
EXCEPT (SEQUENCE a, SEQUENCE b, SEQUENCE c, SEQUENCE d, SEQUENCE e);

~

SUGGESTION:
seqname = fmtQualifiedDumpable(tbinfo);
if (++n_except == 1)
appendPQExpBufferStr(query, " EXCEPT (SEQUENCE %s", seqname);
else
appendPQExpBufferStr(query, ", %s", seqname);

======
src/bin/psql/describe.c

6.
+ if (pset.sversion >= 190000)
+ {
+ appendPQExpBuffer(&buf, "\n AND NOT EXISTS (\n"
+ " SELECT 1\n"
+ " FROM pg_catalog.pg_publication_rel pr\n"
+ " WHERE pr.prpubid = p.oid AND\n"
+ " pr.prrelid = '%s')", oid);

IMO the first line should be split to make it more readable. Also the
other parts of this SQL statement put the \n at the beginning, so
maybe it is better to do same here too.

SUGGESTION
appendPQExpBuffer(&buf,
"\n AND NOT EXISTS ("
"\n SELECT 1"
"\n FROM pg_catalog.pg_publication_rel pr"
"\n WHERE pr.prpubid = p.oid AND"
"\n pr.prrelid = '%s')", oid);

//////////
Patch v12-0002.
//////////

======
src/backend/commands/publicationcmds.c

AlterPublicationRelations:

1.
/*
- * In FOR ALL TABLES mode, relations are tracked as exclusions
- * (EXCEPT clause). Fetch the current excluded relations so they
- * can be reconciled with the specified EXCEPT list.
+ * In FOR ALL TABLES/ SEQUENCES mode, relations are tracked as
+ * exclusions (EXCEPT clause). Fetch the current excluded
+ * relations so they can be reconciled with the specified EXCEPT
+ * list.
*
* This applies only if the existing publication is already
- * defined as FOR ALL TABLES; otherwise, there are no exclusion
- * entries to process.
+ * defined as FOR ALL TABLES/ FOR ALL SEQUENCES; otherwise, there
+ * are no exclusion entries to process.
*/

One place refers to "FOR ALL TABLES/ SEQUENCES" and another says "FOR
ALL TABLES/ FOR ALL SEQUENCES". Should use consistent terminology. I
preferred the 2nd variant.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Jun 23, 2026 at 3:03 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
>
> I have addressed the comments and attached the updated v12 patch
>

Thanks Shlok. I have resumed review of this thread. A few comment for 001:

1)

Can we make 'opt_sequence' (SEQUENCE | EMPTY) similar to opt_table and
then use it here. It will be similar to pub_except_tbl_list and easier
to understand.

2)
getPublications:
+ if (relkind == RELKIND_SEQUENCE)
+ simple_ptr_list_append(&pubinfo[i].except_sequences, tbinfo);
+ else
+ simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);

Can we do this:
if (relkind == RELKIND_SEQUENCE)
...
else if (relkind == RELKIND_RELATION ||
relkind == RELKIND_PARTITIONED_TABLE)
...
else
Assert(false);

3)
describeOneTableDetails:
+ * Skip entries where this sequence appears in the publication's
+ * EXCEPT list.
+ *
+ * For a FOR ALL SEQUENCES publication, pg_publication_rel
+ * contains entries only for sequences specified in the EXCEPT
+ * clause, so there is no need to check pr.prexcept explicitly.

My personal preference will be to have pr.prexcept check in the query
and get rid of this comment.

4)
- char *footers[3] = {NULL, NULL, NULL};
+ char *footers[4] = {NULL, NULL, NULL, NULL};

Is this intentional? We are using only 3 of these though.

thanks
Shveta

On Wed, 24 Jun 2026 at 15:21, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Tue, Jun 23, 2026 at 3:03 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> >
> > I have addressed the comments and attached the updated v12 patch
> >
>
> Thanks Shlok. I have resumed review of this thread. A few comment for 001:
>
>
> 1)
>
> Can we make 'opt_sequence' (SEQUENCE | EMPTY) similar to opt_table and
> then use it here. It will be similar to pub_except_tbl_list and easier
> to understand.
>
The compiler cannot compile if I use a syntax similar to 'pub_except_tbl_list'.
I tried making it like:
pub_except_seq_list: PublicationExceptSeqSpec

{ $$ = list_make1($1); }
| pub_except_seq_list ','
opt_sequence PublicationExceptSeqSpec

{ $$ = lappend($1, $4); } ;
Getting error as:
gram.y: error: shift/reduce conflicts: 1 found, 0 expected
gram.y: warning: shift/reduce conflict on token SEQUENCE
[-Wcounterexamples] First example: pub_except_seq_list • SEQUENCE
PublicationExceptSeqSpec Shift derivation pub_except_seq_list ↳ 1513:
pub_except_seq_list opt_sequence PublicationExceptSeqSpec ↳ 1855: •
SEQUENCE Second example: pub_except_seq_list • SEQUENCE Reduce
derivation pub_except_seq_list ↳ 1513: pub_except_seq_list
opt_sequence SEQUENCE ↳ 1856: ε •

This syntax is not able to compile because it is getting confused:
For example: ".... EXCEPT (SEQUENCE s1, SEQUENCE ... )"
For the second 'SEQUENCE', it's unclear whether we should treat
'SEQUENCE' as a sequence_name or a keyword.

To avoid the error, I wrote it the way in the patch.
'opt_table' does not face the same issue because 'TABLE' is a reserved
keyword, but 'SEQUENCE' is not.

> 2)
> getPublications:
> + if (relkind == RELKIND_SEQUENCE)
> + simple_ptr_list_append(&pubinfo[i].except_sequences, tbinfo);
> + else
> + simple_ptr_list_append(&pubinfo[i].except_tables, tbinfo);
>
> Can we do this:
> if (relkind == RELKIND_SEQUENCE)
> ...
> else if (relkind == RELKIND_RELATION ||
> relkind == RELKIND_PARTITIONED_TABLE)
> ...
> else
> Assert(false);
>
>
> 3)
> describeOneTableDetails:
> + * Skip entries where this sequence appears in the publication's
> + * EXCEPT list.
> + *
> + * For a FOR ALL SEQUENCES publication, pg_publication_rel
> + * contains entries only for sequences specified in the EXCEPT
> + * clause, so there is no need to check pr.prexcept explicitly.
>
> My personal preference will be to have pr.prexcept check in the query
> and get rid of this comment.
>
> 4)
> - char *footers[3] = {NULL, NULL, NULL};
> + char *footers[4] = {NULL, NULL, NULL, NULL};
>
> Is this intentional? We are using only 3 of these though.

Yes, this is intentional. The last element of the footers array must
always be NULL because printQuery() treats it as a NULL-terminated
array:
/* set footers */
if (opt->footers)
{
char **footer;

for (footer = opt->footers; *footer; footer++)
printTableAddFooter(&cont, *footer);
}

The loop terminates only when it encounters a NULL entry, so the last
array element must remain NULL.
This is also consistent with the existing code. For example, in describe.c:
if (tableinfo.relkind == RELKIND_PROPGRAPH)
{
printQueryOpt popt = pset.popt;
char *footers[3] = {NULL, NULL, NULL};

only footers[0] and footers[1] are populated, while footers[2] serve
as the required terminating NULL.

I have addressed the remaining comments and attached the updated v13 patches
I have also addressed Peter's comments in [1].

[1]: https://www.postgresql.org/message-id/CAHut%2BPvMXOR73%2BWdvSWd3B8j6jDQQXRtO_bKEudi76n30GAApQ%40mail.gmail.com

Thanks,
Shlok Kyal

On Thu, Jun 25, 2026 at 2:17 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Wed, 24 Jun 2026 at 15:21, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 23, 2026 at 3:03 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >
> > >
> > > I have addressed the comments and attached the updated v12 patch
> > >
> >
> > Thanks Shlok. I have resumed review of this thread. A few comment for 001:
> >
> >
> > 1)
> >
> > Can we make 'opt_sequence' (SEQUENCE | EMPTY) similar to opt_table and
> > then use it here. It will be similar to pub_except_tbl_list and easier
> > to understand.
> >
> The compiler cannot compile if I use a syntax similar to 'pub_except_tbl_list'.
> I tried making it like:
> pub_except_seq_list: PublicationExceptSeqSpec
>
> { $$ = list_make1($1); }
> | pub_except_seq_list ','
> opt_sequence PublicationExceptSeqSpec
>
> { $$ = lappend($1, $4); } ;
> Getting error as:
> gram.y: error: shift/reduce conflicts: 1 found, 0 expected
> gram.y: warning: shift/reduce conflict on token SEQUENCE
> [-Wcounterexamples] First example: pub_except_seq_list • SEQUENCE
> PublicationExceptSeqSpec Shift derivation pub_except_seq_list ↳ 1513:
> pub_except_seq_list opt_sequence PublicationExceptSeqSpec ↳ 1855: •
> SEQUENCE Second example: pub_except_seq_list • SEQUENCE Reduce
> derivation pub_except_seq_list ↳ 1513: pub_except_seq_list
> opt_sequence SEQUENCE ↳ 1856: ε •
>
> This syntax is not able to compile because it is getting confused:
> For example: ".... EXCEPT (SEQUENCE s1, SEQUENCE ... )"
> For the second 'SEQUENCE', it's unclear whether we should treat
> 'SEQUENCE' as a sequence_name or a keyword.
>
> To avoid the error, I wrote it the way in the patch.
> 'opt_table' does not face the same issue because 'TABLE' is a reserved
> keyword, but 'SEQUENCE' is not.
>

Okay, thanks for clarifying. I am surprised that SEQUENCE is not a
reserved keyword.
postgres=# create sequence sequence;
CREATE SEQUENCE

Anyway, currnet implementation is fine then.

> >
> > 4)
> > - char *footers[3] = {NULL, NULL, NULL};
> > + char *footers[4] = {NULL, NULL, NULL, NULL};
> >
> > Is this intentional? We are using only 3 of these though.
>
> Yes, this is intentional. The last element of the footers array must
> always be NULL because printQuery() treats it as a NULL-terminated
> array:
> /* set footers */
> if (opt->footers)
> {
> char **footer;
>
> for (footer = opt->footers; *footer; footer++)
> printTableAddFooter(&cont, *footer);
> }
>
> The loop terminates only when it encounters a NULL entry, so the last
> array element must remain NULL.
> This is also consistent with the existing code. For example, in describe.c:
> if (tableinfo.relkind == RELKIND_PROPGRAPH)
> {
> printQueryOpt popt = pset.popt;
> char *footers[3] = {NULL, NULL, NULL};
>
> only footers[0] and footers[1] are populated, while footers[2] serve
> as the required terminating NULL.
>

Okay, thanks for the details.

thanks
Shveta

//////
Patch v13-0001
//////

======
src/bin/pg_dump/t/002_pg_dump.pl

1.
Now dump.c can generate a single SEQUENCE exclusion list with multiple
items, but it doesn't seem tested because the 002_pg_dump test file
remained unchanged in v13.

I think you should enhance one of your test cases to exclude multiple sequences.

//////
Patch v13-0002
//////

No comments.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Please find a few trivial comments on v13-001:

1)

check_publication_add_relation:

+ * 'pubrelkind' is the relkind accepted by the publication clause,
+ * while 'targetrelkind' is the relkind of the relation being added.

pubrelkind is the argument while targetrelkind is not. We generally
explain arguements here.

Can we rephrase to:
* 'pubrelkind' is the relkind accepted by the publication clause.
* The relkind of relation in given 'pri' is checked for compatibility
* against it. Error is emitted if they are not compatible.

2)
describePublications:

}
- else
+ if (puballtables)
{

Now since else is converted to independent 'if' block we can add a
blank line before it for better readability.

Same for this:

}
+ if (puballsequences)
+ {

3)

We have each test header like below starting from begining of file:

---------------------------------------------
-- Tests for inherited tables, and
-- EXCEPT clause tests for inherited tables
---------------------------------------------

---------------------------------------------
-- EXCEPT clause tests for partitioned tables
---------------------------------------------

So we can convert ours too in above format so that it is more visible:
+-- Test ALL SEQUENCES with EXCEPT clause

4)
We can add these tests to sql file:

a) SEQ keyword equivalent test to below table one:

-- fail - first table in the EXCEPT list should use TABLE keyword
CREATE PUBLICATION testpub_foralltables_excepttable2 FOR ALL TABLES
EXCEPT (testpub_tbl1, testpub_tbl2);

b) Try to add either of temporay or unlogged seq to except list.

thanks
Shveta

On Fri, 26 Jun 2026 at 14:23, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> Please find a few trivial comments on v13-001:
>
> 1)
>
> check_publication_add_relation:
>
> + * 'pubrelkind' is the relkind accepted by the publication clause,
> + * while 'targetrelkind' is the relkind of the relation being added.
>
> pubrelkind is the argument while targetrelkind is not. We generally
> explain arguements here.
>
> Can we rephrase to:
> * 'pubrelkind' is the relkind accepted by the publication clause.
> * The relkind of relation in given 'pri' is checked for compatibility
> * against it. Error is emitted if they are not compatible.
>
> 2)
> describePublications:
>
> }
> - else
> + if (puballtables)
> {
>
> Now since else is converted to independent 'if' block we can add a
> blank line before it for better readability.
>
> Same for this:
>
> }
> + if (puballsequences)
> + {
>
>
> 3)
>
> We have each test header like below starting from begining of file:
>
> ---------------------------------------------
> -- Tests for inherited tables, and
> -- EXCEPT clause tests for inherited tables
> ---------------------------------------------
>
> ---------------------------------------------
> -- EXCEPT clause tests for partitioned tables
> ---------------------------------------------
>
> So we can convert ours too in above format so that it is more visible:
> +-- Test ALL SEQUENCES with EXCEPT clause
>
> 4)
> We can add these tests to sql file:
>
> a) SEQ keyword equivalent test to below table one:
>
> -- fail - first table in the EXCEPT list should use TABLE keyword
> CREATE PUBLICATION testpub_foralltables_excepttable2 FOR ALL TABLES
> EXCEPT (testpub_tbl1, testpub_tbl2);
>
> b) Try to add either of temporay or unlogged seq to except list.
>
I have addressed all the comments. I have also addressed the comments
by Peter in [1].
Please find the updated v14 patch.

[1]: https://www.postgresql.org/message-id/CAHut%2BPsgCQa2hcT8TNqejV3y4U5ouj%3DZCOLMxgVGvBrUEkLaKg%40mail.gmail.com

Thanks,
Shlok Kyal

Hi Shlok.

I only had one general comment about v14. IMO, it would be better to
try to make the regression test object names more meaningful where
possible (though sometimes it won't be). The xxx1, xxx2, and xxx3
become harder to read as more gets added.

e.g. `regress_pub_seq3` -- What does 'pub' mean here? This is an
unlogged sequence you want to exclude.
e.g. `regress_pub_forallsequences4` -- Doesn't mean much.
e.g. `tab_seq` -- This is meant to be a plain table; Not a table
pretending to be a seq.

Below is an example of some modifications, but there are many more.
Consider checking all the names to see if they can be improved
(sometimes you may be stuck having to accommodate existing names).
(same comment applies to both patches).

~~~

BEFORE
+-- fail - unlogged sequence is specified in EXCEPT sequence list
+CREATE UNLOGGED SEQUENCE regress_pub_seq3;
+CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL SEQUENCES
EXCEPT (SEQUENCE regress_pub_seq3);
+
+-- fail - temporary sequence is specified in EXCEPT sequence list
+CREATE TEMPORARY SEQUENCE regress_pub_seq4;
+CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL SEQUENCES
EXCEPT (SEQUENCE regress_pub_seq4);
+
+-- fail - sequence object is specified in EXCEPT table list
+CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL TABLES EXCEPT
(TABLE regress_pub_seq0);
+
+-- fail - table object is specified in EXCEPT sequence list
+CREATE TABLE tab_seq(a int);
+CREATE PUBLICATION regress_pub_forallsequences4 FOR ALL SEQUENCES
EXCEPT (SEQUENCE tab_seq);
+

SUGGESTION
+-- fail - unlogged sequence is specified in EXCEPT sequence list
+CREATE UNLOGGED SEQUENCE regress_seq_unlogged;
+CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE regress_seq_unlogged);
+
+-- fail - temporary sequence is specified in EXCEPT sequence list
+CREATE TEMPORARY SEQUENCE regress_seq_temp;
+CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE regress_seq_temp);
+
+-- fail - sequence object is specified in EXCEPT table list
+CREATE PUBLICATION regress_pub_should_fail FOR ALL TABLES EXCEPT
(TABLE regress_seq1);
+
+-- fail - table object is specified in EXCEPT sequence list
+CREATE TABLE tab1(a int);
+CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE tab1);
+

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Fri, Jun 26, 2026 at 6:08 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> I have addressed all the comments. I have also addressed the comments
> by Peter in [1].
> Please find the updated v14 patch.
>
> [1]: https://www.postgresql.org/message-id/CAHut%2BPsgCQa2hcT8TNqejV3y4U5ouj%3DZCOLMxgVGvBrUEkLaKg%40mail.gmail.com

Thanks, a few trivial comments:

001:
1)
PublicationRelation

+typedef struct PublicationRelation
{
NodeTag type;
RangeVar *relation; /* publication relation */
Node *whereClause; /* qualifications */
List *columns; /* List of columns in a publication table */
bool except; /* True if listed in the EXCEPT clause */
-} PublicationTable;
+} PublicationRelation;

Can we make comments more clear. Suggestion:

typedef struct PublicationRelation
{
NodeTag type;
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 */
} PublicationRelation;

002:
2)
+ if ((pubrelkind == RELKIND_RELATION && relkind == RELKIND_RELATION) ||
+ (pubrelkind == RELKIND_RELATION && relkind == RELKIND_PARTITIONED_TABLE) ||
+ (pubrelkind == RELKIND_SEQUENCE && relkind == RELKIND_SEQUENCE))

Can we optimize it to:
if ((pubrelkind == RELKIND_RELATION &&
(relkind == RELKIND_RELATION || relkind == RELKIND_PARTITIONED_TABLE)) ||
(pubrelkind == RELKIND_SEQUENCE && relkind == RELKIND_SEQUENCE))

3)
+-- Modify the sequence list in the EXCEPT clause
+ALTER PUBLICATION regress_pub_forallsequences3 SET ALL SEQUENCES
EXCEPT (SEQUENCE regress_pub_seq0);

After this or at the end, can we add a testcase for 'SET ALL
SEQUENCES' to cover the reset except-seq code-flow.

thanks
Shveta