Fix Hash Join performance issue when hashing NULL values

adf97c156 allowed expression evaluation to perform hashing, and
subsequently 9ca67658d fixed a memory stomping bug in that commit
that caused unrelated-to-hashing expression op steps to stomp on the
intermediate hash value.  The intermediate hash value needs to be
maintained when hashing multiple hash keys.  9ca67658d didn't quite get
things right when in "strict" mode when it aborted hashing early after
encountering a NULL hash key.  What was meant to happen was that the
expression returns NULL directly to indicate to the caller the value
hashed to NULL.  The problem was that any EEOP_HASHDATUM_FIRST_STRICT or
EEOP_HASHDATUM_NEXT32_STRICT op step that didn't belong to the final
key to be hashed would have its op->resnull and op->resvalue pointing to
the location to store the intermediate hash value.  That's correct for
non-NULLs since we bit-rotate the intermediate value and continue hashing,
but with the strict case, when we get a NULL key, we immediately jump to
the "jumpdone" step.  The problem is the jumpdone step expects the
ExprState resnull and resvalue fields to be set (as they would be if we
didn't abort hashing early due to the NULL), but when we aborted early,
the ExprState fields never got set.  This would result in inserting
records into the hash table that would never match to any join partner,
which is a waste of CPU and memory.

Here we fix this by having EEOP_HASHDATUM_FIRST_STRICT and
EEOP_HASHDATUM_NEXT32_STRICT populate the ExprState resnull and resvalue
fields directly when the value to hash is NULL.

Although Hash Agg and Hashed Subplans do use hashing from ExprStates,
those were unaffected by this bug, as neither of those uses the STRICT op
steps.

Thanks to Tomas Vondra for finding the offending commit.

Reported-by: Dan Stefura <dstefura@bluecatnetworks.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/YQBPR0101MB89738FB972FBD02A3640C6D3D6C92@YQBPR0101MB8973.CANPRD01.PROD.OUTLOOK.COM
Backpatch-through: 18

diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index d45812c23aa9afcf5cd8170646d85d9182de6161..9bc23cb16fa3da57738243cdac1af0f9712d48b5 100644 (file)
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -1843,8 +1843,8 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
                                 * ignoring NULL input values.  We've nothing more to do after
                                 * finding a NULL.
                                 */
-                               *op->resnull = true;
-                               *op->resvalue = (Datum) 0;
+                               state->resnull = true;
+                               state->resvalue = (Datum) 0;
                                EEO_JUMP(op->d.hashdatum.jumpdone);
                        }
 
@@ -1891,8 +1891,8 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
                                 * ignoring NULL input values.  We've nothing more to do after
                                 * finding a NULL.
                                 */
-                               *op->resnull = true;
-                               *op->resvalue = (Datum) 0;
+                               state->resnull = true;
+                               state->resvalue = (Datum) 0;
                                EEO_JUMP(op->d.hashdatum.jumpdone);
                        }
                        else

diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 0e160b8502c3ddf569960f1471fc8ea714b15d2a..29617437477e6c4e88e23768eade8484ac109690 100644 (file)
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -2111,11 +2111,12 @@ llvm_compile_expr(ExprState *state)
                                                LLVMPositionBuilderAtEnd(b, b_ifnullblock);
 
                                                /*
-                                                * In strict node, NULL inputs result in NULL.  Save
-                                                * the NULL result and goto jumpdone.
+                                                * In strict mode, NULL inputs result in NULL.  Save
+                                                * the NULL to the ExprState's resnull/resvalue fields
+                                                * directly, then goto jumpdone.
                                                 */
-                                               LLVMBuildStore(b, l_sbool_const(1), v_resnullp);
-                                               LLVMBuildStore(b, l_datum_const(0), v_resvaluep);
+                                               LLVMBuildStore(b, l_sbool_const(1), v_tmpisnullp);
+                                               LLVMBuildStore(b, l_datum_const(0), v_tmpvaluep);
                                                LLVMBuildBr(b, opblocks[op->d.hashdatum.jumpdone]);
                                        }
                                        else