Re: Support EXCEPT for TABLES IN SCHEMA publications

Some review comments for v20-0001.

======
src/backend/catalog/pg_publication.c

check_publication_add_relation:

1.
+ else
+ {
+ /*
+ * If not in EXCEPT clause, a partition cannot be independently
+ * included when its partition root is in this publication's
+ * EXCEPT list. Doing so would leave the catalog inconsistent
+ * (root excluded, partition explicitly included) and the
+ * ancestor-cascade rule would silently override the include at
+ * replication time.
+ *
+ * Only the root of a partition hierarchy can ever appear in an
+ * EXCEPT clause (a partition itself is rejected just above), so
+ * checking any intermediate ancestor cannot find a match; the
+ * root is the only one worth a catalog lookup.
+ */
+ Oid root = llast_oid(get_partition_ancestors(relid));
+ HeapTuple atup;
+
+ atup = SearchSysCache2(PUBLICATIONRELMAP,
+ ObjectIdGetDatum(root),
+ ObjectIdGetDatum(pubid));
+ if (HeapTupleIsValid(atup))
+ {
+ bool root_except = ((Form_pg_publication_rel) GETSTRUCT(atup))->prexcept;
+
+ ReleaseSysCache(atup);
+
+ if (root_except)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("cannot add partition \"%s\" to publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ get_publication_name(pubid, false)),
+ 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)),
+ get_rel_name(root)),
+ RelationGetQualifiedRelationName(targetrel)),
+ errhint("Change EXCEPT list using ALTER PUBLICATION ... SET TABLES
IN SCHEMA ... EXCEPT.")));
+ }
+ }

This whole part does not really need to be in `else` because previous
block throws ERROR. So it might be simpler code to remove the `else`
keyword and save some indentation.

~~~

2.
+ /*
+ * If not in EXCEPT clause, a partition cannot be independently
+ * included when its partition root is in this publication's
+ * EXCEPT list. Doing so would leave the catalog inconsistent
+ * (root excluded, partition explicitly included) and the
+ * ancestor-cascade rule would silently override the include at
+ * replication time.

This could be worded simpler

SUGGESTION:
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 ...

~~~

3.
+ * Only the root of a partition hierarchy can ever appear in an
+ * EXCEPT clause (a partition itself is rejected just above), so
+ * checking any intermediate ancestor cannot find a match; the
+ * root is the only one worth a catalog lookup.

This could be worded simpler

SUGGESTION
For partition hierachies, only the partition root can be in an EXCEPT
clause (a partition itself is rejected just above), so we only need to
look up the catalog for the root.

~~~

is_table_publication:

4.
"Returns true if the publication has explicitly included relation"

Typos in function comment.

/has explicitly included relation/has an explicitly-included relation/

~~~

5.
+ /*
+ * A publication can hold both explicit-include (prexcept=false) and
+ * EXCEPT (prexcept=true) rows for the same pubid. Scan until we find an
+ * explicit-include row, rather than relying on the first row's prexcept
+ * value.
+ */

2x typos /explicit-include/explicitly-included/

~~~

6.
+ /*
+ * GetTopMostAncestorInPublication() returns InvalidOid both when no
+ * ancestor matched at all, and when the root is excluded via EXCEPT.
+ * Disambiguate here: if the root is excluded, the partition is never
+ * published through this publication, regardless of whether the
+ * partition's own schema is separately included.
+ */

When you say "the partition's own schema", it seems to be viewed
through the lens of FOR TABLES IN SCHEMA, but the partition could also
be explicitly included (e.g. FOR TABLE xxx).

IOW, does "own schema" need to be mentioned, or can that last sentence
say "regardless of whether the partition is separately included".

~~~

7.
+ *
+ * A pg_publication_rel row with prexcept=true means the table is
+ * explicitly excluded via EXCEPT and must not be reported as published,
+ * even if its schema is otherwise included. A row with prexcept=false
+ * means it is explicitly included. If no pg_publication_rel row exists,
+ * the table is published iff its schema appears in
+ * pg_publication_namespace.

Again, this comment seems viewed the the lens of FOR TABLES IN SCHEMA.

