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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 于2024年10月17日周四 08:56写道：

> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > On Wed, Oct 16, 2024 at 11:26 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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