Hi,
On 2025-05-20 10:59:22 -0400, Andres Freund wrote:
> On 2025-05-18 19:31:33 -0400, Tom Lane wrote:
> > While chasing down Valgrind leakage reports, I was disturbed
> > to realize that some of them arise from a case where the
> > executor scribbles on the plan tree it's given, which it is
> > absolutely not supposed to do:
> >
> > /*
> > * Initialize result tuple slot and assign its rowtype using the
> > first
> > * RETURNING list. We assume the rest will look the same.
> > */
> > mtstate->ps.plan->targetlist = (List *) linitial(returningLists);
> >
> > A bit of git archaeology fingers Andres' commit 4717fdb14, which we
> > can't easily revert since he later got rid of ExecAssignResultType
> > altogether. But I think we need to do something about it --- it's
> > purest luck that this doesn't cause serious problems in some cases.
>
> I have no idea what I was smoking at that time, this clearly is wrong.
>
> I think the reason it doesn't cause problems is that we're just using the
> first child's targetlist every time and we're just assigning the same value
> back every time.
>
>
> What's even more absurd is: Why do we even need to assign the result type at
> all? Before & after 4717fdb14. The planner ought to have already figured this
> out, no?
>
> It seems that if not, we'd have a problem anyway, who says the "calling" nodes
> (say a wCTE) can cope with whatever output we come up with?
>
> Except of course, there is exactly one case in our tests where the tupledescs
> aren't equal :(
That's a harmless difference, as it turns out. The varnos / varattnos differ,
but that's fine, because we don't actually build the projection with that
tlist, we just use to allocate the slot, and that doesn't care about the
tlist. The building of the projection uses the specific child's returning
list.
Afaict the mtstate->ps.plan->targetlist assignment, and the ExecTypeFromTL(),
ExecAssignResultTypeFromTL() before that, are completely superfluous. Am I
missing something?
Wonder if the targetlist assignment is superfluous made me wonder if we would
detect mismatches - and afaict we largely wouldn't. There's basically no
verification in ExecBuildProjectionInfo(). And indeed, adding something very
basic shows:
--- /home/andres/src/postgresql/src/test/regress/expected/merge.out
2025-04-06 22:54:14.078394968 -0400
+++
/srv/dev/build/postgres/m-dev-assert/testrun/regress/regress/results/merge.out
2025-05-20 11:51:51.549525728 -0400
@@ -2653,20 +2653,95 @@
MERGE into measurement m
USING new_measurement nm ON
(m.city_id = nm.city_id and m.logdate=nm.logdate)
WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE
WHEN MATCHED THEN UPDATE
SET peaktemp = greatest(m.peaktemp, nm.peaktemp),
unitsales = m.unitsales + coalesce(nm.unitsales, 0)
WHEN NOT MATCHED THEN INSERT
(city_id, logdate, peaktemp, unitsales)
VALUES (city_id, logdate, peaktemp, unitsales);
+WARNING: type mismatch: resno 1: slot type 0 vs expr type 23
+WARNING: TargetEntry:
+DETAIL: {TARGETENTRY
+ :expr
+ {VAR
+ :varno -1
+ :varattno 3
+ :vartype 23
+ :vartypmod -1
+ :varcollid 0
+ :varnullingrels (b)
+ :varlevelsup 0
+ :varreturningtype 0
+ :varnosyn 2
+ :varattnosyn 1
+ :location 384
+ }
+ :resno 1
+ :resname city_id
+ :ressortgroupref 0
+ :resorigtbl 0
+ :resorigcol 0
+ :resjunk false
+ }
+
+WARNING: type mismatch: resno 2: slot type 23 vs expr type 1082
+WARNING: TargetEntry:
+DETAIL: {TARGETENTRY
+ :expr
+ {VAR
+ :varno -1
+ :varattno 4
+ :vartype 1082
+ :vartypmod -1
+ :varcollid 0
+ :varnullingrels (b)
+ :varlevelsup 0
+ :varreturningtype 0
+ :varnosyn 2
+ :varattnosyn 2
+ :location 393
+ }
+ :resno 2
+ :resname logdate
+ :ressortgroupref 0
+ :resorigtbl 0
+ :resorigcol 0
+ :resjunk false
+ }
+
+WARNING: type mismatch: resno 3: slot type 1082 vs expr type 23
+WARNING: TargetEntry:
+DETAIL: {TARGETENTRY
+ :expr
+ {VAR
+ :varno -1
+ :varattno 1
+ :vartype 23
+ :vartypmod -1
+ :varcollid 0
+ :varnullingrels (b)
+ :varlevelsup 0
+ :varreturningtype 0
+ :varnosyn 2
+ :varattnosyn 3
+ :location 402
+ }
+ :resno 3
...
Greetings,
Andres Freund
