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

Amit Langote <amitlangote09(at)gmail(dot)com> 于2024年10月16日周三 20:00写道：

> On Wed, Oct 16, 2024 at 6:19 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> > Amit Langote <amitlangote09(at)gmail(dot)com> 于2024年10月16日周三 17:14写道：
> >> On Wed, Oct 16, 2024 at 6:11 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> >> > Amit Langote <amitlangote09(at)gmail(dot)com> 于2024年10月16日周三 17:06写道：
> >> >>
> >> >> On Wed, Oct 16, 2024 at 5:20 PM Tender Wang <tndrwang(at)gmail(dot)com>
> wrote:
> >> >> > Amit Langote <amitlangote09(at)gmail(dot)com> 于2024年10月16日周三 08:35写道：
> >> >> >>
> >> >> >> On Wed, Oct 16, 2024 at 9:19 AM Michael Paquier <
> michael(at)paquier(dot)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