Hi,
attached is v9 of the patch, modified along the lines of Tom's comments:
1) uses alen=64 for cases with private context, 8 otherwise
2) reverts removal of element_type from initArrayResultArr()
When element_type=InvalidOid is passed to initArrayResultArr, it
performs lookup using get_element_type(), otherwise reuses the value
it receives from the caller.
3) moves the assert into the 'if (release)' branch
4) includes the comments proposed by Ali Akbar in his reviews
Warnings at makeArrayResult/makeMdArrayResult about freeing memory
with private subcontexts.
Regarding the performance impact of decreasing the size of the
preallocated array from 64 to just 8 elements, I tried this.
CREATE TABLE test AS
SELECT mod(i,100000) a, i FROM generate_series(1,64*100000) s(i);
SELECT a, array_agg(i) AS x FRM test GROUP BY 1;
or actually (to minimize transfer overhead):
SELECT COUNT(x) FROM (
SELECT a, array_agg(i) AS x FRM test GROUP BY 1
) foo;
with work_mem=2GB (so that it really uses HashAggregate). The dataset is
constructed to have exactly 64 items per group, thus exploiting the
difference between alen=8 and alen=64.
With alen=8 I get these timings:
Time: 1892,681 ms
Time: 1879,046 ms
Time: 1892,626 ms
Time: 1892,155 ms
Time: 1880,282 ms
Time: 1868,344 ms
Time: 1873,294 ms
and with alen=64:
Time: 1888,244 ms
Time: 1882,991 ms
Time: 1885,157 ms
Time: 1868,935 ms
Time: 1878,053 ms
Time: 1894,871 ms
Time: 1871,571 ms
That's 1880 vs 1882 on average, so pretty much no difference. Would be
nice if someone else could try this on their machine(s).
regards
--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index f3ce1d7..9eb4d63 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
@@ -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.
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index 600646e..49fc23a 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -498,8 +498,13 @@ array_agg_transfn(PG_FUNCTION_ARGS)
elog(ERROR, "array_agg_transfn called in non-aggregate context");
}
- state = PG_ARGISNULL(0) ? NULL : (ArrayBuildState *) PG_GETARG_POINTER(0);
+ if (PG_ARGISNULL(0))
+ state = initArrayResult(arg1_typeid, aggcontext, false);
+ else
+ state = (ArrayBuildState *) PG_GETARG_POINTER(0);
+
elem = PG_ARGISNULL(1) ? (Datum) 0 : PG_GETARG_DATUM(1);
+
state = accumArrayResult(state,
elem,
PG_ARGISNULL(1),
@@ -573,7 +578,12 @@ array_agg_array_transfn(PG_FUNCTION_ARGS)
elog(ERROR, "array_agg_array_transfn called in non-aggregate context");
}
- state = PG_ARGISNULL(0) ? NULL : (ArrayBuildStateArr *) PG_GETARG_POINTER(0);
+
+ if (PG_ARGISNULL(0))
+ state = initArrayResultArr(arg1_typeid, InvalidOid, aggcontext, false);
+ else
+ state = (ArrayBuildStateArr *) PG_GETARG_POINTER(0);
+
state = accumArrayResultArr(state,
PG_GETARG_DATUM(1),
PG_ARGISNULL(1),
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 5591b46..b8c4fba 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -4606,6 +4606,7 @@ array_insert_slice(ArrayType *destArray,
*
* element_type is the array element type (must be a valid array element type)
* rcontext is where to keep working state
+ * subcontext is a flag determining whether to use a separate memory context
*
* Note: there are two common schemes for using accumArrayResult().
* In the older scheme, you start with a NULL ArrayBuildState pointer, and
@@ -4615,24 +4616,43 @@ array_insert_slice(ArrayType *destArray,
* once per element. In this scheme you always end with a non-NULL pointer
* that you can pass to makeArrayResult; you get an empty array if there
* were no elements. This is preferred if an empty array is what you want.
+ *
+ * It's possible to choose whether to create a separate memory context for the
+ * array builde state, or whether to allocate it directly within rcontext
+ * (along with various other pieces). This influences memory consumption in
+ * different situations.
+ *
+ * When there are many states in parallel (e.g. as in hash aggregate), using
+ * a separate memory context for each one may result in significant memory
+ * bloat (up to causing OOM), because each memory context is 1kB at minimum
+ * (and array_agg uses 8kB memory contexts), even if there's a single value
+ * in the array. In these cases, using a single memory context for all the
+ * array build states is very efficient.
+ *
+ * In cases when the array build states have different lifecycles (e.g. when
+ * building arrays outside an aggregate function), reusing the parent memory
+ * context makes it impossible to free the memory by deleting the whole
+ * context (as there may be other allocated pieces of memory).
*/
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->private_cxt = subcontext;
+ astate->alen = (subcontext ? 64 : 8); /* arbitrary starting array size */
astate->dvalues = (Datum *)
MemoryContextAlloc(arr_context, astate->alen * sizeof(Datum));
astate->dnulls = (bool *)
@@ -4666,7 +4686,7 @@ accumArrayResult(ArrayBuildState *astate,
if (astate == NULL)
{
/* First time through --- initialize */
- astate = initArrayResult(element_type, rcontext);
+ astate = initArrayResult(element_type, rcontext, true);
}
else
{
@@ -4713,6 +4733,12 @@ accumArrayResult(ArrayBuildState *astate,
/*
* makeArrayResult - produce 1-D final result of accumArrayResult
*
+ * beware: if the astate was not initialized within a separate memory
+ * context (i.e. using subcontext=true when calling initArrayResult),
+ * using release=true is illegal as it releases the whole context,
+ * and that may include other memory still used elsewhere (instead use
+ * release=false and release the memory with the parent context later)
+ *
* astate is working state (must not be NULL)
* rcontext is where to construct result
*/
@@ -4729,7 +4755,8 @@ 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,
+ astate->private_cxt);
}
/*
@@ -4738,6 +4765,12 @@ makeArrayResult(ArrayBuildState *astate,
* beware: no check that specified dimensions match the number of values
* accumulated.
*
+ * beware: if the astate was not initialized within a separate memory
+ * context (i.e. using subcontext=true when calling initArrayResult),
+ * using release=true is illegal as it releases the whole context,
+ * and that may include other memory still used elsewhere (instead use
+ * release=false and release the memory with the parent context later)
+ *
* astate is working state (must not be NULL)
* rcontext is where to construct result
* release is true if okay to release working state
@@ -4770,7 +4803,10 @@ makeMdArrayResult(ArrayBuildState *astate,
/* Clean up all the junk */
if (release)
+ {
+ Assert(astate->private_cxt);
MemoryContextDelete(astate->mcontext);
+ }
return PointerGetDatum(result);
}
@@ -4787,26 +4823,42 @@ makeMdArrayResult(ArrayBuildState *astate,
* initArrayResultArr - initialize an empty ArrayBuildStateArr
*
* array_type is the array type (must be a valid varlena array type)
- * element_type is the type of the array's elements
+ * element_type is the type of the array's elements (lookup if InvalidOid)
* rcontext is where to keep working state
+ * subcontext is a flag determining whether to use a separate memory context
*/
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;
+ MemoryContext arr_context = rcontext; /* by default use the parent ctx */
+
+ /* Lookup element type, unless element_type already provided */
+ if (! OidIsValid(element_type))
+ {
+ element_type = get_element_type(array_type);
+
+ if (!OidIsValid(element_type))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("data type %s is not an array type",
+ format_type_be(array_type))));
+ }
/* Make a temporary context to hold all the junk */
- arr_context = AllocSetContextCreate(rcontext,
- "accumArrayResultArr",
- ALLOCSET_DEFAULT_MINSIZE,
- ALLOCSET_DEFAULT_INITSIZE,
- ALLOCSET_DEFAULT_MAXSIZE);
+ if (subcontext)
+ arr_context = AllocSetContextCreate(rcontext,
+ "accumArrayResultArr",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
/* Note we initialize all fields to zero */
astate = (ArrayBuildStateArr *)
MemoryContextAllocZero(arr_context, sizeof(ArrayBuildStateArr));
astate->mcontext = arr_context;
+ astate->private_cxt = subcontext;
/* Save relevant datatype information */
astate->array_type = array_type;
@@ -4853,21 +4905,9 @@ accumArrayResultArr(ArrayBuildStateArr *astate,
arg = DatumGetArrayTypeP(dvalue);
if (astate == NULL)
- {
- /* First time through --- initialize */
- Oid element_type = get_element_type(array_type);
-
- if (!OidIsValid(element_type))
- ereport(ERROR,
- (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, InvalidOid, rcontext, true);
else
- {
Assert(astate->array_type == array_type);
- }
oldcontext = MemoryContextSwitchTo(astate->mcontext);
@@ -5044,6 +5084,9 @@ makeArrayResultArr(ArrayBuildStateArr *astate,
MemoryContextSwitchTo(oldcontext);
+ /* we can only release the context if it's a private one. */
+ Assert(! (release && !astate->private_cxt));
+
/* Clean up all the junk */
if (release)
MemoryContextDelete(astate->mcontext);
@@ -5062,9 +5105,10 @@ makeArrayResultArr(ArrayBuildStateArr *astate,
*
* input_type is the input datatype (either element or array type)
* rcontext is where to keep working state
+ * subcontext is a flag determining whether to use a separate memory context
*/
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);
@@ -5074,7 +5118,7 @@ initArrayResultAny(Oid input_type, MemoryContext rcontext)
/* Array case */
ArrayBuildStateArr *arraystate;
- arraystate = initArrayResultArr(input_type, element_type, rcontext);
+ arraystate = initArrayResultArr(input_type, InvalidOid, rcontext, subcontext);
astate = (ArrayBuildStateAny *)
MemoryContextAlloc(arraystate->mcontext,
sizeof(ArrayBuildStateAny));
@@ -5089,7 +5133,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));
@@ -5115,7 +5159,7 @@ accumArrayResultAny(ArrayBuildStateAny *astate,
MemoryContext rcontext)
{
if (astate == NULL)
- astate = initArrayResultAny(input_type, rcontext);
+ astate = initArrayResultAny(input_type, rcontext, true);
if (astate->scalarstate)
(void) accumArrayResult(astate->scalarstate,
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index bfe9447..8bb7144 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -3948,7 +3948,7 @@ xpath(PG_FUNCTION_ARGS)
ArrayType *namespaces = PG_GETARG_ARRAYTYPE_P(2);
ArrayBuildState *astate;
- astate = initArrayResult(XMLOID, CurrentMemoryContext);
+ astate = initArrayResult(XMLOID, CurrentMemoryContext, true);
xpath_internal(xpath_expr_text, data, namespaces,
NULL, astate);
PG_RETURN_ARRAYTYPE_P(makeArrayResult(astate, CurrentMemoryContext));
diff --git a/src/include/utils/array.h b/src/include/utils/array.h
index 694bce7..1610fe2 100644
--- a/src/include/utils/array.h
+++ b/src/include/utils/array.h
@@ -89,6 +89,7 @@ typedef struct ArrayBuildState
int16 typlen; /* needed info about datatype */
bool typbyval;
char typalign;
+ bool private_cxt; /* use private memory context */
} ArrayBuildState;
/*
@@ -109,6 +110,7 @@ typedef struct ArrayBuildStateArr
int lbs[MAXDIM];
Oid array_type; /* data type of the arrays */
Oid element_type; /* data type of the array elements */
+ bool private_cxt; /* use private memory context */
} ArrayBuildStateArr;
/*
@@ -286,7 +288,7 @@ 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,
@@ -297,7 +299,7 @@ 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,
@@ -306,7 +308,7 @@ 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,
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 492c1ef..e3dda5d 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -1218,7 +1218,7 @@ plperl_array_to_datum(SV *src, Oid typid, int32 typmod)
errmsg("cannot convert Perl array to non-array type %s",
format_type_be(typid))));
- astate = initArrayResult(elemtypid, CurrentMemoryContext);
+ astate = initArrayResult(elemtypid, CurrentMemoryContext, true);
_sv_to_datum_finfo(elemtypid, &finfo, &typioparam);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
