Re: Support EXCEPT for ALL SEQUENCES publications

Some review comments for v19-0001 (no comments for v19-0002)

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

NOTE: The following review comments are all coming from the same PoV
that actually there can be multiple simultaneous EXCEPT clauses (e.g.
FOR ALL TABLES EXCEPT, and FOR ALL SEQUENCES EXCEPT), so it is not
quite correct to just refer to the singular "the EXCEPT clause" --
should use plural, or remove the ambiguity from the text.

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

GetAllTablesPublications:

1.
- * For a FOR ALL TABLES publication, the returned list excludes
tables mentioned
- * in the EXCEPT clause.
+ * For a FOR ALL TABLES publication, the returned list excludes tables
+ * specified in the EXCEPT clause. For a FOR ALL SEQUENCES publication,
+ * it excludes sequences specified in the EXCEPT clause.

SUGGESTION
For a FOR ALL TABLES publication, the returned list excludes tables
specified in the EXCEPT (TABLE ...) clause. For a FOR ALL SEQUENCES
publication, it excludes sequences specified in the EXCEPT (SEQUENCE
...) clause.

======
src/bin/pg_dump/pg_dump.c

getPublications:

2.
- * Get the list of tables for publications specified in the EXCEPT
- * TABLE clause.
+ * Get the list of tables and sequences specified in the publication's
+ * EXCEPT clause.

You cannot exclude tables and sequences from the same EXCEPT, so that
should say "EXCEPT clauses" (plural). But, it may be simpler to just
reword it.

SUGGESTION
Get the list of tables and sequences explicitly excluded from the publication.

======
src/bin/psql/describe.c

describeOneTableDetails:

3.
+ /* Print publications whose EXCEPT clause contains this sequence */
+ if (pset.sversion >= 200000)

This is similar. "whose EXCEPT clause" implies there is only one, but
in practice there can be multiple EXCEPT clauses: (FOR ALL TABLES
EXCEPT + FOR ALL SEQUENCES EXCEPT).

SUGGESTION
Print publications that explicitly exclude this sequence.

~~~

4.
+ /* Get sequences in the EXCEPT clause for this publication */
+ printfPQExpBuffer(&buf, "/* %s */\n",
+ _("Get sequences excluded by this publication"));

This is similar. "the EXCEPT clause" implies there is only one, but
there may be multiple. I saw that you were mimicking the existing
comment, so those should be the same.

SUGGESTION #1 (redundant, because the next string already says what
the code is doing)
Just remove both comments.

SUGGESTION #2 (clarify what clauses you are talking about)
/* Get tables in the EXCEPT (TABLE ...) clause for this publication */
and
/* Get sequences in the EXCEPT (SEQUENCE ...) clause for this publication */

======
src/include/nodes/parsenodes.h

5.
+typedef struct PublicationRelation
{
NodeTag type;
- RangeVar *relation; /* publication relation */
- Node *whereClause; /* qualifications */
+ RangeVar *relation; /* publication table/sequence */
+ Node *whereClause; /* qualifications for publication table */
List *columns; /* List of columns in a publication table */
bool except; /* True if listed in the EXCEPT clause */
-} PublicationTable;
+} PublicationRelation;

Same. /True if listed in the EXCEPT clause/True if listed in an EXCEPT clause/

~~~

6.
PUBLICATIONOBJ_EXCEPT_TABLE, /* A table in the EXCEPT clause */
+ PUBLICATIONOBJ_EXCEPT_SEQUENCE, /* A sequence in the EXCEPT clause */

/A table in the EXCEPT clause/A table in an EXCEPT clause/
/A sequence in the EXCEPT clause/A sequence in an EXCEPT clause/

~~~

7.
- List *except_tables; /* tables specified in the EXCEPT clause */
+ List *except_relations; /* depending on the 'pubobjtype', this is
+ * a list of either tables or sequences
+ * specified in the EXCEPT clause */
ParseLoc location; /* token location, or -1 if unknown */
} PublicationAllObjSpec;

/specified in the EXCEPT clause/specified in an EXCEPT clause/

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

8.
+-- Check that the sequence description shows the publications where
it is listed
+-- in the EXCEPT clause

/the EXCEPT clause/an EXCEPT clause/

~~~

9.
+-- Test combination of ALL SEQUENCES and ALL TABLES with EXCEPT clause

/with EXCEPT clause/, each with an EXCEPT clause/

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

10.
+is($result, '200|t',
+ 'check replication of a sequence in the EXCEPT clause of one
publication but included by another'
+);

(same x2)

SUGGESTION
check replication of a sequence excluded by one publication but
included by another

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