Re: Distinguish publication exclusions in object addresses

On Tue, Sep 15, 2026 at 7:20 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 15 Sept 2026 at 16:05, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Sep 15, 2026 at 2:01 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > >
> > > The attached v2 version patch has the changes for the same. This also
> > > addresses Hou's comments from [1].
> > >
> >
> > --- a/src/include/nodes/parsenodes.h
> > +++ b/src/include/nodes/parsenodes.h
> > @@ -2416,6 +2416,7 @@ typedef enum ObjectType
> > OBJECT_POLICY,
> > OBJECT_PROCEDURE,
> > OBJECT_PUBLICATION,
> > + OBJECT_PUBLICATION_EXCLUDED_REL,
> >
> > I was trying to evaluate whether the above change needs catversion
> > bump and reached conclusion that it doesn't need one because we never
> > store this enum on-disk as part of parse-trees. Do let me know if you
> > or others thinks differently.
> >
> > *
> > static ObjectAddress
> > get_object_address_publication_rel(List *object,
> > - Relation *relp, bool missing_ok)
> > + Relation *relp, bool missing_ok,
> > + bool pubrel_is_exclusion)
> >
> > It is better to use objtype here instead of boolean as we already use
> > at few other places.
> >
> > *
> > + if (!missing_ok)
> > + {
> > + if (pubrel_is_exclusion)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_UNDEFINED_OBJECT),
> > + errmsg("publication excluded relation \"%s\" from publication \"%s\"
> > does not exist",
> > + RelationGetRelationName(relation), pubname)));
> > + else
> > ereport(ERROR,
> > (errcode(ERRCODE_UNDEFINED_OBJECT),
> > errmsg("publication relation \"%s\" in publication \"%s\" does not exist",
> > RelationGetRelationName(relation), pubname)));
> >
> > I think these messages are misleading because actually here the object
> > type is wrong rather than object doesn't exist.
> >
> > Please find a top-patch for the above suggestions.
>
> Thanks for the suggestion, here is an updated v3 merged version with
> the fixes for the same. This patch also addresses Nisha's comments
> from [1].
>
> [1] - https://www.postgresql.org/message-id/CABdArM5AXL7xN2c7CnUYhUw-kRdMw0WUAxMXMeEjzDV91tVDEw%40mail.gmail.com
>

A few nitpicks; please feel free to skip if you don't agree. Rest of
the patch looks good.

1)
+ if (!missing_ok)
+ elog(ERROR, "cache lookup failed for publication table %u",
+ pubreloid);
+
+ /* fallback to "publication relation" for an undefined object */
+ appendStringInfoString(buffer, "publication relation");

Why do we use "publication table" in the cache lookup failure message
while using "publication relation" as the description below? I think
it would be okay to use "relation" in the error message as well,
especially since the next line refers to it as a "publication
relation".

If "publication table" was chosen with the possibility of supporting
sequences as EXCEPT entries in the future, we can always change it
later. In that case, we would also need to decide whether to change
"publication relation" to "publication table" and "publication
sequence"

2)
get_object_address_publication_rel()

if (!HeapTupleIsValid(tup))
{
if (!missing_ok)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("publication relation \"%s\" in publication \"%s\" does not exist",
RelationGetRelationName(relation), pubname)));
}

Don't we need to change this existing error message based on the
object type as well?

postgres=# SELECT pg_get_object_address('publication excluded relation',
'{public, tab2}', '{pub1}');
ERROR: publication relation "tab2" in publication "pub1" does not exist

Shouldn't it instead say:

ERROR: publication excluded relation "tab2" in publication "pub1"
does not exist

since the requested object type is publication excluded relation?

thanks
Shveta