Re: Support EXCEPT for ALL SEQUENCES publications

Some review comments for patch v21-0002.

======
doc/src/sgml/ref/alter_publication.sgml

Description:

1.
- <literal>FOR ALL TABLES</literal> publication. If <literal>EXCEPT</literal>
- is specified with a list of tables, the existing exclusion list is replaced
- with the specified tables. If <literal>EXCEPT</literal> is omitted, the
- existing exclusion list is cleared. The <literal>SET</literal> clause, when
- used with a publication defined with <literal>FOR TABLE</literal> or
+ <literal>FOR ALL TABLES</literal> publication and
+ <literal>SET ALL SEQUENCES</literal> can be used to update the sequences
+ specified in the <literal>EXCEPT</literal> clause of a
+ <literal>FOR ALL SEQUENCES</literal> publication. If
+ <literal>EXCEPT</literal> is specified with a list of tables or sequences,
+ the existing exclusion list is replaced with the specified tables or
+ sequences. If <literal>EXCEPT</literal> is omitted, the existing exclusion
+ list is cleared. The <literal>SET</literal> clause, when used with a
+ publication defined with <literal>FOR TABLE</literal> or

IMO the same information can be conveyed less verbosely.

BEFORE
If <literal>EXCEPT</literal> is specified with a list of tables or
sequences, the existing exclusion list is replaced with the specified
tables or sequences. If <literal>EXCEPT</literal> is omitted, the
existing exclusion list is cleared.

SUGGESTION
If <literal>EXCEPT</literal> is specified, the existing exclusion list
is replaced. If <literal>EXCEPT</literal> is omitted, the existing
exclusion list is cleared.

~~~

Parameters:

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

(This is same as yesterday's review comment [1;comment#2] for the
CREATE PUBLICATION page)

SUGGESTION
sequence_name
Name of an existing sequence.

~~~

EXCEPT:

3.
Nisha's EXCEPT thread docs [2;patch 0005] have an EXCEPT parameter on this page.
e.g.
+ <varlistentry>
+ <term><literal>EXCEPT</literal></term>

I think you should do the same, because that will make merge/rebase
easier later when both these patches eventually get pushed. Please
refer to the other thread to make yours similar.

~~~

4.
+ <para>
+ Replace the sequence list in the publication's <literal>EXCEPT</literal>
+ clause:
+<programlisting>
+ALTER PUBLICATION mypublication SET ALL SEQUENCES EXCEPT (SEQUENCE seq1, seq2);
+</programlisting></para>

I am unsure if the examples are meant to be read from top-to-bottom,
or if every example is independent from every other example.

e.g. If they flow top-to-bottom, then "Replace the sequence list"
probably should say "Set the excluded sequences in..." because there
was no list here to "replace".

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

get_delete_rels:

5.
+ Node *oldrelwhereclause = NULL;
+ Bitmapset *oldcolumns = NULL;

Firstly, I think the code is comparing the WHERE expressions, not
clauses -- a clause is a piece of the grammar, so that may not be the
correct term here. Secondly, these names somehow don't seem
consistent. e.g. One says 'rel', and one does not.

I thought it should be more like:

oldrelwhereexpr/oldrelcolumns
OR
oldwhereexpr/oldcolumns
OR
oldrowfilter/oldcolumns
etc.

~~~

6.
+ foreach(newlc, rels)
+ {
+ PublicationRelInfo *newpubrel;
+ Oid newrelid;
+ Bitmapset *newcolumns = NULL;
+
+ newpubrel = (PublicationRelInfo *) lfirst(newlc);

Could that be more neatly written as a foreach_ptr(...) loop?

~~~

7.
+ /*
+ * Validate the column list. If the column list or WHERE clause
+ * changes, then the validation done here will be duplicated
+ * inside PublicationAddRelations(). The validation is cheap
+ * enough that that seems harmless.
+ */
+ newcolumns = pub_collist_validate(newpubrel->relation,
+ newpubrel->columns);
+
+ found = (newrelid == oldrelid) &&
+ equal(oldrelwhereclause, newpubrel->whereClause) &&
+ bms_equal(oldcolumns, newcolumns);
+
+ if (found)
+ break;

IIUC the function `pub_collist_validate` is being called prematurely.
It looks like it could be just wasted effort often. Why not defer it,
so it only called when needed to complete the comparison.

e.g. something like below:

SUGGESTION
found = false;
...
if (newrelid == oldrelid && equal(oldrelwhereclause, newpubrel->whereClause))
{
Bitmapset *newcolumns = pub_collist_validate(newpubrel->relation,
newpubrel->columns);

if (bms_equal(oldcolumns, newcolumns))
{
found = true;
break;
}
}

~~~

AlterPublication:

8.
relations = list_concat(relations, excepttbls);
- AlterPublicationTables(stmt, tup, relations, pstate->p_sourcetext,
- schemaidlist != NIL);
+ AlterPublicationRelations(stmt, tup, relations, exceptseqs,
+ pstate->p_sourcetext, schemaidlist != NIL);

The argument names here are confusing.

The `AlterPublicationRelations` takes Lists of *tables and *sequences,
so that variable called `relations` is ambiguous in this context. Can
it be renamed or commented to explain how it can only be a tables list
st this point?

======
src/backend/parser/gram.y

9.
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT", "("))
+ COMPLETE_WITH("SEQUENCE");
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL", "SEQUENCES"))
+ COMPLETE_WITH("EXCEPT ( SEQUENCE");
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT"))
+ COMPLETE_WITH("( SEQUENCE");
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT", "("))
+ COMPLETE_WITH("SEQUENCE");
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT", "(", "SEQUENCE"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT", "(", "SEQUENCE", MatchAnyN) &&
ends_with(prev_wd, ','))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_sequences);
+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT", "(", "SEQUENCE", MatchAnyN) &&
!ends_with(prev_wd, ','))
+ COMPLETE_WITH(")");

Huh? An identical condition has been duplicated here:

+ else if (Matches("ALTER", "PUBLICATION", MatchAny, "SET", "ALL",
"SEQUENCES", "EXCEPT", "("))
+ COMPLETE_WITH("SEQUENCE");
+ COMPLETE_WITH("SEQUENCE");

(IMO remove the first one)

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

10.
+# Check ALTER PUBLICATION ... ALL SEQUENCES EXCEPT (SEQUENCE ...)

Missing SET.

SUGGESTION
Check ALTER PUBLICATION ... SET ALL SEQUENCES EXCEPT (SEQUENCE ...)

~~

11.
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT last_value, is_called FROM seq_excluded_in_pub1_2");
+is($result, '1|f', 'sequences in the EXCEPT list are excluded');

/EXCEPT list/EXCEPT clause/

======
[1] https://www.postgresql.org/message-id/CAHut%2BPvG4OjooxEO-OgiGqG9nvAqS6_ZR%2BU2%2B9ThwRUQ_y6K7A%40mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/CABdArM5sw4Q1ZU8HGdo4BSc1A_%2B8xtUNq17j6wcir%3DyMUy19Cg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia