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.
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.
