pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
ISN extension - wrong volatility level for isn_weak() function
15+ messages / 3 participants
[nested] [flat]

Commits from this thread:
  2025-03-16 4489044239  contrib/isn: Make weak mode a GUC setting, and fix related functions.
* ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 11:49  Viktor Holmberg <v@viktorh.net>
  0 siblings, 1 reply; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-14 11:49 UTC (permalink / raw)
  To: pgsql-bugs@lists.postgresql.org

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

^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 13:29  Daniel Gustafsson <daniel@yesql.se>
  parent: Viktor Holmberg <v@viktorh.net>
  0 siblings, 2 replies; 15+ messages in thread

From: Daniel Gustafsson @ 2025-03-14 13:29 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: pgsql-bugs@lists.postgresql.org

> 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






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 13:58  Viktor Holmberg <v@viktorh.net>
  parent: Daniel Gustafsson <daniel@yesql.se>
  1 sibling, 1 reply; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-14 13:58 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-bugs@lists.postgresql.org

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
>

^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 14:09  Daniel Gustafsson <daniel@yesql.se>
  parent: Viktor Holmberg <v@viktorh.net>
  0 siblings, 0 replies; 15+ messages in thread

From: Daniel Gustafsson @ 2025-03-14 14:09 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: pgsql-bugs@lists.postgresql.org

> 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






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 14:16  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Daniel Gustafsson <daniel@yesql.se>
  1 sibling, 1 reply; 15+ messages in thread

From: Tom Lane @ 2025-03-14 14:16 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Viktor Holmberg <v@viktorh.net>; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 14:28  Viktor Holmberg <v@viktorh.net>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-14 14:28 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

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

^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 15:13  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Viktor Holmberg <v@viktorh.net>
  0 siblings, 1 reply; 15+ messages in thread

From: Tom Lane @ 2025-03-14 15:13 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 16:08  Viktor Holmberg <v@viktorh.net>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-14 16:08 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org

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

^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-14 16:18  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Viktor Holmberg <v@viktorh.net>
  0 siblings, 1 reply; 15+ messages in thread

From: Tom Lane @ 2025-03-14 16:18 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-15 15:32  Viktor Holmberg <v@viktorh.net>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-15 15:32 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org

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

Attachments:

