Re: Support EXCEPT for ALL SEQUENCES publications

Some review comments for v21-0001 patch.

======
doc/src/sgml/catalogs.sgml

1.
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>prattrs</structfield> <type>int2vector</type>
(references <link
linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.<structfield>attnum</structfield>)
</para>
<para>
This is an array of values that indicates which table columns are
part of the publication. For example, a value of <literal>1 3</literal>
would mean that the first and the third table columns are published.
A null value indicates that all columns are published.
</para></entry>

That last sentence:
"A null value indicates that all columns are published."
is not strictly correct anymore, because `prattrs` is also always NULL
for sequences.

======
doc/src/sgml/ref/create_publication.sgml

Parameters:

2.
The <replaceable class="parameter">sequence_name</replaceable> needs
to be listed in the Parameters.

(Not sure where is best to put it -- maybe it belongs after the EXCEPT
parameter?)

SUGGESTION
sequence_name
Name of an existing sequence.

~~~

EXCEPT:

3.
<para>
- This clause specifies a list of tables to be excluded from the
+ This clause specifies the tables or sequences to be excluded from an
+ <literal>ALL TABLES</literal> or <literal>ALL SEQUENCES</literal>
publication.
</para>

It's better to reword this to remove any ambiguity.

SUGGESTION
This clause specifies the tables to be excluded from an ALL TABLES
publication, or the sequences to be excluded from an ALL SEQUENCES
publication.

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

check_publication_add_relation:

4.
/*
- * Check if relation can be in given publication and throws appropriate
- * error if not.
+ * Check if the target relation is allowed to be specified in the given
+ * publication and throw an error if not.
+ *
+ * 'pubrelkind' is the relkind accepted by the publication clause. The relkind
+ * of the relation in 'pri' is checked for compatibility against it. Error is
+ * raised if they are not compatible.
*/

Probably this did not need to say " and throw an error if not.". It is
redundant because the function comment elsewhere already says that an
error will be thrown if they are not compatible.

~~~

5.
/*
* Gets list of relation oids that are associated with a publication.
*
- * This should only be used FOR TABLE publications, the FOR ALL
TABLES/SEQUENCES
- * should use GetAllPublicationRelations().
+ * This is mainly used for FOR TABLE publications and must not be called for
+ * ALL TABLES publications. For ALL SEQUENCES publications, the result is an
+ * empty list.
*/
List *
GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)

Hmm. Sure, just returning an empty list for ALL SEQUENCES seems a bit
too convenient doesn't it? IIUC, the condition when calling this
function ideally should have been modified, but the current code took
the easy option because it still works, with zero code changes. OTOH,
I'm not sure avoiding fixing the upstream conditions has been worth
it, given it's causing with multiple review comments all asking why
can't we also assert !allsequences here.

Anyway, if the calling conditions aren't ever going to be fixed, then
at least we can return an empty list directly from here. e.g. AFAICT,
there's no need to call even deeper to do a bunch of wasted logic
given we already know the result is going to be an empty list for FOR
ALL SEQUENCES.

SUGGESTION
/* comment here explaining why we got for ALL SEQUENCES... */
if (GetPublication(pubid)->allsequences)
return NIL;

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

OpenRelationList:

6.
+static List *OpenRelationList(List *tables);

and

static List *
-OpenTableList(List *tables)
+OpenRelationList(List *tables)

Shouldn't that parameter also be renamed `rels` instead of `tables`?

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

7.
- * Although individual table entries in EXCEPT list could be stored in
- * PublicationRelInfo, dumpPublicationTable cannot be used to emit
- * them, because there is no ALTER PUBLICATION ... ADD command to add
- * individual table entries to the EXCEPT list.
+ * Although individual table/sequence entries in EXCEPT list could be
+ * stored in PublicationRelInfo, dumpPublicationTable cannot be used
+ * to emit them, because there is no ALTER PUBLICATION ... ADD command
+ * to add individual table entries to the EXCEPT list.

Maybe should be /EXCEPT list/EXCEPT clause/ (2 times)

~~~

8.
* Therefore, the approach is to dump the complete EXCEPT list in a
* single CREATE PUBLICATION statement. PublicationInfo is used to
* collect this information, which is then emitted by
* dumpPublication().

Now that it is possible to have multiple EXCEPT clauses at the same
time so "the complete EXCEPT list" wording is no longer correct.

SUGGESTION
... is to dump complete EXCEPT clauses in a single CREATE PUBLICATION statement.

~~~

9.
+ /*
+ * Skip entries where this sequence appears in the publication's
+ * EXCEPT list.
+ */
+ if (pset.sversion >= 200000)

/EXCEPT list/EXCEPT (SEQUENCE) clause/

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

10.
-typedef struct PublicationTable
+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;
+ bool except; /* True if listed in an EXCEPT clause */
+} PublicationRelation;

I wondered if those comments for `whereClause` and `columns` should
also say something like "; NULL for sequences."

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

(The following review comments are using terminology more like Nisha's
SCHEMA EXCEPT patches),

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

/where it is listed in an EXCEPT clause/where it is named in an EXCEPT clause/

~~~

12.
+-- fail - first sequence in the EXCEPT list should use SEQUENCE keyword

SUGGESTION
fail - EXCEPT (SEQUENCE) clause must have SEQUENCE keyword

~~~

13.
+-- fail - unlogged sequence is specified in EXCEPT sequence list

SUGGESTION
fail - EXCEPT (SEQUENCE) clause specifies an unlogged sequence

~~~

14.
+-- fail - temporary sequence is specified in EXCEPT sequence list

SUGGESTION
fail - EXCEPT (SEQUENCE) clause specifies a temporary sequence

~~~

15.
+-- fail - sequence object is specified in EXCEPT table list

SUGGESTION
fail - EXCEPT (TABLE) clause specifies a sequence object

~~~

16.
+-- fail - table object is specified in EXCEPT sequence list

SUGGESTION
fail - EXCEPT (SEQUENCE) clause specifies a table object

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

17.
+# Check the initial data on subscriber
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT last_value, is_called FROM seq_excluded_in_pub1");
+is($result, '1|f', 'sequences in the EXCEPT list are excluded');

/EXCEPT list/EXCEPT clause/

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