On Tue, 17 Dec 2024 at 07:10, Nathan Bossart <nathandboss...@gmail.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
v1-0001-Fix-incorrect-slot-type-in-BuildTupleHashTableExt.patch
Description: Binary data
