On Fri, 7 Aug 2026 at 11:42, Peter Smith <[email protected]> wrote: > > Some review comments for v1. > > ====== > src/backend/catalog/objectaddress.c > > 1. > + { > + "publication exclusion", OBJECT_PUBLICATION_REL > + }, > > I wonder if it's better to call this "publication excluded relation". > > e.g. One day there might be the ability to do "FOR ALL TABLES EXCEPT > (SCHEMA s)", but then "publication exclusion" would not know whether > you are referring to relations or schemas.
Vignesh - Agree on this. There is an exception sequence patch already
in discussion.
> ~~~
>
> get_object_address_publication_rel:
>
> 2.
> - if (!OidIsValid(address.objectId))
> + if (OidIsValid(address.objectId) &&
> + pubrel_is_exclusion == isPublicationRelationExcept(address.objectId,
> + missing_ok))
> {
> - if (!missing_ok)
> + *relp = relation;
> + return address;
> + }
>
> 2a.
> I'm not clear on why we are passing `missing_ok` here. If the
> OidIsValid(address.objectId) is true, then AFAICT the row *must* by
> definition exist in the pg_publication_rel, in which case we should
> never pass `missing_ok` as true.
This comment is not applicable anymore as isPublicationRelationExcept
is no more called from get_object_address_publication_rel
> ~
>
> 2b.
> Actually, I found that the isPublicationRelationExcept call in the
> condition made the logic hard to understand. Can it be expanded out
> and code kept more like the original? Maybe something like below?
>
> SUGGESTION
> if (!OidIsvalid(address.objectId))
> {
> if (!missing_ok)
> {
> if (pub_is_exclusion)
> ereport ...
> else
> ereport
> }
> relation_close(relation, AccessShareLock);
> return address;
> }
> else
> {
> /* Found row in pg_publication_rel */
>
> /* Treat a prexcept mismatch as not found. */
> if (pubrel_is_exclusion !=
> isPublicationRelationExcept(address.objectId, false))
> {
> address.objectId = InvalidOid;
> relation_close(relation, AccessShareLock);
> return address;
> }
> }
>
> *relp = relation;
> return address;
This part of code has been changed to handle another comment of
Shveta, let me know if you feel if it requires any other change.
> ~~~
>
> isPublicationRelationExcept:
>
> 3.
> +/*
> + * Return whether an existing pg_publication_rel entry represents a
> publication
> + * EXCEPT entry.
> + */
> +static bool
> +isPublicationRelationExcept(Oid pubreloid, bool missing_ok)
>
> The `missing_ok` parameter means the function handles both missing and
> present entries, so perhaps you don't need to say "an existing" in
> that function comment.
Rest of the comments were handled.
The attached patches have the changes for the same.
v1-0001-Distinguish-publication-exclusions-in-object-addr.patch is the
same Fujii Masao-san's patch from [2]. The comment fixes are present
in v1-0002-Review-comment-fixes.patch which is a top-up patch on top
of Fujii Masao-san's patch.
@Fujii Masao -san Please merge the changes if you are ok with the changes.
[1] -
https://www.postgresql.org/message-id/CANhcyEVSXyQkvmrsOWPdQqnm2J3GMyQQrKhyCJiBQzqs6AvSow%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CAHGQGwHfESBexa7fq99EvFCf31av=o9h9udnw22ymxmm6lm...@mail.gmail.com
Regards,
Vignesh
v1-0001-Distinguish-publication-exclusions-in-object-addr.patch
Description: Binary data
v1-0002-Review-comment-fixes.patch
Description: Binary data
