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: <20230224015417.75yimxbksejpffh3@awork3.anarazel.de>
References: <4060951.1677018870@sss.pgh.pa.us>
 <20230221233349.lj3btvtyqog5jzxd@awork3.anarazel.de>
 <4111538.1677024007@sss.pgh.pa.us>
 <20230222011705.sv75mkhvmxuqv22l@awork3.anarazel.de>
 <20230222192350.wviaslfpo3w2ja3l@awork3.anarazel.de>
 <290812.1677101054@sss.pgh.pa.us> <586533.1677178414@sss.pgh.pa.us>
 <20230223210012.4qigvo6sryn2syu5@awork3.anarazel.de>
 <718944.1677189963@sss.pgh.pa.us>
 <20230223233636.zkt4tqzsc4b6ekwn@awork3.anarazel.de>
 <20230224015417.75yimxbksejpffh3@awork3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Thu, 23 Feb 2023 17:54:17 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <851178.1677259566.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 24 Feb 2023 12:26:06 -0500
Message-ID: <851179.1677259566@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/851179.1677259566%40sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
>> When we build the evaluation step for the Param, we don't yet know that=
 we're
>> dealing with a MULTIEXPR (nor do we have a reference to the relevant
>> SubPlan)). At the end of the targetlist, we have a special SubPlan, whi=
ch make
>> ExecInitSubPlan() set ParamExecData->execPlan to its SubPlanState for a=
ll the
>> output parameters, to let ExecEvalParamExec know that the first referen=
ce to
>> one of the output params needs to evalute the plan. But that means that=
 we
>> need to reset execPlan between rows, which is handled by the no-output
>> ExecScanSubPlan() invocation at the end of the targetlist.  That just s=
eems
>> baroque.

Yup, it absolutely is.  This idea of having the expression compiler just
reorder the tlist entries is definitely interesting.  I recall that I
wondered about whether we could do that when I first made the MULTIEXPR
patch, but doing it in the parse tree causes a lot of problems because
there are places that assume resjunk entries come after not-resjunk ones.
I don't see a reason why we couldn't reorder during compile though ---
and that will work in all the branches we still support.

The main concern I've got about this prototype is that it's not clear
to me whether we can back-patch addition of a new EEOP step type without
causing ABI issues.  However, why do we need a new step type?  Seems to
me that EEOP_SUBPLAN will serve fine, if we just undo the special
treatment of MULTIEXPR in ExecScanSubPlan and let it go ahead and
evaluate the subplan and assign param values.

> There's at least one case in the regression tests where a correlated MUL=
TIEXPR
> is in a non-resjunk TLE. I assume due to subquery pushdown.  Is there a
> problem with that?  I don't immediately see any, but though it's worth
> mentioning.

My recollection is that the planner is pretty cavalier about whether
resjunk entries get marked as such in lower plan levels.  I wouldn't
worry about this (but by the same token, don't do anything that
relies on the resjunk marks being accurate).

> I didn't do the part about evaluating the 'input' parameters as part of =
the
> outer ExprState. Still think that's a good idea, but it's somewhat ortho=
gonal
> to the problems we're trying to fix.

Agreed, that's nothing to be doing in a bug-fix patch.  I think we just
want to re-order the steps to put the EEOP_SUBPLAN at the front of the
tlist, and then get rid of the execPlan manipulations and the other
special-casing of MULTIEXPR.  Anything else would be HEAD-only.

Are you planning to push forward with this, or do you want me to?
It's really my bug, since the existing MULTIEXPR implementation
is my fault.

			regards, tom lane