Subject: Re: Support EXCEPT for ALL SEQUENCES publications

From: Peter Smith <[email-masked]>
Date: Mon, 25 May 2026 17:37:22 +1000

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

---

From: Peter Smith <[email-masked]>
Date: Mon, 25 May 2026 17:37:58 +1000

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

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 26 May 2026 11:30:12 +0530

> 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

---

From: Peter Smith <[email-masked]>
Date: Wed, 27 May 2026 13:37:32 +1000

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?

======

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

---

From: Shlok Kyal <[email-masked]>
Date: Wed, 27 May 2026 18:33:06 +0530

> properly after both your EXCEPT changes and Nisha's EXCEPT [1] changes
I agree.

> The comment does not match the code/assert. Shouldn't it also do
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].
> [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/[email-masked]

Thanks,
Shlok Kyal

---

From: Zsolt Parragi <[email-masked]>
Date: Wed, 27 May 2026 14:51:59 -0700

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?

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 29 May 2026 17:29:11 +0530

Hi Zsolt,
Thanks for reviewing the patch.

> Shouldn't the matching free calls also updated, this now leaks footers[2]?
Yes, correct. Made the change for the same.

> 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 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

---

From: Peter Smith <[email-masked]>
Date: Tue, 2 Jun 2026 10:07:07 +1000

Hi Shlok,

Some review comments for patch v8-0002.

======

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:/

======

> 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?

======

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.

~~~

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

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 5 Jun 2026 13:14:11 +0530

> "ALL SEQUENCES" etc, those should all be using SGML <literal> markup.
Fixed

> /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?

> PUBLICATION mypublication SET ALL SEQUENCES;" example.
Fixed

> /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.

> 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.

> anymore. Those unwanted ones are what the function returns.
I have reworded the comment

> * 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

---

From: Peter Smith <[email-masked]>
Date: Fri, 12 Jun 2026 15:25:27 +1000

Some review comments for v9-0001.

======

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

---

From: Peter Smith <[email-masked]>
Date: Fri, 12 Jun 2026 15:28:46 +1000

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

======

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/

======

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

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 15 Jun 2026 15:43:06 +0530

> 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.

> /relation name/Relation name/

I have addressed the remaining comments in the v10 patches.

Thanks,
Shlok Kyal

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 15 Jun 2026 15:44:00 +0530

> 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.

> 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

---

From: Peter Smith <[email-masked]>
Date: Wed, 17 Jun 2026 17:06:43 +1000

////////////////////
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)

======

2.
> + * it excludes sequences mentioned in the EXCEPT clause.

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

======

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
////////////////////

======

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

---

From: Shlok Kyal <[email-masked]>
Date: Thu, 18 Jun 2026 16:58:52 +0530

Added a TODO to use version check for PG20

> 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".

> EXISTS" part needs another check.
Separated and added a TODO to change the version.

> 200000 as soon as the PG19 version is bumped.
Added

> change the version check to 200000 when the PG version is bumped.
Added a TODO
> +# Subscribe to multiple publications with different EXCEPT sequence list

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

Thanks,
Shlok Kyal

---

From: Peter Smith <[email-masked]>
Date: Mon, 22 Jun 2026 16:35:11 +1200

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.

~~~
...
...
> 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.

======

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

---

From: Peter Smith <[email-masked]>
Date: Tue, 23 Jun 2026 09:03:27 +1200

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.

> alter the sequence's schema, then verify that the moved sequence is

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

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 23 Jun 2026 15:03:20 +0530

> Some review comments for v11-0001 and v11-0002.
Modified
> alter the sequence's schema, then verify that the moved sequence is
Added the test

> perhaps there should be a comment to explain (in all places) why it is
Added the comments

I also think (b) is better.

I have addressed the comments and attached the updated v12 patch

Thanks,
Shlok Kyal

---

From: solai v <[email-masked]>
Date: Tue, 23 Jun 2026 17:09:10 +0530

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

---

From: Peter Smith <[email-masked]>
Date: Wed, 24 Jun 2026 18:04:27 +1200

Hi Shlok.

Some review comments for patches of v12

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

======

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)
+ {
+ if (exceptseqs != NIL)

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

======

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.

======

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.
//////////

======

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

---

From: shveta malik <[email-masked]>
Date: Wed, 24 Jun 2026 15:21:35 +0530

> 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:

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

---

From: Shlok Kyal <[email-masked]>
Date: Thu, 25 Jun 2026 14:17:44 +0530

> then use it here. It will be similar to pub_except_tbl_list and easier
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.

> 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

---

From: shveta malik <[email-masked]>
Date: Thu, 25 Jun 2026 14:46:22 +0530

> 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.

> as the required terminating NULL.

Okay, thanks for the details.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Fri, 26 Jun 2026 11:53:12 +1200

//////
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

---

From: shveta malik <[email-masked]>
Date: Fri, 26 Jun 2026 14:23:04 +0530

Please find a few trivial comments on v13-001:

1)

