Re: Support EXCEPT for TABLES IN SCHEMA publications

Hi Nisha.

Some review comments for v18.

//////////
Patch v18-0001
//////////

src/backend/catalog/pg_publication.c

1.
+ /*
+ * A partition cannot be independently included when any of its partition
+ * ancestors 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. Individual partitions cannot appear in EXCEPT
+ * (rejected above), so this check only applies to non-EXCEPT adds.
+ */
+ if (!pri->except && targetrel->rd_rel->relispartition)

It might be tidier to deal with partitions in one place.

SUGGESTION
if (targetrel->rd_rel->relispartition)
{
if (pri->except)
{
/* If in EXCEPT clause, must be root partitioned table */
...
} else {
/* If not in EXCEPT clause, check ancestors are not excluded */
...
}
}

~~~

2.
+ errmsg("cannot add table \"%s\" to publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ get_publication_name(pubid, false)),
+ errdetail("Partition ancestor \"%s\" of table \"%s\" is currently
listed in the EXCEPT clause of the publication.",

"Partition ancestor .. of table" sounded odd.

How about:
errmsg: cannot add partition \"%s\" to ...
errdetail: Ancestor \"%s\" of partition \"%s\" is currently ...

~~~

publication_add_relation:

3.
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("cannot add table \"%s\" to publication \"%s\"",
+ RelationGetQualifiedRelationName(targetrel),
+ pub->name),
+ errdetail("Table \"%s\" is currently listed in the EXCEPT clause of
the publication.",

The table was already named in the errmsg. It seemed redundant to name
it again in the errdetail.

SUGGESTION
errdetail("The table is currently listed in the EXCEPT clause of the
publication.")

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

4.
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("table \"%s\" cannot appear in both the table list and the
EXCEPT clause",
+ RelationGetQualifiedRelationName(pri->relation)));

Do we need to name the clauses in this errmsg? Users can see what they typed.

SUGGESTION
errmsg("table \"%s\" cannot be both published and excluded")

//////////
Patch v18-0003
//////////

src/backend/commands/publicationcmds.c

PublicationDropTables:

1.
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("cannot drop table \"%s\" from publication \"%s\"",
+ RelationGetQualifiedRelationName(rel),
+ get_publication_name(pubid, false)),
+ errdetail("Table \"%s\" is currently listed in the EXCEPT clause of
the publication.",
+ RelationGetQualifiedRelationName(rel)),
+ errhint("Change EXCEPT list using ALTER PUBLICATION ... SET TABLES
IN SCHEMA ... EXCEPT.")));

1a.
(Same as above review comment for patch 0001).

The table was already named in the errmsg. It seemed redundant to name
it again in the errdetail.

SUGGESTION
errdetail("The table is currently listed in the EXCEPT clause of the
publication.")

~

1b.
Why hint to change the except list -- who says the EXCEPT list is wrong?

It seemed more like a user error to me if they are trying to drop
something from a publication when it is already excluded. Why does
this need any hint at all?

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

2.
I did not recognise the distinction in the test comments between
"fail:" and "error:".

~~~

3.
+-- error: ALTER PUBLICATION ... DROP TABLE on an excluded table is rejected
+-- with an EXCEPT-specific error; an EXCEPT entry is not a member.

I don't think you need to say "an EXCEPT entry is not a member." --
what does that mean, other than to redundantly say an excluded table
is a table that is excluded?

//////////
Patch v18-0004
//////////

doc/src/sgml/ref/create_publication.sgml

1.
<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
+ all of its partitions from replication, even any partition that lives
+ in a schema which is itself included in the publication.

BEFORE
For partitioned tables, only the root partitioned table may be
specified in EXCEPT. Doing so excludes the root table and all of its
partitions from replication, even any partition that lives in a schema
which is itself included in the publication.

SUGGESTION #1
For partitioned tables, only the root partitioned table may be
specified in EXCEPT. Excluding the root table automatically excludes
all of its partitions from replication, including those in schemas
that are otherwise included in the publication.

SUGGESTION #2 (TI doubt you even needed to mention about other schemas)
For partitioned tables, only the root partitioned table may be
specified in EXCEPT. Excluding the root table automatically excludes
all of its partitions from replication, including those that would
otherwise be included in the publication.

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