Re: BUG #17800: ON CONFLICT DO UPDATE fails to detect incompatible fields that leads to a server crash

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, exclusion(at)gmail(dot)com, pgsql-bugs(at)lists(dot)postgresql(dot)org
Subject: Re: BUG #17800: ON CONFLICT DO UPDATE fails to detect incompatible fields that leads to a server crash
Date: 2023-02-21 22:16:02
Message-ID: 20230221221602.42n3pheoafjsmouj@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

Hi,

On 2023-02-21 15:55:15 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2023-02-21 15:16:11 -0500, Tom Lane wrote:
> >> It occurs to me that one possible fix is to make MULTIEXPR_SUBLINK
> >> and the associated output Params use a separate ParamExecData array;
> >> instead of the query-wide es_param_exec_vals array, use one that
> >> is local to the specific targetlist's ExprState. I'm not sure how
> >> much violence that does to the current notion of an ExprState ---
> >> do we think that is read-only during execution?
>
> > I don't think you would need to modify ExprState - the information about
> > params etc comes from the ExprContext, right? So we'd need to build a
> > different ExprContext for partitions, and use that when evaluating the
> > expressions.
> > ...
> > We don't currently have infrastructure for setting
> > econtext->ecxt_param_exec_vals to something else, but that shouldn't be too
> > hard to add.
>
> No, that won't work, because many usages of PARAM_EXEC Params are
> specifically intended to transmit datums from one expression (plan node)
> to another. That's why that array was query-global to begin with.
> What I'm wondering about is adding a separate array, and likely a separate
> ParamKind, that would have a less-than-query-wide scope. We might be able
> to get away with having that be plan-node-wide, but making it local to the
> specific compiled expression feels safer and easier to reason about.

What I was trying to suggest is that you could have a dedicated ExprContext
that'd point to such a separate array. That'd allow you to choose the the
separate array on a per-expression-evaluation basis (not even per ExprState).
We already have multiple ExprContexts in some nodes, so this wouldn't break
new ground.

Greetings,

Andres Freund

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Tom Lane 2023-02-21 22:34:30 Re: BUG #17800: ON CONFLICT DO UPDATE fails to detect incompatible fields that leads to a server crash
Previous Message Tom Lane 2023-02-21 20:55:15 Re: BUG #17800: ON CONFLICT DO UPDATE fails to detect incompatible fields that leads to a server crash