Feedback-ID: i0c21471d:Fastmail
MIME-Version: 1.0
Date: Mon, 12 Aug 2024 18:48:17 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: pgsql-hackers@lists.postgresql.org
Message-Id: <b9666b93-093b-4c9f-a385-772d6ab5688c@app.fastmail.com>
In-Reply-To: 
 <CAMPB6wfe4zLjJL8jiZV5kjjpwBM2=rTRme0UCL7Ra4L8MTVdOg@mail.gmail.com>
References: 
 <CAMPB6wfe4zLjJL8jiZV5kjjpwBM2=rTRme0UCL7Ra4L8MTVdOg@mail.gmail.com>
Subject: Re: [Patch] add new parameter to pg_replication_origin_session_setup
Content-Type: multipart/alternative;
 boundary=11f91cb14eb64e5b8382f64bd25c266f
Archived-At: <https://www.postgresql.org/message-id/b9666b93-093b-4c9f-a385-772d6ab5688c%40app.fastmail.com>
Precedence: bulk

--11f91cb14eb64e5b8382f64bd25c266f
Content-Type: text/plain
Content-Transfer-Encoding: 7bit

On Mon, Aug 12, 2024, at 3:43 PM, Doruk Yilmaz wrote:
> Hello all,

Hi!

> While working on our internal tools that utilise replication, we
> realised that a new parameter was added to the internal C function
> corresponding to pg_replication_origin_session_setup.
> However this parameter wasn't included in the user-facing API [1].

I'm curious about your use case. Is it just because the internal function has a
different signature or your tool is capable of apply logical replication changes
in parallel using the SQL API?

> I made this patch to the master which adds a way to control this
> parameter by adding a new version of the
> pg_replication_origin_session_setup function with user facing
> parameters 'text int4' in place of the current 'text' while keeping
> the existing variant
> (ensuring backwards compatibility). Could someone take a look at it?

I did a quick look at your patch and have a few suggestions.

* no documentation changes. Since the function you are changing has a new
signature, this change should be reflected in the documentation.
* no need for a new internal function. The second parameter (PID) can be
optional and defaults to 0 in this case. See how we changed the
pg_create_logical_replication_slot along the years add some IN parameters like
twophase and failover in the recent versions.
* add a CF entry [1] for this patch so we don't forget it. Another advantage is
that this patch is covered by CI [2][3].


[1] https://commitfest.postgresql.org/49/
[2] https://wiki.postgresql.org/wiki/Cfbot
[3] http://cfbot.cputube.org/index.html


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

--11f91cb14eb64e5b8382f64bd25c266f
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 Mon, Aug 12,=
 2024, at 3:43 PM, Doruk Yilmaz wrote:<br></div><blockquote type=3D"cite=
" id=3D"qt" style=3D""><div>Hello all,<br></div></blockquote><div><br></=
div><div>Hi!</div><div><br></div><blockquote type=3D"cite" id=3D"qt" sty=
le=3D""><div>While working on our internal tools that utilise replicatio=
n, we<br></div><div>realised that a new parameter was added to the inter=
nal C function<br></div><div>corresponding to pg_replication_origin_sess=
ion_setup.<br></div><div>However this parameter wasn't included in the u=
ser-facing API [1].<br></div></blockquote><div><br></div><div>I'm curiou=
s about your use case. Is it just because the internal function has a<br=
></div><div>different signature or your tool is capable of apply logical=
 replication changes<br></div><div>in parallel using the SQL API?<br></d=
iv><div><br></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div>I =
made this patch to the master which adds a way to control this<br></div>=
<div>parameter by adding a new version of the<br></div><div>pg_replicati=
on_origin_session_setup function with user facing<br></div><div>paramete=
rs 'text int4' in place of the current 'text' while keeping<br></div><di=
v>the existing variant<br></div><div>(ensuring backwards compatibility).=
 Could someone take a look at it?<br></div></blockquote><div><br></div><=
div>I did a quick look at your patch and have a few suggestions.<br></di=
v><div><br></div><div>* no documentation changes. Since the function you=
 are changing has a new<br></div><div>signature, this change should be r=
eflected in the documentation.<br></div><div>* no need for a new interna=
l function. The second parameter (PID) can be<br></div><div>optional and=
 defaults to 0 in this case. See how we changed the<br></div><div>pg_cre=
ate_logical_replication_slot along the years add some IN parameters like=
<br></div><div>twophase and failover in the recent versions.<br></div><d=
iv>* add a CF entry [1] for this patch so we don't forget it. Another ad=
vantage is<br></div><div>that this patch is covered by CI [2][3].<br></d=
iv><div><br></div><div><br></div><div>[1] <a href=3D"https://commitfest.=
postgresql.org/49/">https://commitfest.postgresql.org/49/</a><br></div><=
div>[2] <a href=3D"https://wiki.postgresql.org/wiki/Cfbot">https://wiki.=
postgresql.org/wiki/Cfbot</a><br></div><div>[3] <a href=3D"http://cfbot.=
cputube.org/index.html">http://cfbot.cputube.org/index.html</a><br></div=
><div><br></div><div><br></div><div id=3D"sig112409080"><div class=3D"si=
gnature">--<br></div><div class=3D"signature">Euler Taveira<br></div><di=
v class=3D"signature">EDB&nbsp;&nbsp; <a href=3D"https://www.enterprised=
b.com/">https://www.enterprisedb.com/</a><br></div></div><div><br></div>=
</body></html>
--11f91cb14eb64e5b8382f64bd25c266f--