On Tue, 17 Mar 2026 at 16:31, shveta malik <[email protected]> wrote: > > On Tue, Mar 17, 2026 at 12:23 PM vignesh C <[email protected]> wrote: > > > > Thanks for the comments, the agreed comments have been addressed in > > the v64 version patch attached. > > > > Please find a few comments: > > > 1) > + The > + <literal>SET ALL TABLES</literal> clause is used to update the > + <literal>EXCEPT TABLE</literal> list of a <literal>FOR ALL > TABLES</literal> > + publication. If <literal>EXCEPT TABLE</literal> is specified with a list > of > + tables, the existing except table list is replaced with the > specified tables. > + If <literal>EXCEPT TABLE</literal> is omitted, the existing except table > + list is cleared. > > How about changing it to (or anything better to reflect new changes): > > The SET ALL TABLES clause can transform an empty publication, or one > defined for ALL SEQUENCES (or both ALL TABLES and ALL SEQUENCES), into > a publication defined for ALL TABLES. Likewise, SET ALL SEQUENCES can > convert an empty publication, or one defined for ALL TABLES (or both > ALL TABLES and ALL SEQUENCES), into a publication defined for ALL > SEQUENCES. > In addition, SET ALL TABLES may be used to update the EXCEPT TABLE > list of a FOR ALL TABLES publication. If EXCEPT TABLE is specified > with a list of tables, the existing exclusion list is replaced with > the specified tables. If EXCEPT TABLE is omitted, the existing > exclusion list is cleared. > > 2) > +bool > +is_include_relation_publication(Oid pubid) > > The name 'is_include_relation_publication' looks slightly odd to me. > Few options are: is_explicit_table_publication, > is_table_list_publication, is_table_publication. Or anything better if > you can think of? > > 3) > is_include_relation_publication: > > + /* If we find even one included relation, we are done */ > + if (!pubrel->prexcept) > + { > + result = true; > + break; > + } > > we can break the loop irrespective of the 'prexcept' flag as we can > never have a combination of mixed prexcept entries for the same pub. > Whether all will be with prexcept=true or all will be false. Even a > loop is not needed. We can fetch the first entry alone (similar to > is_schema_publication) and if that is valid, we can check the flag and > return accordingly. Something like: > > if (HeapTupleIsValid()) > { > pubrel = (Form_pg_publication_rel) GETSTRUCT(tup); > result = !pubrel->prexcept > } > > > 4) > publication_add_relation: > + /* > + * True if EXCEPT tables require explicit relcache invalidation. If > + * 'puballtables' changes, global invalidation covers them. > + */ > + inval_except_table = (stmt != NULL) && > + (stmt->for_all_tables == pub->alltables); > > > It took me some time to figure out why we don't need invalidation for > the case where we are converting ALL SEQ to ALL TABLEs EXCEPT(..). I > think it is worth adding more comments here. Suggestion: > > /* > * Determine whether EXCEPT tables require explicit relcache invalidation. > * > * For CREATE PUBLICATION with EXCEPT tables, invalidation is not needed, > * since it is handled when marking the publication as ALL TABLES. > * > * For ALTER PUBLICATION, invalidation is needed only when adding an EXCEPT > * table to a publication already marked as ALL TABLES. For publications > * that were originally empty or defined as ALL SEQUENCES and are being > * converted to ALL TABLES, invalidation is skipped here, as it is handled > * when marking the publication as ALL TABLES. > */
These comments are addressed in the v65 version patch attached. Also the comments from [1] have been addressed in this. [1] - https://www.postgresql.org/message-id/CAA4eK1%2BmSpCzj%2BB2PW_68DJpXHA0KMgT9Nrz9P83_c1vdKya8g%40mail.gmail.com Regards, Vignesh
v65-0001-Add-support-for-EXCEPT-TABLE-in-ALTER-PUBLICATIO.patch
Description: Binary data
