Date: Sun, 16 Mar 2025 21:33:21 +0000
From: Viktor Holmberg <v@viktorh.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
Message-ID: <13b851cb-20c8-4a5a-9b7e-90b225635edd@Spark>
In-Reply-To: <3768914.1742148108@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>
 <b698bb0e-766a-4255-a8c2-a1d14ef2a8f5@Spark>
 <3080651.1741969122@sss.pgh.pa.us>
 <742add6c-02ac-42e4-97a3-f2ebd1520d64@Spark>
 <3481904.1742055723@sss.pgh.pa.us>
 <b08ed3b6-eba7-47f5-b354-5e4ec1fb8632@Spark>
 <3768914.1742148108@sss.pgh.pa.us>
Subject: Re: ISN extension - wrong volatility level for isn_weak()
 function
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="67d743ac_22221a70_5a54"
Archived-At: 
 <https://www.postgresql.org/message-id/13b851cb-20c8-4a5a-9b7e-90b225635edd%40Spark>
Precedence: bulk

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

Guess I shouldn=E2=80=99t have said it was easy considering all the edits=
 needed hehe.

Thanks a lot for the help, fixes and push Tom=21 Wasn=E2=80=99t expecting=
 such a lightning turnaround.

/Viktor Holmberg
On 16 Mar 2025 at 18:02 +0000, Tom Lane <tgl=40sss.pgh.pa.us>, wrote:
> Viktor Holmberg <v=40viktorh.net> writes:
> > Oh, sorry=21 Don=E2=80=99t know what I was doing there - not used to =
this patch based workflow. Here comes the real patch.
> > This now uses the GUC - that was a lot easier than I thought.
>
> I reviewed this and pushed it with some corrections. Thanks for
> the contribution=21
>
> Some notes:
>
> * To make g=5Fweak actually act like a GUC, accept=5Fweak=5Finput() has=

> to set it via set=5Fconfig=5Foption() not just overwrite it. Otherwise
> it won't roll back on transaction abort, for example. There was also
> a missing MarkGUCPrefixReserved() call.
Makes sense. I actually thought about calling set=5Fconfig option but dec=
ided not fixing it was more backwards compatible.
But can=E2=80=99t think of any reason why anyone wouldn=E2=80=99t want it=
 working like a true GUC.
>
> * I concluded that the isn=5Fweak() functions ought to be marked like
> set=5Fconfig() (VOLATILE and PARALLEL UNSA=46E) and current=5Fsetting()=

> (STABLE and PARALLEL SA=46E). It's debatable whether a function that
> reacts to a GUC should really be STABLE, since you can surely make its
> output change intra-query if you try. However, we pretty consistently
> do that elsewhere because of the negative performance implications of
> marking all such functions VOLATILE. The previous PARALLEL RESTRICTED
> markings were probably okay when g=5Fweak's value couldn't propagate in=
to
> parallel workers, but they're wrong now.
Makes sense
>
> * You missed updating the module's meson.build file, which is hardly
> your fault since I pointed you at an example commit that predated
> our Meson support. But nowadays pretty much anything done to a
> Makefile has to be reflected in meson.build and vice versa.
>
> * I editorialized on the docs a bit, mostly to be more like existing
> practice in other contrib modules. One point there is that =22GUC=22
> is internal jargon that we prefer to avoid using in user-facing docs.
Agree you edits make it more clear.
>
> All in all though, pretty close for a first contribution.
> Thanks again=21
>
> regards, tom lane

--67d743ac_22221a70_5a54
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>Guess I shouldn=E2=80=99t have said it was easy con=
sidering all the edits needed hehe.<br />
<br />
Thanks a lot for the help, fixes and push Tom=21 Wasn=E2=80=99t expecting=
 such a lightning turnaround.</div>
</div>
<div name=3D=22messageSignatureSection=22><br />
<div class=3D=22match=46ont=22>/Viktor Holmberg</div>
</div>
<div name=3D=22messageReplySection=22>
<div dir=3D=22auto=22>On 16 Mar 2025 at 18:02 +0000, Tom Lane &lt;tgl=40s=
ss.pgh.pa.us&gt;, wrote:</div>
<blockquote style=3D=22border-left-color: rgb(26, 188, 156); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
>Viktor Holmberg &lt;v=40viktorh.net&gt; writes:<br />
<blockquote style=3D=22border-left-color: rgb(230, 126, 34); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
>Oh, sorry=21 Don=E2=80=99t know what I was doing there - not used to thi=
s patch based workflow. Here comes the real patch.<br />
This now uses the GUC - that was a lot easier than I thought.</blockquote=
>
<br />
I reviewed this and pushed it with some corrections. Thanks for<br />
the contribution=21<br />
<br />
Some notes:<br />
<br />
* To make g=5Fweak actually act like a GUC, accept=5Fweak=5Finput() has<b=
r />
to set it via set=5Fconfig=5Foption() not just overwrite it. Otherwise<br=
 />
it won't roll back on transaction abort, for example. There was also<br /=
>
a missing MarkGUCPrefixReserved() call.</blockquote>
<div dir=3D=22auto=22>Makes sense. I actually thought about calling set=5F=
config option but decided not fixing it was more backwards compatible.<br=
 />
But can=E2=80=99t think of any reason why anyone wouldn=E2=80=99t want it=
 working like a true GUC.</div>
<blockquote style=3D=22border-left-color: rgb(26, 188, 156); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
><br />
* I concluded that the isn=5Fweak() functions ought to be marked like<br =
/>
set=5Fconfig() (VOLATILE and PARALLEL UNSA=46E) and current=5Fsetting()<b=
r />
(STABLE and PARALLEL SA=46E). It's debatable whether a function that<br /=
>
reacts to a GUC should really be STABLE, since you can surely make its<br=
 />
output change intra-query if you try. However, we pretty consistently<br =
/>
do that elsewhere because of the negative performance implications of<br =
/>
marking all such functions VOLATILE. The previous PARALLEL RESTRICTED<br =
/>
markings were probably okay when g=5Fweak's value couldn't propagate into=
<br />
parallel workers, but they're wrong now.</blockquote>
<div dir=3D=22auto=22>Makes sense</div>
<blockquote style=3D=22border-left-color: rgb(26, 188, 156); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
><br />
* You missed updating the module's meson.build file, which is hardly<br /=
>
your fault since I pointed you at an example commit that predated<br />
our Meson support. But nowadays pretty much anything done to a<br />
Makefile has to be reflected in meson.build and vice versa.<br />
<br />
* I editorialized on the docs a bit, mostly to be more like existing<br /=
>
practice in other contrib modules. One point there is that =22GUC=22<br /=
>
is internal jargon that we prefer to avoid using in user-facing docs.</bl=
ockquote>
<div dir=3D=22auto=22>Agree you edits make it more clear.</div>
<blockquote style=3D=22border-left-color: rgb(26, 188, 156); margin: 5px;=
 padding-left: 10px; border-left-width: thin; border-left-style: solid;=22=
><br />
All in all though, pretty close for a first contribution.<br />
Thanks again=21<br />
<br />
regards, tom lane</blockquote>
</div>
</body>
</html>

--67d743ac_22221a70_5a54--