Re: Support EXCEPT for TABLES IN SCHEMA publications

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