Re: Support EXCEPT for ALL SEQUENCES publications

On Tue, 28 Jul 2026 at 09:15, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Jul 24, 2026 at 3:50 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Wed, 22 Jul 2026 at 10:30, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Jul 8, 2026 at 5:38 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > > >
> > > > I have addressed the comments and attached the updated v20 patch.
> > > >
> > >
> > > Thanks Shlok. Please find a few comments:
> > >
> > > 1)
> > >
> > > + /*
> > > + * Must be a regular or partitioned table when specified in FOR TABLE or
> > > + * EXCEPT table list
> > > + */
> > >
> > > EXCEPT table list-->EXCEPT (TABLE ...) clause
> > >
> > > + /* Must be a sequence if specified in EXCEPT sequence list */
> > >
> > > EXCEPT sequence list-->EXCEPT (SEQUENCE ...) clause
> > >
> > > 2)
> > > +
> > > + /*
> > > + * TODO: EXCEPT (SEQUENCE ...) is not yet supported with ALTER
> > > + * PUBLICATION.
> > > + */
> > > + Assert(exceptseqs == NIL);
> > >
> > > We can remove 'TODO' as it is a conscious decision to not support this
> > > in patch001
> > >
> > > 3)
> > > Why patch002 needs this change? And is itg onlyh for 002 and not needed by 001?
> > >
> > > get_publication_relations(Oid pubid, PublicationPartOpt pub_partopt,
> > > - bool except_flag)
> > > + bool except_flag, char pubrelkind)
> > >
> > > - result = GetPubPartitionOptionRelations(result, pub_partopt,
> > > - pubrel->prrelid);
> > > + {
> > > + char relkind = get_rel_relkind(pubrel->prrelid);
> > > +
> > > + if ((pubrelkind == RELKIND_RELATION &&
> > > + (relkind == RELKIND_RELATION || relkind == RELKIND_PARTITIONED_TABLE)) ||
> > > + (pubrelkind == RELKIND_SEQUENCE && relkind == RELKIND_SEQUENCE))
> > > + result = GetPubPartitionOptionRelations(result, pub_partopt,
> > > + pubrel->prrelid);
> >
> > It is introduced to handle following scenario in ALTER PUBLICATION:
> > ```
> > if (stmt->for_all_tables || stmt->for_all_sequences)
> > {
> > /*
> > * In FOR ALL TABLES or FOR ALL SEQUENCES mode, relations are
> > * tracked as exclusions (EXCEPT clause). Fetch the current
> > * excluded relations so they can be reconciled with the specified
> > * EXCEPT list.
> > *
> > * This applies only if the existing publication is already
> > * defined as FOR ALL TABLES or FOR ALL SEQUENCES; otherwise,
> > * there are no exclusion entries to process.
> > */
> > if (pubform->puballtables)
> > oldrelids = GetExcludedPublicationRelations(pubid,
> >
> > PUBLICATION_PART_ROOT,
> > RELKIND_RELATION);
> > if (pubform->puballsequences)
> > oldseqids = GetExcludedPublicationRelations(pubid,
> >
> > PUBLICATION_PART_ROOT,
> > RELKIND_SEQUENCE);
> > }
> > .
> > .
> > /* Get tables and sequences to be dropped */
> > delrels = get_delete_rels(pubid, rels, oldrelids);
> > delrels = list_concat(delrels, get_delete_rels(pubid, seqs, oldseqids));
> > ```
> >
> > In ALTER PUBLICATION, we need to fetch the existing EXCEPT lists
> > separately for tables and sequences.
> > That's why GetExcludedPublicationRelations() (and consequently
> > get_publication_relations()) now takes pubrelkind as an argument and
> > filters the returned relations accordingly.
> > This allows us to maintain separate lists (oldrelids and oldseqids)
> > and make separate calls to get_delete_rels() for tables and sequences.
>
> But since we are not doing any separate processing for tables and
> sequences based on relkind in get_delete_rels() or in later part of
> code (once we fetch oldrelids and oldseqids), my understanding was
> that we should be able to get rid of this extra logic, as well as
> calling get_delete_rels twice. Please review the logic in this
> direction.

In 0001, I made a change so that adding tables and sequences to a
publication requires calling PublicationAddRelations() separately for
tables and sequences. This is because the relation-kind checks are
performed in:
PublicationAddRelations() -> publication_add_relation() ->
check_publication_add_relation().

These checks ensure that the EXCEPT list for a FOR ALL TABLES
publication contains only tables, and the EXCEPT list for a FOR ALL
SEQUENCES publication contains only sequences. Therefore,
PublicationAddRelations() must be called separately for FOR ALL TABLES
and FOR ALL SEQUENCES publications.
To support the same behavior in ALTER PUBLICATION, we also need to
maintain separate lists for tables and sequences that are to be added
to the publication's EXCEPT list. Since the relation-kind validation
is performed inside PublicationAddRelations(), we cannot assume that
'rels' and 'seqs' contain only tables and sequences, respectively.
This is also why get_delete_rels() needs to be called separately for
the two lists. (There can be cases where some sequences of 'oldseqids'
are present in 'rels'. So if we call get_delete_rels on combined list,
it will skip deleting those sequences, which is not correct).
One alternative I can think of is to perform the relkind check
earlier, when opening the relations via OpenRelationList(). If we did
that, we might be able to avoid maintaining separate lists and calling
these functions twice. Thoughts?

Thanks,
Shlok Kyal