Hi, Attached is v2 of the patch lowering array_agg memory requirements. Hopefully it addresses the issues issues mentioned by TL in this thread (not handling some of the callers appropriately etc.).
The v2 of the patch does this: * adds 'subcontext' flag to initArrayResult* methods If it's 'true' then a separate context is created for the ArrayBuildState instance, otherwise it's built within the parent context. Currently, only the array_agg_* functions pass 'subcontext=false' so that the array_agg() aggregate does not create separate context for each group. All other callers use 'true' and thus keep using the original implementation (this includes ARRAY_SUBLINK subplans, and various other places building array incrementally). * adds 'release' flag to makeArrayResult This is mostly to make it consistent with makeArrayResultArr and makeArrayResultAny. All current callers use 'release=true'. Also, it seems that using 'subcontext=false' and then 'release=true' would be a bug. Maybe it would be appropriate to store the 'subcontext' value into the ArrayBuildState and then throw an error if makeArrayResult* is called with (release=true && subcontext=false). * modifies all the places calling those functions * decreases number of preallocated elements to 8 The original value was 64 (512B), the current value is 64B. (Not counting the 'nulls' array). More about this later ... Now, some performance measurements - attached is a simple SQL script that executes a few GROUP BY queries with various numbers of groups and group elements. I ran the tests with two dataset sizes: small ===== a) 1M groups, 1 item per group b) 100k groups, 16 items per group c) 100k groups, 64 items per group d) 10k groups, 1024 items per group large ===== a) 10M groups, 1 item per group b) 1M groups, 16 items per group c) 1M groups, 64 items per group d) 100k groups, 1024 items per group So essentially the 'large' dataset uses 10x the number of groups. The results (average from the 5 runs, in ms) look like this: small ===== test | master | patched | diff -----|--------|---------|------- a | 1419 | 834 | -41% b | 595 | 498 | -16% c | 2061 | 1832 | -11% d | 2197 | 1957 | -11% large ===== test | master | patched | diff -----|--------|---------|------- a | OOM | 9144 | n/a b | 7366 | 6257 | -15% c | 29899 | 22940 | -23% d | 35456 | 31347 | -12% So it seems to give solid speedup across the whole test suite - I'm yet to find a case where it's actually slower than what we have now. The test cases (b) and (c) were actually created with this goal, because both should be OK with the original array size (64 elements), but with the new size it requires a few repalloc() calls. But even those are much faster. This is most likely thanks to removing the AllocSetContextCreate call and sharing freelists across groups (although the test cases don't seem extremely suitable for that, as all the groups grow in parallel). I even tried to bump the initial array size back to 64 elements, but the performance actually decreased a bit for some reason. I have no idea why this happens ... The test script is attached - tweak the 'size' variable for different dataset sizes. The (insane) work_mem sizes are used to force a hash aggregate - clearly I don't have 1TB of RAM. regards Tomas
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 18ae318..48e66bf 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1271,13 +1271,13 @@ dblink_get_connections(PG_FUNCTION_ARGS)
/* stash away current value */
astate = accumArrayResult(astate,
CStringGetTextDatum(hentry->name),
- false, TEXTOID, CurrentMemoryContext);
+ false, TEXTOID, CurrentMemoryContext, true);
}
}
if (astate)
PG_RETURN_ARRAYTYPE_P(makeArrayResult(astate,
- CurrentMemoryContext));
+ CurrentMemoryContext, true));
else
PG_RETURN_NULL();
}
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c16b38e..bd5eb32 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -671,7 +671,7 @@ transformRelOptions(Datum oldOptions, List *defList, char *namspace,
/* No match, so keep old option */
astate = accumArrayResult(astate, oldoptions[i],
false, TEXTOID,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
}
}
}
@@ -758,12 +758,12 @@ transformRelOptions(Datum oldOptions, List *defList, char *namspace,
astate = accumArrayResult(astate, PointerGetDatum(t),
false, TEXTOID,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
}
}
if (astate)
- result = makeArrayResult(astate, CurrentMemoryContext);
+ result = makeArrayResult(astate, CurrentMemoryContext, true);
else
result = (Datum) 0;
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index ab4ed6c..804398a 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -82,11 +82,11 @@ optionListToArray(List *options)
astate = accumArrayResult(astate, PointerGetDatum(t),
false, TEXTOID,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
}
if (astate)
- return makeArrayResult(astate, CurrentMemoryContext);
+ return makeArrayResult(astate, CurrentMemoryContext, true);
return PointerGetDatum(NULL);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index d9faf20..39faa4c 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -262,7 +262,7 @@ ExecScanSubPlan(SubPlanState *node,
/* Initialize ArrayBuildStateAny in caller's context, if needed */
if (subLinkType == ARRAY_SUBLINK)
astate = initArrayResultAny(subplan->firstColType,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
/*
* We are probably in a short-lived expression-evaluation context. Switch
@@ -372,7 +372,7 @@ ExecScanSubPlan(SubPlanState *node,
Assert(subplan->firstColType == tdesc->attrs[0]->atttypid);
dvalue = slot_getattr(slot, 1, &disnull);
astate = accumArrayResultAny(astate, dvalue, disnull,
- subplan->firstColType, oldcontext);
+ subplan->firstColType, oldcontext, true);
/* keep scanning subplan to collect all values */
continue;
}
@@ -964,7 +964,7 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
/* Initialize ArrayBuildStateAny in caller's context, if needed */
if (subLinkType == ARRAY_SUBLINK)
astate = initArrayResultAny(subplan->firstColType,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
/*
* Must switch to per-query memory context.
@@ -1026,7 +1026,7 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
Assert(subplan->firstColType == tdesc->attrs[0]->atttypid);
dvalue = slot_getattr(slot, 1, &disnull);
astate = accumArrayResultAny(astate, dvalue, disnull,
- subplan->firstColType, oldcontext);
+ subplan->firstColType, oldcontext, true);
/* keep scanning subplan to collect all values */
continue;
}
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index 50ea4d2..d290a06 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -504,7 +504,7 @@ array_agg_transfn(PG_FUNCTION_ARGS)
elem,
PG_ARGISNULL(1),
arg1_typeid,
- aggcontext);
+ aggcontext, false);
/*
* The transition type for array_agg() is declared to be "internal", which
@@ -578,7 +578,7 @@ array_agg_array_transfn(PG_FUNCTION_ARGS)
PG_GETARG_DATUM(1),
PG_ARGISNULL(1),
arg1_typeid,
- aggcontext);
+ aggcontext, false);
/*
* The transition type for array_agg() is declared to be "internal", which
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 743351b..eba05b4 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -4589,22 +4589,23 @@ array_insert_slice(ArrayType *destArray,
* were no elements. This is preferred if an empty array is what you want.
*/
ArrayBuildState *
-initArrayResult(Oid element_type, MemoryContext rcontext)
+initArrayResult(Oid element_type, MemoryContext rcontext, bool subcontext)
{
ArrayBuildState *astate;
- MemoryContext arr_context;
+ MemoryContext arr_context = rcontext; /* by default use the parent ctx */
/* Make a temporary context to hold all the junk */
- arr_context = AllocSetContextCreate(rcontext,
- "accumArrayResult",
- ALLOCSET_DEFAULT_MINSIZE,
- ALLOCSET_DEFAULT_INITSIZE,
- ALLOCSET_DEFAULT_MAXSIZE);
+ if (subcontext)
+ arr_context = AllocSetContextCreate(rcontext,
+ "accumArrayResult",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
astate = (ArrayBuildState *)
MemoryContextAlloc(arr_context, sizeof(ArrayBuildState));
astate->mcontext = arr_context;
- astate->alen = 64; /* arbitrary starting array size */
+ astate->alen = 8; /* arbitrary starting array size */
astate->dvalues = (Datum *)
MemoryContextAlloc(arr_context, astate->alen * sizeof(Datum));
astate->dnulls = (bool *)
@@ -4631,14 +4632,14 @@ ArrayBuildState *
accumArrayResult(ArrayBuildState *astate,
Datum dvalue, bool disnull,
Oid element_type,
- MemoryContext rcontext)
+ MemoryContext rcontext, bool subcontext)
{
MemoryContext oldcontext;
if (astate == NULL)
{
/* First time through --- initialize */
- astate = initArrayResult(element_type, rcontext);
+ astate = initArrayResult(element_type, rcontext, subcontext);
}
else
{
@@ -4690,7 +4691,8 @@ accumArrayResult(ArrayBuildState *astate,
*/
Datum
makeArrayResult(ArrayBuildState *astate,
- MemoryContext rcontext)
+ MemoryContext rcontext,
+ bool release)
{
int ndims;
int dims[1];
@@ -4701,7 +4703,7 @@ makeArrayResult(ArrayBuildState *astate,
dims[0] = astate->nelems;
lbs[0] = 1;
- return makeMdArrayResult(astate, ndims, dims, lbs, rcontext, true);
+ return makeMdArrayResult(astate, ndims, dims, lbs, rcontext, release);
}
/*
@@ -4763,7 +4765,8 @@ makeMdArrayResult(ArrayBuildState *astate,
* rcontext is where to keep working state
*/
ArrayBuildStateArr *
-initArrayResultArr(Oid array_type, Oid element_type, MemoryContext rcontext)
+initArrayResultArr(Oid array_type, Oid element_type, MemoryContext rcontext,
+ bool subcontext)
{
ArrayBuildStateArr *astate;
MemoryContext arr_context;
@@ -4799,7 +4802,7 @@ ArrayBuildStateArr *
accumArrayResultArr(ArrayBuildStateArr *astate,
Datum dvalue, bool disnull,
Oid array_type,
- MemoryContext rcontext)
+ MemoryContext rcontext, bool subcontext)
{
ArrayType *arg;
MemoryContext oldcontext;
@@ -4834,7 +4837,7 @@ accumArrayResultArr(ArrayBuildStateArr *astate,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("data type %s is not an array type",
format_type_be(array_type))));
- astate = initArrayResultArr(array_type, element_type, rcontext);
+ astate = initArrayResultArr(array_type, element_type, rcontext, subcontext);
}
else
{
@@ -5036,7 +5039,7 @@ makeArrayResultArr(ArrayBuildStateArr *astate,
* rcontext is where to keep working state
*/
ArrayBuildStateAny *
-initArrayResultAny(Oid input_type, MemoryContext rcontext)
+initArrayResultAny(Oid input_type, MemoryContext rcontext, bool subcontext)
{
ArrayBuildStateAny *astate;
Oid element_type = get_element_type(input_type);
@@ -5046,7 +5049,7 @@ initArrayResultAny(Oid input_type, MemoryContext rcontext)
/* Array case */
ArrayBuildStateArr *arraystate;
- arraystate = initArrayResultArr(input_type, element_type, rcontext);
+ arraystate = initArrayResultArr(input_type, element_type, rcontext, subcontext);
astate = (ArrayBuildStateAny *)
MemoryContextAlloc(arraystate->mcontext,
sizeof(ArrayBuildStateAny));
@@ -5061,7 +5064,7 @@ initArrayResultAny(Oid input_type, MemoryContext rcontext)
/* Let's just check that we have a type that can be put into arrays */
Assert(OidIsValid(get_array_type(input_type)));
- scalarstate = initArrayResult(input_type, rcontext);
+ scalarstate = initArrayResult(input_type, rcontext, subcontext);
astate = (ArrayBuildStateAny *)
MemoryContextAlloc(scalarstate->mcontext,
sizeof(ArrayBuildStateAny));
@@ -5084,19 +5087,19 @@ ArrayBuildStateAny *
accumArrayResultAny(ArrayBuildStateAny *astate,
Datum dvalue, bool disnull,
Oid input_type,
- MemoryContext rcontext)
+ MemoryContext rcontext, bool subcontext)
{
if (astate == NULL)
- astate = initArrayResultAny(input_type, rcontext);
+ astate = initArrayResultAny(input_type, rcontext, subcontext);
if (astate->scalarstate)
(void) accumArrayResult(astate->scalarstate,
dvalue, disnull,
- input_type, rcontext);
+ input_type, rcontext, subcontext);
else
(void) accumArrayResultArr(astate->arraystate,
dvalue, disnull,
- input_type, rcontext);
+ input_type, rcontext, subcontext);
return astate;
}
diff --git a/src/backend/utils/adt/regexp.c b/src/backend/utils/adt/regexp.c
index 50b33f6..441a6bd 100644
--- a/src/backend/utils/adt/regexp.c
+++ b/src/backend/utils/adt/regexp.c
@@ -1175,7 +1175,7 @@ regexp_split_to_array(PG_FUNCTION_ARGS)
build_regexp_split_result(splitctx),
false,
TEXTOID,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
splitctx->next_match++;
}
@@ -1185,7 +1185,7 @@ regexp_split_to_array(PG_FUNCTION_ARGS)
* memory context anyway.
*/
- PG_RETURN_ARRAYTYPE_P(makeArrayResult(astate, CurrentMemoryContext));
+ PG_RETURN_ARRAYTYPE_P(makeArrayResult(astate, CurrentMemoryContext, true));
}
/* This is separate to keep the opr_sanity regression test from complaining */
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index b3f397e..aa045a2 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -3576,7 +3576,7 @@ text_to_array_internal(PG_FUNCTION_ARGS)
PointerGetDatum(result_text),
is_null,
TEXTOID,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
pfree(result_text);
@@ -3621,7 +3621,7 @@ text_to_array_internal(PG_FUNCTION_ARGS)
PointerGetDatum(result_text),
is_null,
TEXTOID,
- CurrentMemoryContext);
+ CurrentMemoryContext, true);
pfree(result_text);
@@ -3631,7 +3631,7 @@ text_to_array_internal(PG_FUNCTION_ARGS)
}
PG_RETURN_ARRAYTYPE_P(makeArrayResult(astate,
- CurrentMemoryContext));
+ CurrentMemoryContext, true));
}
/*
diff --git a/src/include/utils/array.h b/src/include/utils/array.h
index e1d5749..385eda8 100644
--- a/src/include/utils/array.h
+++ b/src/include/utils/array.h
@@ -286,31 +286,31 @@ extern void deconstruct_array(ArrayType *array,
extern bool array_contains_nulls(ArrayType *array);
extern ArrayBuildState *initArrayResult(Oid element_type,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool subcontext);
extern ArrayBuildState *accumArrayResult(ArrayBuildState *astate,
Datum dvalue, bool disnull,
Oid element_type,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool subcontext);
extern Datum makeArrayResult(ArrayBuildState *astate,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool release);
extern Datum makeMdArrayResult(ArrayBuildState *astate, int ndims,
int *dims, int *lbs, MemoryContext rcontext, bool release);
extern ArrayBuildStateArr *initArrayResultArr(Oid array_type, Oid element_type,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool subcontext);
extern ArrayBuildStateArr *accumArrayResultArr(ArrayBuildStateArr *astate,
Datum dvalue, bool disnull,
Oid array_type,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool subcontext);
extern Datum makeArrayResultArr(ArrayBuildStateArr *astate,
MemoryContext rcontext, bool release);
extern ArrayBuildStateAny *initArrayResultAny(Oid input_type,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool subcontext);
extern ArrayBuildStateAny *accumArrayResultAny(ArrayBuildStateAny *astate,
Datum dvalue, bool disnull,
Oid input_type,
- MemoryContext rcontext);
+ MemoryContext rcontext, bool subcontext);
extern Datum makeArrayResultAny(ArrayBuildStateAny *astate,
MemoryContext rcontext, bool release);
array-agg.sql
Description: application/sql
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
