Re: Support EXCEPT for ALL SEQUENCES publications

On Tue, 28 Jul 2026 at 06:22, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.
>
I am not sure why an EXCEPT parameter is need on
alter_publication.sgml. We have already described the behaviour of
EXCEPT with ALL SEQUENCE and ALL TABLES in a paragraph earlier in the
same page.
Also we have described the EXCEPT parameter in
create_publication.sgml. Won't it be redundant to add the same
description again?

> ~~~
>
> 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.
>
I have used 'oldwhereexpr/oldcolumns'

> ~~~
>
> 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
>

I have addressed the remaining comments and attached the updated patch in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEU8u2Knq%3DDmJ-yP%3DG2WpvXW6fitSmxcm8CVAdLhwNM4bg%40mail.gmail.com

Thanks,
Shlok Kyal