Re: Logical Replication of sequences - Mailing list pgsql-hackers
From | Masahiko Sawada |
---|---|
Subject | Re: Logical Replication of sequences |
Date | |
Msg-id | CAD21AoBMeCbTuekHtP1-BZPEUW3XMafDVMh54QoKjjRVyt-Uww@mail.gmail.com Whole thread Raw |
In response to | Re: Logical Replication of sequences (Masahiko Sawada <sawada.mshk@gmail.com>) |
List | pgsql-hackers |
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
pgsql-hackers by date: