MIME-Version: 1.0
References: <19520-73873648d44793cf@postgresql.org>
 <CAJTYsWVqtWF+KJUdoKrzUM9QPQA5qD225AoeBeN7cwT4V1qd6A@mail.gmail.com>
 <ajCJw956W3xsl8XP@paquier.xyz> <ajIfUavZvZqep3EQ@paquier.xyz>
In-Reply-To: <ajIfUavZvZqep3EQ@paquier.xyz>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Wed, 17 Jun 2026 13:49:23 +0530
Message-ID: 
 <CAJTYsWWJEchycUwkt6=Hk3j3ROOJKVp2n5yaU7JNqwqOmyoEMA@mail.gmail.com>
Subject: Re: BUG #19520: PANIC when concurrently manipulating stored
 procedures with pg_stat_statements and track_functions =
To: Michael Paquier <michael@paquier.xyz>
Cc: zlh21343@163.com, pgsql-bugs@lists.postgresql.org,
	Andres Freund <andres@anarazel.de>, Sami Imseih <samimseih@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Content-Type: multipart/alternative; boundary="0000000000003c487906546eb823"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWWJEchycUwkt6%3DHk3j3ROOJKVp2n5yaU7JNqwqOmyoEMA%40mail.gmail.com>
Precedence: bulk

--0000000000003c487906546eb823
Content-Type: text/plain; charset="UTF-8"

Hi,

On Wed, 17 Jun 2026 at 09:45, Michael Paquier <michael@paquier.xyz> wrote:

> On Tue, Jun 16, 2026 at 08:24:51AM +0900, Michael Paquier wrote:
> > On Mon, Jun 15, 2026 at 02:44:06PM +0530, Ayush Tiwari wrote:
> >> I've added Andres and Michael on the thread, since they have worked on
> >> this in the past, for their input.
> >
> > Thanks for the poke.  I have marked this thread as something to look
> > at, but was not able to get back to it.  Will investigate..
>
> As far as I can see, pgss is not really a requirement.  Your case is
> taking advantage of the module introducing more slowness to enlarge
> the reproduction window.  Now saying that pgss being slow is a good
> thing, it's bad, but it helps here.  I've tried to reproduce in three
> environments, only my mac is able to get something, because it's
> slower I guess..
>

Yeah, you are right, pgss is not a requirement, it just
makes the delay broader.


> Attached is a script able to reproduce the issue in bash, courtesy of
> Claude because java and I sum up to a value very close to 0, see
> test_bug19520.txt.  The trick of the script is the same as your
> scenario, with two concurrent workloads:
> - One with DROP PROC/CREATE PROC/CALL.
> - One with CALL
>
> I had much more success after adding two sleeps to enlarge the
> conflict window, see also the sleep.patch attached, for reference.
>
> Finally attached is a patch, where I'd like to propose the
> introduction of a path in pgstat_drop_entry() to make the routine able
> to accept double drops.
>

I applied the patch on HEAD and ran my psql harness against it (~60
clients looping DROP / CREATE OR REPLACE / CALL, track_functions=all,
pgss loaded).  Unpatched it PANICs within seconds; with the patch it
stayed up for a ~3 minute run, with the out-of-band drop path firing
several thousand times.  So it clearly closes the hole here.


> The big comment within pgstat_init_function_usage() documents why it
> does its stuff for track_functions, so I was wondering if we should
> enforce the same double-drop-acceptance rule for all the callers
> everybody, but I also see a point in the correctness, by allowing the
> caller to complain if we try to do double drops but error on them,
> pointing to a programming error.  Note that
> pgstat_drop_entry_internal() is not touched on purpose, to keep the
> database-level scans as they are, with double-drops forbidden.
>
> This patch is very close to what Sami has posted on his PGSS thread,
> v3-0002, using a missing_ok instead of a skip_dropped:
>
> https://www.postgresql.org/message-id/CAA5RZ0uoxiQ2_=xHGRnyc4WdM9aR0fzdMhBubnw97po==--yGQ@mail.gmail.com
> I didn't suspect that we would need something like that for a
> backpatch, but well.
>
> I'm adding Sami in CC in case he wishes to comment on this patch, and
> Horiguchi-san as this area of the code concerns him.
>
> Thoughts or comments welcome.
>

A couple of things which I'm not clear about (these are not blockers
just questions for my understanding):

- With the check moved into the wrapper, pgstat_drop_entry_internal()
  still keeps its own "already dropped" elog().  Every path into
  _internal now seems to guarantee the entry isn't dropped, so
  _internal's copy looks unreachable after the patch
  ,and it's the one with the richer refcount/generation detail.  Was
  the idea to leave it as a backstop, or would folding the handling into
  one place (or making _internal's an Assert) be cleaner?

- In the missing_ok path the wrapper returns true, so the post-commit
  caller skips the not_freed_count++/GC request that a "real" not-freed
  drop would do.  That seems harmless since the entry self-heals
  but was returning true there a deliberate choice over mirroring
  the not-freed/false path? I need to take a look again at this, maybe
  I missed something.

Regards,
Ayush

--0000000000003c487906546eb823
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Wed, 17 Jun 2=
026 at 09:45, Michael Paquier &lt;<a href=3D"mailto:michael@paquier.xyz">mi=
chael@paquier.xyz</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote"=
 style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);p=