That last part, ("If no pg_publication_rel row exists, the table is
published iff its schema appears in pg_publication_namespace."), seems
not quite correct. For example, a table can be published by a FOR ALL
TABLES publication, which has nothing in pg_publication_namespace at
all.

======
src/backend/commands/publicationcmds.c

CheckExceptNotInTableList:

8.
/*
* Check that no relation in except_rels is also present in explicitrelids,
* the list of OIDs added to the publication by the explicit TABLE list. A
* table cannot be both explicitly published and excluded in the same DDL.
*
* Also check that no relation in explicitrelids is a partition whose
* partition root is in except_rels; that would leave the catalog
* inconsistent (root excluded, partition explicitly included) with the
* ancestor-cascade rule silently overriding the include at replication
* time.

The wording like "Check that no relation..." is a bit hard to read.

SUGGESTION
Check that a table is not both excluded and published in the same DDL.

An excluded relation (except_rels) cannot also be present in
explicitrelids, the list of OIDs added to the publication by the
explicit TABLE list.

Also reject any relation in explicitrelids whose partition root is in
except_rels. Otherwise the catalog would be inconsistent (root
excluded, partition explicitly included), and the ancestor-cascade
rule would silently override the include at replication time.

~~~

9.
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("partition \"%s\" cannot be both published and excluded",
+ quote_qualified_identifier(get_namespace_name(get_rel_namespace(explicitrelid)),
+ get_rel_name(explicitrelid))),
+ errdetail("Ancestor \"%s\" is also being excluded by this same command.",
+ RelationGetQualifiedRelationName(pri->relation))));

Hmm. Elsewhere there was an errdetail worded like:
errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed in
the EXCEPT clause of the publication.",

Maybe they should both use that wording.

======
src/backend/replication/pgoutput/pgoutput.c

get_rel_sync_entry:

10.
+ /*
+ * GetTopMostAncestorInPublication() itself skips
+ * schema-based ancestor matches when the partition root
+ * is excluded via this publication's EXCEPT clause.
+ */
ancestor = GetTopMostAncestorInPublication(pub->oid,
ancestors,
&level);

I'm not sure if that comment is helpful here or not. Why is the caller
describing the inner logic of the function it is calling?

~~~

11.
+ Oid root_relid;
+ HeapTuple tup;
+ bool is_except = false;

Given how this is used, maybe `root_is_except` is a better var name.

~~~

12.
+ /*
+ * schemaPubids is relid's own schema, independent of the
+ * ancestor walk above -- a partition can live in a
+ * different schema than its root. Still need to check
+ * for exclusion here, using the top-most ancestor since
+ * only a root (non-partition) table can appear in an
+ * EXCEPT clause.
+ */

I don't know what "schemaPubids is relid's own schema" means. AFAIK
the `schemaPubids` was a list of publications this schemas is a member
of (??).

======
src/test/regress/sql/publication.sql

13.
+-- fail: partition still rejected from EXCEPT even when its own root is also
+-- named in the same EXCEPT list
+CREATE PUBLICATION testpub_except_partition_with_root
+ FOR TABLES IN SCHEMA pub_test EXCEPT (TABLE
pub_test.testpub_part_s, TABLE pub_test.testpub_parted_s);
+

Is this test needed? Isn't the previous test ("only root tables are
allowed") covering this?

Alternatively, consider to put the partition in a different schema
(e.g. see the review comment #15 just below)

~~~

14.
+-- fail: same contradiction (explicit partition + EXCEPT-ed ancestor), but
+-- given in a single statement instead of two separate ones; the ancestor's
+-- EXCEPT row doesn't exist in the catalog yet when the explicit partition is
+-- validated, so this must be caught by comparing the in-memory lists

All that "; the ancestor's EXCEPT ..." is more like some explanation
about internals, and nothing really to do with the test case, so I was
not sure it needs to be in this comment.

~~~

15.
+-- test for EXCEPT clause with schema publication, where the excluded root's
+-- partition lives in a different schema that is separately, fully included
+-- (no EXCEPT) in the same publication. The root's exclusion must cascade to
+-- the partition regardless of the partition's own schema membership.
+CREATE SCHEMA gpt_cross_sch1;
+CREATE SCHEMA gpt_cross_sch2;
+CREATE TABLE gpt_cross_sch1.croot (id int) PARTITION BY RANGE (id);
+CREATE TABLE gpt_cross_sch2.cpart1 PARTITION OF gpt_cross_sch1.croot
FOR VALUES FROM (1) TO (10);
+
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION pub_cross_schema_except FOR TABLES IN SCHEMA
gpt_cross_sch1 EXCEPT (TABLE gpt_cross_sch1.croot), TABLES IN SCHEMA
gpt_cross_sch2;

IIUC, a different way to do this test would be to name the partition explicitly:
CREATE PUBLICATION pub_cross_schema_except FOR TABLES IN SCHEMA
gpt_cross_sch1 EXCEPT (TABLE gpt_cross_sch1.croot), TABLE
gpt_cross_sch2.cpart1;

Is it worth adding this way as well?

======
src/test/subscription/t/037_except.pl

16.
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION cross_sch_pub FOR TABLES IN SCHEMA csch1 EXCEPT
(TABLE csch1.croot), TABLES IN SCHEMA csch2"

Wondering if you should also say "WITH (publish_via_partition_root =
false)"... Even though it is the default maybe it helps to be more
explicitLy, since later you do try to insert directly into partitions
to see the effect.

======
Kind Regards,
Peter Smith.
Fujitsu Australia