Distinguish publication exclusions in object addresses

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.

~~~

~~~

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.

~

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;

~~~

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.

~~~

pg_get_object_address:

4.
+ pubrel_is_exclusion = (strcmp(ttype, "publication exclusion") == 0);

Should the assignment be done later, closer to where it is used?

~~~

getObjectDescription:

5.
+ if (prform->prexcept)
+ {
+ /* translator: first %s is, e.g., "table %s" */
+ appendStringInfo(&buffer, _("exclusion of %s from publication %s"),
+ rel.data, pubname);
+ }
+ else
+ {
+ /* translator: first %s is, e.g., "table %s" */
+ appendStringInfo(&buffer, _("publication of %s in publication %s"),
+ rel.data, pubname);
+ }

I didn't find any test cases that call `pg_describe_object` and output
those "exclusion of ..." and "publication of ..." strings. Maybe there
needs to be some test like below:

------
test_pub=# SELECT
p.pubname,
pr.prrelid,
pg_describe_object('pg_publication_rel'::regclass, pr.oid, 0) AS relation,
pr.prexcept
FROM pg_publication_rel pr
JOIN pg_publication p ON p.oid = pr.prpubid
ORDER BY p.pubname, relation;
pubname | prrelid | relation | prexcept
---------+---------+---------------------------------------------+----------
pub1 | 16400 | exclusion of table t2 from publication pub1 | t
pub3 | 16397 | publication of table t1 in publication pub3 | f
(2 rows)
------

~~~

getObjectTypeDescription:

6.
- appendStringInfoString(&buffer, "publication relation");
+ if (isPublicationRelationExcept(object->objectId, missing_ok))
+ appendStringInfoString(&buffer, "publication exclusion");

Same as before. Should it be "publication excluded relation"?

======
src/test/regress/sql/publication.sql

7.
Should these tests (or some of them) be moved to "object_address.sql"?
There's already a couple of publications defined there, so it seems
reasonable there should be a 3rd publication to do FOR ALL TABLES
EXCEPT

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Aug 6, 2026 at 8:19 PM Fujii Masao <masao.fujii@gmail.com> wrote:

Hi,

I'd like to propose the attached patch, which makes object address output
distinguish publication EXCEPT entries from ordinary publication table
mappings.

pg_publication_rel can now represent either an explicitly published table
or a table excluded from a FOR ALL TABLES publication. However,
the object address code currently treats every pg_publication_rel entry
as a publication relation.

For example,

=# CREATE TABLE t (a int);
=# CREATE PUBLICATION p FOR ALL TABLES EXCEPT (TABLE t);
=# SELECT pi.* FROM pg_publication_rel pp CROSS JOIN LATERAL
pg_identify_object('pg_publication_rel'::regclass, pp.oid, 0) AS pi;
type | schema | name | identity
----------------------+--------+--------+---------------------------
publication relation | (null) | (null) | public.t in publication p

=# SELECT pd.* FROM pg_publication_rel pp CROSS JOIN LATERAL
pg_describe_object('pg_publication_rel'::regclass, pp.oid, 0) AS pd;
pd
-----------------------------------------
publication of table t in publication p

Although the pg_publication_rel entry for t represents an exclusion,
the object address output makes it appear to be an ordinary published-table
mapping. This affects pg_identify_object(), pg_describe_object(),
and pg_identify_object_as_address(), and can also be confusing for tools
that use object addresses, such as audit or DDL deparsing tools.

I agree.

The root cause is that objectaddress.c does not check
pg_publication_rel.prexcept when describing or identifying
pg_publication_rel objects.

The patch fixes this by distinguishing EXCEPT entries. They are now
reported as publication exclusion, with identities such as:

=# SELECT pi.* FROM pg_publication_rel pp CROSS JOIN LATERAL
pg_identify_object('pg_publication_rel'::regclass, pp.oid, 0) AS pi;
type | schema | name | identity
-----------------------+--------+--------+--------------------------------------
publication exclusion | (null) | (null) | public.t excluded from
publication p

=# SELECT pd.* FROM pg_publication_rel pp CROSS JOIN LATERAL
pg_describe_object('pg_publication_rel'::regclass, pp.oid, 0) AS pd;
pd
-----------------------------------------
exclusion of table t from publication p

