On Tue, Mar 3, 2026 at 10:07 AM Amit Kapila <[email protected]> wrote: > > On Mon, Mar 2, 2026 at 6:51 PM Shlok Kyal <[email protected]> wrote: > > > > Attached the updated v54 patch. > > > > The patch is getting in shape. I have a few minor comments: > > 1. > + if (!first) > + appendStringInfoString(&pubnames, _(", ")); > + > + first = false; > + > + appendStringInfo(&pubnames, _("\"%s\""), pubname); > > Can we have a comment here like "/* translator: This is a separator in > a list of publication names. */" similar to logicalrep_get_attrs_str. > > 2. > + List *ancestor_puboids = GetRelationIncludedPublications(ancestor); > > - puboids = list_concat_unique_oid(puboids, > - GetRelationPublications(ancestor)); > + puboids = list_concat_unique_oid(puboids, ancestor_puboids); > > Why do you choose to take a variable name instead of changing just a > function name? > > 3. > src/test/subscription/t/037_rep_changes_except_table.pl > > How about naming it as 037_except.pl? We would like to add future > except syntax variations in the same test file, so naming just except > would be better. >
+1 > 4. > + * EXCEPT TABLES is processed here and output directly by > + * dumpPublication(). This differs from the approach used in > + * dumpPublicationTable() and dumpPublicationNamespace(), since that > + * approach would require EXCEPT TABLE support for ALTER PUBLICATION, > + * which is not currently supported. > > Is this comment accurate as per our current implementation of Alter > Publication in 0002 patch? If not, let's change these comments. > > 5. > + * Only the top-most ancestor may appear in the EXCEPT > + * clause. Therefore, for a partition, exclusion must be > + * evaluated at the top-most ancestor. > > /may/can > > -- > With Regards, > Amit Kapila. The patch looks good. I have a few comments already shared by Amit. Apart from that, one trivial comment + * relation with EXCEPT clause; otherwise, returns the list of publications with --> in thanks Shveta
