Re: Pg18 Recursive Crash

On Tue, 17 Dec 2024 at 07:10, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> git-bisect is pointing me to https://postgr.es/c/0f57382. Here is the
> trace I'm seeing:
>
> TRAP: failed Assert("op->d.fetch.kind == slot->tts_ops"), File: "../postgresql/src/backend/executor/execExprInterp.c", Line: 2244, PID: 5031
> 0 postgres 0x000000010112d068 ExceptionalCondition + 108
> 1 postgres 0x0000000100e54f04 ExecInterpExpr + 604
> 2 postgres 0x0000000100e5bd50 LookupTupleHashEntry + 116
> 3 postgres 0x0000000100e8e580 ExecRecursiveUnion + 140

This is caused by me being overly optimistic about getting rid of the
slot deformation step in the ExprState evaluation for hashing. I
must've not fully understood the method of how hash table lookups are
performed for grouping requirements and mistakenly thought it was ok
to use &TTSOpsMinimalTuple for hashing the same as the equality code
is doing in ExecBuildGroupingEqual(). It turns out the ExprState built
by ExecBuildGroupingEqual() always uses slots with minimal tuples due
to how LookupTupleHashEntry_internal() always creates a hash table
entry without a key and then does ExecCopySlotMinimalTuple() to put
the tuple into the hash table after the hash bucket is reserved.
That's not the case for generating the hash value as that uses
whichever slot is passed to LookupTupleHashEntry().

To fix the reported crash, I propose adding a new parameter to
BuildTupleHashTableExt() to allow passing of the TupleTableSlotOps.
Really, I could just set scratch.d.fetch.kind to NULL in
ExecBuildHash32FromAttrs() so that the hashing ExprState is always
built with a deform step, but there are many cases (e.g notSubplan.c)
where a virtual slot is always used, so it would be nice to have some
way to pass the TupleTableSlotOps in for cases where it's known so
that the deform step can be eliminated when it's not needed.

The slightly annoying thing here is that the attached patch passes the
TupleTableSlotOps as NULL in nodeSetOp.c. Per nodeAppend.c line 186,
Append does not go to much effort to setting a fixed
TupleTableSlotOps. Really it could loop over all the child plans and
check if those have fixed slot types of the same type and then fix its
own resulting slot. For nodeSetOps.c use case, since the planner
(currently) injects the flag into the target list, it'll always
project and use a virtual slot type. It's maybe worth coming back and
adjusting nodeAppend.c so it works a bit harder to fix its slot type.
I think that's likely for another patch, however. Tom is also
currently working on nodeSetOps.c to change how all this works so it
no longer uses the flags method to determine the outer and inner
sides.

I plan to push the attached patch soon.

David