v10-0002-Allow-Generic-Type-Subscripting-to-Accept-Dot-No.patch (application/octet-stream, 10.3K) download From 050703ccb4e1786710722b451eabcf1a908cd763 Mon Sep 17 00:00:00 2001 From: Nikita Glukhov <n.gluhov@postgrespro.ru> Date: Sat, 1 Apr 2023 16:21:20 +0300 Subject: [PATCH v10 2/7] Allow Generic Type Subscripting to Accept Dot Notation (.) as Input This change extends generic type subscripting to recognize dot notation (.) in addition to bracket notation ([]). While this does not yet provide full support for dot notation, it enables subscripting containers to process it in the future. For now, container-specific transform functions only handle subscripting indices and stop processing when encountering dot notation. It is up to individual containers to decide how to transform dot notation in subsequent updates. --- src/backend/parser/parse_expr.c | 66 ++++++++++++++++++++----------- src/backend/parser/parse_node.c | 41 +++++++++++++++++-- src/backend/parser/parse_target.c | 3 +- src/backend/utils/adt/arraysubs.c | 13 ++++-- src/backend/utils/adt/jsonbsubs.c | 11 +++++- src/include/parser/parse_node.h | 3 +- 6 files changed, 105 insertions(+), 32 deletions(-) diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 2c0f4a50b21..8ea51176196 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -442,8 +442,9 @@ transformIndirection(ParseState *pstate, A_Indirection *ind) ListCell *i; /* - * We have to split any field-selection operations apart from - * subscripting. Adjacent A_Indices nodes have to be treated as a single + * Combine field names and subscripts into a single indirection list, as + * some subscripting containers, such as jsonb, support field access using + * dot notation. Adjacent A_Indices nodes have to be treated as a single * multidimensional subscript operation. */ foreach(i, ind->indirection) @@ -461,19 +462,43 @@ transformIndirection(ParseState *pstate, A_Indirection *ind) } else { - Node *newresult; - Assert(IsA(n, String)); + subscripts = lappend(subscripts, n); + } + } + + while (subscripts) + { + /* try processing container subscripts first */ + Node *newresult = (Node *) + transformContainerSubscripts(pstate, + result, + exprType(result), + exprTypmod(result), + &subscripts, + false, + true); + + if (!newresult) + { + /* + * generic subscripting failed; falling back to function call or + * field selection for a composite type. + */ + Node *n; + + Assert(subscripts); - /* process subscripts before this field selection */ - while (subscripts) - result = (Node *) transformContainerSubscripts(pstate, - result, - exprType(result), - exprTypmod(result), - &subscripts, - false); + n = linitial(subscripts); + + if (!IsA(n, String)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("cannot subscript type %s because it does not support subscripting", + format_type_be(exprType(result))), + parser_errposition(pstate, exprLocation(result)))); + /* try to find function for field selection */ newresult = ParseFuncOrColumn(pstate, list_make1(n), list_make1(result), @@ -481,19 +506,16 @@ transformIndirection(ParseState *pstate, A_Indirection *ind) NULL, false, location); - if (newresult == NULL) + + if (!newresult) unknown_attribute(pstate, result, strVal(n), location); - result = newresult; + + /* consume field select */ + subscripts = list_delete_first(subscripts); } + + result = newresult; } - /* process trailing subscripts, if any */ - while (subscripts) - result = (Node *) transformContainerSubscripts(pstate, - result, - exprType(result), - exprTypmod(result), - &subscripts, - false); return result; } diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c index 19a6b678e67..b3e476eb181 100644 --- a/src/backend/parser/parse_node.c +++ b/src/backend/parser/parse_node.c @@ -238,6 +238,8 @@ transformContainerType(Oid *containerType, int32 *containerTypmod) * containerTypMod typmod for the container * indirection Untransformed list of subscripts (must not be NIL) * isAssignment True if this will become a container assignment. + * noError True for return NULL with no error, if the container type + * is not subscriptable. */ SubscriptingRef * transformContainerSubscripts(ParseState *pstate, @@ -245,13 +247,15 @@ transformContainerSubscripts(ParseState *pstate, Oid containerType, int32 containerTypMod, List **indirection, - bool isAssignment) + bool isAssignment, + bool noError) { SubscriptingRef *sbsref; const SubscriptRoutines *sbsroutines; Oid elementType; bool isSlice = false; ListCell *idx; + int indirection_length = list_length(*indirection); /* * Determine the actual container type, smashing any domain. In the @@ -267,11 +271,16 @@ transformContainerSubscripts(ParseState *pstate, */ sbsroutines = getSubscriptingRoutines(containerType, &elementType); if (!sbsroutines) + { + if (noError) + return NULL; + ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("cannot subscript type %s because it does not support subscripting", format_type_be(containerType)), parser_errposition(pstate, exprLocation(containerBase)))); + } /* * Detect whether any of the indirection items are slice specifiers. @@ -282,9 +291,9 @@ transformContainerSubscripts(ParseState *pstate, */ foreach(idx, *indirection) { - A_Indices *ai = lfirst_node(A_Indices, idx); + Node *ai = lfirst(idx); - if (ai->is_slice) + if (IsA(ai, A_Indices) && castNode(A_Indices, ai)->is_slice) { isSlice = true; break; @@ -312,6 +321,32 @@ transformContainerSubscripts(ParseState *pstate, sbsroutines->transform(sbsref, indirection, pstate, isSlice, isAssignment); + /* + * Error out, if datatype failed to consume any indirection elements. + */ + if (list_length(*indirection) == indirection_length) + { + Node *ind = linitial(*indirection); + + if (noError) + return NULL; + + if (IsA(ind, String)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("type %s does not support dot notation", + format_type_be(containerType)), + parser_errposition(pstate, exprLocation(containerBase)))); + else if (IsA(ind, A_Indices)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("type %s does not support array subscripting", + format_type_be(containerType)), + parser_errposition(pstate, exprLocation(containerBase)))); + else + elog(ERROR, "invalid indirection operation: %d", nodeTag(ind)); + } + /* * Verify we got a valid type (this defends, for example, against someone * using array_subscript_handler as typsubscript without setting typelem). diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c index 4675a523045..3ef5897f2eb 100644 --- a/src/backend/parser/parse_target.c +++ b/src/backend/parser/parse_target.c @@ -937,7 +937,8 @@ transformAssignmentSubscripts(ParseState *pstate, containerType, containerTypMod, &subscripts, - true); + true, + false); typeNeeded = sbsref->refrestype; typmodNeeded = sbsref->reftypmod; diff --git a/src/backend/utils/adt/arraysubs.c b/src/backend/utils/adt/arraysubs.c index 234c2c278c1..d03d3519dfd 100644 --- a/src/backend/utils/adt/arraysubs.c +++ b/src/backend/utils/adt/arraysubs.c @@ -62,6 +62,7 @@ array_subscript_transform(SubscriptingRef *sbsref, List *upperIndexpr = NIL; List *lowerIndexpr = NIL; ListCell *idx; + int ndim; /* * Transform the subscript expressions, and separate upper and lower @@ -73,9 +74,14 @@ array_subscript_transform(SubscriptingRef *sbsref, */ foreach(idx, *indirection) { - A_Indices *ai = lfirst_node(A_Indices, idx); + A_Indices *ai; Node *subexpr; + if (!IsA(lfirst(idx), A_Indices)) + break; + + ai = lfirst_node(A_Indices, idx); + if (isSlice) { if (ai->lidx) @@ -145,14 +151,15 @@ array_subscript_transform(SubscriptingRef *sbsref, sbsref->reflowerindexpr = lowerIndexpr; /* Verify subscript list lengths are within implementation limit */ - if (list_length(upperIndexpr) > MAXDIM) + ndim = list_length(upperIndexpr); + if (ndim > MAXDIM) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)", list_length(upperIndexpr), MAXDIM))); /* We need not check lowerIndexpr separately */ - *indirection = NIL; + *indirection = list_delete_first_n(*indirection, ndim); /* * Determine the result type of the subscripting operation. It's the same diff --git a/src/backend/utils/adt/jsonbsubs.c b/src/backend/utils/adt/jsonbsubs.c index 8ad6aa1ad4f..a0d38a0fd80 100644 --- a/src/backend/utils/adt/jsonbsubs.c +++ b/src/backend/utils/adt/jsonbsubs.c @@ -55,9 +55,14 @@ jsonb_subscript_transform(SubscriptingRef *sbsref, */ foreach(idx, *indirection) { - A_Indices *ai = lfirst_node(A_Indices, idx); + A_Indices *ai; Node *subExpr; + if (!IsA(lfirst(idx), A_Indices)) + break; + + ai = lfirst_node(A_Indices, idx); + if (isSlice) { Node *expr = ai->uidx ? ai->uidx : ai->lidx; @@ -160,7 +165,9 @@ jsonb_subscript_transform(SubscriptingRef *sbsref, sbsref->refrestype = JSONBOID; sbsref->reftypmod = -1; - *indirection = NIL; + /* Remove processed elements */ + if (upperIndexpr) + *indirection = list_delete_first_n(*indirection, list_length(upperIndexpr)); } /* diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h index 5ae11ccec33..71b04bd503c 100644 --- a/src/include/parser/parse_node.h +++ b/src/include/parser/parse_node.h @@ -378,7 +378,8 @@ extern SubscriptingRef *transformContainerSubscripts(ParseState *pstate, Oid containerType, int32 containerTypMod, List **indirection, - bool isAssignment); + bool isAssignment, + bool noError); extern Const *make_const(ParseState *pstate, A_Const *aconst); #endif /* PARSE_NODE_H */ -- 2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-15 16:22  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Viktor Holmberg <v@viktorh.net>
  0 siblings, 1 reply; 15+ messages in thread

From: Tom Lane @ 2025-03-15 16:22 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: daniel@yesql.se; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-15 21:17  Viktor Holmberg <v@viktorh.net>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 2 replies; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-15 21:17 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org

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

Attachments:

0001-isn-extension-Make-weak-mode-a-GUC-setting.patch (application/octet-stream, 12.9K) download From d3919051536729f3b17792bedfc92dd1cac1505b Mon Sep 17 00:00:00 2001 From: Viktor Holmberg <v@viktorh.net> Date: Sat, 15 Mar 2025 21:07:01 +0000 Subject: [PATCH] isn extension: Make weak mode a GUC setting isn_weak mode used to be settable only using the isn_weak(boolean) function. This wasn't optimal, as this means it doesn't respect transactions. This change makes the isn.weak a GUC parameter instead. The isn_weak functions are retained for backwards compatibility. The also had a bug where they were marked IMMUTABLE, when in fact they are VOLATILE. This is also fixed. --- contrib/isn/Makefile | 2 +- contrib/isn/expected/isn.out | 11 +++ contrib/isn/isn--1.2--1.3.sql | 7 ++ contrib/isn/isn.c | 18 +++-- contrib/isn/isn.control | 2 +- contrib/isn/isn.h | 1 - contrib/isn/sql/isn.sql | 5 ++ doc/src/sgml/isn.sgml | 139 +++++++++++++++++++--------------- 8 files changed, 118 insertions(+), 67 deletions(-) create mode 100644 contrib/isn/isn--1.2--1.3.sql diff --git a/contrib/isn/Makefile b/contrib/isn/Makefile index 1037506c705..35d9249f3a7 100644 --- a/contrib/isn/Makefile +++ b/contrib/isn/Makefile @@ -4,7 +4,7 @@ MODULES = isn EXTENSION = isn DATA = isn--1.1.sql isn--1.1--1.2.sql \ - isn--1.0--1.1.sql + isn--1.0--1.1.sql isn--1.2--1.3.sql PGFILEDESC = "isn - data types for international product numbering standards" # the other .h files are data tables, we don't install those diff --git a/contrib/isn/expected/isn.out b/contrib/isn/expected/isn.out index 2f05b7eb861..b7addb508dd 100644 --- a/contrib/isn/expected/isn.out +++ b/contrib/isn/expected/isn.out @@ -279,6 +279,17 @@ FROM (VALUES ('9780123456786', 'UPC'), 9771234567003 | ISSN | t | | | | (3 rows) +SELECT '2222222222221'::ean13; +ERROR: invalid check digit for EAN13 number: "2222222222221", should be 2 +LINE 1: SELECT '2222222222221'::ean13; + ^ +SET isn.weak TO TRUE; +SELECT '2222222222221'::ean13; + ean13 +------------------ + 222-222222222-2! +(1 row) + -- -- cleanup -- diff --git a/contrib/isn/isn--1.2--1.3.sql b/contrib/isn/isn--1.2--1.3.sql new file mode 100644 index 00000000000..26d1662c2b8 --- /dev/null +++ b/contrib/isn/isn--1.2--1.3.sql @@ -0,0 +1,7 @@ +ALTER FUNCTION isn_weak() VOLATILE; +ALTER FUNCTION isn_weak(boolean) VOLATILE; + +COMMENT ON FUNCTION isn_weak(boolean) IS +'Sets the weak input mode, and returns the new setting. This function is retained for backward compatibility. The recommended way to set weak mode is via the isn.weak GUC parameter.'; +COMMENT ON FUNCTION isn_weak() IS +'Returns the current status of the weak mode. This function is retained for backward compatibility. Users should check the value of isn.weak instead.'; diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c index db765ee490d..82c239f0ec1 100644 --- a/contrib/isn/isn.c +++ b/contrib/isn/isn.c @@ -21,6 +21,7 @@ #include "UPC.h" #include "fmgr.h" #include "isn.h" +#include "utils/guc.h" PG_MODULE_MAGIC; @@ -929,6 +930,18 @@ _PG_init(void) if (!check_table(UPC_range, UPC_index)) elog(ERROR, "UPC failed check"); } + + /* Define a GUC variable for weak mode. */ + DefineCustomBoolVariable("isn.weak", + "Accept invalid isns", + "When enabled, invalid numbers are accepted but flagged with an exclamation mark (!), indicating an incorrect check digit.", + &g_weak, + false, + PGC_USERSET, + 0, + NULL, + NULL, + NULL); } /* isn_out @@ -1109,17 +1122,12 @@ make_valid(PG_FUNCTION_ARGS) /* this function temporarily sets weak input flag * (to lose the strictness of check digit acceptance) - * It's a helper function, not intended to be used!! */ PG_FUNCTION_INFO_V1(accept_weak_input); Datum accept_weak_input(PG_FUNCTION_ARGS) { -#ifdef ISN_WEAK_MODE g_weak = PG_GETARG_BOOL(0); -#else - /* function has no effect */ -#endif /* ISN_WEAK_MODE */ PG_RETURN_BOOL(g_weak); } diff --git a/contrib/isn/isn.control b/contrib/isn/isn.control index 1cb5e2b2340..e7daea52b84 100644 --- a/contrib/isn/isn.control +++ b/contrib/isn/isn.control @@ -1,6 +1,6 @@ # isn extension comment = 'data types for international product numbering standards' -default_version = '1.2' +default_version = '1.3' module_pathname = '$libdir/isn' relocatable = true trusted = true diff --git a/contrib/isn/isn.h b/contrib/isn/isn.h index 038eb362c39..399896ad417 100644 --- a/contrib/isn/isn.h +++ b/contrib/isn/isn.h @@ -18,7 +18,6 @@ #include "fmgr.h" #undef ISN_DEBUG -#define ISN_WEAK_MODE /* * uint64 is the internal storage format for ISNs. diff --git a/contrib/isn/sql/isn.sql b/contrib/isn/sql/isn.sql index 2c2ea077d1e..3154dc7a8e7 100644 --- a/contrib/isn/sql/isn.sql +++ b/contrib/isn/sql/isn.sql @@ -120,6 +120,11 @@ FROM (VALUES ('9780123456786', 'UPC'), AS a(str,typ), LATERAL pg_input_error_info(a.str, a.typ) as errinfo; +SELECT '2222222222221'::ean13; +SET isn.weak TO TRUE; +SELECT '2222222222221'::ean13; + + -- -- cleanup -- diff --git a/doc/src/sgml/isn.sgml b/doc/src/sgml/isn.sgml index 45a867d98c2..75d5afd26b2 100644 --- a/doc/src/sgml/isn.sgml +++ b/doc/src/sgml/isn.sgml @@ -224,15 +224,72 @@ </para> </sect2> + <sect2 id="isn-guc-params"> + <title>GUC Parameters</title> + <variablelist> + <varlistentry id="guc-isn-weak"> + <term> + <varname>isn.weak</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>isn.weak</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the weak input mode. Default: false (strong mode) + </para> + </listitem> + </varlistentry> + </variablelist> + <para> + <firstterm>Weak</firstterm> mode is used to be able to insert invalid data + into a table. Invalid means the check digit is wrong, not that there are + missing numbers. + </para> + + <para> + Why would you want to use the weak mode? Well, it could be that + you have a huge collection of ISBN numbers, and that there are so many of + them that for weird reasons some have the wrong check digit (perhaps the + numbers were scanned from a printed list and the OCR got the numbers wrong, + perhaps the numbers were manually captured... who knows). Anyway, the point + is you might want to clean the mess up, but you still want to be able to + have all the numbers in your database and maybe use an external tool to + locate the invalid numbers in the database so you can verify the + information and validate it more easily; so for example you'd want to + select all the invalid numbers in the table. + </para> + + <para> + When you insert invalid numbers in a table using the weak mode, the number + will be inserted with the corrected check digit, but it will be displayed + with an exclamation mark (<literal>!</literal>) at the end, for example + <literal>0-11-000322-5!</literal>. This invalid marker can be checked with + the <function>is_valid</function> function and cleared with the + <function>make_valid</function> function. + </para> + + <para> + You can also force the insertion of invalid numbers even when not in the + weak mode, by appending the <literal>!</literal> character at the end of the + number. + </para> + + <para> + Another special feature is that during input, you can write + <literal>?</literal> in place of the check digit, and the correct check digit + will be inserted automatically. + </para> +</sect2> + <sect2 id="isn-funcs-ops"> <title>Functions and Operators</title> <para> The <filename>isn</filename> module provides the standard comparison operators, - plus B-tree and hash indexing support for all these data types. In - addition there are several specialized functions; shown in <xref linkend="isn-functions"/>. - In this table, - <type>isn</type> means any one of the module's data types. + plus B-tree and hash indexing support for all these data types. In + addition, there are several specialized functions, shown in <xref linkend="isn-functions"/>. + In this table, <type>isn</type> means any one of the module's data types. </para> <table id="isn-functions"> @@ -252,89 +309,53 @@ <tbody> <row> <entry role="func_table_entry"><para role="func_signature"> - <indexterm><primary>isn_weak</primary></indexterm> - <function>isn_weak</function> ( <type>boolean</type> ) - <returnvalue>boolean</returnvalue> + <indexterm><primary>make_valid</primary></indexterm> + <function>make_valid</function> ( <type>isn</type> ) + <returnvalue>isn</returnvalue> </para> <para> - Sets the weak input mode, and returns new setting. + Validates an invalid number (clears the invalid flag). </para></entry> </row> <row> <entry role="func_table_entry"><para role="func_signature"> - <function>isn_weak</function> () + <indexterm><primary>is_valid</primary></indexterm> + <function>is_valid</function> ( <type>isn</type> ) <returnvalue>boolean</returnvalue> </para> <para> - Returns the current status of the weak mode. + Checks for the presence of the invalid flag. </para></entry> </row> <row> <entry role="func_table_entry"><para role="func_signature"> - <indexterm><primary>make_valid</primary></indexterm> - <function>make_valid</function> ( <type>isn</type> ) - <returnvalue>isn</returnvalue> + <indexterm><primary>isn_weak</primary></indexterm> + <function>isn_weak</function> ( <type>boolean</type> ) + <returnvalue>boolean</returnvalue> </para> <para> - Validates an invalid number (clears the invalid flag). + Sets the weak input mode, and returns the new setting. + This function is retained for backward compatibility. + The recommended way to set weak mode is via the <varname>isn.weak</varname> GUC parameter. </para></entry> </row> <row> <entry role="func_table_entry"><para role="func_signature"> - <indexterm><primary>is_valid</primary></indexterm> - <function>is_valid</function> ( <type>isn</type> ) + <function>isn_weak</function> () <returnvalue>boolean</returnvalue> </para> <para> - Checks for the presence of the invalid flag. + Returns the current status of the weak mode. + This function is retained for backward compatibility. + Users should check the value of <varname>isn.weak</varname> instead. </para></entry> </row> </tbody> </tgroup> </table> - - <para> - <firstterm>Weak</firstterm> mode is used to be able to insert invalid data - into a table. Invalid means the check digit is wrong, not that there are - missing numbers. - </para> - - <para> - Why would you want to use the weak mode? Well, it could be that - you have a huge collection of ISBN numbers, and that there are so many of - them that for weird reasons some have the wrong check digit (perhaps the - numbers were scanned from a printed list and the OCR got the numbers wrong, - perhaps the numbers were manually captured... who knows). Anyway, the point - is you might want to clean the mess up, but you still want to be able to - have all the numbers in your database and maybe use an external tool to - locate the invalid numbers in the database so you can verify the - information and validate it more easily; so for example you'd want to - select all the invalid numbers in the table. - </para> - - <para> - When you insert invalid numbers in a table using the weak mode, the number - will be inserted with the corrected check digit, but it will be displayed - with an exclamation mark (<literal>!</literal>) at the end, for example - <literal>0-11-000322-5!</literal>. This invalid marker can be checked with - the <function>is_valid</function> function and cleared with the - <function>make_valid</function> function. - </para> - - <para> - You can also force the insertion of invalid numbers even when not in the - weak mode, by appending the <literal>!</literal> character at the end of the - number. - </para> - - <para> - Another special feature is that during input, you can write - <literal>?</literal> in place of the check digit, and the correct check digit - will be inserted automatically. - </para> </sect2> <sect2 id="isn-examples"> @@ -366,11 +387,11 @@ SELECT issn('3251231?'); SELECT ismn('979047213542?'); --Using the weak mode: -SELECT isn_weak(true); +SET isn.weak TO true; INSERT INTO test VALUES('978-0-11-000533-4'); INSERT INTO test VALUES('9780141219307'); INSERT INTO test VALUES('2-205-00876-X'); -SELECT isn_weak(false); +SET isn.weak TO false; SELECT id FROM test WHERE NOT is_valid(id); UPDATE test SET id = make_valid(id) WHERE id = '2-205-00876-X!'; -- 2.48.1
^ permalink raw reply [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-15 21:35  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Viktor Holmberg <v@viktorh.net>
  1 sibling, 0 replies; 15+ messages in thread

From: Tom Lane @ 2025-03-15 21:35 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-16 18:01  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Viktor Holmberg <v@viktorh.net>
  1 sibling, 1 reply; 15+ messages in thread

From: Tom Lane @ 2025-03-16 18:01 UTC (permalink / raw)
  To: Viktor Holmberg <v@viktorh.net>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: ISN extension - wrong volatility level for isn_weak() function
@ 2025-03-16 21:33  Viktor Holmberg <v@viktorh.net>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 0 replies; 15+ messages in thread

From: Viktor Holmberg @ 2025-03-16 21:33 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org

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

^ permalink  raw  reply  [nested|flat] 15+ messages in thread


end of thread, other threads:[~2025-03-16 21:33 UTC | newest]

Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-03-14 11:49 ISN extension - wrong volatility level for isn_weak() function Viktor Holmberg <v@viktorh.net>
2025-03-14 13:29 ` Daniel Gustafsson <daniel@yesql.se>
2025-03-14 13:58   ` Viktor Holmberg <v@viktorh.net>
2025-03-14 14:09     ` Daniel Gustafsson <daniel@yesql.se>
2025-03-14 14:16   ` Tom Lane <tgl@sss.pgh.pa.us>
2025-03-14 14:28     ` Viktor Holmberg <v@viktorh.net>
2025-03-14 15:13       ` Tom Lane <tgl@sss.pgh.pa.us>
2025-03-14 16:08         ` Viktor Holmberg <v@viktorh.net>
2025-03-14 16:18           ` Tom Lane <tgl@sss.pgh.pa.us>
2025-03-15 15:32             ` Viktor Holmberg <v@viktorh.net>
2025-03-15 16:22               ` Tom Lane <tgl@sss.pgh.pa.us>
2025-03-15 21:17                 ` Viktor Holmberg <v@viktorh.net>
2025-03-15 21:35                   ` Tom Lane <tgl@sss.pgh.pa.us>
2025-03-16 18:01                   ` Tom Lane <tgl@sss.pgh.pa.us>
2025-03-16 21:33                     ` Viktor Holmberg <v@viktorh.net>

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