+ * '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

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 26 Jun 2026 18:08:27 +0530

> We have each test header like below starting from begining of file:
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

---

From: Peter Smith <[email-masked]>
Date: Mon, 29 Jun 2026 12:31:52 +1200

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.

---

From: shveta malik <[email-masked]>
Date: Mon, 29 Jun 2026 16:02:16 +0530

> [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

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 30 Jun 2026 12:13:39 +0530

> +CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
I agree with your suggestions, I have made the changes.
I have also made changes for other objects as well in publication.sql
and 037_except.pl files.

I also agree with the suggestions by Shveta in [1] and made the
required changes.

Please find the updated v15 patch attached.

[1]: https://www.postgresql.org/message-id/CAJpy0uDv7mDc=MqTP-QXWZcRM_m-MW-wYtRh3xwqD0sm-F+=[email-masked]

Thanks,
Shlok Kyal

---

From: Ashutosh Sharma <[email-masked]>
Date: Tue, 30 Jun 2026 13:16:53 +0530

Hi,

> Please find the updated v15 patch attached.

+ /* TODO : Add a version check for PG 20 */

I've few questions related to above change:

1) Should the second printfPQExpBuffer() be appendPQExpBuffer()? As
written, it overwrites the query comment added just above, so psql -E
will not show "Get sequences excluded by this publication".

2) The above query may work on PG 19, but since the feature is going
to be part of PG20, why do you want this query to be part of PG19?

--

Do we also need to include a tap test for "ALTER PUBLICATION ... SET
ALL SEQUENCES EXCEPT (...)" followed by "ALTER SUBSCRIPTION ...
REFRESH SEQUENCES", and then sequence replication behavior is
verified?

--
With Regards,
Ashutosh Sharma.

---

From: Peter Smith <[email-masked]>
Date: Wed, 1 Jul 2026 11:49:13 +1200

Hi Shlok.

Some review comments for v15.

//////
v15-0001
//////

======
GENERAL

1.
The "20devel" is available, so now you can modify those version checks
in pg_dump.c and describe.c code.

======
src/test/regress/sql/publication.sql

2.

All the other objects have a `regress_` prefix so should this be `regress_tab1`?

~~~

3.
+-- Test combination of ALL SEQUENCES and ALL TABLES with EXCEPT clause
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION regress_pub_for_allsequences_alltables_except FOR
ALL TABLES EXCEPT (TABLE testpub_tbl1), ALL SEQUENCES EXCEPT (SEQUENCE
regress_seq0);
+\dRp+ regress_pub_for_allsequences_alltables_except
+RESET client_min_messages;

3a.
I suggest moving this test to be grouped with the earlier non-failing
tests. Probably then you won't need the extra set/reset
client_min_messages.

~

3b.
Maybe you could have excluded the same `tab1` (renamed `regress_tab1`)
that was created, instead of excluding some random table from
elsewhere.

======

4.
+$node_subscriber->safe_psql('postgres',
+ 'DROP SUBSCRIPTION tap_sub_all_seq_except');
+$node_publisher->safe_psql('postgres',
+ 'DROP PUBLICATION tap_pub_all_seq_except1');
+$node_publisher->safe_psql('postgres',
+ 'DROP PUBLICATION tap_pub_all_seq_except2');
+
 $node_publisher->stop('fast');

Missing a comment that says "Cleanup" ?

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

---

From: Shlok Kyal <[email-masked]>
Date: Wed, 1 Jul 2026 14:00:06 +0530

> will not show "Get sequences excluded by this publication".
I agree, the second printfPQExpBuffer() should be appendPQExpBuffer().
Made the change.

> to be part of PG20, why do you want this query to be part of PG19?
I added the checks for PG19, as the major version on master was still
pointing to PG19.
But now I noticed that the major version is now bumped to PG20, I have
modified the checks

Yes, I agree

I have made the changes and attached the updated v16 patches. I have
also addressed Peter's comment in [1].

[1]: https://www.postgresql.org/message-id/[email-masked]

Thanks,
Shlok Kyal

---

From: Peter Smith <[email-masked]>
Date: Thu, 2 Jul 2026 11:22:32 +1200

Only trivial comments for v16*

//////
v16-0001
//////

======

getPublictions:

1.
  /*
- * Get the list of tables for publications specified in the EXCEPT
- * TABLE clause.
+ * Get the list of tables and sequences for publications specified in
+ * the EXCEPT clause.
  *

Perhaps this comment should say that EXCEPT (TABLE ...) is introduced
in PG19, and EXCEPT (SEQUENCE ...) is introduced in PG20. That way
there are no surprises about the different version checks that follow.

======

2.
+# Check the initial data on subscriber
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT last_value, is_called FROM seq_excluded_in_pub1");
+is($result, '1|f', 'sequences in EXCEPT list is excluded');

typo: plural. Should be "sequences in the EXCEPT list are excluded"

//////
v16-0002
//////

======

1.
+is($result, '1|f', 'sequences in EXCEPT list is excluded');

typo: plural. Should be "sequences in the EXCEPT list are excluded"

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

---

From: Shlok Kyal <[email-masked]>
Date: Thu, 2 Jul 2026 11:52:46 +0530

> typo: plural. Should be "sequences in the EXCEPT list are excluded"
Thanks Peter for the review. I have addressed the comments.
Also the patch needed a rebase after the recent commits.

Please find the updated v17 patch attached.

Thanks,
Shlok Kyal

---

From: Ashutosh Sharma <[email-masked]>
Date: Fri, 3 Jul 2026 11:23:52 +0530

Hi,

> Please find the updated v17 patch attached.

Thank you for the updated patches. I see that the following command
works with your patch:

CREATE PUBLICATION combined_seq_pub_1
  FOR ALL SEQUENCES
  EXCEPT (SEQUENCE ONLY s1_seq);

May I know what the purpose of ONLY is here?

AFAIU, for FOR ALL TABLES, ONLY was added to handle inherited tables.
However, sequences do not have inheritance, so I am not sure what
semantics ONLY is intended to provide for sequences. Could you please
clarify?

--
With Regards,
Ashutosh Sharma.

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 3 Jul 2026 15:18:02 +0530

> semantics ONLY is intended to provide for sequences. Could you please
It was unintentional. ONLY should not be allowed with SEQUENCE.
This happened because, in gram.y, the EXCEPT SEQUENCE production uses
relation_expr:
+PublicationExceptSeqSpec:
+            relation_expr
+               {
+                   $$ = makeNode(PublicationObjSpec);
+                   $$->pubobjtype = PUBLICATIONOBJ_EXCEPT_SEQUENCE;
+                   $$->pubrelation = makeNode(PublicationRelation);
+                   $$->pubrelation->except = true;
+                   $$->pubrelation->relation = $1;
+                   $$->location = @1;
+               }
+   ;
relation_expr accepts ONLY, so it inadvertently allows syntax such as
EXCEPT (SEQUENCE ONLY s1).
I think it would be better to use qualified_name instead. That's also
what CREATE SEQUENCE uses, so it would be consistent with the rest of
the grammar and would prevent ONLY from being accepted.

With this change we are now throwing syntax error:
postgres=# CREATE PUBLICATION combined_seq_pub_1
  FOR ALL SEQUENCES
  EXCEPT (SEQUENCE ONLY s1_seq);
ERROR:  syntax error at or near "ONLY"
LINE 3:   EXCEPT (SEQUENCE ONLY s1_seq);

While making the change, I also found that I missed updating the
comments in gram.y for this feature.
I have updated the same and attached the updated v18 patches.

Thanks,
Shlok Kyal

---

From: Ashutosh Sharma <[email-masked]>
Date: Mon, 6 Jul 2026 11:34:04 +0530

Hi,

> I have updated the same and attached the updated v18 patches.

Thanks for fixing this. I found a few cosmetics issues while reviewing
the latest patches, please see if these make sense to you:

+# Check ALTER PUBLICATION ... ALL SEQUENCES (EXCEPT SEQUENCE ...)
+$node_publisher->safe_psql(

The placement of the EXCEPT clause here doesn't match the actual
supported syntax. The correct form is:

CREATE PUBLICATION combined_pub_seq FOR ALL SEQUENCES EXCEPT ( SEQUENCE ... )

--

Above comment doesn't look clearer to me. Perhaps something like "for
tables and sequences specified in the publication's EXCEPT clause"
would read more clearly. What do you think?

--

+ /* Print publications where the sequence is in the EXCEPT clause */
+ if (pset.sversion >= 200000)

This doesn't read well as well, how about "Print publications whose
EXCEPT clause contains this sequence"?

--

+ * In FOR ALL TABLES/ FOR ALL SEQUENCES mode, relations are

This seems to be having uneven spacing. I would rather replace '/'
with 'or' here.

--
With Regards,
Ashutosh Sharma.

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 7 Jul 2026 12:06:48 +0530

> CREATE PUBLICATION combined_pub_seq FOR ALL SEQUENCES EXCEPT ( SEQUENCE ... )
Hi Ashutosh,

Thanks for reviewing the patch.
I agree with your comments and addressed it in the lastest v19 patch.

I also noticed that the CI runs are failing.
It is failing because of the new test added in 0002 patch.
After we alter the publication and change the sequences in the EXCEPT
list, we should run 'ALTER SUBSCRIPTION ... REFRESH PUBLICATION' on
the subscription, so the same is reflected on the pg_subscription_rel.
I have made the change for the same in the updated patch.

Thanks
Shlok Kyal

---

From: Peter Smith <[email-masked]>
Date: Wed, 8 Jul 2026 12:25:52 +1000

Some review comments for v19-0001 (no comments for v19-0002)

//////////
Patch 0001
//////////

NOTE: The following review comments are all coming from the same PoV
that actually there can be multiple simultaneous EXCEPT clauses (e.g.
FOR ALL TABLES EXCEPT, and FOR ALL SEQUENCES EXCEPT), so it is not
quite correct to just refer to the singular "the EXCEPT clause" --
should use plural, or remove the ambiguity from the text.

======

1.
- * For a FOR ALL TABLES publication, the returned list excludes
tables mentioned
- * in the EXCEPT clause.
+ * For a FOR ALL TABLES publication, the returned list excludes tables
+ * specified in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
+ * it excludes sequences specified in the EXCEPT clause.

SUGGESTION
For a FOR ALL TABLES publication, the returned list excludes tables
specified in the EXCEPT (TABLE ...) clause. For a FOR ALL SEQUENCES
publication, it excludes sequences specified in the EXCEPT (SEQUENCE
...) clause.

======

getPublications:

2.
- * Get the list of tables for publications specified in the EXCEPT
- * TABLE clause.
+ * Get the list of tables and sequences specified in the publication's
+ * EXCEPT clause.

You cannot exclude tables and sequences from the same EXCEPT, so that
should say "EXCEPT clauses" (plural). But, it may be simpler to just
reword it.

SUGGESTION
Get the list of tables and sequences explicitly excluded from the publication.

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

describeOneTableDetails:

3.
+ /* Print publications whose EXCEPT clause contains this sequence */
+ if (pset.sversion >= 200000)

This is similar.  "whose EXCEPT clause" implies there is only one, but
in practice there can be multiple EXCEPT clauses: (FOR ALL TABLES
EXCEPT + FOR ALL SEQUENCES EXCEPT).

SUGGESTION
Print publications that explicitly exclude this sequence.

~~~

4.

This is similar. "the EXCEPT clause" implies there is only one, but
there may be multiple. I saw that you were mimicking the existing
comment, so those should be the same.

SUGGESTION #1 (redundant, because the next string already says what
the code is doing)
Just remove both comments.

SUGGESTION #2 (clarify what clauses you are talking about)
/* Get tables in the EXCEPT (TABLE ...) clause for this publication */
and
/* Get sequences in the EXCEPT (SEQUENCE ...) clause for this publication */

======
src/include/nodes/parsenodes.h

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

Same. /True if listed in the EXCEPT clause/True if listed in an EXCEPT clause/

~~~

6.
  PUBLICATIONOBJ_EXCEPT_TABLE, /* A table in the EXCEPT clause */
+ PUBLICATIONOBJ_EXCEPT_SEQUENCE, /* A sequence in the EXCEPT clause */

/A table in the EXCEPT clause/A table in an EXCEPT clause/
/A sequence in the EXCEPT clause/A sequence in an EXCEPT clause/

~~~

7.
- List    *except_tables; /* tables specified in the EXCEPT clause */
+ List    *except_relations; /* depending on the 'pubobjtype', this is
+ * a list of either tables or sequences
+ * specified in the EXCEPT clause */
  ParseLoc location; /* token location, or -1 if unknown */
 } PublicationAllObjSpec;

/specified in the EXCEPT clause/specified in an EXCEPT clause/

======

8.
+-- Check that the sequence description shows the publications where
it is listed
+-- in the EXCEPT clause

/the EXCEPT clause/an EXCEPT clause/

~~~

9.

/with EXCEPT clause/, each with an EXCEPT clause/

======

10.
+is($result, '200|t',
+ 'check replication of a sequence in the EXCEPT clause of one
publication but included by another'
+);

(same x2)

SUGGESTION
check replication of a sequence excluded by one publication but
included by another

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

---

From: Shlok Kyal <[email-masked]>
Date: Wed, 8 Jul 2026 17:38:13 +0530

> check replication of a sequence excluded by one publication but
I have addressed the comments and attached the updated v20 patch.

Thanks

Shlok Kyal