Thoughts?

Agree with the problem statement and the approach. Please find a few
initial comments, will review in detail next week.

1)
getObjectTypeDescription:

+ if (isPublicationRelationExcept(object->objectId, missing_ok))
+ appendStringInfoString(&buffer, "publication exclusion");
+ else
+ appendStringInfoString(&buffer, "publication relation");

It seems strange initially that when missing_okay is true and say
cache-tuple is missing, we ccnsider it as 'publication relation'. But
then I checked other calls accepting 'missing_ok' in the same function
such as: getProcedureTypeDescription, getConstraintTypeDescription.
They have a fallback option for undefined object. The comment there
makes it clear. Perhaps we should add similar comment here.

2)
It seems that get_object_address_publication_rel() currently performs
two cache lookups to obtain the complete details of the relation.
Ideally, a single lookup would suffice, although that would likely
require restructuring the implementation instead of going through
isPublicationRelationExcept(). That said, I understand that
isPublicationRelationExcept() is still needed in other code paths, so
even though I would prefer a single cache lookup, keeping the current
approach is fine as well.

thanks
Shveta

On Fri, 7 Aug 2026 at 11:42, Peter Smith <smithpb2250@gmail.com> 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]/messages/by-id/CAHGQGwHfESBexa7fq99EvFCf31av=O9h9udnw22ymxmm6LMZzw@mail.gmail.com. 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]: /messages/by-id/CANhcyEVSXyQkvmrsOWPdQqnm2J3GMyQQrKhyCJiBQzqs6AvSow@mail.gmail.com
[2]: /messages/by-id/CAHGQGwHfESBexa7fq99EvFCf31av=O9h9udnw22ymxmm6LMZzw@mail.gmail.com

Regards,
Vignesh

On Fri, 7 Aug 2026 at 16:20, shveta malik <shveta.malik@gmail.com> wrote:

Agree with the problem statement and the approach. Please find a few
initial comments, will review in detail next week.

1)
getObjectTypeDescription:

+ if (isPublicationRelationExcept(object->objectId, missing_ok))
+ appendStringInfoString(&buffer, "publication exclusion");
+ else
+ appendStringInfoString(&buffer, "publication relation");

It seems strange initially that when missing_okay is true and say
cache-tuple is missing, we ccnsider it as 'publication relation'. But
then I checked other calls accepting 'missing_ok' in the same function
such as: getProcedureTypeDescription, getConstraintTypeDescription.
They have a fallback option for undefined object. The comment there
makes it clear. Perhaps we should add similar comment here.

Comment seems sufficient here, added a comment.

2)
It seems that get_object_address_publication_rel() currently performs
two cache lookups to obtain the complete details of the relation.
Ideally, a single lookup would suffice, although that would likely
require restructuring the implementation instead of going through
isPublicationRelationExcept(). That said, I understand that
isPublicationRelationExcept() is still needed in other code paths, so
even though I would prefer a single cache lookup, keeping the current
approach is fine as well.

I preferred the single lookup approach. The
v1-0002-Review-comment-fixes.patch at [1]/messages/by-id/CALDaNm14MGg8cw3WXyDyt-ry2rh4ifVbsNRVUnwJZcBmxVo+Rg@mail.gmail.com has the changes for the
same.
[1]: /messages/by-id/CALDaNm14MGg8cw3WXyDyt-ry2rh4ifVbsNRVUnwJZcBmxVo+Rg@mail.gmail.com

Regards,
Vignesh

Hi,

On Monday, September 14, 2026 12:29 AM vignesh C <vignesh21@gmail.com> wrote:

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.

Thanks for sharing the patches.

I have one question for 0001:

The changes in pg_get_object_address() look a bit hacky to me. Instead of
hard-coding a string comparison and adding a special branch to handle the
excluded publication relation, wouldn't it be more standard to introduce a new
object type, such as OBJECT_PUBLICATION_EXCLUDED_REL? I think that would make
the code more elegant, and we could pass the object type directly to
get_object_address_publication_rel() instead of using a Boolean flag. That said, are
there any reasons we cannot add a new object type?