adding-left:1ex">On Tue, Jun 16, 2026 at 08:24:51AM +0900, Michael Paquier =
wrote:<br>
&gt; On Mon, Jun 15, 2026 at 02:44:06PM +0530, Ayush Tiwari wrote:<br>
&gt;&gt; I&#39;ve added Andres and Michael on the thread, since they have w=
orked on<br>
&gt;&gt; this in the past, for their input.<br>
&gt; <br>
&gt; Thanks for the poke.=C2=A0 I have marked this thread as something to l=
ook<br>
&gt; at, but was not able to get back to it.=C2=A0 Will investigate..<br>
<br>
As far as I can see, pgss is not really a requirement.=C2=A0 Your case is<b=
r>
taking advantage of the module introducing more slowness to enlarge<br>
the reproduction window.=C2=A0 Now saying that pgss being slow is a good<br=
>
thing, it&#39;s bad, but it helps here.=C2=A0 I&#39;ve tried to reproduce i=
n three<br>
environments, only my mac is able to get something, because it&#39;s<br>
slower I guess..<br></blockquote><div><br></div><div>Yeah, you are right, p=
gss is not a requirement, it just</div><div>makes the delay broader.</div><=
div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0=
px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Attached is a script able to reproduce the issue in bash, courtesy of<br>
Claude because java and I sum up to a value very close to 0, see<br>
test_bug19520.txt.=C2=A0 The trick of the script is the same as your<br>
scenario, with two concurrent workloads:<br>
- One with DROP PROC/CREATE PROC/CALL.<br>
- One with CALL<br>
<br>
I had much more success after adding two sleeps to enlarge the<br>
conflict window, see also the sleep.patch attached, for reference.<br>
<br>
Finally attached is a patch, where I&#39;d like to propose the<br>
introduction of a path in pgstat_drop_entry() to make the routine able<br>
to accept double drops.<br></blockquote><div>=C2=A0</div><div>I applied the=
 patch on HEAD and ran my psql harness against it (~60<br>clients looping D=
ROP / CREATE OR REPLACE / CALL, track_functions=3Dall,<br>pgss loaded).=C2=
=A0 Unpatched it PANICs within seconds; with the patch it<br>stayed up for =
a ~3 minute run, with the out-of-band drop path firing<br>several thousand =
times.=C2=A0 So it clearly closes the hole here.</div><div>=C2=A0</div><blo=
ckquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left=
:1px solid rgb(204,204,204);padding-left:1ex">
The big comment within pgstat_init_function_usage() documents why it<br>
does its stuff for track_functions, so I was wondering if we should<br>
enforce the same double-drop-acceptance rule for all the callers<br>
everybody, but I also see a point in the correctness, by allowing the<br>
caller to complain if we try to do double drops but error on them,<br>
pointing to a programming error.=C2=A0 Note that<br>
pgstat_drop_entry_internal() is not touched on purpose, to keep the<br>
database-level scans as they are, with double-drops forbidden.<br>
<br>
This patch is very close to what Sami has posted on his PGSS thread,<br>
v3-0002, using a missing_ok instead of a skip_dropped:<br>
<a href=3D"https://www.postgresql.org/message-id/CAA5RZ0uoxiQ2_=3DxHGRnyc4W=
dM9aR0fzdMhBubnw97po=3D=3D--yGQ@mail.gmail.com" rel=3D"noreferrer" target=
=3D"_blank">https://www.postgresql.org/message-id/CAA5RZ0uoxiQ2_=3DxHGRnyc4=
WdM9aR0fzdMhBubnw97po=3D=3D--yGQ@mail.gmail.com</a><br>
I didn&#39;t suspect that we would need something like that for a<br>
backpatch, but well.<br>
<br>
I&#39;m adding Sami in CC in case he wishes to comment on this patch, and<b=
r>
Horiguchi-san as this area of the code concerns him.<br>
<br>
Thoughts or comments welcome.<br></blockquote><div><br></div>A couple of th=
ings which I&#39;m not clear about (these are not blockers</div><div class=
=3D"gmail_quote gmail_quote_container">just questions for my understanding)=
:<br><br>- With the check moved into the wrapper, pgstat_drop_entry_interna=
l()<br>=C2=A0 still keeps its own &quot;already dropped&quot; elog().=C2=A0=
 Every path into<br>=C2=A0 _internal now seems to guarantee the entry isn&#=
39;t dropped, so <br>=C2=A0 _internal&#39;s copy looks unreachable after th=
e patch<br>=C2=A0 ,and it&#39;s the one with the richer refcount/generation=
 detail.=C2=A0 Was<br>=C2=A0 the idea to leave it as a backstop, or would f=
olding the handling into<br>=C2=A0 one place (or making _internal&#39;s an =
Assert) be cleaner?<br><br>- In the missing_ok path the wrapper returns tru=
e, so the post-commit<br>=C2=A0 caller skips the not_freed_count++/GC reque=
st that a &quot;real&quot; not-freed<br>=C2=A0 drop would do.=C2=A0 That se=
ems harmless since the entry self-heals <br>=C2=A0 but was returning true t=
here a deliberate choice over mirroring <br>=C2=A0 the not-freed/false path=
? I need to take a look again at this, maybe<br>=C2=A0 I missed something.<=
br><br>Regards,<br>Ayush</div></div>

--0000000000003c487906546eb823--