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. ~~~ 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:/ ====== 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? ====== 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. */ ====== Kind Regards, Peter Smith. Fujitsu Australia
