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>
 <CAHut+PvOWjKJfNorGO8whnaAtqZb-4zN8u6aOajCw3hn-tMwyA@mail.gmail.com>
 <CANhcyEV_Sv+xQzsjo6hbowDbGV5J7RhFWuQQxWeUWPNPd0k1=w@mail.gmail.com>
In-Reply-To: 
 <CANhcyEV_Sv+xQzsjo6hbowDbGV5J7RhFWuQQxWeUWPNPd0k1=w@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Tue, 2 Jun 2026 10:07:07 +1000
Message-ID: 
 <CAHut+Pt6O3YD8h61RCtzMrRwgvS=RDRNf4KuXAUn1WYnoFU_uQ@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"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPt6O3YD8h61RCtzMrRwgvS%3DRDRNf4KuXAUn1WYnoFU_uQ%40mail.gmail.com>
Precedence: bulk

Hi Shlok,

Some review comments for patch v8-0002.

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

Examples.

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

~~~

2.
+  <para>
+   Replace the sequence list in the publication's EXCEPT clause:
+<programlisting>
+ALTER PUBLICATION mypublication SET ALL SEQUENCES EXCEPT (SEQUENCE seq1, s=
eq2);
+</programlisting></para>
+
+  <para>
+   Reset the publication to be a ALL SEQUENCES publication with no exclude=
d
+   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:/

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

GetIncludedPublicationRelations:

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

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

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

get_delete_rels:

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

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

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

~~~

AlterPublicationRelations:

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

Maybe this can be reworded more simply:

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

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