Best Regards,
Zhijie Hou

On Sun, Sep 13, 2026 at 9:01 PM vignesh C <vignesh21@gmail.com> wrote:

I preferred the single lookup approach. The
v1-0002-Review-comment-fixes.patch at [1] has the changes for the
same.

Thanks Vignesh.

Instead of isPublicationRelationExcept() with the exclusion logic
outside and the fallback logic inside -- which is also not very clear:

+ /* fallback to a non-exclusion entry for an undefined object */
+ return false;

I think we could introduce a function similar to
getProcedureTypeDescription() and getConstraintTypeDescription().
Please see the attached patch. Take the changes if you agree.

thanks
Shveta

On Mon, Sep 14, 2026 at 7:23 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Hi,

On Monday, September 14, 2026 12:29 AM vignesh C <vignesh21@gmail.com> wrote:

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.

Thanks for sharing the patches.

I have one question for 0001:

The changes in pg_get_object_address() look a bit hacky to me. Instead of
hard-coding a string comparison and adding a special branch to handle the
excluded publication relation, wouldn't it be more standard to introduce a new
object type, such as OBJECT_PUBLICATION_EXCLUDED_REL?

+1. pg_get_object_address() copies a part of get_object_address() in
'if' branch while 'else' branch still relies on get_object_address().
If in future concerned part of get_object_address() changes, the
similar change will be needed in 'if-branch', which an easily be
missed.

On Tue, 15 Sept 2026 at 10:07, shveta malik <shveta.malik@gmail.com> wrote:

On Sun, Sep 13, 2026 at 9:01 PM vignesh C <vignesh21@gmail.com> wrote:

I preferred the single lookup approach. The
v1-0002-Review-comment-fixes.patch at [1] has the changes for the
same.

Thanks Vignesh.

Instead of isPublicationRelationExcept() with the exclusion logic
outside and the fallback logic inside -- which is also not very clear:

+ /* fallback to a non-exclusion entry for an undefined object */
+ return false;

I think we could introduce a function similar to
getProcedureTypeDescription() and getConstraintTypeDescription().
Please see the attached patch. Take the changes if you agree.

Your suggestion looks better, I have merged the proposed changes with
a couple of minor changes a) changed function name
getPublicationRelationDescription to
getPublicationRelationTypeDescription. This will keep it consistent by
having Type in the function name similar to
getConstraintTypeDescription, getProcedureTypeDescription and
getRelationTypeDescription. b) Changed "failed for publication
relation" to "failed for publication table" so that it is consistent
with other search sys cache failures of PUBLICATIONREL.

The attached v2 version patch has the changes for the same. This also
addresses Hou's comments from [1]/messages/by-id/TY4PR01MB1771860516F5316ECB8CD275494BB2@TY4PR01MB17718.jpnprd01.prod.outlook.com.

[1]: /messages/by-id/TY4PR01MB1771860516F5316ECB8CD275494BB2@TY4PR01MB17718.jpnprd01.prod.outlook.com

Regards,
Vignesh

On Tue, Sep 15, 2026 at 2:01 PM vignesh C <vignesh21@gmail.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.

--
With Regards,
Amit Kapila.

On Tue, Sep 15, 2026 at 2:01 PM vignesh C <vignesh21@gmail.com> wrote:

The attached v2 version patch has the changes for the same. This also
addresses Hou's comments from [1].

Thanks for the patch. I tested it across object reporting, address
resolution and error paths, event triggers, and dependency reporting,
and did not find any functional issues.

A couple of observations:
1) object_address.sql:105-106
The DO block here runs all accepted object type strings through
pg_get_object_address(). It seems worth adding the new value here as
well:

         ('operator of access method'), ('function of access method'),
-        ('publication namespace'), ('publication relation')
+        ('publication namespace'), ('publication relation'),
+        ('publication excluded relation')
     LOOP

2. aclchl.c, dropcmds.c, event_trigger.c, seclabel.c :

case OBJECT_PUBLICATION_REL:
+ case OBJECT_PUBLICATION_EXCLUDED_REL:

The case labels in these places appear to be in alphabetical order.
Should we keep the new case in the same order as well? Not necessary,
but it would keep the existing ordering consistent.

--
Thanks,
Nisha

On Tue, 15 Sept 2026 at 16:05, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 15, 2026 at 2:01 PM vignesh C <vignesh21@gmail.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]/messages/by-id/CABdArM5AXL7xN2c7CnUYhUw-kRdMw0WUAxMXMeEjzDV91tVDEw@mail.gmail.com.

[1]: /messages/by-id/CABdArM5AXL7xN2c7CnUYhUw-kRdMw0WUAxMXMeEjzDV91tVDEw@mail.gmail.com

Regards,
Vignesh

On Tue, Sep 15, 2026 at 7:20 PM vignesh C <vignesh21@gmail.com> wrote:

On Tue, 15 Sept 2026 at 16:05, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Sep 15, 2026 at 2:01 PM vignesh C <vignesh21@gmail.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] - /messages/by-id/CABdArM5AXL7xN2c7CnUYhUw-kRdMw0WUAxMXMeEjzDV91tVDEw@mail.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

On Tue, Sep 15, 2026 at 7:20 PM vignesh C <vignesh21@gmail.com> wrote:

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].

Thanks for the patch, it LGTM.
I only have one nitpick in object_address.sql: the new line has an
extra leading space.

('publication namespace'), ('publication relation'),
+ ('publication excluded relation')

--
Thanks,
Nisha

On Wed, Sep 16, 2026 at 9:43 AM shveta malik <shveta.malik@gmail.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]tup = SearchSysCache2(PUBLICATIONRELMAP, ObjectIdGetDatum(RelationGetRelid(relation)), ObjectIdGetDatum(pub->oid)); if (!HeapTupleIsValid(tup)), 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))
tup = SearchSysCache2(PUBLICATIONRELMAP,
ObjectIdGetDatum(RelationGetRelid(relation)),
ObjectIdGetDatum(pub->oid));
if (!HeapTupleIsValid(tup))

--
With Regards,
Amit Kapila.

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?

======
src/backend/catalog/aclchk.c

+ case OBJECT_PUBLICATION_EXCLUDED_REL:
case OBJECT_PUBLICATION_NAMESPACE:
case OBJECT_PUBLICATION_REL:

The new enum name OBJECT_PUBLICATION_EXCLUDED_REL is closely related
to OBJECT_PUBLICATION_REL.

IMO, a better name would be OBJECT_PUBLICATION_REL_EXCLUDED, so these
related things are kept adjacent alphabetically and in the code.

(same comment affects multiple other files but not repeating all those
in this post)

======
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.

~~~

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.

~~~

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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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]: errmsg("publication relation \"%s\" in publication \"%s\" does not exist", RelationGetRelationName(relation), pubname)));
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)));
errmsg("publication relation \"%s\" in publication \"%s\" does not exist",
RelationGetRelationName(relation), pubname)));

--
With Regards,
Amit Kapila.

On Wednesday, September 16, 2026 12:51 PM Peter Smith <smithpb2250@gmail.com> wrote:

Some review comments for v3:

======
src/backend/catalog/aclchk.c

+ case OBJECT_PUBLICATION_EXCLUDED_REL:
case OBJECT_PUBLICATION_NAMESPACE:
case OBJECT_PUBLICATION_REL:

The new enum name OBJECT_PUBLICATION_EXCLUDED_REL is closely
related to OBJECT_PUBLICATION_REL.

IMO, a better name would be OBJECT_PUBLICATION_REL_EXCLUDED, so
these related things are kept adjacent alphabetically and in the code.

I think all object type names end with a noun, whereas the proposed name
doesn't, so I don't find it better. Also, the order of an enum value doesn't
provide enough value to justify the change, in my view.

Best Regards,
Zhijie Hou

On Wed, Sep 16, 2026 at 11:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

Should the new Assert be at the start of the function? At the current
position, we have already locked the relation and done some
unnecessary processing by then.

--
Thanks,
Nisha

On Wed, Sep 16, 2026 at 12:00 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Wed, Sep 16, 2026 at 11:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

Should the new Assert be at the start of the function? At the current
position, we have already locked the relation and done some
unnecessary processing by then.

Yes, that will be better IMO.

thanks
Shveta