On Thu, Jul 16, 2026 at 2:04 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Nisha.
>
> Some review comments for v21-0001.
>
Thanks for the review. I've applied all the suggestions in v22.
> ~~~
>
> 7.
> + errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed
> in the EXCEPT clause of the publication.",
> + quote_qualified_identifier(get_namespace_name(get_rel_namespace(root)),
>
> Similar (singular) comment to before.
>
> /in the EXCEPT clause/in an EXCEPT clause/
>
It looks like comment #4 is repeated here. In any case, I addressed
both occurrences of the errdetail (in check_publication_add_relation()
and CheckExceptNotInTableList()):
"Ancestor \"%s\" of partition \"%s\" is curre..".
> ~~~
> 9.
> + /* filter out any tables that appear in the EXCEPT list */
> + ListCell *rlc;
> +
> + foreach(rlc, schemaRels)
> + {
> + Oid relid = lfirst_oid(rlc);
>
> Isn't this loop something that can be simplified using foreach_oid macro?
>
Yes, that's possible. I've simplified it.
> ~~~
>
> 10.
> + * A partition whose root is in the publication's EXCEPT list
> + * is also excluded, even if the partition itself lives in a
> + * different (included) schema. Only the topmost root of a
>
> SUGGESTION #1
> A partition whose root is in an EXCEPT list of the publication is also
> excluded, even if...
>
> SUGGESTION #2
> A partition whose root is excluded from the publication is also
> excluded, even if...
>
Changed as per SUGGESTION #2.
> ======
> src/include/catalog/pg_publication.h
>
> 13.
> +extern bool get_publication_rel_entry(Oid pubid, Oid relid, bool *is_except);
>
> Do you think this function name should be CamelCase (more common for
> non-static functions), instead of snake_case?
>
I kept it consistent with the nearby functions, but I agree, it should
use CamelCase for a non-static function.
~~~
Attached is the v22 patch set. Also incorporated all the other
suggestions and feedback from Shveta and Peter at [1], [2], [3], and
[4].
[1] https://www.postgresql.org/message-id/CAJpy0uCj8NdSuWSWkn8iGriz1D1i1g-rMLs9Fk8SEf8uWs60ww%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPsqq%3DNT%3DbhuKtqTU%2B0esbk2cxhxPYp3jjn3KTR%2BL4wc9w%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAHut%2BPuA8YUdqMLS9QjU991bb6N1i5Eo9gq5wUzA7WJbG4FB3g%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CAHut%2BPvCeJ6yDdRu3Qo7-M5fMzZnV1aEDPckW3oLmgY4xPw3HA%40mail.gmail.com
--
Thanks,
Nisha
