Re: Logical Replication of sequences

On Fri, Oct 17, 2025 at 12:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Oct 17, 2025 at 1:35 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Oct 17, 2025 at 10:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Oct 16, 2025 at 4:53 PM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > Regarding whether we can avoid creating slot/origin for seq-only publication.
> > > > I think the main challenge lies in ensuring the apply worker operates smoothly
> > > > without a replication slot. Currently, the apply worker uses the
> > > > START_REPLICATION command with a replication slot to acquire the slot on the
> > > > publisher. To bypass this, it's essential to skip starting the replication and
> > > > specifically, avoid entering the LogicalRepApplyLoop().
> > > >
> > > > To address this, I thought to implement a separate loop dedicated to
> > > > sequence-only subscriptions. Within this loop, the apply worker would only call
> > > > functions like ProcessSyncingSequencesForApply() to manage sequence
> > > > synchronization while periodically checking for any new tables added to the
> > > > subscription. If new tables are detected, the apply worker would exit this loop
> > > > and enter the LogicalRepApplyLoop().
> > > >
> > > > I chose not to consider allowing the START_REPLICATION command to operate
> > > > without a logical slot, as it seems like an unconventional approach requiring
> > > > modifications in walsender and to skip logical decoding and related processes.
> > > >
> > > > Another consideration is whether to address scenarios where tables are
> > > > subsequently removed from the subscription, given that slots and origins would
> > > > already have been created in such cases.
> > > >
> > > > Since it might introduce addition complexity to the patches, and considering
> > > > that we already allow slot/origin to be created for empty subscription, it might
> > > > also be acceptable to allow it to be created for sequence-only subscription. So,
> > > > I chose to add some comments to explain the reason for it in latest version.
> > > >
> > > > Origin case might be slightly easier to handle, but it could also require some
> > > > amount of implementations. Since origin is less harmful than a replication slot
> > > > and maintaining it does not have noticeable overhead, it seems OK to me to
> > > > retain the current behaviour and add some comments in the patch to clarify the
> > > > same.
> > > >
> > >
> > > I agree that avoiding to create a slot/origin for sequence-only
> > > subscription is not worth the additional complexity at other places,
> > > especially when we do create them for empty subscriptions.
> >
> > +1.
> >
> > While testeing 001 patch alone, I found that for sequence-only
> > subscription, we get error in tablesync worker :
> > ERROR:  relation "public.seq1" type mismatch: source "table", target "sequence"
> >
> > This error comes because during copy_table(),
> > logicalrep_relmap_update() does not update relkind and thus later
> > CheckSubscriptionRelkind() ends up giving the above error.
>
> I faced the same error while reviewing the 0001 patch. I think if
> we're going to push these patches separately the 0001 patch should
> have at least minimal regression tests. Otherwise, I'm concerned that
> buildfarm animals won't complain but we could end up blocking other
> logical replication developments.
>

One minor comment for 0001 patch is:

+       /*
+        * Skip sequence tuples. If even a single table tuple exists then the
+        * subscription has tables.
+        */
+       if (get_rel_relkind(subrel->srrelid) == RELKIND_RELATION ||
+           get_rel_relkind(subrel->srrelid) == RELKIND_PARTITIONED_TABLE)
+       {
+           has_subrels = true;
+           break;
+       }

How about storing the relkind to a variable here and avoiding calling
get_rel_relkind() twice (to save one syscache lookup)?

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com