On Wed, Sep 16, 2026 at 10:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for v3:
>
> ======
> Commit Message
>
> 1.
> Fix this by distinguishing EXCEPT entries in the object address code.
> They are now reported as publication excluded relation, with object
> identities indicating that the table is excluded from the publication.
>
> ~
>
> Should "publication excluded relation" be quoted here?
>
I have changed the commit message.
>
> ======
> src/backend/catalog/objectaddress.c
>
> 2.
> static ObjectAddress
> -get_object_address_publication_rel(List *object,
> +get_object_address_publication_rel(ObjectType objtype, List *object,
> Relation *relp, bool missing_ok)
>
> If not going to describe parameter `objType` then maybe a
> self-documenting Assert would be good to have here.
>
Added assert in the attached patch and changed the if/else to simplify the code.
> ~~~
>
> 3.
> + /*
> + * The same relation and publication pair identifies either a published or
> + * an excluded relation, so reject an entry of the kind that was not asked
> + * for.
> + */
>
> The wording looks a bit strange. Particularly the 2nd part ("reject an
> entry of the kind that was not asked for")
>
> SUGGESTION:
> A given relation/publication pair can represent either a published
> relation or an excluded one, but not both. Reject the entry if it is
> not the kind the caller asked for.
>
> ~~~
>
I am not sure which one to prefer here. I have kept the proposed one
based on its conciseness.
> 4.
> + if (isexcept)
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("\"%s\" is not a published relation of publication \"%s\"",
> + RelationGetRelationName(relation), pubname)));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("\"%s\" is not an excluded relation of publication \"%s\"",
> + RelationGetRelationName(relation), pubname)));
>
> Publications can have the same table name in multiple schemas. The
> name should be fully-qualified in the errmsg to eliminate any
> ambiguity.
>
But I don't see any ambiguity here as the user can only pass one
relation name. Also, we use the unqualified name in nearby message
[1]. We use RelationGetRelationName all over the code in the error
messages and I don't see the need to qualify here. We will simply
return the name the user has passed.
[1]:
errmsg("publication relation \"%s\" in publication \"%s\" does not exist",
RelationGetRelationName(relation), pubname)));
--
With Regards,
Amit Kapila.
