diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index fe74e49814..bc7eeaf84d 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -1107,6 +1107,11 @@ finalize_aggregate(AggState *aggstate, { *resultVal = FunctionCallInvoke(fcinfo); *resultIsNull = fcinfo->isnull; + + /* ensure by-ref types are allocated in CurrentMemoryContext */ + Assert(peragg->resulttypeByVal || *resultIsNull || + MemoryContextContains(CurrentMemoryContext, (void *) *resultVal)); + } aggstate->curperagg = NULL; } @@ -1115,17 +1120,13 @@ finalize_aggregate(AggState *aggstate, /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */ *resultVal = pergroupstate->transValue; *resultIsNull = pergroupstate->transValueIsNull; - } - /* - * If result is pass-by-ref, make sure it is in the right context. - */ - if (!peragg->resulttypeByVal && !*resultIsNull && - !MemoryContextContains(CurrentMemoryContext, - DatumGetPointer(*resultVal))) - *resultVal = datumCopy(*resultVal, - peragg->resulttypeByVal, - peragg->resulttypeLen); + /* if result is pass-by-ref, copy into the CurrentMemoryContext */ + if (!peragg->resulttypeByVal && !*resultIsNull) + *resultVal = datumCopy(*resultVal, + peragg->resulttypeByVal, + peragg->resulttypeLen); + } MemoryContextSwitchTo(oldContext); } @@ -1172,6 +1173,11 @@ finalize_partialaggregate(AggState *aggstate, *resultVal = FunctionCallInvoke(fcinfo); *resultIsNull = fcinfo->isnull; + + /* ensure by-ref types are allocated in CurrentMemoryContext */ + Assert(peragg->resulttypeByVal || *resultIsNull || + MemoryContextContains(CurrentMemoryContext, (void *) *resultVal)); + } } else @@ -1179,15 +1185,14 @@ finalize_partialaggregate(AggState *aggstate, /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */ *resultVal = pergroupstate->transValue; *resultIsNull = pergroupstate->transValueIsNull; - } - /* If result is pass-by-ref, make sure it is in the right context. */ - if (!peragg->resulttypeByVal && !*resultIsNull && - !MemoryContextContains(CurrentMemoryContext, - DatumGetPointer(*resultVal))) - *resultVal = datumCopy(*resultVal, - peragg->resulttypeByVal, - peragg->resulttypeLen); + /* if result is pass-by-ref, copy into the CurrentMemoryContext */ + if (!peragg->resulttypeByVal && !*resultIsNull) + *resultVal = datumCopy(*resultVal, + peragg->resulttypeByVal, + peragg->resulttypeLen); + + } MemoryContextSwitchTo(oldContext); } diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 8b0858e9f5..b196328060 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -626,6 +626,10 @@ finalize_windowaggregate(WindowAggState *winstate, *result = FunctionCallInvoke(fcinfo); winstate->curaggcontext = NULL; *isnull = fcinfo->isnull; + + /* ensure by-ref types are allocated in CurrentMemoryContext */ + Assert(peraggstate->resulttypeByVal || *isnull || + MemoryContextContains(CurrentMemoryContext, (void *) *result)); } } else @@ -633,17 +637,14 @@ finalize_windowaggregate(WindowAggState *winstate, /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */ *result = peraggstate->transValue; *isnull = peraggstate->transValueIsNull; + + /* if result is pass-by-ref, copy into the CurrentMemoryContext */ + if (!peraggstate->resulttypeByVal && !*isnull) + *result = datumCopy(*result, + peraggstate->resulttypeByVal, + peraggstate->resulttypeLen); } - /* - * If result is pass-by-ref, make sure it is in the right context. - */ - if (!peraggstate->resulttypeByVal && !*isnull && - !MemoryContextContains(CurrentMemoryContext, - DatumGetPointer(*result))) - *result = datumCopy(*result, - peraggstate->resulttypeByVal, - peraggstate->resulttypeLen); MemoryContextSwitchTo(oldContext); } @@ -1034,9 +1035,13 @@ eval_windowfunction(WindowAggState *winstate, WindowStatePerFunc perfuncstate, { LOCAL_FCINFO(fcinfo, FUNC_MAX_ARGS); MemoryContext oldContext; + WindowResultInfo resultinfo; oldContext = MemoryContextSwitchTo(winstate->ss.ps.ps_ExprContext->ecxt_per_tuple_memory); + resultinfo.resulttypeLen = perfuncstate->resulttypeLen; + resultinfo.resulttypeByVal = perfuncstate->resulttypeByVal; + /* * We don't pass any normal arguments to a window function, but we do pass * it the number of arguments, in order to permit window function @@ -1046,7 +1051,8 @@ eval_windowfunction(WindowAggState *winstate, WindowStatePerFunc perfuncstate, InitFunctionCallInfoData(*fcinfo, &(perfuncstate->flinfo), perfuncstate->numArguments, perfuncstate->winCollation, - (void *) perfuncstate->winobj, NULL); + (void *) perfuncstate->winobj, + (void *) &resultinfo); /* Just in case, make all the regular argument slots be null */ for (int argno = 0; argno < perfuncstate->numArguments; argno++) fcinfo->args[argno].isnull = true; @@ -1057,16 +1063,15 @@ eval_windowfunction(WindowAggState *winstate, WindowStatePerFunc perfuncstate, *isnull = fcinfo->isnull; /* - * Make sure pass-by-ref data is allocated in the appropriate context. (We - * need this in case the function returns a pointer into some short-lived - * tuple, as is entirely possible.) + * Ensure byref Datums are allocated in the tuple or query context. The + * result may also be a Const which is stored in the MessageContext or + * cached by some cached plan. */ - if (!perfuncstate->resulttypeByVal && !fcinfo->isnull && - !MemoryContextContains(CurrentMemoryContext, - DatumGetPointer(*result))) - *result = datumCopy(*result, - perfuncstate->resulttypeByVal, - perfuncstate->resulttypeLen); + Assert(perfuncstate->resulttypeByVal || fcinfo->isnull || + MemoryContextContains(winstate->ss.ps.ps_ExprContext->ecxt_per_tuple_memory, (void *) *result) || + MemoryContextContains(winstate->ss.ps.ps_ExprContext->ecxt_per_query_memory, (void *) *result) || + MemoryContextContains(MessageContext, (void *) *result) || + MemoryContextOrChildOfContains(CacheMemoryContext, (void *) *result)); MemoryContextSwitchTo(oldContext); } diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index 85ee9ec426..f9eaa413e9 100644 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -6643,7 +6643,7 @@ int2int4_sum(PG_FUNCTION_ARGS) if (transdata->count == 0) PG_RETURN_NULL(); - PG_RETURN_DATUM(Int64GetDatumFast(transdata->sum)); + PG_RETURN_DATUM(Int64GetDatum(transdata->sum)); } diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c index ec423375ae..a67d72016e 100644 --- a/src/backend/utils/mmgr/aset.c +++ b/src/backend/utils/mmgr/aset.c @@ -1333,6 +1333,28 @@ AllocSetGetChunkContext(void *pointer) return &set->header; } +#ifdef MEMORY_CONTEXT_CHECKING +/* + * AllocSetContains + * Attempt to determine if 'pointer' is memory which was allocated by + * 'context'. Return true if it is, otherwise false. + */ +bool +AllocSetContains(MemoryContext context, void *pointer) +{ + AllocSet set = (AllocSet) context; + + for (AllocBlock blk = set->blocks; blk != NULL; blk = blk->next) + { + /* see if pointer is in range of this block */ + if ((char *) pointer >= (char *) blk + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ && + (char *) pointer < blk->endptr) + return true; + } + return false; +} +#endif /* MEMORY_CONTEXT_CHECKING */ + /* * AllocSetGetChunkSpace * Given a currently-allocated chunk, determine the total space diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c index c743b24fa7..e44f765f35 100644 --- a/src/backend/utils/mmgr/generation.c +++ b/src/backend/utils/mmgr/generation.c @@ -848,6 +848,32 @@ GenerationGetChunkContext(void *pointer) return &block->context->header; } + +#ifdef MEMORY_CONTEXT_CHECKING +/* + * GenerationContains + * Attempt to determine if 'pointer' is memory which was allocated by + * 'context'. Return true if it is, otherwise false. + */ +bool +GenerationContains(MemoryContext context, void *pointer) +{ + GenerationContext *set = (GenerationContext *) context; + dlist_iter iter; + + dlist_foreach(iter, &set->blocks) + { + GenerationBlock *blk = dlist_container(GenerationBlock, node, iter.cur); + + /* see if pointer is in range of this block */ + if ((char *) pointer >= (char *) blk + Generation_BLOCKHDRSZ + Generation_CHUNKHDRSZ && + (char *) pointer < blk->endptr) + return true; + } + return false; +} +#endif /* MEMORY_CONTEXT_CHECKING */ + /* * GenerationGetChunkSpace * Given a currently-allocated chunk, determine the total space diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 115a64cfe4..637cb1a569 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -49,6 +49,7 @@ static const MemoryContextMethods mcxt_methods[] = { [MCTX_ASET_ID].stats = AllocSetStats, #ifdef MEMORY_CONTEXT_CHECKING [MCTX_ASET_ID].check = AllocSetCheck, + [MCTX_ASET_ID].contains = AllocSetContains, #endif /* generation.c */ @@ -63,6 +64,7 @@ static const MemoryContextMethods mcxt_methods[] = { [MCTX_GENERATION_ID].stats = GenerationStats, #ifdef MEMORY_CONTEXT_CHECKING [MCTX_GENERATION_ID].check = GenerationCheck, + [MCTX_GENERATION_ID].contains = GenerationContains, #endif /* slab.c */ @@ -76,7 +78,8 @@ static const MemoryContextMethods mcxt_methods[] = { [MCTX_SLAB_ID].is_empty = SlabIsEmpty, [MCTX_SLAB_ID].stats = SlabStats #ifdef MEMORY_CONTEXT_CHECKING - ,[MCTX_SLAB_ID].check = SlabCheck + ,[MCTX_SLAB_ID].check = SlabCheck, + [MCTX_SLAB_ID].contains = SlabContains #endif }; @@ -813,48 +816,51 @@ MemoryContextCheck(MemoryContext context) } #endif +#ifdef MEMORY_CONTEXT_CHECKING /* * MemoryContextContains * Detect whether an allocated chunk of memory belongs to a given * context or not. * - * Caution: 'pointer' must point to a pointer which was allocated by a - * MemoryContext. It's not safe or valid to use this function on arbitrary - * pointers as obtaining the MemoryContext which 'pointer' belongs to requires - * possibly several pointer dereferences. + * Caution: this test is reliable as long as 'pointer' does point to + * a chunk of memory allocated from *some* context. If 'pointer' points + * at memory obtained in some other way, there is a chance of a segmentation + * violation due to accessing the chunk header, which look for in the 8 bytes + * prior to the pointer. */ bool MemoryContextContains(MemoryContext context, void *pointer) { - /* - * Temporarily make this always return false as we don't yet have a fully - * baked idea on how to make it work correctly with the new MemoryChunk - * code. - */ - return false; + if (pointer == NULL) + return false; -#ifdef NOT_USED - MemoryContext ptr_context; + return context->methods->contains(context, pointer); +} - /* - * NB: We must perform run-time checks here which GetMemoryChunkContext() - * does as assertions before calling GetMemoryChunkContext(). - * - * Try to detect bogus pointers handed to us, poorly though we can. - * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an - * allocated chunk. - */ - if (pointer == NULL || pointer != (void *) MAXALIGN(pointer)) - return false; +/* + * MemoryContextOrChildOfContains + * Recursive version of MemoryContextContains which checks if any of + * context's child contexts contains the pointer. + */ +bool +MemoryContextOrChildOfContains(MemoryContext context, void *pointer) +{ + if (MemoryContextContains(context, pointer)) + return true; + else + { + MemoryContext child; - /* - * OK, it's probably safe to look at the context. - */ - ptr_context = GetMemoryChunkContext(pointer); + for (child = context->firstchild; child != NULL; child = child->nextchild) + { + if (MemoryContextOrChildOfContains(child, pointer)) + return true; + } + } - return ptr_context == context; -#endif + return false; } +#endif /* MEMORY_CONTEXT_CHECKING */ /* * MemoryContextCreate @@ -969,6 +975,8 @@ MemoryContextAlloc(MemoryContext context, Size size) VALGRIND_MEMPOOL_ALLOC(context, ret, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1007,6 +1015,8 @@ MemoryContextAllocZero(MemoryContext context, Size size) MemSetAligned(ret, 0, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1045,6 +1055,8 @@ MemoryContextAllocZeroAligned(MemoryContext context, Size size) MemSetLoop(ret, 0, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1086,6 +1098,8 @@ MemoryContextAllocExtended(MemoryContext context, Size size, int flags) if ((flags & MCXT_ALLOC_ZERO) != 0) MemSetAligned(ret, 0, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1169,6 +1183,8 @@ palloc(Size size) VALGRIND_MEMPOOL_ALLOC(context, ret, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1202,6 +1218,8 @@ palloc0(Size size) MemSetAligned(ret, 0, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1241,6 +1259,8 @@ palloc_extended(Size size, int flags) if ((flags & MCXT_ALLOC_ZERO) != 0) MemSetAligned(ret, 0, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1294,6 +1314,8 @@ repalloc(void *pointer, Size size) VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1329,6 +1351,8 @@ MemoryContextAllocHuge(MemoryContext context, Size size) VALGRIND_MEMPOOL_ALLOC(context, ret, size); + Assert(MemoryContextContains(context, ret)); + return ret; } @@ -1368,6 +1392,8 @@ repalloc_huge(void *pointer, Size size) VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size); + Assert(MemoryContextContains(context, ret)); + return ret; } diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c index 9149aaafcb..f5ad699760 100644 --- a/src/backend/utils/mmgr/slab.c +++ b/src/backend/utils/mmgr/slab.c @@ -567,6 +567,35 @@ SlabGetChunkContext(void *pointer) return &slab->header; } +#ifdef MEMORY_CONTEXT_CHECKING +/* + * SlabContains + * Attempt to determine if 'pointer' is memory which was allocated by + * 'context'. Return true if it is, otherwise false. + */ +bool +SlabContains(MemoryContext context, void *pointer) +{ + SlabContext *set = (SlabContext *) context; + + for (int i = 0; i <= set->chunksPerBlock; i++) + { + dlist_iter iter; + + dlist_foreach(iter, &set->freelist[i]) + { + SlabBlock *blk = dlist_container(SlabBlock, node, iter.cur); + + /* see if pointer is in range of this block */ + if ((char *) pointer >= (char *) blk + Slab_BLOCKHDRSZ + Slab_CHUNKHDRSZ && + (char *) pointer < (char *) blk + set->blockSize) + return true; + } + } + return false; +} +#endif /* MEMORY_CONTEXT_CHECKING */ + /* * SlabGetChunkSpace * Given a currently-allocated chunk, determine the total space diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h index 63d07358cd..c1d6fd5391 100644 --- a/src/include/nodes/memnodes.h +++ b/src/include/nodes/memnodes.h @@ -72,6 +72,7 @@ typedef struct MemoryContextMethods bool print_to_stderr); #ifdef MEMORY_CONTEXT_CHECKING void (*check) (MemoryContext context); + bool (*contains) (MemoryContext context, void *pointer); #endif } MemoryContextMethods; diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h index 52bc41ec53..3bfadbd465 100644 --- a/src/include/utils/memutils.h +++ b/src/include/utils/memutils.h @@ -95,8 +95,10 @@ extern void MemoryContextAllowInCriticalSection(MemoryContext context, #ifdef MEMORY_CONTEXT_CHECKING extern void MemoryContextCheck(MemoryContext context); -#endif extern bool MemoryContextContains(MemoryContext context, void *pointer); +extern bool MemoryContextOrChildOfContains(MemoryContext context, + void *pointer); +#endif /* Handy macro for copying and assigning context ID ... but note double eval */ #define MemoryContextCopyAndSetIdentifier(cxt, id) \ diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h index 377cee7a84..b767982896 100644 --- a/src/include/utils/memutils_internal.h +++ b/src/include/utils/memutils_internal.h @@ -33,6 +33,7 @@ extern void AllocSetStats(MemoryContext context, bool print_to_stderr); #ifdef MEMORY_CONTEXT_CHECKING extern void AllocSetCheck(MemoryContext context); +extern bool AllocSetContains(MemoryContext context, void *pointer); #endif /* These functions implement the MemoryContext API for Generation context. */ @@ -50,6 +51,7 @@ extern void GenerationStats(MemoryContext context, bool print_to_stderr); #ifdef MEMORY_CONTEXT_CHECKING extern void GenerationCheck(MemoryContext context); +extern bool GenerationContains(MemoryContext context, void *pointer); #endif @@ -68,6 +70,7 @@ extern void SlabStats(MemoryContext context, bool print_to_stderr); #ifdef MEMORY_CONTEXT_CHECKING extern void SlabCheck(MemoryContext context); +extern bool SlabContains(MemoryContext context, void *pointer); #endif /* diff --git a/src/include/utils/memutils_memorychunk.h b/src/include/utils/memutils_memorychunk.h index 2eefc138e3..a6a0137e5d 100644 --- a/src/include/utils/memutils_memorychunk.h +++ b/src/include/utils/memutils_memorychunk.h @@ -107,9 +107,18 @@ MEMORYCHUNK_VALUE_BASEBIT << \ MEMORYCHUNK_VALUE_BASEBIT) +#ifdef MEMORY_CONTEXT_CHECKING +#if SIZEOF_SIZE_T >= 8 +#define MEMORYCHUNK_MAGIC2 UINT64CONST(0xB1A8DB858EB6EFBA) +#else +#define MEMORYCHUNK_MAGIC2 0x8EB6EFBA +#endif +#endif + typedef struct MemoryChunk { #ifdef MEMORY_CONTEXT_CHECKING + Size chunk_magic; /* magic number */ Size requested_size; #endif @@ -161,6 +170,9 @@ MemoryChunkSetHdrMask(MemoryChunk *chunk, void *block, Assert(value <= MEMORYCHUNK_MAX_VALUE); Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK); +#ifdef MEMORY_CONTEXT_CHECKING + chunk->chunk_magic = MEMORYCHUNK_MAGIC2; +#endif chunk->hdrmask = (((uint64) blockoffset) << MEMORYCHUNK_BLOCKOFFSET_BASEBIT) | (((uint64) value) << MEMORYCHUNK_VALUE_BASEBIT) | methodid; @@ -177,6 +189,9 @@ MemoryChunkSetHdrMaskExternal(MemoryChunk *chunk, { Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK); +#ifdef MEMORY_CONTEXT_CHECKING + chunk->chunk_magic = MEMORYCHUNK_MAGIC2; +#endif chunk->hdrmask = MEMORYCHUNK_MAGIC | (((uint64) 1) << MEMORYCHUNK_EXTERNAL_BASEBIT) | methodid; } @@ -194,6 +209,7 @@ MemoryChunkIsExternal(MemoryChunk *chunk) */ Assert(!HdrMaskIsExternal(chunk->hdrmask) || HdrMaskCheckMagic(chunk->hdrmask)); + Assert(chunk->chunk_magic == MEMORYCHUNK_MAGIC2); return HdrMaskIsExternal(chunk->hdrmask); } @@ -207,6 +223,7 @@ static inline Size MemoryChunkGetValue(MemoryChunk *chunk) { Assert(!HdrMaskIsExternal(chunk->hdrmask)); + Assert(chunk->chunk_magic == MEMORYCHUNK_MAGIC2); return HdrMaskGetValue(chunk->hdrmask); } @@ -220,6 +237,7 @@ static inline void * MemoryChunkGetBlock(MemoryChunk *chunk) { Assert(!HdrMaskIsExternal(chunk->hdrmask)); + Assert(chunk->chunk_magic == MEMORYCHUNK_MAGIC2); return (void *) ((char *) chunk - HdrMaskBlockOffset(chunk->hdrmask)); } @@ -229,6 +247,7 @@ MemoryChunkGetBlock(MemoryChunk *chunk) #undef MEMORYCHUNK_VALUE_BASEBIT #undef MEMORYCHUNK_BLOCKOFFSET_BASEBIT #undef MEMORYCHUNK_MAGIC +#undef MEMORYCHUNK_MAGIC2 #undef HdrMaskIsExternal #undef HdrMaskGetValue #undef HdrMaskBlockOffset diff --git a/src/include/windowapi.h b/src/include/windowapi.h index 5a620a2e42..c3d65b161c 100644 --- a/src/include/windowapi.h +++ b/src/include/windowapi.h @@ -36,7 +36,19 @@ /* this struct is private in nodeWindowAgg.c */ typedef struct WindowObjectData *WindowObject; +/* + * Evaluation of window functions are passed this object and it's available + * via fcinfo->resultinfo and can be accessed in the window function via the + * PG_WINDOW_RESULTINFO macro. + */ +typedef struct WindowResultInfo +{ + int16 resulttypeLen; /* type length of wfunc's result type */ + bool resulttypeByVal; /* true if wfunc's result is byval */ +} WindowResultInfo; + #define PG_WINDOW_OBJECT() ((WindowObject) fcinfo->context) +#define PG_WINDOW_RESULTINFO() ((WindowResultInfo *) fcinfo->resultinfo) #define WindowObjectIsValid(winobj) \ ((winobj) != NULL && IsA(winobj, WindowObjectData))