Thread: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault

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.

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

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

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

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

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

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

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

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

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

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

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

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

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