Thread: ISN extension - wrong volatility level for isn_weak() function

> On 14 Mar 2025, at 12:49, Viktor Holmberg <v@viktorh.net> wrote:
>
> Hello. Apologies if this is not the right place to bug report extensions

For an extension bundled in postgres contrib it's absolutely the right place.

> The isn_weak function in the isn extension reports the wrong value if you look at it inside a transaction:

> I can manually fix this by changing it to STABLE:
>
> ALTER FUNCTION isn_weak() STABLE;
>
> Am I missing something or isn’t this quite weird? Would it at least be possible to change the documentation to
explainthis? 

I wonder if this should really be marked VOLATILE instead as it has a side
effect.

--
Daniel Gustafsson

> On 14 Mar 2025, at 14:58, Viktor Holmberg <v@viktorh.net> wrote:
>
> Yes, you’re right Daniel. I ended up settling on the following workaround:
>
> ALTER FUNCTION isn_weak() VOLATILE;
> ALTER FUNCTION public.isn_weak(boolean) VOLATILE;

Thanks for confirming.  This would need a backpatch into all supported versions
it seems.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
> On 14 Mar 2025, at 12:49, Viktor Holmberg <v@viktorh.net> wrote:
>> The isn_weak function in the isn extension reports the wrong value if you look at it inside a transaction:

> I wonder if this should really be marked VOLATILE instead as it has a side
> effect.

Indeed.  This whole area seems really poorly considered.  The comment
in isn--1.1.sql says

-- isn_weak(boolean) - Sets the weak input mode.
-- This function is intended for testing use only!

despite which it's documented at length in isn.sgml.  On the other
hand, so far as I can find it's tested nowhere, which means none
of the "weak mode" code is getting exercised.

            regards, tom lane

Viktor Holmberg <v@viktorh.net> writes:
> I haven’t checked the source code, but yes the isn_weak feature has some footgun potential. As it doesn’t respect
transactions,but rather sets a flag on session level, it’s easy for the “isn weakness” 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/C12AE0A2A752416C79F3EE81%40teje

but we don't seem to have pulled the trigger on changing anything.

            regards, tom lane

Viktor Holmberg <v@viktorh.net> writes:
> However, cleaning things up to use GUC seems like it’d be bigger task, and also would only be an extra thing, as
isn_weakfunction would need to stay in for backwards compatibility I assume. 

Wouldn't be a big deal --- yes, accept_weak_input would need a bit of
modification, but it's not much.  The main reason I suggested it was
that a GUC would be subject to RESET ALL and so it'd fix the pooler
hazard you pointed out.

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

No, we'd need to create an update script that uses ALTER FUNCTION.
Extension scripts are basically frozen once shipped.

            regards, tom lane

Viktor Holmberg <v@viktorh.net> writes:
> On 14 Mar 2025 at 16:18 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
>> No, we'd need to create an update script that uses ALTER FUNCTION.
>> Extension scripts are basically frozen once shipped.

> Ah, thanks for the clarification. I’ve attached a patch that fixes the volatility. I thought it best to at least get
somefeedback on that before I try to dust off my C knowledge and try to fix the GUC stuff. 

Uh .. looks like you attached a patch for something else altogether.

If you want a sample extension-updating patch to look at, you
could see 44ba2920644903d7dfceda810e5facdbcbab58a8, or lots of
other examples.

            regards, tom lane

Viktor Holmberg <v@viktorh.net> writes:
> One thing I couldn’t figure out: Should I add some sort of change log somewhere, describing what changed in version
1.3of the ISN extension? If so, where? 

We just use the git commit log for that.

            regards, tom lane

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.

* 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.

* 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.

All in all though, pretty close for a first contribution.
Thanks again!

            regards, tom lane