From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Michael Paquier <michael@paquier.xyz>, exclusion@gmail.com,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17800: ON CONFLICT DO UPDATE fails to detect incompatible
 fields that leads to a server crash
In-reply-to: <20230221204201.idad3kjv3ue7gb6s@awork3.anarazel.de>
References: <17800-ff90866b3906c964@postgresql.org>
 <Y/P49Yov2m+VzfFf@paquier.xyz>
 <20230221193352.mvzpuvoujfmckvr3@awork3.anarazel.de>
 <3990862.1677010571@sss.pgh.pa.us>
 <20230221204201.idad3kjv3ue7gb6s@awork3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Tue, 21 Feb 2023 12:42:01 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3995668.1677012915.1@sss.pgh.pa.us>
Date: Tue, 21 Feb 2023 15:55:15 -0500
Message-ID: <3995669.1677012915@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3995669.1677012915%40sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.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.

>> If we did have a local-to-the-expression ParamExecData array, maybe that
>> could be used to get a cleaner fix for things like the domain VALUES
>> and case-test-expression hacks.

> Hm, I'm not quite following along here.

I'm just arm-waving at this point, it's not real clear to me either.
But I do remember that we have some ugly hacks centered around the
fact that domain VALUES and CaseTestExpr are implemented with a single
datum slot per EContext.  I'd rather convert them into something like
PARAM_EXEC with no sharing.

			regards, tom lane