MIME-Version: 1.0
References: <463a28db-0c0b-4af6-bac6-3891828bbbfe@iki.fi>
In-Reply-To: <463a28db-0c0b-4af6-bac6-3891828bbbfe@iki.fi>
From: Sami Imseih <samimseih@gmail.com>
Date: Sun, 5 Apr 2026 11:05:14 -0500
Message-ID: 
 <CAA5RZ0u+=t0HO_wBWnw-fB=AAoriyHTO0=rH3T+GFgsLM1Ukqw@mail.gmail.com>
Subject: Re: Duplicate RequestNamedLWLocktranche() names and
 test_lwlock_tranches improvements
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Nathan Bossart <nathandbossart@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000de9891064eb8b71e"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA5RZ0u%2B%3Dt0HO_wBWnw-fB%3DAAoriyHTO0%3DrH3T%2BGFgsLM1Ukqw%40mail.gmail.com>
Precedence: bulk

--000000000000de9891064eb8b71e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

> Starting new thread for this thing that Matthias noticed in my
> work-in-progress patch at
>
> https://www.postgresql.org/message-id/CAEze2WjgCROMMXY0+j8FFdm3iFcr7By-+6=
Mwiz=3DPgGSEydiW3A@mail.gmail.com
> .
>
> On 05/04/2026 02:17, Matthias van de Meent wrote:
> > 0006: I don't think it is a great idea to make the LwLock machinery
> > the first to get allocation requests:
> > It has the RequestNamedLWLockTranche infrastructure, which can only
> > register new requests while process_shmem_requests_in_progress, and
> > making it request its memory ahead of everything else is likely to
> > cause an undersized tranche to be allocated. You could make sure that
> > this isn't an issue by maintaining a flag in lwlock.c that's set when
> > the shmem request is made (and reset on shmem exit), which must be
> > false when RequestNamedLWLockTranche() is called, and if not then it
> > should throw an error.
>
> Good catch, RequestNamedLWLocktranche() was quite broken with the patch.
> I'm surprised it didn't cause test failures. We even have unit tests for
> that at src/test/modules/test_lwlock_tranches.
>
> Looking at src/test/modules/test_lwlock_tranches, I realized that we
> don't currently check that the tranche name registered with
> RequestNamedLWLocktranche() is unique. If two extensions registered a
> tranche with same name, we'd allocate two separate tranches for them,
> but GetNamedLWLockTranche() would always return the first one.


Yes, while that is not very likely scenario, it=E2=80=99s wrong. The caller=
 will
get the wrong pointer to the locks.

>
>
> Attached patches add a uniqueness check, and improves
> test_lwlock_tranches so that it actually uses the requested LWLocks. And
> I couldn't resist doing some more refactoring of the test while I was at
> it; IMO it's more readable now.
>
> Barring objections, I will commit these shortly.
>

LGTM

--
Sami Imseih
Amazon Web Services (AWS)

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

<div dir=3D"auto"><br></div><div><div class=3D"gmail_quote gmail_quote_cont=
ainer"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;=
border-left-width:1px;border-left-style:solid;padding-left:1ex;border-left-=
color:rgb(204,204,204)">Starting new thread for this thing that Matthias no=
ticed in my <br>
work-in-progress patch at <br>
<a href=3D"https://www.postgresql.org/message-id/CAEze2WjgCROMMXY0+j8FFdm3i=
Fcr7By-+6Mwiz=3DPgGSEydiW3A@mail.gmail.com" rel=3D"noreferrer" target=3D"_b=
lank">https://www.postgresql.org/message-id/CAEze2WjgCROMMXY0+j8FFdm3iFcr7B=
y-+6Mwiz=3DPgGSEydiW3A@mail.gmail.com</a>.<br>
<br>
On 05/04/2026 02:17, Matthias van de Meent wrote:<br>
&gt; 0006: I don&#39;t think it is a great idea to make the LwLock machiner=
y<br>
&gt; the first to get allocation requests:<br>
&gt; It has the RequestNamedLWLockTranche infrastructure, which can only<br=
>
&gt; register new requests while process_shmem_requests_in_progress, and<br=
>
&gt; making it request its memory ahead of everything else is likely to<br>
&gt; cause an undersized tranche to be allocated. You could make sure that<=
br>
&gt; this isn&#39;t an issue by maintaining a flag in lwlock.c that&#39;s s=
et when<br>
&gt; the shmem request is made (and reset on shmem exit), which must be<br>
&gt; false when RequestNamedLWLockTranche() is called, and if not then it<b=
r>
&gt; should throw an error.<br>
<br>
Good catch, RequestNamedLWLocktranche() was quite broken with the patch. <b=
r>
I&#39;m surprised it didn&#39;t cause test failures. We even have unit test=
s for <br>
that at src/test/modules/test_lwlock_tranches.<br>
<br>
Looking at src/test/modules/test_lwlock_tranches, I realized that we <br>
don&#39;t currently check that the tranche name registered with <br>
RequestNamedLWLocktranche() is unique. If two extensions registered a <br>
tranche with same name, we&#39;d allocate two separate tranches for them, <=
br>
but GetNamedLWLockTranche() would always return the first one.</blockquote>=
<div dir=3D"auto"><br></div><div dir=3D"auto">Yes, while that is not very l=
ikely scenario, it=E2=80=99s wrong. The caller will get the wrong pointer t=
o the locks.</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px=
 0px 0.8ex;border-left-width:1px;border-left-style:solid;padding-left:1ex;b=
order-left-color:rgb(204,204,204)" dir=3D"auto"><br>
<br>
Attached patches add a uniqueness check, and improves <br>
test_lwlock_tranches so that it actually uses the requested LWLocks. And <b=
r>
I couldn&#39;t resist doing some more refactoring of the test while I was a=
t <br>
it; IMO it&#39;s more readable now.<br>
<br>
Barring objections, I will commit these shortly.<br></blockquote><div dir=
=3D"auto"><br>LGTM</div><div dir=3D"auto"><br></div><div dir=3D"auto"><div =
style=3D"font-size:inherit"><div style=3D"font-size:inherit;font-style:norm=
al;font-weight:400;letter-spacing:normal;text-indent:0px;text-transform:non=
e;white-space:normal;word-spacing:0px;background-color:rgba(0,0,0,0);border=
-color:rgb(0,0,0);color:rgb(0,0,0)"><div style=3D"background-color:rgba(0,0=
,0,0);border-color:rgb(0,0,0);color:rgb(0,0,0)"><div dir=3D"ltr" data-smart=
mail=3D"gmail_signature" style=3D"background-color:rgba(0,0,0,0);border-col=
or:rgb(0,0,0);color:rgb(0,0,0)"><div dir=3D"ltr" style=3D"background-color:=
rgba(0,0,0,0);border-color:rgb(0,0,0);color:rgb(0,0,0)"><div style=3D"backg=
round-color:rgba(0,0,0,0);border-color:rgb(0,0,0);color:rgb(0,0,0)">--</div=
><div style=3D"background-color:rgba(0,0,0,0);border-color:rgb(0,0,0);color=
:rgb(0,0,0)">Sami Imseih</div><div style=3D"background-color:rgba(0,0,0,0);=
border-color:rgb(0,0,0);color:rgb(0,0,0)" dir=3D"auto">Amazon Web Services =
(AWS)</div></div></div></div></div></div></div><div dir=3D"auto"><br></div>=
<div dir=3D"auto"><br></div><div dir=3D"auto"><br></div></div></div>

--000000000000de9891064eb8b71e--