On Tue, Mar 3, 2026 at 10:07 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Mar 2, 2026 at 6:51 PM Shlok Kyal <shlok.kyal.oss@gmail.com> 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
