Feedback-ID: i0c21471d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1108-g3a29173c6d-fm-20231031.005-g3a29173c
MIME-Version: 1.0
Message-Id: <a24899ab-858d-4497-a447-c02852fe21e9@app.fastmail.com>
In-Reply-To: <ZUrEclC-uZV-rGd2@paquier.xyz>
References: <5ac50071-f2ed-4ace-a8fd-b892cffd33eb@www.fastmail.com>
 <4710d895-689a-4ae6-b8e3-344286dbf970@app.fastmail.com>
 <da078f11-d2de-420e-9611-f3b7d4dadd9a@eisentraut.org>
 <ZUrEclC-uZV-rGd2@paquier.xyz>
Date: Wed, 08 Nov 2023 09:50:47 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Michael Paquier" <michael@paquier.xyz>,
 "Peter Eisentraut" <peter@eisentraut.org>
Cc: pgsql-hackers@lists.postgresql.org,
 "Amit Kapila" <amit.kapila16@gmail.com>, "Andres Freund" <andres@anarazel.de>
Subject: Re: speed up a logical replica setup
Content-Type: multipart/alternative;
 boundary=24687dd2a16d4f86912e2f0920b889ae
Archived-At: 
 <https://www.postgresql.org/message-id/a24899ab-858d-4497-a447-c02852fe21e9%40app.fastmail.com>
Precedence: bulk

--24687dd2a16d4f86912e2f0920b889ae
Content-Type: text/plain

On Tue, Nov 7, 2023, at 8:12 PM, Michael Paquier wrote:
> On Tue, Nov 07, 2023 at 10:00:39PM +0100, Peter Eisentraut wrote:
> > Speaking of which, would it make sense to put this tool (whatever the name)
> > into the pg_basebackup directory?  It's sort of related, and it also shares
> > some code.

I used the CreateReplicationSlot() from streamutil.h but decided to use the
CREATE_REPLICATION_SLOT command directly because it needs the LSN as output. As
you noticed at that time I wouldn't like a dependency in the pg_basebackup
header files; if we move this binary to base backup directory, it seems natural
to refactor the referred function and use it.

> I've read the patch, and the additions to streamutil.h and
> streamutil.c make it kind of natural to have it sit in pg_basebackup/.
> There's pg_recvlogical already there.  I am wondering about two
> things, though:
> - Should the subdirectory pg_basebackup be renamed into something more
> generic at this point?  All these things are frontend tools that deal
> in some way with the replication protocol to do their work.  Say
> a replication_tools?

It is a good fit for this tool since it is another replication tool. I also
agree with the directory renaming; it seems confusing that the directory has
the same name as one binary but also contains other related binaries in it.

> - And if it would be better to refactor some of the code generic to
> all these streaming tools to fe_utils.  What makes streamutil.h a bit
> less pluggable are all its extern variables to control the connection,
> but perhaps that can be an advantage, as well, in some cases.

I like it. There are common functions such as GetConnection(),
CreateReplicationSlot(), DropReplicationSlot() and RunIdentifySystem() that is
used by all of these replication tools. We can move the extern variables into
parameters to have a pluggable streamutil.h.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--24687dd2a16d4f86912e2f0920b889ae
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Tue, Nov 7, =
2023, at 8:12 PM, Michael Paquier wrote:<br></div><blockquote type=3D"ci=
te" id=3D"qt" style=3D""><div>On Tue, Nov 07, 2023 at 10:00:39PM +0100, =
Peter Eisentraut wrote:<br></div><div>&gt; Speaking of which, would it m=
ake sense to put this tool (whatever the name)<br></div><div>&gt; into t=
he pg_basebackup directory?&nbsp; It's sort of related, and it also shar=
es<br></div><div>&gt; some code.<br></div></blockquote><div><br></div><d=
iv>I used the CreateReplicationSlot() from streamutil.h but decided to u=
se the<br></div><div>CREATE_REPLICATION_SLOT command directly because it=
 needs the LSN as output. As<br></div><div>you noticed at that time I wo=
uldn't like a dependency in the pg_basebackup<br></div><div>header files=
; if we move this binary to base backup directory, it seems natural<br><=
/div><div>to refactor the referred function and use it.<br></div><div><b=
r></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div>I've read th=
e patch, and the additions to streamutil.h and<br></div><div>streamutil.=
c make it kind of natural to have it sit in pg_basebackup/.<br></div><di=
v>There's pg_recvlogical already there.&nbsp; I am wondering about two<b=
r></div><div>things, though:<br></div><div>- Should the subdirectory pg_=
basebackup be renamed into something more<br></div><div>generic at this =
point?&nbsp; All these things are frontend tools that deal<br></div><div=
>in some way with the replication protocol to do their work.&nbsp; Say<b=
r></div><div>a replication_tools?<br></div></blockquote><div><br></div><=
div>It is a good fit for this tool since it is another replication tool.=
 I also<br></div><div>agree with the directory renaming; it seems confus=
ing that the directory has<br></div><div>the same name as one binary but=
 also contains other related binaries in it.<br></div><div><br></div><bl=
ockquote type=3D"cite" id=3D"qt" style=3D""><div>- And if it would be be=
tter to refactor some of the code generic to<br></div><div>all these str=
eaming tools to fe_utils.&nbsp; What makes streamutil.h a bit<br></div><=
div>less pluggable are all its extern variables to control the connectio=
n,<br></div><div>but perhaps that can be an advantage, as well, in some =
cases.<br></div></blockquote><div><br></div><div>I like it. There are co=
mmon functions such as GetConnection(),<br></div><div>CreateReplicationS=
lot(), DropReplicationSlot() and RunIdentifySystem() that is<br></div><d=
iv>used by all of these replication tools. We can move the extern variab=
les into<br></div><div>parameters to have a pluggable streamutil.h.<br><=
/div><div><br></div><div><br></div><div id=3D"sig112409080"><div class=3D=
"signature">--<br></div><div class=3D"signature">Euler Taveira<br></div>=
<div class=3D"signature">EDB&nbsp;&nbsp; <a href=3D"https://www.enterpri=
sedb.com/">https://www.enterprisedb.com/</a><br></div></div><div><br></d=
iv></body></html>
--24687dd2a16d4f86912e2f0920b889ae--