Date: Fri, 14 Mar 2025 16:08:36 +0000
From: Viktor Holmberg <v@viktorh.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Daniel Gustafsson <daniel@yesql.se>,
 pgsql-bugs@lists.postgresql.org
Message-ID: <b698bb0e-766a-4255-a8c2-a1d14ef2a8f5@Spark>
In-Reply-To: <3074065.1741965229@sss.pgh.pa.us>
References: <ae6ed432-4491-4030-be20-3ea69d03a098@Spark>
 <790bc1f9-74dc-4b50-94d2-8147315b1556@Spark>
 <51C655A4-46FB-4A40-BAE5-26A76B31D3EE@yesql.se>
 <3052531.1741961784@sss.pgh.pa.us>
 <de89bd5c-9f51-4891-8a38-d03ed06ebd1d@Spark>
 <3074065.1741965229@sss.pgh.pa.us>
Subject: Re: ISN extension - wrong volatility level for isn_weak()
 function
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="67d4548e_1f48eaa1_1881"
Archived-At: 
 <https://www.postgresql.org/message-id/b698bb0e-766a-4255-a8c2-a1d14ef2a8f5%40Spark>
Precedence: bulk

--67d4548e_1f48eaa1_1881
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

Thanks, I did check that thread during my desperate attempts to figure th=
is bug out.
In terms of the ergonomics of is=5Fvalid I think it=E2=80=99s actually qu=
ite nice. A GUC variable would be very nice - I kinda assumed the ISN mod=
ule was create before GUC, hence the somewhat idiosyncratic is=5Fweak(boo=
l) session level setting.

However, cleaning things up to use GUC seems like it=E2=80=99d be bigger =
task, and also would only be an extra thing, as isn=5Fweak function would=
 need to stay in for backwards compatibility I assume.

In terms of just fixing the immediate bug, I believe it=E2=80=99d just be=
 to change isn.sql line 3423 and 2433:

CREATE =46UNCTION isn=5Fweak()
	RETURNS boolean
	AS 'MODULE=5FPATHNAME', 'weak=5Finput=5Fstatus'
	LANGUAGE C
	IMMUTABLE STRICT <=E2=80=94 should be VOLATILE STRICT
	PARALLEL RESTRICTED;

I believe even I could do this change, unless one of you pros would be op=
en to doing it.
Am I right in understanding the next steps to do that would be to create =
a patch, and email it to pgsql-hackers=3F Or does that patch go here=3F T=
hanks

/Viktor
On 14 Mar 2025 at 15:14 +0000, Tom Lane <tgl=40sss.pgh.pa.us>, wrote:
> Viktor Holmberg <v=40viktorh.net> writes:
> > I haven=E2=80=99t checked the source code, but yes the isn=5Fweak fea=
ture has some footgun potential. As it doesn=E2=80=99t respect transactio=
ns, but rather sets a flag on session level, it=E2=80=99s easy for the =E2=
=80=9Cisn weakness=E2=80=9D to leak out into a connection pool.
>
> Yeah, really that ought to be a GUC I should think. There isn't
> anything well-designed about it ...
>
> I found some prior discussion here:
>
> https://www.postgresql.org/message-id/flat/C12AE0A2A752416C79=463EE81%4=
0teje
>
> but we don't seem to have pulled the trigger on changing anything.
>
> regards, tom lane

--67d4548e_1f48eaa1_1881
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

<html xmlns=3D=22http://www.w3.org/1999/xhtml=22>
<head>
<title></title>
</head>
<body>
<div name=3D=22messageBodySection=22>
<div dir=3D=22auto=22><span style=3D=22font-family:Helvetica=22>Thanks, I=
 did check that thread during my desperate attempts to figure this bug ou=
t.</span><br />
<span style=3D=22font-family:Helvetica=22>In terms of the ergonomics of i=
s=5Fvalid I think it=E2=80=99s actually quite nice. A GUC variable would =
be very nice - I kinda assumed the ISN module was create before GUC, henc=
e the somewhat idiosyncratic is=5Fweak(bool) session level setting.</span=
><br />
<br />
<span style=3D=22font-family:Helvetica=22>However, cleaning things up to =
use GUC seems like it=E2=80=99d be bigger task, and also would only be an=
 extra thing, as isn=5Fweak function would need to stay in for backwards =
compatibility I assume.</span><br />
<br />
<span style=3D=22font-family:Helvetica=22>In terms of just fixing the imm=
ediate bug, I believe it=E2=80=99d just be to change isn.sql line 3423 an=
d 2433:</span><br />
<br />
CREATE =46UNCTION isn=5Fweak()<br />
<span style=3D=22white-space:pre=22>	RETURNS boolean</span><br />
<span style=3D=22white-space:pre=22>	AS 'MODULE=5FPATHNAME', 'weak=5Finpu=
t=5Fstatus'</span><br />
<span style=3D=22white-space:pre=22>	LANGUAGE C</span><br />
<span style=3D=22white-space:pre=22>	IMMUTABLE STRICT &lt;=E2=80=94 shoul=
d be VOLATILE STRICT</span><br />
<span style=3D=22white-space:pre=22>	PARALLEL RESTRICTED;</span><br />
<br />
I believe even I could do this change, unless one of you pros would be op=
en to doing it.&=23160;<br />
Am I right in understanding the next steps to do that would be to create =
a patch, and email it to pgsql-hackers=3F Or does that patch go here=3F T=
hanks&=23160;</div>
</div>
<div name=3D=22messageSignatureSection=22><br />
<div class=3D=22match=46ont=22>/Viktor</div>
</div>
<div name=3D=22messageReplySection=22>On 14 Mar 2025 at 15:14 +0000, Tom =
Lane &lt;tgl=40sss.pgh.pa.us&gt;, wrote:<br />
<blockquote type=3D=22cite=22 style=3D=22border-left-color: grey; border-=
left-width: thin; border-left-style: solid; margin: 5px 5px;padding-left:=
 10px;=22>Viktor Holmberg &lt;v=40viktorh.net&gt; writes:<br />
<blockquote type=3D=22cite=22>I haven=E2=80=99t checked the source code, =
but yes the isn=5Fweak feature has some footgun potential. As it doesn=E2=
=80=99t respect transactions, but rather sets a flag on session level, it=
=E2=80=99s easy for the =E2=80=9Cisn weakness=E2=80=9D to leak out into a=
 connection pool.<br /></blockquote>
<br />
Yeah, really that ought to be a GUC I should think. There isn't<br />
anything well-designed about it ...<br />
<br />
I found some prior discussion here:<br />
<br />
https://www.postgresql.org/message-id/flat/C12AE0A2A752416C79=463EE81%40t=
eje<br />
<br />
but we don't seem to have pulled the trigger on changing anything.<br />
<br />
regards, tom lane<br /></blockquote>
</div>
</body>
</html>

--67d4548e_1f48eaa1_1881--