Re: 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