On Tue, 2 Jun 2026 at 05:37, Peter Smith <[email protected]> wrote: > > Hi Shlok, > > Some review comments for patch v8-0002. > > ====== > doc/src/sgml/ref/alter_publication.sgml > > Examples. > > 1. > Everywhere in these example descriptions where you say "EXCEPT" or > "ALL SEQUENCES" etc, those should all be using SGML <literal> markup. > Fixed
> ~~~ > > 2. > + <para> > + Replace the sequence list in the publication's EXCEPT clause: > +<programlisting> > +ALTER PUBLICATION mypublication SET ALL SEQUENCES EXCEPT (SEQUENCE seq1, > seq2); > +</programlisting></para> > + > + <para> > + Reset the publication to be a ALL SEQUENCES publication with no excluded > + sequences: > +<programlisting> > +ALTER PUBLICATION mypublication SET ALL SEQUENCES; > +</programlisting></para> > > 2a. > Too wordy. > > /in the publication's EXCEPT clause/in the EXCEPT clause/ > In the existing documentation we already have similar documentation: <para> Replace the table list in the publication's EXCEPT clause: <programlisting> ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users, departments); </programlisting></para> I think we should keep the wording consistent. Thoughts? > ~ > > 2b. > The 1st example comment saying "Replace the..." doesn't really make > sense because reading from top-to-bottom, this was not even publishing > sequences. > > So, I think the "ALTER PUBLICATION mypublication SET ALL SEQUENCES > EXCEPT (SEQUENCE seq1, seq2);" example should come *after* the "ALTER > PUBLICATION mypublication SET ALL SEQUENCES;" example. > Fixed > ~~~ > > 3. > + <para> > + Replace the table and sequence list in the publication's EXCEPT clause: > +<programlisting> > +ALTER PUBLICATION mypublication SET ALL TABLES EXCEPT (TABLE users, > departments), ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2); > </programlisting></para> > > Too wordy. Should be plural. > > /in the publication's EXCEPT clause:/in the EXCEPT clauses:/ > I have kept the wording the same. Reason same as 2(a). I have made it plural. > ====== > src/backend/catalog/pg_publication.c > > GetIncludedPublicationRelations: > > On Wed, May 27, 2026 at 11:03 PM Shlok Kyal <[email protected]> wrote: > > > > On Wed, 27 May 2026 at 09:08, Peter Smith <[email protected]> wrote: > > > > > > GetIncludedPublicationRelations: > > > > > > 4. > > > /* > > > * Gets list of relation oids that are associated with a publication. > > > * > > > * This should only be used FOR TABLE publications, the FOR ALL > > > TABLES/SEQUENCES > > > * should use GetAllPublicationRelations(). > > > */ > > > List * > > > GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt) > > > { > > > Assert(!GetPublication(pubid)->alltables); > > > > > > return get_publication_relations(pubid, pub_partopt, false, > > > RELKIND_RELATION); > > > } > > > > > > The comment does not match the code/assert. Shouldn't it also do > > > assert !allsequences? > > > > > This function can be called for ALL SEQUENCES publication. For example > > 'pg_get_publication_tables', 'InvalidatePubRelSyncCache', etc can call > > this function for all sequence publications, but in this case the list > > returned is empty. So, there is no overall impact. > > So, we should not add an 'assert !allsequences'. > > Hmm. The reply seems contrary to the function comment that says "This > should only be used FOR TABLE publications", so if not going to add an > Assert then doesn't the function comment need fixing? > Yes, the function comment is not correct and a patch was already proposed for the same in [1]. But it is a stale state there. Since this patch is related to ALL SEQUENCES and EXCEPT, I think we can update it here. I have updated the comment. > ====== > src/backend/commands/publicationcmds.c > > get_delete_rels: > > 5. > /* > - * Add or remove table to/from publication. > + * Recreate list of tables/sequences to be dropped from the publication. > + * To recreate the relation list for the publication, look for existing > + * relations that do not need to be dropped. > + * > + * 'rels' contains the given list of relations, and 'oldrelids' contains > + * the OIDs of existing relations in the publication identified by 'pubid'. > + */ > > Why say "recreate" everywhere? Can it just say "Returns the list of > ...." instead of "Recreate ...". > > Also, I think this function comment needs to explain in more detail > that this is called during ALTER SET. IIUC, the 'rels' is the reids > you want to end up with in the publication; so those need to be > compared with the 'oldrelids' to find which old ones are not wanted > anymore. Those unwanted ones are what the function returns. > I have reworded the comment > ~~~ > > AlterPublicationRelations: > > 6. > /* > * Nothing to do if no objects, except in SET: for that it is quite > - * possible that user has not specified any tables in which case we need > - * to remove all the existing tables. > + * possible that user has not specified any tables or sequences in which > + * case we need to remove all the existing tables and sequences. > */ > > Maybe this can be reworded more simply: > > SUGGESTION > /* > * Nothing to do if no objects were specified, unless this is a SET > * command, which may need to remove all existing tables and sequences. > */ Fixed [1]:https://www.postgresql.org/message-id/CANhcyEUkV-T6cK142w9wfME9nobFHOvn1f4itJLMG-oR4QoPbQ%40mail.gmail.com Thanks, Shlok Kyal
v9-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLICA.patch
Description: Binary data
v9-0001-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLIC.patch
Description: Binary data
