On Fri, Jul 24, 2026 at 3:50 PM Shlok Kyal <[email protected]> wrote:
>
> On Wed, 22 Jul 2026 at 10:30, shveta malik <[email protected]> wrote:
> >
> > On Wed, Jul 8, 2026 at 5:38 PM Shlok Kyal <[email protected]> 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.
> 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.
Okay, so originally this change does not belong to this thread.
> I agree that this comment change is independent of the patch logic and
> can be moved to patch 0001, so I have done that.
okay, that makes sense.
thanks
Shveta
