pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
31+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2024-10-20 11c87216d1 fa4f11854c 7148cb3e30 SQL/JSON: Fix some oversights in commit b6e1157e7
* BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
@ 2024-10-15 11:00 PG Bug reporting form <noreply@postgresql.org>
2024-10-15 22:24 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Fabio R. Sluzala <fabio3rs@gmail.com>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 31+ messages in thread
From: PG Bug reporting form @ 2024-10-15 11:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18657
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 17.0
Operating system: Ubuntu 22.04
Description:
The following query:
SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': random() RETURNING text) FORMAT
JSON);
triggers a server crash with the following stack trace:
Core was generated by `postgres: law regression [local] SELECT
'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000055b54914ee1b in ExecBuildAggTrans (...) at execExpr.c:3514
3514 expr_setup_walker((Node *)
pertrans->aggref->aggdirectargs,
(gdb) bt
#0 0x000055b54914ee1b in ExecBuildAggTrans (...) at execExpr.c:3514
#1 0x000055b549180ee9 in ExecInitAgg (...) at nodeAgg.c:4017
#2 0x000055b54916eb13 in ExecInitNode (...) at execProcnode.c:341
#3 0x000055b549162d3f in InitPlan (...) at execMain.c:968
#4 0x000055b549162732 in standard_ExecutorStart (...) at execMain.c:263
#5 0x000055b54916245a in ExecutorStart (...) at execMain.c:139
#6 0x000055b549411063 in PortalStart (...) at pquery.c:517
#7 0x000055b54940d172 in exec_simple_query (
query_string=0x55b54b3f4430 "SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b':
random() RETURNING text) FORMAT JSON);") at postgres.c:1239
(gdb) p pertrans->aggref
$1 = (Aggref *) 0x0
First bad commit for this anomaly is b6e1157e7.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
@ 2024-10-15 22:24 ` Fabio R. Sluzala <fabio3rs@gmail.com>
1 sibling, 0 replies; 31+ messages in thread
From: Fabio R. Sluzala @ 2024-10-15 22:24 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
It seems that reverting the code at src/backend/executor/execExpr.c case
T_JsonValueExpr: fix the problem
Revert patch:
diff --git a/src/backend/executor/execExpr.c
b/src/backend/executor/execExpr.c
index a5395536a1..4143ff1730 100644
inline patch 1: src/backend/executor/execExpr.c (+15/-2 lines) download inline-1.patch
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2317,8 +2317,21 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- Assert(jve->formatted_expr != NULL);
- ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
+
+ if (jve->formatted_expr)
+ {
+ Datum *innermost_caseval = state->innermost_caseval;
+ bool *innermost_isnull = state->innermost_casenull;
+
+ state->innermost_caseval = resv;
+ state->innermost_casenull = resnull;
+
+ ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
+
+ state->innermost_caseval = innermost_caseval;
+ state->innermost_casenull = innermost_isnull;
+ }
break;
}
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
@ 2024-10-16 00:19 ` Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 31+ messages in thread
From: Michael Paquier @ 2024-10-16 00:19 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org; +Cc: Amit Langote <amitlangote09@gmail.com>
On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
> First bad commit for this anomaly is b6e1157e7.
Amit, any thoughts?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
@ 2024-10-16 00:35 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-16 00:35 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
> > First bad commit for this anomaly is b6e1157e7.
>
> Amit, any thoughts?
Will look into it, thanks.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-16 08:20 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 09:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 31+ messages in thread
From: Tender Wang @ 2024-10-16 08:20 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz>
> wrote:
> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
> > > First bad commit for this anomaly is b6e1157e7.
> >
> > Amit, any thoughts?
>
> Will look into it, thanks.
>
Hi,
I debug this issue, and I find that:
after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in
expression_tree_walker_impl(), which
is called in preprocess_aggrefs().
I try to above solution, no crashed again.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
@ 2024-10-16 09:04 ` Tender Wang <tndrwang@gmail.com>
1 sibling, 0 replies; 31+ messages in thread
From: Tender Wang @ 2024-10-16 09:04 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> 于2024年10月16日周三 16:20写道：
>
>
> Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
>
>> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz>
>> wrote:
>> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
>> > > First bad commit for this anomaly is b6e1157e7.
>> >
>> > Amit, any thoughts?
>>
>> Will look into it, thanks.
>>
>
> Hi,
>
> I debug this issue, and I find that:
> after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in
> expression_tree_walker_impl(), which
> is called in preprocess_aggrefs().
>
> I try to above solution, no crashed again.
>
>
I attached a patch. I found a typo comment when I worked on this issue.
But I don't include a test case, because random() result is different
every time, maybe using explain statement.
Any thoughts?
--
Thanks,
Tender Wang
Attachments:
0001-Fix-json_objectagg-crashed-when-including-random-fun.patch (application/octet-stream, 1.5K) download
From 2687b83fee7f1951df3022cf24666ebec2ed5094 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Wed, 16 Oct 2024 16:59:47 +0800
Subject: [PATCH] Fix json_objectagg() crashed when including random() func.
---
src/backend/nodes/nodeFuncs.c | 2 --
src/backend/parser/parse_expr.c | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 0d00e029f3..9cf5832192 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2334,8 +2334,6 @@ expression_tree_walker_impl(Node *node,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- if (WALK(jve->raw_expr))
- return true;
if (WALK(jve->formatted_expr))
return true;
}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 36c1b7a88f..ef0b560f5e 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -3890,7 +3890,7 @@ transformJsonAggConstructor(ParseState *pstate, JsonAggConstructor *agg_ctor,
/*
* Transform JSON_OBJECTAGG() aggregate function.
*
- * JSON_OBJECT() is transformed into a JsonConstructorExpr node of type
+ * JSON_OBJECTAGG() is transformed into a JsonConstructorExpr node of type
* JSCTOR_JSON_OBJECTAGG, which at runtime becomes a
* json[b]_object_agg[_unique][_strict](agg->arg->key, agg->arg->value) call
* depending on the output JSON format. The result is coerced to the target
--
2.25.1
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
@ 2024-10-16 09:06 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
1 sibling, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-16 09:06 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
>>
>> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
>> > > First bad commit for this anomaly is b6e1157e7.
>> >
>> > Amit, any thoughts?
>>
>> Will look into it, thanks.
> Hi,
>
> I debug this issue, and I find that:
> after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in expression_tree_walker_impl(), which
> is called in preprocess_aggrefs().
>
> I try to above solution, no crashed again.
Thanks for the analysis. That is my conclusion as well.
Attached a patch.
--
Thanks, Amit Langote
Attachments:
v1-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/x-patch, 8.8K) download
From 7d69cb42d276afc78db78375bbe5cf871b80aad3 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Wed, 16 Oct 2024 18:04:54 +0900
Subject: [PATCH v1] SQL/JSON: Fix some oversights in commit b6e1157e7
JsonValueExpr.raw_expr should not be touched by
expression_tree_walker() and expression_tree_mutator(). One problem
it causes, as demonstrated in BUG #18657, is that the Aggref nodes
referenced in formatted_expr, which is evaluated, end up with the
wrong value of aggtransno. This occurs because the Aggref's copy in
raw_expr is also added to PlannerInfo.aggtransinfos by
preprocess_aggref(), leading to incorrect state in the planner.
This can result in a crash during ExecBuildAggTrans(), as seen in the
bug report.
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/nodes/nodeFuncs.c | 3 --
src/test/regress/expected/sqljson.out | 45 ++++++++++++++++++++-------
src/test/regress/sql/sqljson.sql | 10 ++++++
3 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 0d00e029f3..579c85f0d0 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2334,8 +2334,6 @@ expression_tree_walker_impl(Node *node,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- if (WALK(jve->raw_expr))
- return true;
if (WALK(jve->formatted_expr))
return true;
}
@@ -3370,7 +3368,6 @@ expression_tree_mutator_impl(Node *node,
JsonValueExpr *newnode;
FLATCOPY(newnode, jve, JsonValueExpr);
- MUTATE(newnode->raw_expr, jve->raw_expr, Expr *);
MUTATE(newnode->formatted_expr, jve->formatted_expr, Expr *);
MUTATE(newnode->format, jve->format, JsonFormat *);
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..dd30a5323f 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -995,10 +995,10 @@ DROP VIEW json_array_view;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING text) FILTER (WHERE i > 3)
FROM generate_series(1,5) i;
- QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------------------
Aggregate
- Output: JSON_OBJECTAGG(i : (('111'::text || (i)::text))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) FILTER (WHERE (i > 3))
+ Output: JSON_OBJECTAGG(i : (('111'::text || i))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) FILTER (WHERE (i > 3))
-> Function Scan on pg_catalog.generate_series i
Output: i
Function Call: generate_series(1, 5)
@@ -1007,10 +1007,10 @@ FROM generate_series(1,5) i;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING text) OVER (PARTITION BY i % 2)
FROM generate_series(1,5) i;
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------
WindowAgg
- Output: JSON_OBJECTAGG(i : (('111'::text || (i)::text))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) OVER (?), ((i % 2))
+ Output: JSON_OBJECTAGG(i : (('111'::text || i))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) OVER (?), ((i % 2))
-> Sort
Output: ((i % 2)), i
Sort Key: ((i.i % 2))
@@ -1031,10 +1031,10 @@ DROP VIEW json_objectagg_view;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_ARRAYAGG(('111' || i)::bytea FORMAT JSON NULL ON NULL RETURNING text) FILTER (WHERE i > 3)
FROM generate_series(1,5) i;
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------
Aggregate
- Output: JSON_ARRAYAGG((('111'::text || (i)::text))::bytea FORMAT JSON NULL ON NULL RETURNING text) FILTER (WHERE (i > 3))
+ Output: JSON_ARRAYAGG((('111'::text || i))::bytea FORMAT JSON NULL ON NULL RETURNING text) FILTER (WHERE (i > 3))
-> Function Scan on pg_catalog.generate_series i
Output: i
Function Call: generate_series(1, 5)
@@ -1043,10 +1043,10 @@ FROM generate_series(1,5) i;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_ARRAYAGG(('111' || i)::bytea FORMAT JSON NULL ON NULL RETURNING text) OVER (PARTITION BY i % 2)
FROM generate_series(1,5) i;
- QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------
WindowAgg
- Output: JSON_ARRAYAGG((('111'::text || (i)::text))::bytea FORMAT JSON NULL ON NULL RETURNING text) OVER (?), ((i % 2))
+ Output: JSON_ARRAYAGG((('111'::text || i))::bytea FORMAT JSON NULL ON NULL RETURNING text) OVER (?), ((i % 2))
-> Sort
Output: ((i % 2)), i
Sort Key: ((i.i % 2))
@@ -1300,3 +1300,24 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..2100da02d3 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,13 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-16 09:11 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Tender Wang @ 2024-10-16 09:11 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:06写道：
> On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
> >>
> >> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz>
> wrote:
> >> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
> >> > > First bad commit for this anomaly is b6e1157e7.
> >> >
> >> > Amit, any thoughts?
> >>
> >> Will look into it, thanks.
> > Hi,
> >
> > I debug this issue, and I find that:
> > after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in
> expression_tree_walker_impl(), which
> > is called in preprocess_aggrefs().
> >
> > I try to above solution, no crashed again.
>
> Thanks for the analysis. That is my conclusion as well.
>
> Attached a patch.
>
>
Yeah, yours look more better than mine. And the typo in my attached patch,
is that right?
JSON_OBJECT() should be JSON_OBJECTAGG() near transformJsonObjectAgg()
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
@ 2024-10-16 09:14 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-16 09:14 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 16, 2024 at 6:11 PM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:06写道：
>>
>> On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang@gmail.com> wrote:
>> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
>> >>
>> >> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>> >> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
>> >> > > First bad commit for this anomaly is b6e1157e7.
>> >> >
>> >> > Amit, any thoughts?
>> >>
>> >> Will look into it, thanks.
>> > Hi,
>> >
>> > I debug this issue, and I find that:
>> > after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in expression_tree_walker_impl(), which
>> > is called in preprocess_aggrefs().
>> >
>> > I try to above solution, no crashed again.
>>
>> Thanks for the analysis. That is my conclusion as well.
>>
>> Attached a patch.
>>
>
> Yeah, yours look more better than mine. And the typo in my attached patch, is that right?
> JSON_OBJECT() should be JSON_OBJECTAGG() near transformJsonObjectAgg()
Yeah, the typo needs to be fixed as well. Thanks for the patch.
So, attaching 0002 for that.
--
Thanks, Amit Langote
Attachments: download all (2 files, .tar.gz)
v2-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/octet-stream, 8.8K) download
From 7d69cb42d276afc78db78375bbe5cf871b80aad3 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Wed, 16 Oct 2024 18:04:54 +0900
Subject: [PATCH v2 1/2] SQL/JSON: Fix some oversights in commit b6e1157e7
JsonValueExpr.raw_expr should not be touched by
expression_tree_walker() and expression_tree_mutator(). One problem
it causes, as demonstrated in BUG #18657, is that the Aggref nodes
referenced in formatted_expr, which is evaluated, end up with the
wrong value of aggtransno. This occurs because the Aggref's copy in
raw_expr is also added to PlannerInfo.aggtransinfos by
preprocess_aggref(), leading to incorrect state in the planner.
This can result in a crash during ExecBuildAggTrans(), as seen in the
bug report.
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/nodes/nodeFuncs.c | 3 --
src/test/regress/expected/sqljson.out | 45 ++++++++++++++++++++-------
src/test/regress/sql/sqljson.sql | 10 ++++++
3 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 0d00e029f3..579c85f0d0 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2334,8 +2334,6 @@ expression_tree_walker_impl(Node *node,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- if (WALK(jve->raw_expr))
- return true;
if (WALK(jve->formatted_expr))
return true;
}
@@ -3370,7 +3368,6 @@ expression_tree_mutator_impl(Node *node,
JsonValueExpr *newnode;
FLATCOPY(newnode, jve, JsonValueExpr);
- MUTATE(newnode->raw_expr, jve->raw_expr, Expr *);
MUTATE(newnode->formatted_expr, jve->formatted_expr, Expr *);
MUTATE(newnode->format, jve->format, JsonFormat *);
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..dd30a5323f 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -995,10 +995,10 @@ DROP VIEW json_array_view;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING text) FILTER (WHERE i > 3)
FROM generate_series(1,5) i;
- QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------------------
Aggregate
- Output: JSON_OBJECTAGG(i : (('111'::text || (i)::text))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) FILTER (WHERE (i > 3))
+ Output: JSON_OBJECTAGG(i : (('111'::text || i))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) FILTER (WHERE (i > 3))
-> Function Scan on pg_catalog.generate_series i
Output: i
Function Call: generate_series(1, 5)
@@ -1007,10 +1007,10 @@ FROM generate_series(1,5) i;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING text) OVER (PARTITION BY i % 2)
FROM generate_series(1,5) i;
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------
WindowAgg
- Output: JSON_OBJECTAGG(i : (('111'::text || (i)::text))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) OVER (?), ((i % 2))
+ Output: JSON_OBJECTAGG(i : (('111'::text || i))::bytea FORMAT JSON WITH UNIQUE KEYS RETURNING text) OVER (?), ((i % 2))
-> Sort
Output: ((i % 2)), i
Sort Key: ((i.i % 2))
@@ -1031,10 +1031,10 @@ DROP VIEW json_objectagg_view;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_ARRAYAGG(('111' || i)::bytea FORMAT JSON NULL ON NULL RETURNING text) FILTER (WHERE i > 3)
FROM generate_series(1,5) i;
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------
Aggregate
- Output: JSON_ARRAYAGG((('111'::text || (i)::text))::bytea FORMAT JSON NULL ON NULL RETURNING text) FILTER (WHERE (i > 3))
+ Output: JSON_ARRAYAGG((('111'::text || i))::bytea FORMAT JSON NULL ON NULL RETURNING text) FILTER (WHERE (i > 3))
-> Function Scan on pg_catalog.generate_series i
Output: i
Function Call: generate_series(1, 5)
@@ -1043,10 +1043,10 @@ FROM generate_series(1,5) i;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT JSON_ARRAYAGG(('111' || i)::bytea FORMAT JSON NULL ON NULL RETURNING text) OVER (PARTITION BY i % 2)
FROM generate_series(1,5) i;
- QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------
WindowAgg
- Output: JSON_ARRAYAGG((('111'::text || (i)::text))::bytea FORMAT JSON NULL ON NULL RETURNING text) OVER (?), ((i % 2))
+ Output: JSON_ARRAYAGG((('111'::text || i))::bytea FORMAT JSON NULL ON NULL RETURNING text) OVER (?), ((i % 2))
-> Sort
Output: ((i % 2)), i
Sort Key: ((i.i % 2))
@@ -1300,3 +1300,24 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..2100da02d3 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,13 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one;
--
2.43.0
v2-0002-Fix-typo-in-comment-of-transformJsonAggConstructo.patch (application/octet-stream, 1.2K) download
From 0883e6327f1d709e744fcf6fe28dd4cd91885a65 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Wed, 16 Oct 2024 18:11:53 +0900
Subject: [PATCH v2 2/2] Fix typo in comment of transformJsonAggConstructor()
An oversight of 3a8a1f3254b.
Reported-by: Tender Wang <tndrwang@gmail.com>
Author: Tender Wang <tndrwang@gmail.com>
Backpatch-through: 16
---
src/backend/parser/parse_expr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 36c1b7a88f..ef0b560f5e 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -3890,7 +3890,7 @@ transformJsonAggConstructor(ParseState *pstate, JsonAggConstructor *agg_ctor,
/*
* Transform JSON_OBJECTAGG() aggregate function.
*
- * JSON_OBJECT() is transformed into a JsonConstructorExpr node of type
+ * JSON_OBJECTAGG() is transformed into a JsonConstructorExpr node of type
* JSCTOR_JSON_OBJECTAGG, which at runtime becomes a
* json[b]_object_agg[_unique][_strict](agg->arg->key, agg->arg->value) call
* depending on the output JSON format. The result is coerced to the target
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-16 09:18 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Tender Wang @ 2024-10-16 09:18 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:14写道：
> On Wed, Oct 16, 2024 at 6:11 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:06写道：
> >>
> >> On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang@gmail.com> wrote:
> >> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
> >> >>
> >> >> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz>
> wrote:
> >> >> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form
> wrote:
> >> >> > > First bad commit for this anomaly is b6e1157e7.
> >> >> >
> >> >> > Amit, any thoughts?
> >> >>
> >> >> Will look into it, thanks.
> >> > Hi,
> >> >
> >> > I debug this issue, and I find that:
> >> > after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in
> expression_tree_walker_impl(), which
> >> > is called in preprocess_aggrefs().
> >> >
> >> > I try to above solution, no crashed again.
> >>
> >> Thanks for the analysis. That is my conclusion as well.
> >>
> >> Attached a patch.
> >>
> >
> > Yeah, yours look more better than mine. And the typo in my attached
> patch, is that right?
> > JSON_OBJECT() should be JSON_OBJECTAGG() near transformJsonObjectAgg()
>
> Yeah, the typo needs to be fixed as well. Thanks for the patch.
>
> So, attaching 0002 for that.
>
LGTM
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
@ 2024-10-16 12:00 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 31+ messages in thread
From: Amit Langote @ 2024-10-16 12:00 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 16, 2024 at 6:19 PM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:14写道：
>> On Wed, Oct 16, 2024 at 6:11 PM Tender Wang <tndrwang@gmail.com> wrote:
>> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:06写道：
>> >>
>> >> On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang@gmail.com> wrote:
>> >> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
>> >> >>
>> >> >> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>> >> >> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form wrote:
>> >> >> > > First bad commit for this anomaly is b6e1157e7.
>> >> >> >
>> >> >> > Amit, any thoughts?
>> >> >>
>> >> >> Will look into it, thanks.
>> >> > Hi,
>> >> >
>> >> > I debug this issue, and I find that:
>> >> > after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in expression_tree_walker_impl(), which
>> >> > is called in preprocess_aggrefs().
>> >> >
>> >> > I try to above solution, no crashed again.
>> >>
>> >> Thanks for the analysis. That is my conclusion as well.
>> >>
>> >> Attached a patch.
>> >>
>> >
>> > Yeah, yours look more better than mine. And the typo in my attached patch, is that right?
>> > JSON_OBJECT() should be JSON_OBJECTAGG() near transformJsonObjectAgg()
>>
>> Yeah, the typo needs to be fixed as well. Thanks for the patch.
>>
>> So, attaching 0002 for that.
I've pushed 0002 in advance.
I realized I hadn't explained in the commit message why the outputs of
some existing tests changed, so I decided to give it more thought
before pushing 0001. The changes seem harmless at first glance, but I
want to consider them further.
Also, it might be better to leave a comment where the commit is
removing code, as follows:
- if (WALK(jve->raw_expr))
- return true;
+ /* Ignore raw_expr because it's not relevant at runtime. */
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-16 14:14 ` Tender Wang <tndrwang@gmail.com>
1 sibling, 0 replies; 31+ messages in thread
From: Tender Wang @ 2024-10-16 14:14 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 20:00写道：
> On Wed, Oct 16, 2024 at 6:19 PM Tender Wang <tndrwang@gmail.com> wrote:
> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:14写道：
> >> On Wed, Oct 16, 2024 at 6:11 PM Tender Wang <tndrwang@gmail.com> wrote:
> >> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 17:06写道：
> >> >>
> >> >> On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang@gmail.com>
> wrote:
> >> >> > Amit Langote <amitlangote09@gmail.com> 于2024年10月16日周三 08:35写道：
> >> >> >>
> >> >> >> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <
> michael@paquier.xyz> wrote:
> >> >> >> > On Tue, Oct 15, 2024 at 11:00:01AM +0000, PG Bug reporting form
> wrote:
> >> >> >> > > First bad commit for this anomaly is b6e1157e7.
> >> >> >> >
> >> >> >> > Amit, any thoughts?
> >> >> >>
> >> >> >> Will look into it, thanks.
> >> >> > Hi,
> >> >> >
> >> >> > I debug this issue, and I find that:
> >> >> > after b6e1157e7, we shouldn't walk JsonValueExpr.raw_expr in
> expression_tree_walker_impl(), which
> >> >> > is called in preprocess_aggrefs().
> >> >> >
> >> >> > I try to above solution, no crashed again.
> >> >>
> >> >> Thanks for the analysis. That is my conclusion as well.
> >> >>
> >> >> Attached a patch.
> >> >>
> >> >
> >> > Yeah, yours look more better than mine. And the typo in my attached
> patch, is that right?
> >> > JSON_OBJECT() should be JSON_OBJECTAGG() near
> transformJsonObjectAgg()
> >>
> >> Yeah, the typo needs to be fixed as well. Thanks for the patch.
> >>
> >> So, attaching 0002 for that.
>
> I've pushed 0002 in advance.
>
> I realized I hadn't explained in the commit message why the outputs of
> some existing tests changed, so I decided to give it more thought
> before pushing 0001. The changes seem harmless at first glance, but I
> want to consider them further.
>
I didn't looked more in details. I guessed it is related with below codes:
- MUTATE(newnode->raw_expr, jve->raw_expr, Expr *);
Because in my patch I didn't remove above code, so I didn't have the plan
diffs.
The output in these plan diffs seemed same with their SQL statements. If we
removed
MUTATE(newnode->raw_expr, jve->raw_expr, Expr *);
the output seemd to make sense.
Anyway, above is my guess. I didn't debug these.
> Also, it might be better to leave a comment where the commit is
> removing code, as follows:
>
> - if (WALK(jve->raw_expr))
> - return true;
> + /* Ignore raw_expr because it's not relevant at runtime.
> */
>
+1
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-16 14:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 31+ messages in thread
From: Tom Lane @ 2024-10-16 14:26 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> writes:
> Also, it might be better to leave a comment where the commit is
> removing code, as follows:
> - if (WALK(jve->raw_expr))
> - return true;
> + /* Ignore raw_expr because it's not relevant at runtime. */
Would it be better for parse analysis to explicitly NULL out this
field once it's done looking at it? Carrying unmaintained pieces
of an expression tree around seems both inefficient and prone to
future failures of this same ilk. The further the raw_expr gets
out of step with current reality, the worse the hazards.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-17 00:38 ` Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-17 00:38 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 16, 2024 at 11:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > Also, it might be better to leave a comment where the commit is
> > removing code, as follows:
>
> > - if (WALK(jve->raw_expr))
> > - return true;
> > + /* Ignore raw_expr because it's not relevant at runtime. */
>
> Would it be better for parse analysis to explicitly NULL out this
> field once it's done looking at it? Carrying unmaintained pieces
> of an expression tree around seems both inefficient and prone to
> future failures of this same ilk. The further the raw_expr gets
> out of step with current reality, the worse the hazards.
It seems we can't do that in this case, because get_rule_expr() wants
to read it.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-17 00:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 03:52 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
0 siblings, 3 replies; 31+ messages in thread
From: Tom Lane @ 2024-10-17 00:56 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> writes:
> On Wed, Oct 16, 2024 at 11:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Would it be better for parse analysis to explicitly NULL out this
>> field once it's done looking at it? Carrying unmaintained pieces
>> of an expression tree around seems both inefficient and prone to
>> future failures of this same ilk. The further the raw_expr gets
>> out of step with current reality, the worse the hazards.
> It seems we can't do that in this case, because get_rule_expr() wants
> to read it.
I think you just increased my level of concern. Aren't you saying
that get_rule_expr is likely to deliver utter garbage, because it
will be working from an expression that has not tracked
transformations made to other parts of the tree?
Also, I see that ruleutils is applying get_rule_expr to raw_expr,
which AFAICT means it's *not* raw-parser output, else that wouldn't
work at all. Which means the field is badly named and incorrectly
documented. But in particular it means you cannot just make the
tree-walking routines ignore it --- you *must* update it during
transformations such as subquery pullup, or you will have garbage
that will cause EXPLAIN to crash or at least produce wrong results.
In other words, the proposed patch is dangerously wrong. I suspect at
this point that the true bug might be the opposite: somebody feeling
that they could dispense with updating raw_expr when they shouldn't.
In particular, I think "... is not evaluated by
eval_const_expressions_mutator()" is already a huge red flag.
There are transformations that eval_const_expressions does that
are not optional.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-17 03:52 ` Amit Langote <amitlangote09@gmail.com>
2 siblings, 0 replies; 31+ messages in thread
From: Amit Langote @ 2024-10-17 03:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 17, 2024 at 9:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Wed, Oct 16, 2024 at 11:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Would it be better for parse analysis to explicitly NULL out this
> >> field once it's done looking at it? Carrying unmaintained pieces
> >> of an expression tree around seems both inefficient and prone to
> >> future failures of this same ilk. The further the raw_expr gets
> >> out of step with current reality, the worse the hazards.
>
> > It seems we can't do that in this case, because get_rule_expr() wants
> > to read it.
>
> I think you just increased my level of concern. Aren't you saying
> that get_rule_expr is likely to deliver utter garbage, because it
> will be working from an expression that has not tracked
> transformations made to other parts of the tree?
>
> Also, I see that ruleutils is applying get_rule_expr to raw_expr,
> which AFAICT means it's *not* raw-parser output, else that wouldn't
> work at all. Which means the field is badly named and incorrectly
> documented. But in particular it means you cannot just make the
> tree-walking routines ignore it --- you *must* update it during
> transformations such as subquery pullup, or you will have garbage
> that will cause EXPLAIN to crash or at least produce wrong results.
One thing we could do is have get_rule_expr() print formatted_expr.
While this would make the output a bit verbose as shown in one of the
diffs I get, it would at least avoid the issues you mentioned with
printing raw_expr.
EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON('123'::bytea FORMAT JSON);
- QUERY PLAN
------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------------------
Result
- Output: JSON('\x313233'::bytea FORMAT JSON)
+ Output: JSON((convert_from('\x313233'::bytea, 'UTF8'::name))::json
FORMAT JSON)
> In other words, the proposed patch is dangerously wrong. I suspect at
> this point that the true bug might be the opposite: somebody feeling
> that they could dispense with updating raw_expr when they shouldn't.
The issue in this particular case is that preprocess_aggref() adds the
AggTransInfo of the Aggref mentioned in raw_expr because
expression_tree_walker() processes it. However, since raw_expr is
ignored by the executor, the Aggref contained in it is not added to
AggState.aggs. This causes the aggno and aggtransno values of the
Aggrefs that are added to become out of sync, leading to a crash.
I don't see a way to control whether raw_expr is processed by
preprocess_aggref other than ignoring it in expression_tree_walker().
But maybe I misunderstood you.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-17 04:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2 siblings, 1 reply; 31+ messages in thread
From: Tom Lane @ 2024-10-17 04:12 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
I wrote:
> In other words, the proposed patch is dangerously wrong. I suspect at
> this point that the true bug might be the opposite: somebody feeling
> that they could dispense with updating raw_expr when they shouldn't.
To expand on that: the proximate cause of the bug is that
ExecBuildAggTrans is expecting that expression initialization found
an Aggref for every aggtransno index. In the plan as emitted by
the planner, there are two json_object_agg Aggref nodes, one in
the JsonValueExpr's raw_expr and the other in the formatted_expr.
preprocess_aggrefs() assigned aggno 0 and aggtransno 0 to the
first one, aggno 1 and aggtransno 1 to the second. As already
noted upthread, the core of the problem is that ExecInitExprRec's
T_JsonValueExpr case is not recursing into raw_expr, so that
only the second Aggref gets found and linked into the parent
AggState, thus leaving a hole in the per-transno array.
The proposed patch fixes that indirectly by lobotomizing
expression_tree_walker so that preprocess_aggrefs doesn't
find the raw_expr's Aggref either. However, the side effects
of that will be spectacularly unpleasant, because there are
approximately zero other callers of expression_tree_walker
that will be pleased with it.
In the short term, I suspect the only workable fix is to undo the
optimization of having ExecInitExprRec not recurse into both raw_expr
and formatted_expr. In the longer run, I'd look hard at why the
tree needs to have two copies of that subexpression at all.
At least in this example, it appears that the only difference is
a type coercion expression wrapped around the formatted_expr, and
there are surely better ways to handle that.
BTW, the reason that using a volatile function matters is that
that stops find_compatible_agg from deciding that the two identical
Aggrefs can be given the same aggno and aggtransno, which masks
the bug in another way. That also means that the planner thinks
the two Aggrefs represent distinct computations. At minimum that
messes up our estimates of aggregate evaluation costs, and there
might be more subtle semantic consequences.
I remain concerned about whether it's really okay to skip raw_expr
in eval_const_expressions_mutator, too.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-17 08:15 ` Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-17 08:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 17, 2024 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > In other words, the proposed patch is dangerously wrong. I suspect at
> > this point that the true bug might be the opposite: somebody feeling
> > that they could dispense with updating raw_expr when they shouldn't.
>
> To expand on that: the proximate cause of the bug is that
> ExecBuildAggTrans is expecting that expression initialization found
> an Aggref for every aggtransno index. In the plan as emitted by
> the planner, there are two json_object_agg Aggref nodes, one in
> the JsonValueExpr's raw_expr and the other in the formatted_expr.
> preprocess_aggrefs() assigned aggno 0 and aggtransno 0 to the
> first one, aggno 1 and aggtransno 1 to the second. As already
> noted upthread, the core of the problem is that ExecInitExprRec's
> T_JsonValueExpr case is not recursing into raw_expr, so that
> only the second Aggref gets found and linked into the parent
> AggState, thus leaving a hole in the per-transno array.
>
> The proposed patch fixes that indirectly by lobotomizing
> expression_tree_walker so that preprocess_aggrefs doesn't
> find the raw_expr's Aggref either. However, the side effects
> of that will be spectacularly unpleasant, because there are
> approximately zero other callers of expression_tree_walker
> that will be pleased with it.
>
> In the short term, I suspect the only workable fix is to undo the
> optimization of having ExecInitExprRec not recurse into both raw_expr
> and formatted_expr.
This or actually I'm tempted to simply revert the whole thing
(b6e1157e7d3) as an ill-considered refactoring, because I am not able
to convince myself that calling ExecAggPlainTransByVal() twice, via
both raw_expr and formatted_expr, is always safe.
> In the longer run, I'd look hard at why the
> tree needs to have two copies of that subexpression at all.
> At least in this example, it appears that the only difference is
> a type coercion expression wrapped around the formatted_expr, and
> there are surely better ways to handle that.
Before commit b6e1157e, raw_expr and formatted_expr were evaluated
separately, but only raw_expr would contain the actual expression
(such as an Aggref in this case). The result of raw_expr was passed as
an argument to the formatting expression (like CoerceViaIO) using a
CaseValueExpr. That commit removed the CaseValueExpr indirection and
embedded the expression from raw_expr directly into formatted_expr,
making the separate runtime evaluation of raw_expr redundant.
However, this change was ill-advised given this report.
Thoughts on the attached?
--
Thanks, Amit Langote
Attachments:
v3-0001-Revert-Don-t-include-CaseTestExpr-in-JsonValueExp.patch (application/octet-stream, 10.9K) download
From 92deabcaac100256baa25009dd656b1015490327 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 17 Oct 2024 16:03:18 +0900
Subject: [PATCH v3] Revert "Don't include CaseTestExpr in
JsonValueExpr.formatted_expr"
This reverts commit b6e1157e7d339c4e20d68448125a4cef42b1ac9d.
---
src/backend/executor/execExpr.c | 17 +++++++--
src/backend/optimizer/util/clauses.c | 23 +++++++++---
src/backend/parser/parse_expr.c | 32 +++++++++++------
src/include/nodes/primnodes.h | 10 +++---
src/test/regress/expected/sqljson.out | 50 +++++++++++++++++++++++++++
src/test/regress/sql/sqljson.sql | 15 ++++++++
6 files changed, 124 insertions(+), 23 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c8077aa57b..daa4cb9d69 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,8 +2307,21 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- Assert(jve->formatted_expr != NULL);
- ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
+
+ if (jve->formatted_expr)
+ {
+ Datum *innermost_caseval = state->innermost_caseval;
+ bool *innermost_isnull = state->innermost_casenull;
+
+ state->innermost_caseval = resv;
+ state->innermost_casenull = resnull;
+
+ ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
+
+ state->innermost_caseval = innermost_caseval;
+ state->innermost_casenull = innermost_isnull;
+ }
break;
}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..6872dad5c2 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2915,12 +2915,25 @@ eval_const_expressions_mutator(Node *node,
case T_JsonValueExpr:
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- Node *formatted;
+ Node *raw;
- formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
- context);
- if (formatted && IsA(formatted, Const))
- return formatted;
+ raw = eval_const_expressions_mutator((Node *) jve->raw_expr,
+ context);
+ if (raw && IsA(raw, Const))
+ {
+ Node *formatted;
+ Node *save_case_val = context->case_val;
+
+ context->case_val = raw;
+
+ formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
+ context);
+
+ context->case_val = save_case_val;
+
+ if (formatted && IsA(formatted, Const))
+ return formatted;
+ }
break;
}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index ef0b560f5e..40736730cc 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -3268,6 +3268,21 @@ makeJsonByteaToTextConversion(Node *expr, JsonFormat *format, int location)
return (Node *) fexpr;
}
+/*
+ * Make a CaseTestExpr node.
+ */
+static Node *
+makeCaseTestExpr(Node *expr)
+{
+ CaseTestExpr *placeholder = makeNode(CaseTestExpr);
+
+ placeholder->typeId = exprType(expr);
+ placeholder->typeMod = exprTypmod(expr);
+ placeholder->collation = exprCollation(expr);
+
+ return (Node *) placeholder;
+}
+
/*
* Transform JSON value expression using specified input JSON format or
* default format otherwise, coercing to the targettype if needed.
@@ -3354,9 +3369,12 @@ transformJsonValueExpr(ParseState *pstate, const char *constructName,
if (format != JS_FORMAT_DEFAULT ||
(OidIsValid(targettype) && exprtype != targettype))
{
+ Node *orig = makeCaseTestExpr(expr);
Node *coerced;
bool only_allow_cast = OidIsValid(targettype);
+ expr = orig;
+
/*
* PASSING args are handled appropriately by GetJsonPathVar() /
* JsonItemFromDatum().
@@ -3416,7 +3434,7 @@ transformJsonValueExpr(ParseState *pstate, const char *constructName,
coerced = (Node *) fexpr;
}
- if (coerced == expr)
+ if (coerced == orig)
expr = rawexpr;
else
{
@@ -3666,15 +3684,7 @@ makeJsonConstructorExpr(ParseState *pstate, JsonConstructorType type,
* that is of type JSON or JSONB to the coercion function.
*/
if (fexpr)
- {
- CaseTestExpr *cte = makeNode(CaseTestExpr);
-
- cte->typeId = exprType((Node *) fexpr);
- cte->typeMod = exprTypmod((Node *) fexpr);
- cte->collation = exprCollation((Node *) fexpr);
-
- placeholder = (Node *) cte;
- }
+ placeholder = makeCaseTestExpr((Node *) fexpr);
else
{
CaseTestExpr *cte = makeNode(CaseTestExpr);
@@ -4042,7 +4052,7 @@ transformJsonParseArg(ParseState *pstate, Node *jsexpr, JsonFormat *format,
{
JsonValueExpr *jve;
- expr = raw_expr;
+ expr = makeCaseTestExpr(raw_expr);
expr = makeJsonByteaToTextConversion(expr, format, exprLocation(expr));
*exprtype = TEXTOID;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index ea47652adb..fa3cbc858f 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1338,7 +1338,7 @@ typedef struct CaseWhen
* * Nested FieldStore/SubscriptingRef assignment expressions in INSERT/UPDATE;
* see transformAssignmentIndirection().
* * Placeholder for intermediate results in some SQL/JSON expression nodes,
- * such as JsonConstructorExpr.
+ * such as JsonConstructorExpr.coercion, JsonValueExpr.formatted_expr.
*
* The uses in CaseExpr and ArrayCoerceExpr are safe only to the extent that
* there is not any other CaseExpr or ArrayCoerceExpr between the value source
@@ -1669,15 +1669,15 @@ typedef struct JsonReturning
* JsonValueExpr -
* representation of JSON value expression (expr [FORMAT JsonFormat])
*
- * The actual value is obtained by evaluating formatted_expr. raw_expr is
- * only there for displaying the original user-written expression and is not
- * evaluated by ExecInterpExpr() and eval_const_expressions_mutator().
+ * formatted_expr, if needed, contains a formatting expression such as
+ * CoerceViaIO, where the argument is a CaseTestExpr. At runtime, this
+ * CaseTestExpr points to the value obtained by evaluating raw_expr.
*/
typedef struct JsonValueExpr
{
NodeTag type;
Expr *raw_expr; /* raw expression */
- Expr *formatted_expr; /* formatted expression */
+ Expr *formatted_expr; /* formatted expression or NULL */
JsonFormat *format; /* FORMAT clause, if specified */
} JsonValueExpr;
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..5fe5968b42 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,53 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ QUERY PLAN
+------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one, stable_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..031cab7365 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,18 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one, stable_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-17 20:30 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 01:34 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 2 replies; 31+ messages in thread
From: Tom Lane @ 2024-10-17 20:30 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Oct 17, 2024 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In the short term, I suspect the only workable fix is to undo the
>> optimization of having ExecInitExprRec not recurse into both raw_expr
>> and formatted_expr.
> This or actually I'm tempted to simply revert the whole thing
> (b6e1157e7d3) as an ill-considered refactoring, because I am not able
> to convince myself that calling ExecAggPlainTransByVal() twice, via
> both raw_expr and formatted_expr, is always safe.
Not following the concern here? As far as nodeAgg is concerned,
they'd be two independent aggregates.
In any case, whatever we do in master, you can't "simply revert"
b6e1157e7 in released branches. It changed the way JsonValueExpr is
represented in stored rules, and you don't get to undo that midstream.
(The commit should have included a catversion bump, in fact.)
I do think that reverting the ExecInitExprRec and
eval_const_expressions_mutator changes would be good, but we
can't undo the parser changes, at least not in v16/v17.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-18 01:34 ` Tender Wang <tndrwang@gmail.com>
2024-10-18 01:46 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 31+ messages in thread
From: Tender Wang @ 2024-10-18 01:34 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlangote09@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tom Lane <tgl@sss.pgh.pa.us> 于2024年10月18日周五 04:30写道：
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Oct 17, 2024 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> In the short term, I suspect the only workable fix is to undo the
> >> optimization of having ExecInitExprRec not recurse into both raw_expr
> >> and formatted_expr.
>
> > This or actually I'm tempted to simply revert the whole thing
> > (b6e1157e7d3) as an ill-considered refactoring, because I am not able
> > to convince myself that calling ExecAggPlainTransByVal() twice, via
> > both raw_expr and formatted_expr, is always safe.
>
> Not following the concern here? As far as nodeAgg is concerned,
> they'd be two independent aggregates.
>
> In any case, whatever we do in master, you can't "simply revert"
> b6e1157e7 in released branches. It changed the way JsonValueExpr is
> represented in stored rules, and you don't get to undo that midstream.
>
Sorry, I can't fully understand what you said above. What's the stored
rule?
And "you don't get to undo that midstream." What is the Scenario that we
get to undo?
Can you give more explanation? Thanks.
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 01:34 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
@ 2024-10-18 01:46 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 31+ messages in thread
From: Tom Lane @ 2024-10-18 01:46 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: Amit Langote <amitlangote09@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> writes:
>> In any case, whatever we do in master, you can't "simply revert"
>> b6e1157e7 in released branches. It changed the way JsonValueExpr is
>> represented in stored rules, and you don't get to undo that midstream.
> Sorry, I can't fully understand what you said above. What's the stored
> rule?
> And "you don't get to undo that midstream." What is the Scenario that we
> get to undo?
If somebody has created a view that contains a JsonValueExpr, then
the post-parse-analysis query tree for that is stored in pg_rewrite.
A minor version update can't change that query tree. So basically,
once a major version has shipped, its parse-analysis output format
is frozen.
We can redefine that sort of thing in major releases, because either
pg_upgrade or dump/reload will result in re-parsing view definitions.
But that doesn't happen in minor-version updates.
This restriction doesn't apply to planner output trees, since those
don't have any lifespan longer than a session. That's how come it's
okay to consider changing the behavior of eval_const_expressions.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-18 02:04 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 02:07 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
1 sibling, 2 replies; 31+ messages in thread
From: Amit Langote @ 2024-10-18 02:04 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 18, 2024 at 5:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Oct 17, 2024 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> In the short term, I suspect the only workable fix is to undo the
> >> optimization of having ExecInitExprRec not recurse into both raw_expr
> >> and formatted_expr.
>
> > This or actually I'm tempted to simply revert the whole thing
> > (b6e1157e7d3) as an ill-considered refactoring, because I am not able
> > to convince myself that calling ExecAggPlainTransByVal() twice, via
> > both raw_expr and formatted_expr, is always safe.
>
> Not following the concern here? As far as nodeAgg is concerned,
> they'd be two independent aggregates.
Ok, thanks for clarifying that. I can see that both instances of the
Aggref contain identical values in this case and only the latter gets
*used*.
> In any case, whatever we do in master, you can't "simply revert"
> b6e1157e7 in released branches. It changed the way JsonValueExpr is
> represented in stored rules, and you don't get to undo that midstream.
> (The commit should have included a catversion bump, in fact.)
While I think I understand the general rule around when to bump the
catversion, I’m not entirely sure how this particular change or its
reversion would break anything. get_rule_expr() doesn’t, and didn’t
previously, inspect formatted_expr, which is where the content will
change. That said, I will err on the side of following the general
rule here. :-)
> I do think that reverting the ExecInitExprRec and
> eval_const_expressions_mutator changes would be good,
What was there in eval_const_expressions_mutator() before b6e1157e was
there because only raw_expr contained the actual expression and
formatted_expr only a wrapper for coercion, but that's not true after
b6e1157e. So I don't think we can revert
eval_const_expressions_mutator() changes. The expression tree in
raw_expr does get the eval_const_expressions_mutator() treatment via
ece_generic_processing() at the bottom.
> but we can't undo the parser changes, at least not in v16/v17.
Ok, understood.
I think we can apply the attached down to v16.
--
Thanks, Amit Langote
Attachments:
v4-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/octet-stream, 7.1K) download
From 27d45cbf9c3cdbb8efef344b43dd44cfaf8f4cee Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 17 Oct 2024 15:15:08 +0900
Subject: [PATCH v4] SQL/JSON: Fix some oversights in commit b6e1157e7
Initialize JsonValueExpr.raw_expr in ExecInitExprRec() even if it
should not be touched by
expression_tree_walker() and expression_tree_mutator(). One problem
it causes, as demonstrated in BUG #18657, is that the Aggref nodes
referenced in formatted_expr, which is evaluated, end up with the
wrong value of aggtransno. This occurs because the Aggref's copy in
raw_expr is also added to PlannerInfo.aggtransinfos by
preprocess_aggref(), leading to incorrect state in the planner.
This can result in a crash during ExecBuildAggTrans(), as seen in the
bug report.
Outputs of some tests in sqljson.out is modified to account for the
fact that raw_expr remains unchanged during
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/executor/execExpr.c | 2 ++
src/backend/optimizer/util/clauses.c | 3 +-
src/test/regress/expected/sqljson.out | 50 +++++++++++++++++++++++++++
src/test/regress/sql/sqljson.sql | 15 ++++++++
4 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c8077aa57b..b8d69536ac 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
+ Assert(jve->raw_expr != NULL);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
Assert(jve->formatted_expr != NULL);
ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
break;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..07edc02df3 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2917,9 +2917,10 @@ eval_const_expressions_mutator(Node *node,
JsonValueExpr *jve = (JsonValueExpr *) node;
Node *formatted;
+ Assert(jve->formatted_expr);
formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
context);
- if (formatted && IsA(formatted, Const))
+ if (IsA(formatted, Const))
return formatted;
break;
}
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..5fe5968b42 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,53 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ QUERY PLAN
+------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one, stable_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..031cab7365 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,18 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one, stable_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-18 02:07 ` Amit Langote <amitlangote09@gmail.com>
1 sibling, 0 replies; 31+ messages in thread
From: Amit Langote @ 2024-10-18 02:07 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 18, 2024 at 11:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> I think we can apply the attached down to v16.
Sorry, I sent the patch without finishing my rewrite of the commit message.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-18 04:38 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
1 sibling, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-18 04:38 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 18, 2024 at 11:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Oct 18, 2024 at 5:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Amit Langote <amitlangote09@gmail.com> writes:
> > > On Thu, Oct 17, 2024 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> In the short term, I suspect the only workable fix is to undo the
> > >> optimization of having ExecInitExprRec not recurse into both raw_expr
> > >> and formatted_expr.
> >
> > > This or actually I'm tempted to simply revert the whole thing
> > > (b6e1157e7d3) as an ill-considered refactoring, because I am not able
> > > to convince myself that calling ExecAggPlainTransByVal() twice, via
> > > both raw_expr and formatted_expr, is always safe.
> >
> > Not following the concern here? As far as nodeAgg is concerned,
> > they'd be two independent aggregates.
>
> Ok, thanks for clarifying that. I can see that both instances of the
> Aggref contain identical values in this case and only the latter gets
> *used*.
>
> > In any case, whatever we do in master, you can't "simply revert"
> > b6e1157e7 in released branches. It changed the way JsonValueExpr is
> > represented in stored rules, and you don't get to undo that midstream.
> > (The commit should have included a catversion bump, in fact.)
>
> While I think I understand the general rule around when to bump the
> catversion, I’m not entirely sure how this particular change or its
> reversion would break anything. get_rule_expr() doesn’t, and didn’t
> previously, inspect formatted_expr, which is where the content will
> change. That said, I will err on the side of following the general
> rule here. :-)
>
> > I do think that reverting the ExecInitExprRec and
> > eval_const_expressions_mutator changes would be good,
>
> What was there in eval_const_expressions_mutator() before b6e1157e was
> there because only raw_expr contained the actual expression and
> formatted_expr only a wrapper for coercion, but that's not true after
> b6e1157e. So I don't think we can revert
> eval_const_expressions_mutator() changes. The expression tree in
> raw_expr does get the eval_const_expressions_mutator() treatment via
> ece_generic_processing() at the bottom.
>
> > but we can't undo the parser changes, at least not in v16/v17.
>
> Ok, understood.
>
> I think we can apply the attached down to v16.
Here's a version with an updated commit message and the comments of
JsonValueExpr struct definition updated to describe raw_expr and
formatted_expr more clearly.
--
Thanks, Amit Langote
Attachments:
v5-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/octet-stream, 8.0K) download
From 9763c1969c29bb9cf4f45855b540411cb06e14fb Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 18 Oct 2024 12:33:03 +0900
Subject: [PATCH v5] SQL/JSON: Fix some oversights in commit b6e1157e7
The decision in b6e1157e7 to ignore raw_expr when evaluating a
JsonValueExpr was incorrect. While its value is not ultimately
used (since formatted_expr's value is), failing to initialize it
can lead to problems, for instance, when the expression tree in
raw_expr contains Aggref nodes, which must be initialized to
ensure the parent Agg node works correctly.
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/executor/execExpr.c | 2 ++
src/backend/optimizer/util/clauses.c | 3 +-
src/include/nodes/primnodes.h | 11 +++---
src/test/regress/expected/sqljson.out | 50 +++++++++++++++++++++++++++
src/test/regress/sql/sqljson.sql | 15 ++++++++
5 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c8077aa57b..b8d69536ac 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
+ Assert(jve->raw_expr != NULL);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
Assert(jve->formatted_expr != NULL);
ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
break;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..07edc02df3 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2917,9 +2917,10 @@ eval_const_expressions_mutator(Node *node,
JsonValueExpr *jve = (JsonValueExpr *) node;
Node *formatted;
+ Assert(jve->formatted_expr);
formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
context);
- if (formatted && IsA(formatted, Const))
+ if (IsA(formatted, Const))
return formatted;
break;
}
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index ea47652adb..c8a41b81ef 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1669,15 +1669,16 @@ typedef struct JsonReturning
* JsonValueExpr -
* representation of JSON value expression (expr [FORMAT JsonFormat])
*
- * The actual value is obtained by evaluating formatted_expr. raw_expr is
- * only there for displaying the original user-written expression and is not
- * evaluated by ExecInterpExpr() and eval_const_expressions_mutator().
+ * raw_expr is the user-specified value, while formatted_expr is the value
+ * obtained by coercing raw_expr to the type required by either the FORMAT
+ * clause or an enclosing node's RETURNING clause. get_rule_expr() prints
+ * raw_expr when displaying the JsonValueExpr.
*/
typedef struct JsonValueExpr
{
NodeTag type;
- Expr *raw_expr; /* raw expression */
- Expr *formatted_expr; /* formatted expression */
+ Expr *raw_expr; /* user-specified raw expression */
+ Expr *formatted_expr; /* coerced formatted expression */
JsonFormat *format; /* FORMAT clause, if specified */
} JsonValueExpr;
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..5fe5968b42 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,53 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ QUERY PLAN
+------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one, stable_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..031cab7365 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,18 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was accidentally processed by the
+-- planner (preprocess_aggref()) causing the plan to carry Aggrefs containing
+-- incorrect / out-of-sync value of aggtransno. One symptom of that is the
+-- crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one, stable_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-18 05:03 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 06:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-18 05:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 18, 2024 at 1:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Oct 18, 2024 at 11:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Oct 18, 2024 at 5:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > I do think that reverting the ExecInitExprRec and
> > > eval_const_expressions_mutator changes would be good,
> >
> > What was there in eval_const_expressions_mutator() before b6e1157e was
> > there because only raw_expr contained the actual expression and
> > formatted_expr only a wrapper for coercion, but that's not true after
> > b6e1157e. So I don't think we can revert
> > eval_const_expressions_mutator() changes. The expression tree in
> > raw_expr does get the eval_const_expressions_mutator() treatment via
> > ece_generic_processing() at the bottom.
> >
> > > but we can't undo the parser changes, at least not in v16/v17.
> >
> > Ok, understood.
> >
> > I think we can apply the attached down to v16.
>
> Here's a version with an updated commit message and the comments of
> JsonValueExpr struct definition updated to describe raw_expr and
> formatted_expr more clearly.
Oops, had forgotten to update the regression test comment which still
contained the previous root cause description.
Updated again.
--
Thanks, Amit Langote
Attachments:
v6-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/octet-stream, 7.9K) download
From faee27c674ad05d91ac02b087f83877c205cb8ba Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 18 Oct 2024 12:33:03 +0900
Subject: [PATCH v6] SQL/JSON: Fix some oversights in commit b6e1157e7
The decision in b6e1157e7 to ignore raw_expr when evaluating a
JsonValueExpr was incorrect. While its value is not ultimately
used (since formatted_expr's value is), failing to initialize it
can lead to problems, for instance, when the expression tree in
raw_expr contains Aggref nodes, which must be initialized to
ensure the parent Agg node works correctly.
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/executor/execExpr.c | 2 ++
src/backend/optimizer/util/clauses.c | 3 +-
src/include/nodes/primnodes.h | 11 +++---
src/test/regress/expected/sqljson.out | 49 +++++++++++++++++++++++++++
src/test/regress/sql/sqljson.sql | 14 ++++++++
5 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c8077aa57b..b8d69536ac 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
+ Assert(jve->raw_expr != NULL);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
Assert(jve->formatted_expr != NULL);
ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
break;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..07edc02df3 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2917,9 +2917,10 @@ eval_const_expressions_mutator(Node *node,
JsonValueExpr *jve = (JsonValueExpr *) node;
Node *formatted;
+ Assert(jve->formatted_expr);
formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
context);
- if (formatted && IsA(formatted, Const))
+ if (IsA(formatted, Const))
return formatted;
break;
}
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index ea47652adb..c8a41b81ef 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1669,15 +1669,16 @@ typedef struct JsonReturning
* JsonValueExpr -
* representation of JSON value expression (expr [FORMAT JsonFormat])
*
- * The actual value is obtained by evaluating formatted_expr. raw_expr is
- * only there for displaying the original user-written expression and is not
- * evaluated by ExecInterpExpr() and eval_const_expressions_mutator().
+ * raw_expr is the user-specified value, while formatted_expr is the value
+ * obtained by coercing raw_expr to the type required by either the FORMAT
+ * clause or an enclosing node's RETURNING clause. get_rule_expr() prints
+ * raw_expr when displaying the JsonValueExpr.
*/
typedef struct JsonValueExpr
{
NodeTag type;
- Expr *raw_expr; /* raw expression */
- Expr *formatted_expr; /* formatted expression */
+ Expr *raw_expr; /* user-specified raw expression */
+ Expr *formatted_expr; /* coerced formatted expression */
JsonFormat *format; /* FORMAT clause, if specified */
} JsonValueExpr;
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..70721c9a5a 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,52 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
+-- causing the Aggrefs contained in it to also not be initialized, which led
+-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ QUERY PLAN
+------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one, stable_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..9b5d82602d 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,17 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
+-- causing the Aggrefs contained in it to also not be initialized, which led
+-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one, stable_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-18 06:14 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 17:22 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-18 06:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 18, 2024 at 2:03 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Oct 18, 2024 at 1:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Oct 18, 2024 at 11:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Fri, Oct 18, 2024 at 5:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > > I do think that reverting the ExecInitExprRec and
> > > > eval_const_expressions_mutator changes would be good,
> > >
> > > What was there in eval_const_expressions_mutator() before b6e1157e was
> > > there because only raw_expr contained the actual expression and
> > > formatted_expr only a wrapper for coercion, but that's not true after
> > > b6e1157e. So I don't think we can revert
> > > eval_const_expressions_mutator() changes. The expression tree in
> > > raw_expr does get the eval_const_expressions_mutator() treatment via
> > > ece_generic_processing() at the bottom.
> > >
> > > > but we can't undo the parser changes, at least not in v16/v17.
> > >
> > > Ok, understood.
> > >
> > > I think we can apply the attached down to v16.
> >
> > Here's a version with an updated commit message and the comments of
> > JsonValueExpr struct definition updated to describe raw_expr and
> > formatted_expr more clearly.
>
> Oops, had forgotten to update the regression test comment which still
> contained the previous root cause description.
>
> Updated again.
Another version which expands the comment of JsonValueExpr a little further.
--
Thanks, Amit Langote
Attachments:
v7-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/octet-stream, 8.1K) download
From f1ac9dd53b9f371aef9a8f3187a1a00f8502e38d Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 18 Oct 2024 15:03:12 +0900
Subject: [PATCH v7] SQL/JSON: Fix some oversights in commit b6e1157e7
The decision in b6e1157e7 to ignore raw_expr when evaluating a
JsonValueExpr was incorrect. While its value is not ultimately
used (since formatted_expr's value is), failing to initialize it
can lead to problems, for instance, when the expression tree in
raw_expr contains Aggref nodes, which must be initialized to
ensure the parent Agg node works correctly.
While at it, update the comment of JsonValueExpr to describe the
roles of raw_expr and formatted_expr more clearly.
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/executor/execExpr.c | 2 ++
src/backend/optimizer/util/clauses.c | 3 +-
src/include/nodes/primnodes.h | 14 +++++---
src/test/regress/expected/sqljson.out | 49 +++++++++++++++++++++++++++
src/test/regress/sql/sqljson.sql | 14 ++++++++
5 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c8077aa57b..b8d69536ac 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
+ Assert(jve->raw_expr != NULL);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
Assert(jve->formatted_expr != NULL);
ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
break;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..07edc02df3 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2917,9 +2917,10 @@ eval_const_expressions_mutator(Node *node,
JsonValueExpr *jve = (JsonValueExpr *) node;
Node *formatted;
+ Assert(jve->formatted_expr);
formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
context);
- if (formatted && IsA(formatted, Const))
+ if (IsA(formatted, Const))
return formatted;
break;
}
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index ea47652adb..99408157f9 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1669,15 +1669,19 @@ typedef struct JsonReturning
* JsonValueExpr -
* representation of JSON value expression (expr [FORMAT JsonFormat])
*
- * The actual value is obtained by evaluating formatted_expr. raw_expr is
- * only there for displaying the original user-written expression and is not
- * evaluated by ExecInterpExpr() and eval_const_expressions_mutator().
+ * raw_expr is the user-specified value, while formatted_expr is the value
+ * obtained by coercing raw_expr to the type required by either the FORMAT
+ * clause or an enclosing node's RETURNING clause.
+ *
+ * When deparsing a JsonValueExpr, get_rule_expr() prints raw_expr. However,
+ * during the evaluation of a JsonValueExpr, the value of formatted_expr
+ * takes precedence over that of raw_expr.
*/
typedef struct JsonValueExpr
{
NodeTag type;
- Expr *raw_expr; /* raw expression */
- Expr *formatted_expr; /* formatted expression */
+ Expr *raw_expr; /* user-specified raw expression */
+ Expr *formatted_expr; /* coerced formatted expression */
JsonFormat *format; /* FORMAT clause, if specified */
} JsonValueExpr;
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..70721c9a5a 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,52 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
+-- causing the Aggrefs contained in it to also not be initialized, which led
+-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ QUERY PLAN
+------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one, stable_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..9b5d82602d 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,17 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
+-- causing the Aggrefs contained in it to also not be initialized, which led
+-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one, stable_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 06:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-18 17:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-19 03:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Tom Lane @ 2024-10-18 17:22 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> writes:
> Another version which expands the comment of JsonValueExpr a little further.
I still don't like this approach to eval_const_expressions one bit.
It's undocumented and it pessimizes as many cases as it optimizes.
Sure, in the case where we can fold formatted_expr to a constant,
we avoid processing raw_expr at all --- but if we fail to do that,
we end by recursively simplifying formatted_expr twice (and raw_expr
once). That's not a win.
I think it should look more like
/*
* If we can fold formatted_expr to a constant, we can elide
* the JsonValueExpr altogether. Otherwise we must process
* raw_expr too. But JsonFormat is a flat node and requires
* no simplification, only copying.
*/
formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
context);
if (formatted && IsA(formatted, Const))
return formatted;
newnode = makeNode(JsonValueExpr);
newnode->formatted_expr = formatted;
newnode->raw_expr = eval_const_expressions_mutator((Node *) jve->raw_expr,
context);
newnode->format = copyObject(jve->format);
return newnode;
(Untested, probably requires more casts than I wrote.)
Also, in primnodes.h, personally I'd write
+ Expr *raw_expr; /* user-specified expression */
"raw" is a misnomer here, and even if we're not going to rename the
field, we're not helping anyone by using that word in the comment.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 06:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 17:22 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-19 03:12 ` Amit Langote <amitlangote09@gmail.com>
2024-10-19 16:05 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 31+ messages in thread
From: Amit Langote @ 2024-10-19 03:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Oct 19, 2024 at 2:22 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > Another version which expands the comment of JsonValueExpr a little further.
Thanks for the review.
> I still don't like this approach to eval_const_expressions one bit.
> It's undocumented and it pessimizes as many cases as it optimizes.
> Sure, in the case where we can fold formatted_expr to a constant,
> we avoid processing raw_expr at all --- but if we fail to do that,
> we end by recursively simplifying formatted_expr twice (and raw_expr
> once). That's not a win.
Ah, I hadn't considered that. Good point.
> I think it should look more like
>
> /*
> * If we can fold formatted_expr to a constant, we can elide
> * the JsonValueExpr altogether. Otherwise we must process
> * raw_expr too. But JsonFormat is a flat node and requires
> * no simplification, only copying.
> */
> formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
> context);
> if (formatted && IsA(formatted, Const))
> return formatted;
> newnode = makeNode(JsonValueExpr);
> newnode->formatted_expr = formatted;
> newnode->raw_expr = eval_const_expressions_mutator((Node *) jve->raw_expr,
> context);
> newnode->format = copyObject(jve->format);
> return newnode;
I decided to use makeJsonValueExpr() here.
> Also, in primnodes.h, personally I'd write
>
> + Expr *raw_expr; /* user-specified expression */
>
> "raw" is a misnomer here, and even if we're not going to rename the
> field, we're not helping anyone by using that word in the comment.
Good point.
Updated patch attached.
--
Thanks, Amit Langote
Attachments:
v8-0001-SQL-JSON-Fix-some-oversights-in-commit-b6e1157e7.patch (application/octet-stream, 9.3K) download
From fc6aeebbbf149ca278491aff1f314a26307b2e29 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Sat, 19 Oct 2024 11:34:48 +0900
Subject: [PATCH v8] SQL/JSON: Fix some oversights in commit b6e1157e7
The decision in b6e1157e7 to ignore raw_expr when evaluating a
JsonValueExpr was incorrect. While its value is not ultimately
used (since formatted_expr's value is), failing to initialize it
can lead to problems, for instance, when the expression tree in
raw_expr contains Aggref nodes, which must be initialized to
ensure the parent Agg node works correctly.
Also, optimize eval_const_expressions_mutator()'s handling of
JsonValueExpr a bit. Currently, when formatted_expr cannot be folded
into a constant, we end up processing it twice -- once directly in
eval_const_expressions_mutator() and again recursively via
ece_generic_processing(). This recursive processing is required to
handle raw_expr. To avoid this redundant recursive processing, we
now process raw_expr directly in eval_const_expressions_mutator().
Finally, update the comment of JsonValueExpr to describe the
roles of raw_expr and formatted_expr more clearly.
Bug: #18657
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Diagnosed-by: Fabio R. Sluzala <fabio3rs@gmail.com>
Diagnosed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
Backpatch-through: 16
---
src/backend/executor/execExpr.c | 2 ++
src/backend/optimizer/util/clauses.c | 26 ++++++++++----
src/include/nodes/primnodes.h | 14 +++++---
src/test/regress/expected/sqljson.out | 49 +++++++++++++++++++++++++++
src/test/regress/sql/sqljson.sql | 14 ++++++++
5 files changed, 94 insertions(+), 11 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index a343d0bc6a..45954d979f 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2307,6 +2307,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
JsonValueExpr *jve = (JsonValueExpr *) node;
+ Assert(jve->raw_expr != NULL);
+ ExecInitExprRec(jve->raw_expr, state, resv, resnull);
Assert(jve->formatted_expr != NULL);
ExecInitExprRec(jve->formatted_expr, state, resv, resnull);
break;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b4e085e9d4..ee57c0a9dc 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2915,13 +2915,27 @@ eval_const_expressions_mutator(Node *node,
case T_JsonValueExpr:
{
JsonValueExpr *jve = (JsonValueExpr *) node;
- Node *formatted;
+ Node *raw_expr = (Node *) jve->raw_expr;
+ Node *formatted_expr = (Node *) jve->formatted_expr;
- formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
- context);
- if (formatted && IsA(formatted, Const))
- return formatted;
- break;
+ /*
+ * If we can fold formatted_expr to a constant, we can elide
+ * the JsonValueExpr altogether. Otherwise we must process
+ * raw_expr too. But JsonFormat is a flat node and requires
+ * no simplification, only copying.
+ */
+ Assert(formatted_expr);
+ formatted_expr = eval_const_expressions_mutator(formatted_expr,
+ context);
+ if (formatted_expr && IsA(formatted_expr, Const))
+ return formatted_expr;
+
+ Assert(raw_expr);
+ raw_expr = eval_const_expressions_mutator(raw_expr, context);
+
+ return (Node *) makeJsonValueExpr((Expr *) raw_expr,
+ (Expr *) formatted_expr,
+ copyObject(jve->format));
}
case T_SubPlan:
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 5b7b87dea5..b0ef1952e8 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1669,15 +1669,19 @@ typedef struct JsonReturning
* JsonValueExpr -
* representation of JSON value expression (expr [FORMAT JsonFormat])
*
- * The actual value is obtained by evaluating formatted_expr. raw_expr is
- * only there for displaying the original user-written expression and is not
- * evaluated by ExecInterpExpr() and eval_const_expressions_mutator().
+ * raw_expr is the user-specified value, while formatted_expr is the value
+ * obtained by coercing raw_expr to the type required by either the FORMAT
+ * clause or an enclosing node's RETURNING clause.
+ *
+ * When deparsing a JsonValueExpr, get_rule_expr() prints raw_expr. However,
+ * during the evaluation of a JsonValueExpr, the value of formatted_expr
+ * takes precedence over that of raw_expr.
*/
typedef struct JsonValueExpr
{
NodeTag type;
- Expr *raw_expr; /* raw expression */
- Expr *formatted_expr; /* formatted expression */
+ Expr *raw_expr; /* user-specified expression */
+ Expr *formatted_expr; /* coerced formatted expression */
JsonFormat *format; /* FORMAT clause, if specified */
} JsonValueExpr;
diff --git a/src/test/regress/expected/sqljson.out b/src/test/regress/expected/sqljson.out
index 9adfbe15f6..70721c9a5a 100644
--- a/src/test/regress/expected/sqljson.out
+++ b/src/test/regress/expected/sqljson.out
@@ -1300,3 +1300,52 @@ SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
ERROR: value too long for type character(2)
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
ERROR: value for domain sqljson_char2 violates check constraint "sqljson_char2_check"
+-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
+-- causing the Aggrefs contained in it to also not be initialized, which led
+-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : volatile_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : stable_one() RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ QUERY PLAN
+------------------------------------------------------------------------------------------------
+ Aggregate
+ Output: JSON_OBJECT('a' : JSON_OBJECTAGG('b' : 1 RETURNING text) FORMAT JSON RETURNING json)
+ -> Result
+(3 rows)
+
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+ json_object
+---------------------
+ {"a" : { "b" : 1 }}
+(1 row)
+
+DROP FUNCTION volatile_one, stable_one;
diff --git a/src/test/regress/sql/sqljson.sql b/src/test/regress/sql/sqljson.sql
index 42dbec26d6..9b5d82602d 100644
--- a/src/test/regress/sql/sqljson.sql
+++ b/src/test/regress/sql/sqljson.sql
@@ -480,3 +480,17 @@ SELECT JSON_OBJECTAGG(i: ('111' || i)::bytea FORMAT JSON WITH UNIQUE RETURNING v
CREATE DOMAIN sqljson_char2 AS char(2) CHECK (VALUE NOT IN ('12'));
SELECT JSON_SERIALIZE('123' RETURNING sqljson_char2);
SELECT JSON_SERIALIZE('12' RETURNING sqljson_char2);
+
+-- Bug #18657: JsonValueExpr.raw_expr was not initialized in ExecInitExprRec()
+-- causing the Aggrefs contained in it to also not be initialized, which led
+-- to a crash in ExecBuildAggTrans() as mentioned in the bug report:
+-- https://postgr.es/m/18657-1b90ccce2b16bdb8@postgresql.org
+CREATE FUNCTION volatile_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql VOLATILE;
+CREATE FUNCTION stable_one() RETURNS int AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': volatile_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': stable_one() RETURNING text) FORMAT JSON);
+EXPLAIN (VERBOSE, COSTS OFF) SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+SELECT JSON_OBJECT('a': JSON_OBJECTAGG('b': 1 RETURNING text) FORMAT JSON);
+DROP FUNCTION volatile_one, stable_one;
--
2.43.0
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 06:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 17:22 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-19 03:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
@ 2024-10-19 16:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-20 03:41 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Tom Lane @ 2024-10-19 16:05 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Amit Langote <amitlangote09@gmail.com> writes:
> Updated patch attached.
v8 is OK by me. Personally I would not bother with the Asserts
that raw_expr/formatted_expr are not null, in either place;
the code doesn't actually depend on that, and IMO it's inconsistent
with the style of surrounding code. But that's not a hill to die on.
regards, tom lane
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 04:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 06:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-18 17:22 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-19 03:12 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-19 16:05 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-20 03:41 ` Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 31+ messages in thread
From: Amit Langote @ 2024-10-20 03:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sun, Oct 20, 2024 at 1:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > Updated patch attached.
>
> v8 is OK by me. Personally I would not bother with the Asserts
> that raw_expr/formatted_expr are not null, in either place;
> the code doesn't actually depend on that, and IMO it's inconsistent
> with the style of surrounding code. But that's not a hill to die on.
Pushed after removing the Assert and tweaking the commit message a bit.
Thanks a lot for looking at this.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-16 00:19 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:26 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-17 04:18 ` Tender Wang <tndrwang@gmail.com>
2 siblings, 0 replies; 31+ messages in thread
From: Tender Wang @ 2024-10-17 04:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlangote09@gmail.com>; Michael Paquier <michael@paquier.xyz>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tom Lane <tgl@sss.pgh.pa.us> 于2024年10月17日周四 08:56写道：
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Wed, Oct 16, 2024 at 11:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Would it be better for parse analysis to explicitly NULL out this
> >> field once it's done looking at it? Carrying unmaintained pieces
> >> of an expression tree around seems both inefficient and prone to
> >> future failures of this same ilk. The further the raw_expr gets
> >> out of step with current reality, the worse the hazards.
>
> > It seems we can't do that in this case, because get_rule_expr() wants
> > to read it.
>
> I think you just increased my level of concern. Aren't you saying
> that get_rule_expr is likely to deliver utter garbage, because it
> will be working from an expression that has not tracked
> transformations made to other parts of the tree?
>
> Also, I see that ruleutils is applying get_rule_expr to raw_expr,
> which AFAICT means it's *not* raw-parser output, else that wouldn't
> work at all. Which means the field is badly named and incorrectly
> documented. But in particular it means you cannot just make the
> tree-walking routines ignore it --- you *must* update it during
> transformations such as subquery pullup, or you will have garbage
> that will cause EXPLAIN to crash or at least produce wrong results.
>
When I first see the "raw_expr", I think it is output after syntax
parsing.
But it is not. The "raw_expr" is transformed in transformJsonValueExpr().
And the formated_expr is different from raw_expr when the targettype is not
same with the expr's type.
I do some codes hack that make the raw_expr store the output of syntax
parsing not
transformed result. As you said above, explain will report error.
In other words, the proposed patch is dangerously wrong. I suspect at
> this point that the true bug might be the opposite: somebody feeling
> that they could dispense with updating raw_expr when they shouldn't.
> In particular, I think "... is not evaluated by
> eval_const_expressions_mutator()" is already a huge red flag.
> There are transformations that eval_const_expressions does that
> are not optional.
>
Can we make the raw_expr just save the raw_parser output, and formated_expr
store the transformed output, and we only touch the formated_expr when need
to
evaluating expression?
--
Thanks,
Tender Wang
^ permalink raw reply [nested|flat] 31+ messages in thread
end of thread, other threads:[~2024-10-20 03:41 UTC | newest]
Thread overview: 31+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-10-15 11:00 BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault PG Bug reporting form <noreply@postgresql.org>
2024-10-15 22:24 ` Fabio R. Sluzala <fabio3rs@gmail.com>
2024-10-16 00:19 ` Michael Paquier <michael@paquier.xyz>
2024-10-16 00:35 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 08:20 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 09:04 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 09:06 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:11 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 09:14 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 09:18 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 12:00 ` Amit Langote <amitlangote09@gmail.com>
2024-10-16 14:14 ` Tender Wang <tndrwang@gmail.com>
2024-10-16 14:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 00:38 ` Amit Langote <amitlangote09@gmail.com>
2024-10-17 00:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 03:52 ` Amit Langote <amitlangote09@gmail.com>
2024-10-17 04:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-17 08:15 ` Amit Langote <amitlangote09@gmail.com>
2024-10-17 20:30 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 01:34 ` Tender Wang <tndrwang@gmail.com>
2024-10-18 01:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-18 02:04 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 02:07 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 04:38 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 05:03 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 06:14 ` Amit Langote <amitlangote09@gmail.com>
2024-10-18 17:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-19 03:12 ` Amit Langote <amitlangote09@gmail.com>
2024-10-19 16:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-20 03:41 ` Amit Langote <amitlangote09@gmail.com>
2024-10-17 04:18 ` Tender Wang <tndrwang@gmail.com>
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