On Mon, 2020-03-16 at 11:45 -0700, Jeff Davis wrote:
> AllocSet allocates memory for itself in blocks, which double in size
> up
> to maxBlockSize. So, the current block (the last one malloc'd) may
> represent half of the total memory allocated for the context itself.
Narrower approach that doesn't touch memory context internals:
If the blocks double up in size to maxBlockSize, why not just create
the memory context with a smaller maxBlockSize? I had originally
dismissed this as a hack that could slow down some workloads when
work_mem is large.
But we can simply make it proportional to work_mem, which makes a lot
of sense for an operator like HashAgg that controls its memory usage.
It can allocate in blocks large enough that we don't call malloc() too
often when work_mem is large; but small enough that we don't overrun
work_mem when work_mem is small.
I have attached a patch to do this only for HashAgg, using a new entry
point in execUtils.c called CreateWorkExprContext(). It sets
maxBlockSize to 1/16th of work_mem (rounded down to a power of two),
with a minimum of initBlockSize.
This could be a good general solution for other operators as well, but
that requires a bit more investigation, so I'll leave that for v14.
The attached patch is narrow and solves the problem for HashAgg nicely
without interfering with anything else, so I plan to commit it soon for
v13.
Regards,
Jeff Davis
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index cc5177cc2b9..ca973882d01 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -53,6 +53,7 @@
#include "executor/executor.h"
#include "jit/jit.h"
#include "mb/pg_wchar.h"
+#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "parser/parsetree.h"
#include "partitioning/partdesc.h"
@@ -227,21 +228,13 @@ FreeExecutorState(EState *estate)
MemoryContextDelete(estate->es_query_cxt);
}
-/* ----------------
- * CreateExprContext
- *
- * Create a context for expression evaluation within an EState.
- *
- * An executor run may require multiple ExprContexts (we usually make one
- * for each Plan node, and a separate one for per-output-tuple processing
- * such as constraint checking). Each ExprContext has its own "per-tuple"
- * memory context.
- *
- * Note we make no assumption about the caller's memory context.
- * ----------------
+/*
+ * Internal implementation for CreateExprContext() and CreateWorkExprContext()
+ * that allows control over the AllocSet parameters.
*/
-ExprContext *
-CreateExprContext(EState *estate)
+static ExprContext *
+CreateExprContextInternal(EState *estate, Size minContextSize,
+ Size initBlockSize, Size maxBlockSize)
{
ExprContext *econtext;
MemoryContext oldcontext;
@@ -264,7 +257,9 @@ CreateExprContext(EState *estate)
econtext->ecxt_per_tuple_memory =
AllocSetContextCreate(estate->es_query_cxt,
"ExprContext",
- ALLOCSET_DEFAULT_SIZES);
+ minContextSize,
+ initBlockSize,
+ maxBlockSize);
econtext->ecxt_param_exec_vals = estate->es_param_exec_vals;
econtext->ecxt_param_list_info = estate->es_param_list_info;
@@ -294,6 +289,52 @@ CreateExprContext(EState *estate)
return econtext;
}
+/* ----------------
+ * CreateExprContext
+ *
+ * Create a context for expression evaluation within an EState.
+ *
+ * An executor run may require multiple ExprContexts (we usually make one
+ * for each Plan node, and a separate one for per-output-tuple processing
+ * such as constraint checking). Each ExprContext has its own "per-tuple"
+ * memory context.
+ *
+ * Note we make no assumption about the caller's memory context.
+ * ----------------
+ */
+ExprContext *
+CreateExprContext(EState *estate)
+{
+ return CreateExprContextInternal(estate, ALLOCSET_DEFAULT_SIZES);
+}
+
+
+/* ----------------
+ * CreateWorkExprContext
+ *
+ * Like CreateExprContext, but specifies the AllocSet sizes to be reasonable
+ * in proportion to work_mem. If the maximum block allocation size is too
+ * large, it's easy to skip right past work_mem with a single allocation.
+ * ----------------
+ */
+ExprContext *
+CreateWorkExprContext(EState *estate)
+{
+ Size minContextSize = ALLOCSET_DEFAULT_MINSIZE;
+ Size initBlockSize = ALLOCSET_DEFAULT_INITSIZE;
+ Size maxBlockSize = ALLOCSET_DEFAULT_MAXSIZE;
+
+ /* choose the maxBlockSize to be no larger than 1/16 of work_mem */
+ while (16 * maxBlockSize > work_mem * 1024L)
+ maxBlockSize >>= 1;
+
+ if (maxBlockSize < ALLOCSET_DEFAULT_INITSIZE)
+ maxBlockSize = ALLOCSET_DEFAULT_INITSIZE;
+
+ return CreateExprContextInternal(estate, minContextSize,
+ initBlockSize, maxBlockSize);
+}
+
/* ----------------
* CreateStandaloneExprContext
*
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 4e100e5755f..44587a84bae 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -3277,10 +3277,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
}
if (use_hashing)
- {
- ExecAssignExprContext(estate, &aggstate->ss.ps);
- aggstate->hashcontext = aggstate->ss.ps.ps_ExprContext;
- }
+ aggstate->hashcontext = CreateWorkExprContext(estate);
ExecAssignExprContext(estate, &aggstate->ss.ps);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 94890512dc8..c7deeac662f 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -493,6 +493,7 @@ extern void end_tup_output(TupOutputState *tstate);
extern EState *CreateExecutorState(void);
extern void FreeExecutorState(EState *estate);
extern ExprContext *CreateExprContext(EState *estate);
+extern ExprContext *CreateWorkExprContext(EState *estate);
extern ExprContext *CreateStandaloneExprContext(void);
extern void FreeExprContext(ExprContext *econtext, bool isCommit);
extern void ReScanExprContext(ExprContext *econtext);
