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