Some review comments for patch v27-0001.
(Have not finished looking at the other patches yet)
======
Commit Message
1.
What are the implemented rules for parent/child (inherited) tables?
Shouldn't the commit message have something to say about those?
Perhaps it is covered by "A table cannot be both explicitly published
and excluded in the same statement", but even so I still think the
rule for inherited tables should be mentioned for clarity.
======
src/backend/catalog/pg_publication.c
publication_add_relation:
2.
- if (if_not_exists)
+ /*
+ * if_not_exists asks us to skip an entry that is already present. An
+ * existing entry of the opposite kind is not the entry the caller
+ * asked for, though: skipping it would silently leave the relation
+ * excluded when the caller wanted it published, or published when the
+ * caller wanted it excluded. So only skip when the existing entry is
+ * of the same kind, and report the conflict otherwise.
+ */
+ if (if_not_exists && is_except == pri->except)
return InvalidObjectAddress;
The parameter `if_not_exists` seems like a poor name. There is no verb
to say *what* to do if not exists, so it only makes sense if read in
context of the function name (e.g. "add relation" "if not exists").
You really should not need that first comment sentence to explain the
meaning of the parameter because if it had a good name it would be
already obvious. IMO rename this parameter to something like
'skip_if_already_exists'
~~~
3.
+ if (is_except)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot add table \"%s\" to publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ pub->name),
+ errdetail("The table is named in the publication's EXCEPT clause for
schema \"%s\".",
+ get_namespace_name(RelationGetNamespace(targetrel))),
+ errhint("Change the EXCEPT clause using ALTER PUBLICATION ... SET
TABLES IN SCHEMA ... EXCEPT.")));
+ else if (pri->except)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot exclude table \"%s\" from publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ pub->name),
+ errdetail("The table is a member of the publication."),
+ errhint("Remove the table from the publication using ALTER
PUBLICATION ... DROP TABLE.")));
I feel these messages should better convey that the problem is really
due to an "inconsistent" specification. To do this, you can use the
same common errmsg that is already used elsewhere in this patch.
SUGGESTION
errmsg("table \"%s\" cannot be both published and excluded",
errdetail "The table is already in the publication EXCEPT clause for
schema \"%s\"."
and
SUGGESTION
errmsg("table \"%s\" cannot be both published and excluded",
errdetail "The table is already in the publication"
(I am not sure; should this 2nd errdetail mention it is in the
publication via a FOR TABLE clause?)
~~~
GetPublicationRelationsOfAnyKind:
4.
+/*
+ * Gets list of relation oids associated with a publication, covering both
+ * explicitly included relations and relations named in an EXCEPT clause.
+ *
+ * This is used by ALTER PUBLICATION ... SET to replaces a publication's entire
+ * relation list.
+ */
+List *
+GetPublicationRelationsOfAnyKind(Oid pubid, PublicationPartOpt pub_partopt)
+{
+ List *result;
+
+ result = get_publication_relations(pubid, pub_partopt, false);
+
+ return list_concat_unique_oid(result,
+ get_publication_relations(pubid, pub_partopt,
+ true));
+}
+
4a.
/replaces/replace/
~
4b
Calling this same get_publication_relations multiple times seemed
overkill. Won't it be better to change the signature of
get_publication_relations like:
static List *
get_publication_relations(Oid pubid, PublicationPartOpt pub_partopt,
bool want_included,
bool want_excluded);
then
a) it would be more efficient; e.g. 1 scan instead of 2
b) then GetPublicationRelationsOfAnyKind not needed; just call
get_publication_relations(..., true, true);
======
src/backend/commands/publicationcmds.c
CheckExceptNotInTableList:
5.
+ /*
+ * An EXCEPT clause also applies to inheritance children of a parent
+ * named without ONLY. Children in other schemas are not added to
+ * except_rels, but are still excluded by the EXCEPT clause. Explicitly
+ * publishing such a child therefore conflicts with the EXCEPT clause.
+ */
+ if (list_member_oid(cross_schema_children, explicitrelid))
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s\" cannot be both published and excluded",
+
quote_qualified_identifier(get_namespace_name(get_rel_namespace(explicitrelid)),
+ get_rel_name(explicitrelid))),
+ errdetail("It inherits from a table named in an EXCEPT clause of
this statement."),
+ errhint("Use ONLY in the EXCEPT clause to exclude just the parent,
or do not publish this table explicitly."));
5a.
Here is a reworded comment. See what you think.
- e.g. I didn't like "not added to except_rels" because this function
is not adding that anyhow
SUGGESTION
An EXCEPT clause for a parent table named without ONLY also excludes
its inheritance children. Therefore, explicitly publishing such a
child conflicts with the EXCEPT clause. Note that children in other
schemas are not present in `except_rels`.
~~~
5b.
IMO the erdetail should look more like the other one.
So something like:
errdetail("It inherits from parent table \"%s\" named in the
publication's EXCEPT clause for schema \"%s\".",
Alternatively, if you word it like below, then you can remove the errhint too.
errdetail("It inherits from parent table \"%s\" named without ONLY in
the publication's EXCEPT clause for schema \"%s\".",
~~~
CheckExceptChildNotInSchemaList:
6.
+/*
+ * Check that an EXCEPT clause does not conflict with a TABLES IN SCHEMA
+ * clause in the same statement.
+ *
+ * cross_schema_children contains inheritance children excluded through a
+ * parent in another schema. If such a child is also included through a
+ * TABLES IN SCHEMA clause in the same statement, report the conflict.
+ *
+ * If the child's schema also has an EXCEPT clause specified through TABLES
+ * IN SCHEMA, there is no conflict, since both clauses exclude the child.
+ *
+ * Conflicts with explicitly listed tables are handled by
+ * CheckExceptNotInTableList().
+ */
An example would help to clarify this comment
Something like this?
e.g.
When: s1.parent has s2.child
Error: FOR TABLES IN SCHEMA s1 EXCEPT (TABLE parent), s2;
OK: FOR TABLES IN SCHEMA s1 EXCEPT (TABLE parent), s2 EXCEPT (TABLE child);
~~~
7.
+ if (excluded)
+ continue;
+
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s\" cannot be both published and excluded",
+ quote_qualified_identifier(get_namespace_name(childnsp),
+ get_rel_name(childrelid))),
+ errdetail("It inherits from a table named in the EXCEPT clause of
another schema, and its own schema \"%s\" is published in full.",
+ get_namespace_name(childnsp)),
+ errhint("Use ONLY in the EXCEPT clause to exclude just the parent,
or name this table in the EXCEPT clause of schema \"%s\".",
+ get_namespace_name(childnsp)));
7a.
This is coded strangely. Can't it simply say
if (!excluded)
ereport(ERROR, ...)
~~~
7b.
This errdetail can be reworded similar to the one in an early review
comment #5b in this post.
~~~
CreatePublication:
8.
+ if (except_pubtables != NIL)
+ {
+ List *except_rels;
+ List *cross_schema_children = NIL;
+
+ except_rels = OpenTableList(except_pubtables,
+ &cross_schema_children);
+
+ /*
+ * Validate that a table is not both explicitly included and
+ * excluded by the schema's EXCEPT clause.
+ */
+ CheckExceptNotInTableList(except_rels, cross_schema_children,
+ explicitrelids);
+
+ /*
+ * Validate that an inheritance child is not both excluded by
+ * an EXCEPT clause and included by its schema.
+ */
+ CheckExceptChildNotInSchemaList(cross_schema_children,
+ schemaidlist, except_rels);
+
+ PublicationAddTables(puboid, except_rels, false, NULL);
+ CloseTableList(except_rels);
+ }
8a.
It seems unusual that the functions CheckExceptNotInTableList() and
CheckExceptChildNotInSchemaList() have the same parameters but are in
different order. It might be better if they are in the same order.
~
8b.
In fact, both of these static functions not only have the same
parameters, but they are both only called from one place, which is
here. So is there a reason why these aren't combined in a single
function?
~~~
AlterPublicationTables:
9.
+ /*
+ * SET replaces the publication's whole relation list, so every
+ * existing entry has to be reconciled against the new one -- an
+ * EXCEPT entry included. If the same relation is now requested
+ * with the opposite kind, the old entry is dropped below and the
+ * new one added, rather than the old one being silently kept.
+ */
I'm not sure you need to mention "If the same relation is now..."
here. That part seems more appropriate to say where you actually do
it, rather than saying that it's something that's going to happen
later "below".
~~~
10.
+ * existing relations in the publication. An entry matches
+ * only if it is of the same kind, that is, both are ordinary
+ * members or both are EXCEPT entries. Additionally, if the
"Ordinary members" versus "EXCEPT entries"
Simpler just to say:
SUGGESTION
An entry matches only if it is of the same kind (e.g. the EXCEPT flags
must be the same).
~~~
11.
if (newrelid == oldrelid)
{
- if (equal(oldrelwhereclause, newpubrel->whereClause) &&
+ if (newpubrel->except == oldexcept &&
+ equal(oldrelwhereclause, newpubrel->whereClause) &&
bms_equal(oldcolumns, newcolumns))
{
Should all this be combined to a single "if"?
~~~
OpenTableList:
12.
/*
* Open relations specified by a PublicationTable list.
* The returned tables are locked in ShareUpdateExclusiveLock mode in order to
* add them to a publication.
*/
static List *
-OpenTableList(List *tables)
+OpenTableList(List *tables, List **cross_schema_children)
The function comment describes the meaning of `cross_schema_children`.
~~~
13.
Since there are lots of changes here, you can also change the
foreach() loop to foreach_oid() to make things a little bit neater.
~~~
14.
+ /*
+ * An EXCEPT clause also excludes inheritance children in other
+ * schemas. Keep track of such children separately so the
+ * caller can detect if the same statement also includes them
+ * through another table or schema clause.
+ */
for "another table or schema clause."
SUGGESTION
another FOR TABLE clause or FOR TABLES IN SCHEMA clause.
======
src/include/nodes/parsenodes.h
15.
List *columns; /* List of columns in a publication table */
bool except; /* True if listed in the EXCEPT clause */
+ bool except_in_schema; /* True if listed in the EXCEPT clause of a
+ * TABLES IN SCHEMA clause, whose scope is
+ * limited to that schema */
} PublicationTable;
Now, that ("True if listed in the EXCEPT clause") is ambiguous. e.g.
Is that first flag also true for SCHEMA EXCEPT or not? It would be
better to give these attributes explicit names so there is no doubt.
e.g.
bool all_tables_except;
bool tables_in_schema_except;
(or something else if there are better names)
======
src/test/regress/sql/publication.sql
16.
+-- ALTER PUBLICATION ... SET replaces the publication's whole relation list, so
+-- an existing entry for a relation now requested with the opposite
kind must be
+-- dropped and re-added, not silently left in place.
+CREATE PUBLICATION testpub_except_alter1
+ FOR TABLES IN SCHEMA pub_test EXCEPT (TABLE pub_test.testpub_tbl_s1);
+-- the excluded table becomes an ordinary member: one non-EXCEPT entry, no
+-- EXCEPT entry, and no schema left in the publication
+ALTER PUBLICATION testpub_except_alter1 SET TABLE pub_test.testpub_tbl_s1;
+\dRp+ testpub_except_alter1
+DROP PUBLICATION testpub_except_alter1;
Maybe this can also do "\dRp+ testpub_except_alter1" for the "before"
case so you can more easily see the before/after.
~~~
17.
+-- Cross-schema inheritance children and EXCEPT. An EXCEPT clause also
+-- excludes inheritance children of a parent named without ONLY.
+CREATE TABLE pub_test.testpub_inh_parent (a int);
+CREATE TABLE pub_test.testpub_inh_sibling (b int) INHERITS
(pub_test.testpub_inh_parent);
+CREATE TABLE testpub_inh_child (b int) INHERITS (pub_test.testpub_inh_parent);
+CREATE PUBLICATION testpub_inh
+ FOR TABLES IN SCHEMA pub_test EXCEPT (TABLE pub_test.testpub_inh_parent);
+-- Two EXCEPT entries: the parent and the child in pub_test.
+\dRp+ testpub_inh
+DROP PUBLICATION testpub_inh;
The names were a bit confusing. Maybe just `child1` and `child2` would
be easier than testpub_inh_sibling and testpub_inh_child.
~~~
18.
+-- fail: the child testpub_inh_child is also included through TABLES IN SCHEM
+-- public
/SCHEM/SCHEMA/
~~~
19.
+-- ONLY resolves the conflict: only the parent is excluded, so the child can
+-- be published explicitly
+CREATE PUBLICATION testpub_inh
+ FOR TABLES IN SCHEMA pub_test EXCEPT (TABLE ONLY
pub_test.testpub_inh_parent),
+ TABLE testpub_inh_child;
+\dRp+ testpub_inh
+DROP PUBLICATION testpub_inh;
Maybe there is a missing test combination where you use ONLY to
exclude the parent, and you include the entire public schema of the
testpub_inh_child.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
