Hi Nisha.
Here are my review comments for all the rest of the v20* patches
//////////
Patch v20-0002.
//////////
======
Commit message.
1.
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 does more than that though -- it also checks there are no clashes
of the same schema with/without EXCEPT.
======
src/backend/commands/publicationcmds.c
ProcessSchemaExceptTables:
2.
+ * Also rejects a repeated mention of the same schema in the same TABLES IN
+ * SCHEMA list if either mention specifies a non-empty EXCEPT list, without
+ * checking whether the two EXCEPT lists 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):
+ * two mentions of the same schema wouldn't have a well-defined single
+ * EXCEPT list to enforce, so it's rejected as conflicting or redundant
+ * rather than silently unioned or compared for equivalence.
+ * *schemas_with_except accumulates the OIDs of schemas already seen with a
+ * non-empty EXCEPT list in this statement.
Some rewording might be needed. The "if either" and "the two" make it
sound like there are only 2 members in the schema list, but there
might be many schemas:
e.g. FOR TABLES IN SCHEMA S1,S1,S1,S1,S1,S1,S1,S1,S1,S1;
~~~
3.
Something about this logic seems over-complicated to me.
e.g. All this stuff about `schema_repeated`?
AFAICT, you only need to be accumulating 2 lists
- list1 = schemas without EXCEPT
- list2 = schemas with EXCEPT
To check conflicting the logic is simple:
- before adding to list1 ensure same schema is not already in list2, else error
- before adding to list2 ensure same schema is not already in list1, else error
Those checks can be done before you verify the qualified schema names
match their schema...
======
src/backend/parser/gram.y
4.
/*
* Process pubobjspec_list to check for errors in any of the objects and
* convert PUBLICATIONOBJ_CONTINUATION into appropriate PublicationObjSpecType.
- * Also flattens except_tables from TABLES IN SCHEMA nodes into the list so
- * that ObjectsInPublicationToOids() sees them as top-level
EXCEPT_TABLE entries.
+ * The except_tables attached to TABLES IN SCHEMA nodes are left in place here;
+ * ObjectsInPublicationToOids() qualifies their names, validates schema
+ * membership, and merges the qualified tables into its except_pubtables
+ * output list once the schema OID is known.
*/
Maybe add a blank line before the new text here.
//////////
Patch v20-0003.
//////////
======
src/test/regress/sql/publication.sql
1.
+-- fail: partition still rejected from EXCEPT even when its own root is also
+-- named in the same EXCEPT list, via ALTER ... ADD
+ALTER PUBLICATION testpub_alter_except
+ ADD TABLES IN SCHEMA pub_test EXCEPT (TABLE
pub_test.testpub_part_s, TABLE pub_test.testpub_parted_s);
+
Not sure how this test case differs from a plain-ol'
cannot-specify-a-partition-in-EXCEPT test case.
//////////
Patch v20-0004
//////////
======
src/backend/commands/publicationcmds.c
AlterPublicationSchemaExceptTables:
1.
+ /*
+ * Validate that no excluded table is also already explicitly
+ * published (exact match), and that no already-published partition
+ * has its root among the tables being newly excluded here.
+ */
The comment seems to be over-explaining the logic of the function it is calling.
How about something simpler.
SUGGESTION
Check that there is no clash between what tables are included and excluded.
~~~
PublicationDropTables:
2.
+ if (is_except)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("cannot drop table \"%s\" from publication \"%s\"",
+ RelationGetQualifiedRelationName(rel),
+ get_publication_name(pubid, false)),
+ errdetail("The table is currently listed in the EXCEPT clause of the
publication."),
+ errhint("Use ALTER PUBLICATION ... SET TABLES IN SCHEMA ... EXCEPT
to drop the table from the EXCEPT list.")));
+
This new errhint makes more sense, but I felt it's more natural to say
you are "removing" something from a list, not "dropping" something
from a list.
So:
SUGGESTION #1
Use ALTER PUBLICATION ... SET TABLES IN SCHEMA ... EXCEPT to remove a
table from the EXCEPT list.
or, more generically:
SUGGESTION #2
Use ALTER PUBLICATION ... SET TABLES IN SCHEMA ... EXCEPT to modify
the EXCEPT list.
//////////
Patch v20-0005 (docs)
//////////
======
doc/src/sgml/ref/create_publication.sgml
<para>
For partitioned tables, only the root partitioned table may be specified
- in <literal>EXCEPT</literal>. Doing so excludes the root table and
- all of its partitions from replication. The optional
+ in <literal>EXCEPT</literal>. Excluding the root table automatically
+ excludes all of its partitions from replication, including those in
+ schemas that are otherwise included in the publication. The optional
<literal>ONLY</literal> and <literal>*</literal> has no effect for
partitioned tables.
</para>
In hindsight, I thought it would seem weird to have the word
"including" in the sentence where you are trying to describe excluded
tables.
/including those/even those/
======
Kind Regards,
Peter Smith.
Fujitsu Australia
