On Thu, 7 May 2026 at 10:35, Shlok Kyal <[email protected]> wrote: > > On Tue, 14 Apr 2026 at 18:12, Shlok Kyal <[email protected]> wrote: > > > > On Tue, 14 Apr 2026 at 15:12, shveta malik <[email protected]> wrote: > > > > > > On Mon, Apr 13, 2026 at 10:48 AM shveta malik <[email protected]> > > > wrote: > > > > > > > > On Sun, Apr 12, 2026 at 12:33 AM Shlok Kyal <[email protected]> > > > > wrote: > > > > > > > > > > Hi Vignesh and Shveta, > > > > > > > > > > Thanks for reviewing the patches. > > > > > I have addressed the comments and attached the updated patch. > > > > > > > > > > This also addressed the comments shared by Shveta in [1]. > > > > > [1]: > > > > > https://www.postgresql.org/message-id/CAJpy0uDU0PrH=gvFZjpphOX7t=2jH5wTqYry=C22vnuJJ9Q5=g...@mail.gmail.com > > > > > > > > > > > > > Please find few comments on 001 and 002: > > > > > > > > > > > > v-001: > > > > 1) > > > > - List *except_tables; /* tables specified in the EXCEPT clause */ > > > > + List *except_relations; /* relations specified in the EXCEPT > > > > + * clause */ > > > > Since we have not changed the comments for anything else in patch001, > > > > we can keep this comment too same as old and changeit in 002. > > > > > > > > > > > > v-002: > > > > 2) > > > > pg_publication_rel's prrelid doc says: > > > > Reference to table > > > > We shall change it now to 'Reference to table or sequence' > > > > > > > > 3) > > > > In doc, do we eed to change pg_publication_rel's prqual too? IMO, it > > > > is not applicable to sequence and thus we can change 'relation' to > > > > 'table' in explanation.. > > > > > > > > 4) > > > > Marks the publication as one that synchronizes changes for all > > > > sequences > > > > - in the database, including sequences created in the future. > > > > + in the database, including sequences created in the future. > > > > Sequences > > > > + listed in <literal>EXCEPT</literal> clause are excluded from the > > > > + publication. > > > > > > > > I think we should place it the end of second paragraph rather than at > > > > the end of first. How about something liek this: > > > > > > > > Marks the publication as one that synchronizes changes for all > > > > sequences in the database, including sequences created in the future. > > > > > > > > Only persistent sequences are included in the publication. Temporary > > > > sequences and unlogged sequences are excluded from the publication. > > > > Sequences listed in EXCEPT clause are also excluded from the > > > > publication. > > > > > > > > 5) > > > > + In such a case, a table or partition or sequence that is > > > > included in one > > > > + publication but excluded (explicitly or implicitly) by the > > > > + <literal>EXCEPT</literal> clause of another is considered > > > > included for > > > > + replication. > > > > > > > > 'a table or partition or sequence' can be changed to 'a table, > > > > partition, or sequence' > > > > > > > > 6) > > > > In existign doc, shall we give example of publication creation for > > > > both tables and sequences, each having its except list? This is > > > > important to show that EXCEPT to be given with individual ALL OBJ. We > > > > can cahnge last example of doc file to make this. This one: > > > > 'Create a publication that publishes all sequences for > > > > synchronization, and all changes in all tables except users and > > > > departments:' > > > > > > > > 7) > > > > getPublications: > > > > + * Get the list of tables/sequences for publications specified in the > > > > + * EXCEPT clause. > > > > > > > > We can have both tables and sequences in single publication. We should > > > > change > > > > > > > > 'tables/sequences' --> tables and sequences > > > > > > > > 8) > > > > In describePublications(), > > > > > > > > We had: > > > > if (!puballtables) > > > > else > > > > * Get tables in the EXCEPT clause for this publication */ > > > > > > > > now we have added: > > > > if (puballsequences) > > > > /* Get sequences in the EXCEPT clause for this publication */ > > > > > > > > Since now we can hit this function for 'all-seq' pub too, shall we > > > > change if-block's condition to: > > > > > > > > if (!puballtables && !puballsequences) > > > > > > > > and else-block to > > > > > > > > else if (puballtables) > > > > > > > > otherwise all-seq case will unnecessary enter these blocks and will > > > > exceute the logic > > > > > > > > Please review other functions too in pg_dump to see if we need such > > > > conditions altering. > > > > > > > > > > > > 9) > > > > +# Check the initial data on subscriber > > > > +$result = $node_subscriber->safe_psql('postgres', > > > > + "SELECT last_value, is_called FROM seq1"); > > > > +is($result, '200|t', 'sequences in EXCEPT list is excluded'); > > > > + > > > > +$result = $node_subscriber->safe_psql('postgres', > > > > + "SELECT last_value, is_called FROM seq2"); > > > > +is($result, '200|t', 'initial test data replicated for seq2'); > > > > > > > > Since both are replicated now because of conflicting EXCEPT in > > > > multi-pub, shall we change > > > > comment in 'is(..)'? > > > > > > > > > > > > > For v-003, one trivial thing: > > > > > > Shall we change the name of AlterPublicationTables() as well? It now > > > deals in both tables and sequences. > > > > > Thanks for reviewing the patch. I agree that we should change the name > > here. Modified the patch. > > > > I have also addressed the remaining comments by you and Vignesh in [1], > > [2], [3] > > Attached the updated version. > > > > [1]: > > https://www.postgresql.org/message-id/caldanm2pgi3fakn+x+10nqfknhoudmwegqt_ttjlbvz04f3...@mail.gmail.com > > [2]: > > https://www.postgresql.org/message-id/cajpy0ubb4n8korhchdgprvi2ws1+gtcer+bc2a_ziahoczc...@mail.gmail.com > > [3]: > > https://www.postgresql.org/message-id/caldanm1brq1s9na_gwlwn3byer9be+4qnn4v8sdjimuvao2...@mail.gmail.com > > I noticed that CFbot is failling. > The test case in this patch needs to be updated due to changes > introduced by the recent commit 2e1d4fd. > I have made the change and attached the updated patch. I have also > made some cosmetic changes. > The patch needed a rebase after commit a49b9cf. Attached the rebased patches.
Thanks, Shlok Kyal
v5-0003-Support-EXCEPT-for-ALL-SEQUENCES-in-ALTER-PUBLICA.patch
Description: Binary data
v5-0001-Rename-identifiers-to-use-generic-relation-orient.patch
Description: Binary data
v5-0002-Support-EXCEPT-for-ALL-SEQUENCES-in-CREATE-PUBLIC.patch
Description: Binary data
