On Tue, 20 Aug 2024 at 13:40, David Rowley <dgrowle...@gmail.com> wrote:
> I made a few more tweaks to the comments and pushed the result.
While working on the patch to make HashAgg / hashed subplans and
hashed setops to use ExprState hashing, I discovered a bug in that
code when hashing 0 hashkeys (Possible with SELECT FROM t INTERSECT
SELECT FROM t;) where the initial value wasn't being stored properly.
I was storing it in the intermediate hash location expecting that the
step to process the final hash key would fetch that and store the
final result in ExprState.resvalue. That's clearly not going to work
when there are 0 expressions to hash. The correct thing to do is just
store the initial value in the ExprState.resvalue field directly when
there are zero expressions to hash.
The attached patch fixes the same mistake in ExecBuildHash32Expr().
This bug is only hypothetical as currently, nothing calls that
function passing a non-zero initial value.
I plan on pushing this shortly.
David
From 3bf35f185849e77f431fe2d351ce538cd1e9ca95 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrow...@gmail.com>
Date: Tue, 5 Nov 2024 12:29:33 +1300
Subject: [PATCH] Fix hypothetical bug in ExprState building for hashing
adf97c156 gave ExprStates the ability to hash expressions and return a
single hash value. That commit supports seeding the hash value with an
initial value to have that blended into the final hash value.
Here we fix a hypothetical bug where if there are zero expressions to
hash, the initial value is stored in the wrong location. The existing
code stored the initial value in an intermediate location expecting that
when the expressions were hashed that those steps would store the final
hash value in the ExprState.resvalue field. However, that wouldn't happen
when there are zero expressions to hash. The correct thing to do instead
is to have a special case for zero expressions and when we hit that case,
store the initial value directly in the ExprState.resvalue. The reason
that this is a hypothetical bug is that no code currently calls
ExecBuildHash32Expr passing a non-zero initial value.
---
src/backend/executor/execExpr.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 45954d979f..8f7a534005 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -4004,8 +4004,9 @@ ExecBuildHash32Expr(TupleDesc desc, const
TupleTableSlotOps *ops,
ListCell *lc2;
intptr_t strict_opcode;
intptr_t opcode;
+ int num_exprs = list_length(hash_exprs);
- Assert(list_length(hash_exprs) == list_length(collations));
+ Assert(num_exprs == list_length(collations));
state->parent = parent;
@@ -4013,11 +4014,11 @@ ExecBuildHash32Expr(TupleDesc desc, const
TupleTableSlotOps *ops,
ExecCreateExprSetupSteps(state, (Node *) hash_exprs);
/*
- * When hashing more than 1 expression or if we have an init value, we
- * need somewhere to store the intermediate hash value so that it's
- * available to be combined with the result of subsequent hashing.
+ * Make a place to store intermediate hash values between subsequent
+ * hashing of individual expressions. We only need this if there is
more
+ * than one expression to hash or an initial value plus one expression.
*/
- if (list_length(hash_exprs) > 1 || init_value != 0)
+ if ((int64) num_exprs + (init_value != 0) > 1)
iresult = palloc(sizeof(NullableDatum));
if (init_value == 0)
@@ -4032,11 +4033,15 @@ ExecBuildHash32Expr(TupleDesc desc, const
TupleTableSlotOps *ops,
}
else
{
- /* Set up operation to set the initial value. */
+ /*
+ * Set up operation to set the initial value. Normally we
store this
+ * in the intermediate hash value location, but if there are no
exprs
+ * to hash, store it in the ExprState's result field.
+ */
scratch.opcode = EEOP_HASHDATUM_SET_INITVAL;
scratch.d.hashdatum_initvalue.init_value =
UInt32GetDatum(init_value);
- scratch.resvalue = &iresult->value;
- scratch.resnull = &iresult->isnull;
+ scratch.resvalue = num_exprs > 0 ? &iresult->value :
&state->resvalue;
+ scratch.resnull = num_exprs > 0 ? &iresult->isnull :
&state->resnull;
ExprEvalPushStep(state, &scratch);
@@ -4074,7 +4079,7 @@ ExecBuildHash32Expr(TupleDesc desc, const
TupleTableSlotOps *ops,
&fcinfo->args[0].value,
&fcinfo->args[0].isnull);
- if (i == list_length(hash_exprs) - 1)
+ if (i == num_exprs - 1)
{
/* the result for hashing the final expr is stored in
the state */
scratch.resvalue = &state->resvalue;
--
2.34.1
