Re: Support EXCEPT for ALL SEQUENCES publications

On Mon, 27 Jul 2026 at 14:27, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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;
>
I have added the suggested if condition.

> ======
> 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`?
>
The function already has the 'rels' variable declared. I have renamed
'tables' to 'relations'.

> ======
> 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/
>
A similar comment for the EXCEPT (TABLE) case needs an update. Made
that change as well.

> ======
> 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/
>

I have addressed the remaining comments and attached the updated v22 patch.

Thanks,
Shlok Kyal