Re: Support EXCEPT for TABLES IN SCHEMA publications

Hi Nisha.

Some review comments for v21-0001.

======
src/backend/catalog/pg_publication.c

1.
+bool
+get_publication_rel_entry(Oid pubid, Oid relid, bool *is_except)

This is a bit like the 'is_table_publication' function and others.
Perhaps it should be moved nearer that one?

~~~

check_publication_add_relation:

2.
+ Oid relid = RelationGetRelid(targetrel);
+ Oid root;
+ bool root_except;

Those declarations can all be moved closer to where they are used.

~~~

3.
+ * A partition cannot be included when its partition root is in the
+ * same publication's EXCEPT list. Doing so would leave the catalog
+ * inconsistent (root excluded, partition included) and the
+ * ancestor-cascade rule would silently override the include at
+ * replication time.

Hmm. A single publication may have multiple EXCEPT clauses, but that
first sentence wording seems to be suggesting there is only one.
Perhaps better to say:

SUGGESTION
A partition cannot be included when its partition root is excluded by
the same publication.

~~~

4.
+ errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed
in the EXCEPT clause of the publication.",

For the same reason (a publication may have multiple EXCEPT clauses, I
think this errdetail should change:

/the EXCEPT clause/an EXCEPT clause/

~~~

GetTopMostAncestorInPublication:

5.
+ * If the partition root (the last element of ancestors) is excluded from
+ * this publication via its EXCEPT clause, the partition is not published
+ * through this publication at all, so we short-circuit and return
+ * InvalidOid without walking the ancestor list. Only the root of a

Similar to above. Should not be singular "its EXCEPT clause".

/via its EXCEPT clause/via an EXCEPT clause/

~~~

6.
{
- table_close(rel, RowExclusiveLock);
+ bool is_except;

- if (if_not_exists)
- return InvalidObjectAddress;
-
- ereport(ERROR,
- (errcode(ERRCODE_DUPLICATE_OBJECT),
- errmsg("relation \"%s\" is already member of publication \"%s\"",
- RelationGetRelationName(targetrel), pub->name)));
+ if (get_publication_rel_entry(pubid, relid, &is_except))
+ {
...
+ }
}

I think having a code block like this just for the variable
declaration is unconventional for PG source code; it's not necessary
here, so maybe don't do it.

~~~

7.
+ errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed
in the EXCEPT clause of the publication.",
+ quote_qualified_identifier(get_namespace_name(get_rel_namespace(root)),

Similar (singular) comment to before.

/in the EXCEPT clause/in an EXCEPT clause/

~~~

GetAllSchemaPublicationRelations:

8.
/*
* Gets the list of all relations published by FOR TABLES IN SCHEMA
- * publication.
+ * publication, excluding any tables listed in the EXCEPT clause.
*/

/EXCEPT clause./EXCEPT clauses./

~~~

9.
+ /* filter out any tables that appear in the EXCEPT list */
+ ListCell *rlc;
+
+ foreach(rlc, schemaRels)
+ {
+ Oid relid = lfirst_oid(rlc);

Isn't this loop something that can be simplified using foreach_oid macro?

~~~

10.
+ * A partition whose root is in the publication's EXCEPT list
+ * is also excluded, even if the partition itself lives in a
+ * different (included) schema. Only the topmost root of a

SUGGESTION #1
A partition whose root is in an EXCEPT list of the publication is also
excluded, even if...

SUGGESTION #2
A partition whose root is excluded from the publication is also
excluded, even if...

~~~

is_table_publishable_in_publication:

11.
+ {
+ bool is_except;
+
+ if (get_publication_rel_entry(pub->oid, relid, &is_except))
+ return !is_except;
+ }

Here is another code block which AFAIK is uncommon in PG source code.
IMO would be better to remove this and have a new function variable
`is_except`. You can also remove the other existing `root_except`
declaration because they can bother share this same var. So, the
result will be fewer vars, less unusual code blocks, less code.

======
src/backend/commands/tablecmds.c

12.
+ errhint("%s", has_alltables_pub ?
+ _("Change the publication's EXCEPT clause using ALTER PUBLICATION
... SET ALL TABLES ... EXCEPT.") :
+ _("Change the publication's EXCEPT clause using ALTER PUBLICATION
... SET TABLES IN SCHEMA ... EXCEPT.")));

12a.
The message wording is slightly broken now because there is potential
for even a single publication to have multiple EXCEPT clauses.

Here it might be OK to reword it like:

BEFORE
Change the publication's EXCEPT clause...

SUGGESTION
Change the EXCEPT clause...

~

12b.
Also, IMO the errmsg_plural might be better if reworded:

BEFORE
"cannot attach table \"%s\" as partition because it is referenced in
publication %s EXCEPT clause",
"cannot attach table \"%s\" as partition because it is referenced in
publications %s EXCEPT clause",

SUGGESTION
"cannot attach table \"%s\" as partition because it is referenced in
EXCEPT clause of publication %s",
"cannot attach table \"%s\" as partition because it is referenced in
EXCEPT clause of publications %s",

======
src/include/catalog/pg_publication.h

13.
+extern bool get_publication_rel_entry(Oid pubid, Oid relid, bool *is_except);

Do you think this function name should be CamelCase (more common for
non-static functions), instead of snake_case?

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