On Fri, Oct 17, 2025 at 12:44 PM Masahiko Sawada <[email protected]> wrote:
>
> On Fri, Oct 17, 2025 at 1:35 AM shveta malik <[email protected]> wrote:
> >
> > On Fri, Oct 17, 2025 at 10:01 AM Amit Kapila <[email protected]>
> > wrote:
> > >
> > > On Thu, Oct 16, 2025 at 4:53 PM Zhijie Hou (Fujitsu)
> > > <[email protected]> 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
