Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. contrib/isn: Make weak mode a GUC setting, and fix related functions.

  2. Update contrib/seg for new scalarlesel/scalargesel selectivity functions.

  1. ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-14T11:49:06Z

    Hello. Apologies if this is not the right place to bug report extensions, but I couldn’t find a better place. An at the very least I’d like to save someone a day of debugging this issue.

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

BEGIN;

-- Prepare a statement for isn_weak():
PREPARE s_1 AS
 SELECT isn_weak() AS prepared_check;

-- First check - locks in the isn_weak value
EXECUTE s_1; -- => FALSE

-- Set isn_weak(true):
SELECT isn_weak(true) AS setting_true;

-- Check using normal query:
SELECT isn_weak() AS direct_check; -- => TRUE

EXECUTE s_1; -- => FALSE (!!!!)

This is because by default, that function is marked as immutable:

d6kpcv43m9du6> \df+ isn_weak
List of functions
─[ RECORD 1 ]───────┬──────────────────
Schema │ public
Name │ isn_weak
Result data type │ boolean
Argument data types │
Type │ func
Volatility │ immutable <————— bad
Parallel │ restricted
Owner │ postgres
Security │ invoker
Access privileges │
Language │ c
Internal name │ weak_input_status

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 explain this?

/Viktor Holmberg

  2. Re: ISN extension - wrong volatility level for isn_weak() function

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-14T13:29:55Z

    > 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 explain this?

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

--
Daniel Gustafsson

  3. Re: ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-14T13:58:25Z

    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;

/Viktor Holmberg
On 14 Mar 2025 at 13:30 +0000, Daniel Gustafsson <daniel@yesql.se>, wrote:
> > 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 explain this?
>
> I wonder if this should really be marked VOLATILE instead as it has a side
> effect.
>
> --
> Daniel Gustafsson
>

  4. Re: ISN extension - wrong volatility level for isn_weak() function

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-14T14:09:23Z

    > 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

  5. Re: ISN extension - wrong volatility level for isn_weak() function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-14T14:16:24Z

    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

  6. Re: ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-14T14:28:01Z

    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.
Still the feature is very useful as I work with many suppliers who give us product lists full of invalid EANs, which still need to be ingested.

/Viktor
On 14 Mar 2025 at 14:16 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
> 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

  7. Re: ISN extension - wrong volatility level for isn_weak() function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-14T15:13:49Z

    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

  8. Re: ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-14T16:08:36Z

    Thanks, I did check that thread during my desperate attempts to figure this bug out.
In terms of the ergonomics of is_valid I think it’s actually quite nice. A GUC variable would be very nice - I kinda assumed the ISN module was create before GUC, hence the somewhat idiosyncratic is_weak(bool) session level setting.

However, cleaning things up to use GUC seems like it’d be bigger task, and also would only be an extra thing, as isn_weak function would need to stay in for backwards compatibility I assume.

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

CREATE FUNCTION isn_weak()
	RETURNS boolean
	AS 'MODULE_PATHNAME', 'weak_input_status'
	LANGUAGE C
	IMMUTABLE STRICT <— should be VOLATILE STRICT
	PARALLEL RESTRICTED;

I believe even I could do this change, unless one of you pros would be open 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? Or does that patch go here? Thanks

/Viktor
On 14 Mar 2025 at 15:14 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
> 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

  9. Re: ISN extension - wrong volatility level for isn_weak() function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-14T16:18:42Z

    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_weak function 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

  10. Re: ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-15T15:32:39Z

    On 14 Mar 2025 at 16:18 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
> 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_weak function 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.
You’re right it’d definitely be much nicer. I’ll give it a go.
>
> > 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.
Ah, thanks for the clarification. I’ve attached a patch that fixes the volatility. I thought it best to at least get some feedback on that before I try to dust off my C knowledge and try to fix the GUC stuff.
>
> regards, tom lane

  11. Re: ISN extension - wrong volatility level for isn_weak() function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-15T16:22:03Z

    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 some feedback 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

  12. Re: ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-15T21:17:16Z

    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.
One thing I couldn’t figure out: Should I add some sort of change log somewhere, describing what changed in version 1.3 of the ISN extension? If so, where?

/Viktor Holmberg
On 15 Mar 2025 at 16:22 +0000, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
> 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 some feedback 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

  13. Re: ISN extension - wrong volatility level for isn_weak() function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-15T21:35:57Z

    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.3 of the ISN extension? If so, where?

We just use the git commit log for that.

			regards, tom lane

  14. Re: ISN extension - wrong volatility level for isn_weak() function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-16T18:01:48Z

    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

  15. Re: ISN extension - wrong volatility level for isn_weak() function

    Viktor Holmberg <v@viktorh.net> — 2025-03-16T21:33:21Z

    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