Re: Support EXCEPT for ALL SEQUENCES publications

On Wed, 27 May 2026 at 09:08, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok,
>
> No more comments for v6-0001.
>
> Below are some review comments for v6-0002.
>
> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> 1.
> <literal>SET ALL TABLES</literal> can be used to update the tables specified
> in the <literal>EXCEPT</literal> clause of a
> - <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
> <literal>FOR TABLES IN SCHEMA</literal>, replaces the list of tables/schemas
> in the publication with the specified list; the existing tables or schemas
> that were present in the publication will be removed.
>
> TBH, I think this part of the description is repetitive and
> unnecessarily difficult to read. OTOH, fixing it is probably outside
> the scope of this patch. Perhaps we can revisit and address all this
> properly after both your EXCEPT changes and Nisha's EXCEPT [1] changes
> get combined?
>
I agree.

> ~
>
> Examples:
>
> 2.
> There are many other small examples, so should you also add one also
> for this syntax?
>
> ======
> src/backend/catalog/pg_publication.c
>
> 3.
> static List *
> get_publication_relations(Oid pubid, PublicationPartOpt pub_partopt,
> - bool except_flag)
> + bool except_flag, char pubrelkind)
>
> IIUC there should be a sanity Assert in this function to check that
> `pubrelkind` can only be either RELKIND_RELATION or RELKIND_SEQUENCE.
>
> ~~~
>
> GetIncludedPublicationRelations:
>
> 4.
> /*
> * 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().
> */
> List *
> GetIncludedPublicationRelations(Oid pubid, PublicationPartOpt pub_partopt)
> {
> Assert(!GetPublication(pubid)->alltables);
>
> return get_publication_relations(pubid, pub_partopt, false, RELKIND_RELATION);
> }
>
> The comment does not match the code/assert. Shouldn't it also do
> assert !allsequences?
>
This function can be called for ALL SEQUENCES publication. For example
'pg_get_publication_tables', 'InvalidatePubRelSyncCache', etc can call
this function for all sequence publications, but in this case the list
returned is empty. So, there is no overall impact.
So, we should not add an 'assert !allsequences'.

This has been already discussed here [1].
> ~~~
>
> GetAllPublicationRelations:
>
> 5.
> exceptlist = GetExcludedPublicationRelations(pubid, pubviaroot ?
> PUBLICATION_PART_ROOT :
> - PUBLICATION_PART_LEAF);
> + PUBLICATION_PART_LEAF,
> + relkind);
>
> IIUC, that `relkind` should be renamed to pubrelkind.
>
> ======
> src/backend/commands/publicationcmds.c
>
> get_delete_rels:
>
> 6.
> +get_delete_rels(Oid pubid, List *rels, List *oldrelids, List **delrels)
>
> Add some function comments to describe these parameters.
>
> I also wondered if it would be better to return `delrels` from this
> function instead of void. Let the caller accumulate them.
>
> ~~~
>
> 7.
> + foreach(newlc, rels)
> + {
> + PublicationRelInfo *newpubrel;
> + Oid newrelid;
> + Bitmapset *newcolumns = NULL;
> +
> + newpubrel = (PublicationRelInfo *) lfirst(newlc);
> + newrelid = RelationGetRelid(newpubrel->relation);
> +
> + /*
> + * 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);
> +
> + /*
> + * Check if any of the new set of relations matches with the
> + * existing relations in the publication. Additionally, if the
> + * relation has an associated WHERE clause, check the WHERE
> + * expressions also match. Same for the column list. Drop the
> + * rest.
> + */
>
> IIUC, that comment "Check if any of the new..." is really describing
> the purpose of this entire loop. IMO, this comment would be better put
> atop the "foreach(newlc, rels)".
>
> ~
>
> 8.
> + if (newrelid == oldrelid)
> + {
> + if (equal(oldrelwhereclause, newpubrel->whereClause) &&
> + bms_equal(oldcolumns, newcolumns))
> + {
> + found = true;
> + break;
> + }
> + }
> + }
>
> Consider writing that in a simpler way.
>
> SUGGESTION
> found = (newrelid == oldrelid) &&
> equal(oldrelwhereclause, newpubrel->whereClause) &&
> bms_equal(oldcolumns, newcolumns);
>
> if (found)
> break;
>
> ~~~
>
> 9.
> + /*
> + * Add the non-matched relations to a list so that they can be
> + * dropped.
> + */
> + if (!found)
> + {
> + oldrel = palloc_object(PublicationRelInfo);
> + oldrel->whereClause = NULL;
> + oldrel->columns = NIL;
> + oldrel->except = false;
> + oldrel->relation = table_open(oldrelid,
> + ShareUpdateExclusiveLock);
> + *delrels = lappend(*delrels, oldrel);
> + }
>
> 9a.
> There seems to be some implicit knowledge that the later DROP is only
> going to care about the relation and the other whereClause/columns/etc
> will have nothing to do with the dropping. Maybe the comment needs to
> say something abouit that?
>
> ~
>
> 9b.
> Maybe palloc0_object can be used instead of explicitly setting
> everything else you don't care about to NULL/NIL/false?
>
> ~~~
>
> AlterPublicationRelations:
>
> 10.
> /*
> * Nothing to do if no objects, except in SET: for that it is quite
> * possible that user has not specified any tables in which case we need
> * to remove all the existing tables.
> */
> if (!tables && stmt->action != AP_SetObjects)
> return;
>
> ~
>
> The above code comment seems stale because it is not saying anything
> about sequences, and I think it ought to be.
>
> ~~~
>
> 11.
> + CloseRelationList(seqs);
> CloseRelationList(rels);
>
> Everywhere else rels came before seqs. So maybe reverse these to match.
>
> ~~~
>
> PublicationDropRelations:
>
> 12.
> * Remove listed tables from the publication.
> */
> static void
> -PublicationDropTables(Oid pubid, List *rels, bool missing_ok)
> +PublicationDropRelations(Oid pubid, List *rels, bool missing_ok)
>
> The function comment should mention sequences as well.
>
> ======
> [1] https://www.postgresql.org/message-id/flat/CAHut%2BPvBjw8JJOksjJsCN%2BU4Lda0vWAQTYaYy7ucuMMr8stj0w%40mail.gmail.com#0f1c9caea31ce84b161ec776fe0994b4

I have addressed the remaining comments as well and attached the v7 patch.

[1]: https://www.postgresql.org/message-id/CAA4eK1+rnjBOvkiQC2r4LuTwuje653iVPPAXcmJZXPpKvsNbOQ@mail.gmail.com

Thanks,
Shlok Kyal