MIME-Version: 1.0
References: 
 <CANhcyEVSXyQkvmrsOWPdQqnm2J3GMyQQrKhyCJiBQzqs6AvSow@mail.gmail.com>
 <CALDaNm125dv88fUDgBPBM-N-hXbF0NLqKe-ymEpMRNymUYRQAA@mail.gmail.com>
 <CANhcyEUxgzaNJNNO_-12wYwGLihpuKmsMH2g4TFnRPx5AQVZmg@mail.gmail.com>
 <CAJpy0uBB4N8KOrHchdgprVi2Ws1+gTcEr+bC2A_ziAHOcZcTqA@mail.gmail.com>
 <CAJpy0uCAJQvBjD7qNWWGnZP_LDwS8AiUJC7YMict9UcYqH=XeQ@mail.gmail.com>
 <CANhcyEU_Yq9ZJ2n5Sqa7RoHze0TD0RGxLQQgV1F6Jm2AROEh8g@mail.gmail.com>
 <CANhcyEWj-ECj=WC+HD_kv27Dn6FkTngFQCVJVTVZfJnjCTKMBQ@mail.gmail.com>
 <CANhcyEW03XO5tLb7opt1yQGHWJ7Ew=L65EWcdrKH=F0mUpuR3A@mail.gmail.com>
 <CAHut+Ptu0Bkwnr5eetdmFhJC7SsEtKjNe_cTwOg5wF65fjyV8w@mail.gmail.com>
 <CANhcyEU+4Z6NXMEk6OmctFv=_pY8K5AiazkfVmn7zxhwQO6CEQ@mail.gmail.com>
In-Reply-To: 
 <CANhcyEU+4Z6NXMEk6OmctFv=_pY8K5AiazkfVmn7zxhwQO6CEQ@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Wed, 27 May 2026 13:37:32 +1000
Message-ID: 
 <CAHut+PvOWjKJfNorGO8whnaAtqZb-4zN8u6aOajCw3hn-tMwyA@mail.gmail.com>
Subject: Re: Support EXCEPT for ALL SEQUENCES publications
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, vignesh C <vignesh21@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPvOWjKJfNorGO8whnaAtqZb-4zN8u6aOajCw3hn-tMwyA%40mail.gmail.com>
Precedence: bulk

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