Re: Support EXCEPT for ALL SEQUENCES publications

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.

The change is only needed in patch 0002 because this logic is
introduced there. Patch 0001 did not require separate handling of
tables and sequences when reconciling the existing EXCEPT lists, so
adding pubrelkind there would not have provided any benefit.
>
> 4)
> It is not clear why 002 has made this change? Why the behaviour is
> different for ALL Tables and ALL Seqeunces.
>
>
> - * 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)
>
There are multiple places in the code that look like this:
```
if (puballtables)
{
...
}
else
{
relids = GetIncludedPublicationRelations(...);
}
```
GetIncludedPublicationRelations() is not called for ALL TABLES
publications, but it can be called for ALL SEQUENCES publications. In
that case, it simply returns an empty list. Because of this, we cannot
add an Assert similar to the ALL TABLES case.
I had raised this concern in the original thread [1], and it was
decided that we should document this behavior with a comment. Since I
was already modifying the surrounding code, I updated the comment
there as well.

I agree that this comment change is independent of the patch logic and
can be moved to patch 0001, so I have done that.

>
> 5)
> AlterPublicationRelations:
>
> Shall we add this Assert in else block as except-sequences must be
> NULL if this is case of non all-seq pub.
>
> if (stmt->for_all_tables || stmt->for_all_sequences)
> {
> ....
> }
> else
> {
> Assert(!sequences)
> oldrelids = GetIncludedPublicationRelations .....
> }
>

I have addressed other comments as well and attached the v21 patches.

[1]: https://www.postgresql.org/message-id/CANhcyEUkV-T6cK142w9wfME9nobFHOvn1f4itJLMG-oR4QoPbQ@mail.gmail.com

Thanks,
Shlok Kyal