Re: Support EXCEPT for TABLES IN SCHEMA publications

Some review comments for the v23* patches

//////////
Patch 0001
//////////

src/backend/catalog/pg_publication.c

check_publication_add_relation:

1.
+ if (targetrel->rd_rel->relispartition)
+ {
+ Oid relid = RelationGetRelid(targetrel);
+ Oid root;
+ bool root_except;
+
+ if (pri->except)
+ {
+ /* If in EXCEPT clause, must be root partitioned table */
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg(errormsg, relname),
+ errdetail("This operation is not supported for individual partitions.")));
+ }
+
+ /*
+ * A partition cannot be added to a publication if its partition root
+ * is excluded by the publication. We follow the rule that excluding a
+ * partition root means that the entire partition tree is excluded.
+ * Thus, check whether the root is in the EXCEPT clause and reject
+ * adding the individual partition to the publication.
+ */

The comment says "Thus, check whether the root is in the EXCEPT clause
and reject", but in fact that has already been done in the code before
this comment.

The comment is good, but IMO it belongs atop the outer `if
(targetrel->rd_rel->relispartition)`

~~~

is_table_publication:

2.
/*
- * Returns true if the publication has explicitly included relation (i.e.,
+ * Returns true if the publication has an explicitly-included relation (i.e.,
* not marked as EXCEPT).
*/

In hindsight, the function comment seems a bit odd. It doesn't need to
mention EXCEPT -- those are more like logic details about *how* it
works, nothing to do with the purpose of the function.

SUGGESTION
Returns true if the publication has a FOR TABLE clause.

~

Similarly, the `is_schema_publication` function comment could simply say:
Returns true if the publication has a FOR TABLES IN SCHEMA clause.

~~~

GetAllSchemaPublicationRelations:

3.
+ if (except_relids != NIL)
+ {
+ /* filter out any tables that appear in the EXCEPT clause */

But `except_relids` really is a List so I think the comment saying
"EXCEPT list" was probably fine here.

~~~

is_table_publishable_in_publication:

5.
/*
* Check whether the table is explicitly published via pg_publication_rel
* or pg_publication_namespace.
+ *
+ * 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.
*/
- return (SearchSysCacheExists2(PUBLICATIONRELMAP,
- ObjectIdGetDatum(relid),
- ObjectIdGetDatum(pub->oid)) ||
- SearchSysCacheExists2(PUBLICATIONNAMESPACEMAP,
- ObjectIdGetDatum(get_rel_namespace(relid)),
- ObjectIdGetDatum(pub->oid)));
+ if (CheckPublicationRelEntry(pub->oid, relid, &is_except))
+ return !is_except;
+
+ return SearchSysCacheExists2(PUBLICATIONNAMESPACEMAP,
+ ObjectIdGetDatum(get_rel_namespace(relid)),
+ ObjectIdGetDatum(pub->oid));

I think that the last `if` would be better written as if/else. That's
because your big comment applies to *both* of those returns, but it is
not immediately obvious due to the blank line.

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

AlterPublication:

6.
Code was moved out of patch 0001.

TBH, I thought this could've stayed in patch 0001 -- Although code was
in the AlterPublication function, AFAICT it had nothing really to do
with "alter" patch -- Isn't it more related just to the signature
change of `ObjectsInPublicationToOids` which is elsewhere in here
patch 0001?

//////////
Patch 0002
//////////

No comments.

//////////
Patch 0003
//////////

src/backend/commands/publicationcmds.c

1.
- List *exceptrelations = NIL;
+ List *except_pubtables = NIL;
List *schemaidlist = NIL;
Oid pubid = pubform->oid;

ObjectsInPublicationToOids(stmt->pubobjects, pstate, &relations,
- &exceptrelations, &schemaidlist);
+ &except_pubtables, &schemaidlist);

(same as patch 0001 review comment #6)

Wondering if this change belonged in patch 0001, which is where
ObjectsInPublicationToOids signature was modified.

//////////
Patch 0004
//////////

src/backend/commands/publicationcmds.c

AlterPublicationSchemas:

1.
/*
- * Alter the EXCEPT list of a schema-level publication.
+ * Alter the EXCEPT clause of a schema-level publication.
*
* Adds, removes, or replaces except-table entries in pg_publication_rel
* (rows with prexcept = true). These entries suppress publication of the

Maybe "EXCEPT list" might have been fine terminology for this one --
The "clause" is just a piece of the grammar, whereas this is mostly
about modifying the list of excluded tables within that clause. So no
change needed here?

~~~

AlterPublicationSchemaExceptTables:

2.
+ /* Collect OIDs of the desired new EXCEPT clause. */
+ foreach_ptr(PublicationRelInfo, pri, rels)
+ newexceptrelids = lappend_oid(newexceptrelids,
+ RelationGetRelid(pri->relation));

Perhaps "EXCEPT list" instead of EXCEPT clause was fine for this one
too. e.g. It really is building a list here.

~~~

Publication DropTables:

3.
+ * true - delete them silently. Used by the SET path to clear stale
+ * EXCEPT rows that are being replaced by a new EXCEPT clause.

I can't decide if it always makes sense to say the EXCEPT clause is
being "replaced". It may be getting removed entirely. Perhaps some
rewording helps?

SUGGESTION
Clears stale EXCEPT rows when ALTER PUBLICATION ... SET replaces or
removes the EXCEPT clause.

//////////
Patch 0005
//////////

There's a couple of threads [1][2] which might get pushed soon. Those
are going to mean a rebase will be needed, so watch out for them.

doc/src/sgml/logical-replication.sgml
======
doc/src/sgml/ref/alter_publication.sgml

1.
+ <varlistentry>
+ <term><literal>EXCEPT</literal></term>
+ <listitem>
+ <para>
+ When used with <literal>ADD TABLES IN SCHEMA</literal>
+ or <literal>SET TABLES IN SCHEMA</literal>, specifies
+ tables to be excluded from the publication. Each named
+ table must belong to the schema specified in the same
+ <literal>TABLES IN SCHEMA</literal> clause. Table names may be
+ schema-qualified or unqualified; unqualified names are implicitly
+ qualified with the schema named in the same clause. See
+ <xref linkend="sql-createpublication"/> for further details on the
+ semantics of <literal>EXCEPT</literal>.
+ </para>
+ <para>
+ Dropping a table always removes it from the <literal>EXCEPT</literal>
+ clause.
+ </para>
+ </listitem>
+ </varlistentry>

Curiously, this does not mention FOR ALL TABLES EXCEPT?

I felt there was a missing earlier paragraph "When used with ALL TABLES ..."?

======
[1] https://www.postgresql.org/message-id/flat/CAHGQGwG2VA0iW8S%3DPDn34Ytx07F0TCH0vOP9y8jtPwDO5Tn7Qw%40mail.gmail.com#118ae769d6154efa7b3b13a9f316d2f6
[2] https://www.postgresql.org/message-id/flat/CAHGQGwFJQNmiGX8tsmuCsv1yRWkS_sZkWA%2BZ8Hq6O2efstR3oA%40mail.gmail.com#200b80e3c317f420282c0b34a771b032

Kind Regards,
Peter Smith.
Fujitsu Australia