Re: Skipping schema changes in publication

On Mon, Mar 2, 2026 at 6:51 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)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.

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.