Re: Synchronizing slots from primary to standby

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?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com