MIME-Version: 1.0
References: <CAA4eK1+AFY4E0f9h0-JF-navrf-vRMTMrN6H7PDKiR=C6p84Nw@mail.gmail.com>
 <CAJpy0uCcFKiofcp5V_hRvHrY_qVNX+eKvB0CPDNxSk2FbyMJCA@mail.gmail.com>
 <OS0PR01MB5716E731AC1B97A96B014B929460A@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAJpy0uDhR=m2Pr76aJZnvtKZpCEiLbMbjbPReCv5fS0MA3ch2w@mail.gmail.com>
 <CAJpy0uDWgSBUsTav1Eeo7E7i38GjXr_o0WdWamm3jGdctcoC4A@mail.gmail.com> <ZZa4pLFCe2mAks1m@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <ZZa4pLFCe2mAks1m@ip-10-97-1-34.eu-west-3.compute.internal>
From: shveta malik <shveta.malik@gmail.com>
Date: Fri, 5 Jan 2024 08:59:31 +0530
Message-ID: <CAJpy0uBGSjwhZTBo6MxT6TkwO2AX5fa1kw7UC3RQLiapUJReuw@mail.gmail.com>
Subject: Re: Synchronizing slots from primary to standby
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, 
	Masahiko Sawada <sawada.mshk@gmail.com>, Peter Smith <smithpb2250@gmail.com>, 
	Nisha Moond <nisha.moond412@gmail.com>, 
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, 
	Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>, Ajin Cherian <itsajin@gmail.com>, 
	Alvaro Herrera <alvherre@alvh.no-ip.org>, shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAJpy0uBGSjwhZTBo6MxT6TkwO2AX5fa1kw7UC3RQLiapUJReuw%40mail.gmail.com>
Precedence: bulk

On Thu, Jan 4, 2024 at 7:24=E2=80=AFPM 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=E2=80=AFAM shveta malik <shveta.malik@gmail=
.com> wrote:
> > >
> > > On Wed, Jan 3, 2024 at 6:33=E2=80=AFPM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > On Tuesday, January 2, 2024 6:32 PM shveta malik <shveta.malik@gmai=
l.com> wrote:
> > > > > On Fri, Dec 29, 2023 at 10:25=E2=80=AFAM Amit Kapila <amit.kapila=
16@gmail.com>
> > > > >
> > > > > The topup patch has also changed app_name to
> > > > > {cluster_name}_slotsyncworker so that we do not confuse between w=
alreceiver
> > > > > and slotsyncworker entry.
> > > > >
> > > > > Please note that there is no change in rest of the patches, chang=
es are in
> > > > > additional 0004 patch alone.
> > > >
> > > > Attach the V56 patch set which supports ALTER SUBSCRIPTION SET (fai=
lover).
> > > > This is useful when user want to refresh the publication tables, th=
ey 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 =3D=3D=3D=3D
>
> -libpqrcv_connect(const char *conninfo, bool logical, bool must_use_passw=
ord,
> -                                const char *appname, char **err)
> +libpqrcv_connect(const char *conninfo, bool replication, bool logical,
> +                                bool must_use_password, const char *appn=
ame, char **err)
>
> What about adjusting the preceding comment a bit to describe what the new=
 replication
> parameter is for?
>
> 2 =3D=3D=3D=3D
>
> +       /* We can not have logical w/o replication */
>
> what about replacing w/o by without?
>
> 3 =3D=3D=3D
>
> +       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-replic=
ation
> connection in slotsync worker.
>
> 4 =3D=3D=3D
>
> Looking closer, the only place where walrcv_connect() is called with repl=
ication
> set to false and logical set to false is in ReplSlotSyncWorkerMain().
>
> That does make sense, but what do you think about creating dedicated libp=
qslotsyncwrkr_connect
> and slotsyncwrkr_connect (instead of using the libpqrcv_connect / walrcv_=
connect ones)?
>
> That way we could make use of slotsyncwrkr_connect() in ReplSlotSyncWorke=
rMain()
> 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=3Dfalse. If I make it specific to slot-sync
worker, then it will not be reusable by other modules (if needed).


thanks
Shveta