Some review comments for v21-0002.
======
Commit message.
1.
Add ProcessSchemaExceptTables() to qualify each mention's EXCEPT
entries, build a sorted signature per schema OID, and compare it
against any prior mention of the same schema in the same statement,
erroring on a mismatch. It also checks there are no clashes of the
same schema with or without EXCEPT.
~
IMO it's too confusing to explain the implementation detail here
instead of the purpose. Also, it should say "with *and* without".
SUGGESTION
Add ProcessSchemaExceptTables(), which errors if the same schema is
mentioned more than once with EXCEPT, or both with and without EXCEPT.
It also checks that a schema's EXCEPT clause only lists tables from
that same schema.
======
src/backend/commands/publicationcmds.c
ProcessSchemaExceptTables:
2.
+/*
+ * Qualify unqualified EXCEPT table names with the given schema, reject
+ * entries explicitly qualified with a different schema, and append the (now
+ * qualified) tables to *except_pubtables.
+ *
+ * Also rejects a repeated mention of the same schema in the same TABLES IN
+ * SCHEMA list if it, or an earlier mention of the same schema, specified a
+ * non-empty EXCEPT list, without checking whether the EXCEPT lists across
+ * all the mentions actually name the same tables. This mirrors
+ * OpenTableList()'s handling of repeated tables with column lists (e.g.
+ * "FOR TABLE t1(a), t1(a)" is rejected even though the lists match):
+ * repeated mentions of the same schema wouldn't have a well-defined single
+ * EXCEPT list to enforce, so they're rejected as conflicting or redundant
+ * rather than silently unioned or compared for equivalence. A schema may
+ * still be mentioned multiple times as long as none of the mentions
+ * specify EXCEPT.
+ * *schemas_with_except accumulates the OIDs of schemas already seen with a
+ * non-empty EXCEPT list in this statement.
+ */
2a.
This function comment seems very complex. Here is a simpler version;
AFAICT it says the same:
SUGGESTION
/*
* Qualify unqualified EXCEPT table names with the given schema (rejecting
* any explicitly qualified with a different schema), and append them to
* *except_pubtables.
*
* Also rejects a schema being mentioned more than once with an EXCEPT
* clause, even if the EXCEPT lists are identical — much like
* OpenTableList() rejects "FOR TABLE t1(a), t1(a)" despite the column
* lists matching. A schema can still be mentioned multiple times, just
* not more than once with EXCEPT.
*
* *schemas_with_except tracks which schema OIDs have already been seen
* with an EXCEPT clause in this statement.
*/
~~
2b.
It would also be helpful to explain what the other parameters are.
IIUC, it is something like:
`*except_tables` = "the EXCEPT list (or NIL) for this schema"
`*schemas` = "the list of all schema OIDs seen so far for this statement"
~~~
3.
+ /*
+ * A repeat is only a problem if this mention has EXCEPT, or an earlier
+ * mention of the same schema did.
+ */
/A repeat is only a problem/A repeating schema is only a problem/
~~~
4.
+ /*
+ * A repeat is only a problem if this mention has EXCEPT, or an earlier
+ * mention of the same schema did.
+ */
+ if (list_member_oid(*schemas, schemaid))
+ {
+ if (except_tables != NIL ||
+ list_member_oid(*schemas_with_except, schemaid))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("conflicting or redundant EXCEPT lists for schema \"%s\"",
+ schema_name));
+ }
+
+ if (except_tables != NIL)
+ *schemas_with_except = lappend_oid(*schemas_with_except, schemaid);
+
+ /* Filter out duplicates if the user specifies "sch1, sch1" */
+ *schemas = list_append_unique_oid(*schemas, schemaid);
+
+ if (except_tables == NIL)
+ return;
All the except_tables checks are making this code harder to read than
it needs to be. How about having another variable:
bool schema_has_except = (except_tables != NIL);
======
Kind Regards,
Peter Smith.
Fujitsu Australia
