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
