On 12.1.2015 01:28, Ali Akbar wrote: > > > Or else we implement what you suggest below (more comments below): > > > > Thinking about the 'release' flag a bit more - maybe we > could do > > this > > instead: > > > > if (release && astate->private_cxt) > > MemoryContextDelete(astate->mcontext); > > else if (release) > > { > > pfree(astate->dvalues); > > pfree(astate->dnulls); > > pfree(astate); > > } > > > > i.e. either destroy the whole context if possible, and > just free the > > memory when using a shared memory context. But I'm afraid > this would > > penalize the shared memory context, because that's > intended for > > cases where all the build states coexist in parallel and > then at some > > point are all converted into a result and thrown away. > Adding pfree() > > calls is no improvement here, and just wastes cycles. > > > > > > As per Tom's comment, i'm using "parent memory context" instead of > > "shared memory context" below. > > > > In the future, if some code writer decided to use > subcontext=false, > > to save memory in cases where there are many array > accumulation, and > > the parent memory context is long-living, current code can cause > > memory leak. So i think we should implement your suggestion > > (pfreeing astate), and warn the implication in the API > comment. The > > API user must choose between release=true, wasting cycles but > > preventing memory leak, or managing memory from the parent memory > > context. > > I'm wondering whether this is necessary after fixing makeArrayResult to > use the privat_cxt flag. It's still possible to call makeMdArrayResult > directly (with the wrong 'release' value). > > Another option might be to get rid of the 'release' flag altogether, and > just use the 'private_cxt' - I'm not aware of a code using release=false > with private_cxt=true (e.g. to build the same array twice from the same > astate). But maybe there's such code, and another downside is that it'd > break the existing API. > > > In one possible use case, for efficiency maybe the caller will > > create a special parent memory context for all array accumulation. > > Then uses makeArrayResult* with release=false, and in the end > > releasing the parent memory context once for all. > > Yeah, although I'd much rather not break the existing code at all. That > is - my goal is not to make it slower unless absolutely necessary (and > in that case the code may be fixed per your suggestion). But I'm not > convinced it's worth it. > > > OK. Do you think we need to note this in the comments? Something like > this: If using subcontext=false, the caller must be careful about memory > usage, because makeArrayResult* will not free the memory used.
Yes, I think it's worth mentioning. > But I think it makes sense to move the error handling into > initArrayResultArr(), including the get_element_type() call, and remove > the element_type from the signature. This means initArrayResultAny() > will call the get_element_type() twice, but I guess that's negligible. > And everyone who calls initArrayResultArr() will get the error handling > for free. > > Patch v7 attached, implementing those two changes, i.e. > > * makeMdArrayResult(..., astate->private_cxt) > * move error handling into initArrayResultArr() > * remove element_type from initArrayResultArr() signature > > > Reviewing the v7 patch: > - applies cleanly to current master. patch format, whitespace, etc is good > - make check runs without error > - performance & memory usage still consistent > > If you think we don't have to add the comment (see above), i'll mark > this as ready for committer Attached is v8 patch, with a few comments added: 1) before initArrayResult() - explaining when it's better to use a single memory context, and when it's more efficient to use a separate memory context for each array build state 2) before makeArrayResult() - explaining that it won't free memory when allocated in a single memory context (and that a pfree() has to be used if necessary) 3) before makeMdArrayResult() - explaining that it's illegal to use release=true unless using a subcontext Otherwise the v8 patch is exactly the same as v7. Assuming the comments make it sufficiently clear, I agree with marking this as 'ready for committer'. 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..1386a71 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, 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..889083d 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 = 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,11 @@ accumArrayResult(ArrayBuildState *astate, /* * makeArrayResult - produce 1-D final result of accumArrayResult * + * If the array build state was initialized with a separate memory context, + * this also frees all the memory (by deleting the subcontext). If a parent + * context was used directly, the memory may be freed by an explicit pfree() + * call (unless it's meant to be freed by destroying the parent context). + * * astate is working state (must not be NULL) * rcontext is where to construct result */ @@ -4729,7 +4754,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 +4764,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 an explicit pfree() call) + * * astate is working state (must not be NULL) * rcontext is where to construct result * release is true if okay to release working state @@ -4768,6 +4800,9 @@ makeMdArrayResult(ArrayBuildState *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); @@ -4789,24 +4824,35 @@ makeMdArrayResult(ArrayBuildState *astate, * array_type is the array type (must be a valid varlena array type) * element_type is the type of the array's elements * 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, MemoryContext rcontext, bool subcontext) { ArrayBuildStateArr *astate; - MemoryContext arr_context; + MemoryContext arr_context = rcontext; /* by default use the parent ctx */ + + 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)))); /* 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 +4899,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, rcontext, true); else - { Assert(astate->array_type == array_type); - } oldcontext = MemoryContextSwitchTo(astate->mcontext); @@ -5044,6 +5078,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 +5099,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 +5112,7 @@ initArrayResultAny(Oid input_type, MemoryContext rcontext) /* Array case */ ArrayBuildStateArr *arraystate; - arraystate = initArrayResultArr(input_type, element_type, rcontext); + arraystate = initArrayResultArr(input_type, rcontext, subcontext); astate = (ArrayBuildStateAny *) MemoryContextAlloc(arraystate->mcontext, sizeof(ArrayBuildStateAny)); @@ -5089,7 +5127,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 +5153,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..d0b6836 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, @@ -296,8 +298,8 @@ extern Datum makeArrayResult(ArrayBuildState *astate, 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); +extern ArrayBuildStateArr *initArrayResultArr(Oid array_type, + 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