Feedback-ID: i0c21471d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-236-g06c0f70e43-fm-20230313.001-g06c0f70e
Mime-Version: 1.0
Message-Id: <c42d5634-ca9b-49a7-85cd-9eff9feb33b4@app.fastmail.com>
In-Reply-To: 
 <CALj2ACXvqFcxro64q4gjDeBNLgyPH0E6bG-WJ6w7qBqbggj3=w@mail.gmail.com>
References: 
 <CAA4eK1+CoHVhP6hs+5-_1_A9EymSyqDPzHxBs1jSrTm5fj6CvQ@mail.gmail.com>
 <20230323100726.vzrgvpjt3il7uemq@alvherre.pgsql>
 <CALj2ACXvqFcxro64q4gjDeBNLgyPH0E6bG-WJ6w7qBqbggj3=w@mail.gmail.com>
Date: Thu, 23 Mar 2023 18:40:26 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com>,
 "Alvaro Herrera" <alvherre@alvh.no-ip.org>
Cc: "Amit Kapila" <amit.kapila16@gmail.com>,
 "Kyotaro Horiguchi" <horikyota.ntt@gmail.com>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Subject: Re: add log messages when replication slots become active and inactive (was
 Re: Is it worth adding ReplicationSlot active_pid to
 ReplicationSlotPersistentData?)
Content-Type: multipart/alternative;
 boundary=f21ffa602bcc4ef2ae83cc8286ab1b18
Archived-At: <https://www.postgresql.org/message-id/c42d5634-ca9b-49a7-85cd-9eff9feb33b4%40app.fastmail.com>
Precedence: bulk

--f21ffa602bcc4ef2ae83cc8286ab1b18
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 23, 2023, at 2:33 PM, Bharath Rupireddy wrote:
> On Thu, Mar 23, 2023 at 3:37=E2=80=AFPM Alvaro Herrera <alvherre@alvh.=
no-ip.org> wrote:
> >
> > On 2023-Mar-22, Amit Kapila wrote:
> >
> > > I see that you have modified the patch to address the comments from
> > > Alvaro. Personally, I feel it would be better to add such a messag=
e at
> > > a centralized location instead of spreading these in different cal=
lers
> > > of slot acquire/release functionality to avoid getting these misse=
d in
> > > the new callers in the future. However, if Alvaro and others think
> > > that the current style is better then we should go ahead and do it
> > > that way. I hope that we should be able to decide on this and get =
it
> > > into PG16. Anyone else would like to weigh in here?
> >
> > I like Peter Smith's suggestion downthread.
>=20
> +1. Please review the attached v8 patch further.
If you are adding separate functions as suggested, you should add a comm=
ent at
the top of ReplicationSlotAcquire() and ReplicationSlotRelease() functio=
ns
saying that LogReplicationSlotAquired() and LogReplicationSlotReleased()
functions should be called  respectively after it.

My suggestion is that the functions should have the same name with a "Lo=
g"
prefix. On of them has a typo "Aquired" in its name. Hence,
LogReplicationSlotAcquire() and LogReplicationSlotRelease() as names. It=
 is
easier to find if someone is grepping by the origin function.

I prefer a sentence that includes a verb.

  physical replication slot \"%s\" is acquired
  logical replication slot \"%s\" is released

Isn't the PID important for this use case? If so, of course, you can rel=
y on
log_line_prefix (%p) but if the PID is crucial for an investigation then=
 it
should also be included in the message.


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

--f21ffa602bcc4ef2ae83cc8286ab1b18
Content-Type: text/html;charset=utf-8
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 Thu, Mar 23,=
 2023, at 2:33 PM, Bharath Rupireddy wrote:<br></div><blockquote type=3D=
"cite" id=3D"qt" style=3D""><div>On Thu, Mar 23, 2023 at 3:37=E2=80=AFPM=
 Alvaro Herrera &lt;<a href=3D"mailto:alvherre@alvh.no-ip.org">alvherre@=
alvh.no-ip.org</a>&gt; wrote:<br></div><div>&gt;<br></div><div>&gt; On 2=
023-Mar-22, Amit Kapila wrote:<br></div><div>&gt;<br></div><div>&gt; &gt=
; I see that you have modified the patch to address the comments from<br=
></div><div>&gt; &gt; Alvaro. Personally, I feel it would be better to a=
dd such a message at<br></div><div>&gt; &gt; a centralized location inst=
ead of spreading these in different callers<br></div><div>&gt; &gt; of s=
lot acquire/release functionality to avoid getting these missed in<br></=
div><div>&gt; &gt; the new callers in the future. However, if Alvaro and=
 others think<br></div><div>&gt; &gt; that the current style is better t=
hen we should go ahead and do it<br></div><div>&gt; &gt; that way. I hop=
e that we should be able to decide on this and get it<br></div><div>&gt;=
 &gt; into PG16. Anyone else would like to weigh in here?<br></div><div>=
&gt;<br></div><div>&gt; I like Peter Smith's suggestion downthread.<br><=
/div><div><br></div><div>+1. Please review the attached v8 patch further=
.<br></div></blockquote><div>If you are adding separate functions as sug=
gested, you should add a comment at<br></div><div>the top of Replication=
SlotAcquire() and ReplicationSlotRelease() functions<br></div><div>sayin=
g that LogReplicationSlotAquired() and LogReplicationSlotReleased()<br><=
/div><div>functions should be called&nbsp; respectively after it.<br></d=
iv><div><br></div><div>My suggestion is that the functions should have t=
he same name with a "Log"<br></div><div>prefix. On of them has a typo "A=
quired" in its name. Hence,<br></div><div>LogReplicationSlotAcquire() an=
d LogReplicationSlotRelease() as names. It is<br></div><div>easier to fi=
nd if someone is grepping by the origin function.<br></div><div><br></di=
v><div>I prefer a sentence that includes a verb.<br></div><div><br></div=
><div>&nbsp; physical replication slot \"%s\" is acquired<br></div><div>=
&nbsp; logical replication slot \"%s\" is released<br></div><div><br></d=
iv><div>Isn't the PID important for this use case? If so, of course, you=
 can rely on<br></div><div>log_line_prefix (%p) but if the PID is crucia=
l for an investigation then it<br></div><div>should also be included in =
the message.<br></div><div><br></div><div><br></div><div id=3D"sig112409=
080"><div class=3D"signature">--<br></div><div class=3D"signature">Euler=
 Taveira<br></div><div class=3D"signature">EDB&nbsp;&nbsp; <a href=3D"ht=
tps://www.enterprisedb.com/">https://www.enterprisedb.com/</a><br></div>=
</div><div><br></div></body></html>
--f21ffa602bcc4ef2ae83cc8286ab1b18--