From: Viktor Holmberg <v@viktorh.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: ISN extension - wrong volatility level for isn_weak() function
Date: Sun, 16 Mar 2025 21:33:21 +0000
Message-ID: <13b851cb-20c8-4a5a-9b7e-90b225635edd@Spark> (raw)
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>
Guess I shouldn’t have said it was easy considering all the edits needed hehe.
Thanks a lot for the help, fixes and push Tom! Wasn’t expecting such a lightning turnaround.
/Viktor Holmberg
On 16 Mar 2025 at 18:02 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
> Viktor Holmberg <v@viktorh.net> writes:
> > Oh, sorry! Don’t 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!
>
> Some notes:
>
> * To make g_weak actually act like a GUC, accept_weak_input() has
> to set it via set_config_option() 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_config option but decided not fixing it was more backwards compatible.
But can’t think of any reason why anyone wouldn’t want it working like a true GUC.
>
> * I concluded that the isn_weak() functions ought to be marked like
> set_config() (VOLATILE and PARALLEL UNSAFE) and current_setting()
> (STABLE and PARALLEL SAFE). 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_weak's value couldn't propagate into
> 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 "GUC"
> 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!
>
> regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: v@viktorh.net, tgl@sss.pgh.pa.us, pgsql-bugs@lists.postgresql.org
Subject: Re: ISN extension - wrong volatility level for isn_weak() function
In-Reply-To: <13b851cb-20c8-4a5a-9b7e-90b225635edd@Spark>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox