Re: Distinguish publication exclusions in object addresses

On Wed, Sep 16, 2026 at 9:43 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> 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".
>

We can go either way but there is some precedent as well for using
different names, see:
if (!HeapTupleIsValid(procTup))
{
if (!missing_ok)
elog(ERROR, "cache lookup failed for procedure %u", procid);

/* fallback to "procedure" for an undefined object */
appendStringInfoString(buffer, "routine");

Also, we should change one other existing place than:
case PublicationRelRelationId:
{
HeapTuple tup;
char *pubname;
Form_pg_publication_rel prform;

tup = SearchSysCache1(PUBLICATIONREL,
ObjectIdGetDatum(object->objectId));
if (!HeapTupleIsValid(tup))
{
if (!missing_ok)
elog(ERROR, "cache lookup failed for publication table %u",
object->objectId);

I would like to retain the one used by patch. If you or others still
want to insist for consistency here then we need to change at both
places.

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

Yes, but if we see the code [1], we are just trying to fetch the
publication table from pg_publication_rel, so I am okay with the
current ERROR message. OTOH, I do see the argument of giving more
appropriate error message for the example you shared but I feel it
will add more checks in the code which doesn't sound worth to me.

[1]:
tup = SearchSysCache2(PUBLICATIONRELMAP,
ObjectIdGetDatum(RelationGetRelid(relation)),
ObjectIdGetDatum(pub->oid));
if (!HeapTupleIsValid(tup))

--
With Regards,
Amit Kapila.