Re: Synchronizing slots from primary to standby

On Thu, Jan 4, 2024 at 7:24 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Thu, Jan 04, 2024 at 10:27:31AM +0530, shveta malik wrote:
> > On Thu, Jan 4, 2024 at 9:18 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Wed, Jan 3, 2024 at 6:33 PM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > On Tuesday, January 2, 2024 6:32 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > > > On Fri, Dec 29, 2023 at 10:25 AM Amit Kapila <amit.kapila16@gmail.com>
> > > > >
> > > > > The topup patch has also changed app_name to
> > > > > {cluster_name}_slotsyncworker so that we do not confuse between walreceiver
> > > > > and slotsyncworker entry.
> > > > >
> > > > > Please note that there is no change in rest of the patches, changes are in
> > > > > additional 0004 patch alone.
> > > >
> > > > Attach the V56 patch set which supports ALTER SUBSCRIPTION SET (failover).
> > > > This is useful when user want to refresh the publication tables, they can now alter the
> > > > failover option to false and then execute the refresh command.
> > > >
> > > > Best Regards,
> > > > Hou zj
> > >
> > > The patches no longer apply to HEAD due to a recent commit 007693f. I
> > > am working on rebasing and will post the new patches soon
> > >
> > > thanks
> > > Shveta
> >
> > Commit 007693f has changed 'conflicting' to 'conflict_reason', so
> > adjusted the code around that in the slotsync worker.
> >
> > Also removed function 'pg_get_slot_invalidation_cause' as now
> > conflict_reason tells the same.
> >
> > PFA rebased patches with above changes.
> >
>
> Thanks!
>
> Looking at 0004:
>
> 1 ====
>
> -libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
> -                                const char *appname, char **err)
> +libpqrcv_connect(const char *conninfo, bool replication, bool logical,
> +                                bool must_use_password, const char *appname, char **err)
>
> What about adjusting the preceding comment a bit to describe what the new replication
> parameter is for?
>
> 2 ====
>
> +       /* We can not have logical w/o replication */
>
> what about replacing w/o by without?
>
> 3 ===
>
> +       if(!replication)
> +               Assert(!logical);
> +
> +       if (replication)
>         {
>
> what about using "if () else" instead (to avoid unnecessary test)?
>
>
> Having said that the patch seems a reasonable way to implement non-replication
> connection in slotsync worker.
>
> 4 ===
>
> Looking closer, the only place where walrcv_connect() is called with replication
> set to false and logical set to false is in ReplSlotSyncWorkerMain().
>
> That does make sense, but what do you think about creating dedicated libpqslotsyncwrkr_connect
> and slotsyncwrkr_connect (instead of using the libpqrcv_connect / walrcv_connect ones)?
>
> That way we could make use of slotsyncwrkr_connect() in ReplSlotSyncWorkerMain()
> as I think it's confusing to use "rcv" functions while the process using them is
> not of backend type walreceiver.
>
> I'm not sure that worth the extra complexity though, what do you think?

I gave it a thought earlier, but then I was not sure even if I create
a new function w/o "rcv" in it then where should it be placed as the
existing file name itself is libpq'walreceiver'.c. Shall we be
creating a new file then? But it does not seem good to create a new
setup (new file, function pointers  other stuff) around 1 function.
And thus reusing the same function with 'replication' (new arg) felt
like a better choice than other options. If in future, there is any
other module trying to do the same, then it can use current
walrcv_connect() with rep=false. If I make it specific to slot-sync
worker, then it will not be reusable by other modules (if needed).


thanks
Shveta