diff options
| -rw-r--r-- | src/backend/utils/adt/orderedsetaggs.c | 21 | ||||
| -rw-r--r-- | src/backend/utils/sort/tuplesort.c | 70 |
2 files changed, 60 insertions, 31 deletions
diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c index fe44d561295..4d1e201f808 100644 --- a/src/backend/utils/adt/orderedsetaggs.c +++ b/src/backend/utils/adt/orderedsetaggs.c @@ -457,10 +457,9 @@ percentile_disc_final(PG_FUNCTION_ARGS) elog(ERROR, "missing row in percentile_disc"); /* - * Note: we *cannot* clean up the tuplesort object here, because the value - * to be returned is allocated inside its sortcontext. We could use - * datumCopy to copy it out of there, but it doesn't seem worth the - * trouble, since the cleanup callback will clear the tuplesort later. + * Note: we could clean up the tuplesort object here, but it doesn't seem + * worth the trouble, since the cleanup callback will clear the tuplesort + * later. */ /* We shouldn't have stored any nulls, but do the right thing anyway */ @@ -575,10 +574,9 @@ percentile_cont_final_common(FunctionCallInfo fcinfo, } /* - * Note: we *cannot* clean up the tuplesort object here, because the value - * to be returned may be allocated inside its sortcontext. We could use - * datumCopy to copy it out of there, but it doesn't seem worth the - * trouble, since the cleanup callback will clear the tuplesort later. + * Note: we could clean up the tuplesort object here, but it doesn't seem + * worth the trouble, since the cleanup callback will clear the tuplesort + * later. */ PG_RETURN_DATUM(val); @@ -1097,10 +1095,9 @@ mode_final(PG_FUNCTION_ARGS) pfree(DatumGetPointer(last_val)); /* - * Note: we *cannot* clean up the tuplesort object here, because the value - * to be returned is allocated inside its sortcontext. We could use - * datumCopy to copy it out of there, but it doesn't seem worth the - * trouble, since the cleanup callback will clear the tuplesort later. + * Note: we could clean up the tuplesort object here, but it doesn't seem + * worth the trouble, since the cleanup callback will clear the tuplesort + * later. */ if (mode_freq) diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index 4dd5407f741..47efd2e3b80 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -1824,6 +1824,11 @@ tuplesort_performsort(Tuplesortstate *state) * direction into *stup. Returns FALSE if no more tuples. * If *should_free is set, the caller must pfree stup.tuple when done with it. * Otherwise, caller should not use tuple following next call here. + * + * Note: Public tuplesort fetch routine callers cannot rely on tuple being + * allocated in their own memory context when should_free is TRUE. It may be + * necessary to create a new copy of the tuple to meet the requirements of + * public fetch routine callers. */ static bool tuplesort_gettuple_common(Tuplesortstate *state, bool forward, @@ -2046,9 +2051,10 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward, * NULL value in leading attribute will set abbreviated value to zeroed * representation, which caller may rely on in abbreviated inequality check. * - * The slot receives a copied tuple (sometimes allocated in caller memory - * context) that will stay valid regardless of future manipulations of the - * tuplesort's state. + * The slot receives a tuple that's been copied into the caller's memory + * context, so that it will stay valid regardless of future manipulations of + * the tuplesort's state (up to and including deleting the tuplesort). + * This differs from similar routines for other types of tuplesorts. */ bool tuplesort_gettupleslot(Tuplesortstate *state, bool forward, @@ -2069,12 +2075,24 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward, if (state->sortKeys->abbrev_converter && abbrev) *abbrev = stup.datum1; - if (!should_free) - { - stup.tuple = heap_copy_minimal_tuple((MinimalTuple) stup.tuple); - should_free = true; - } - ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, should_free); + /* + * Callers rely on tuple being in their own memory context, which is + * not guaranteed by tuplesort_gettuple_common(), even when should_free + * is set to TRUE. We must always copy here, since our interface does + * not allow callers to opt into arrangement where tuple memory can go + * away on the next call here, or after tuplesort_end() is called. + */ + ExecStoreMinimalTuple(heap_copy_minimal_tuple((MinimalTuple) stup.tuple), + slot, true); + + /* + * Free local copy if needed. It would be very invasive to get + * tuplesort_gettuple_common() to allocate tuple in caller's context + * for us, so we just do this instead. + */ + if (should_free) + pfree(stup.tuple); + return true; } else @@ -2089,7 +2107,7 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward, * Returns NULL if no more tuples. If *should_free is set, the * caller must pfree the returned tuple when done with it. * If it is not set, caller should not use tuple following next - * call here. + * call here. It's never okay to use it after tuplesort_end(). */ HeapTuple tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free) @@ -2110,7 +2128,7 @@ tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free) * Returns NULL if no more tuples. If *should_free is set, the * caller must pfree the returned tuple when done with it. * If it is not set, caller should not use tuple following next - * call here. + * call here. It's never okay to use it after tuplesort_end(). */ IndexTuple tuplesort_getindextuple(Tuplesortstate *state, bool forward, @@ -2132,7 +2150,8 @@ tuplesort_getindextuple(Tuplesortstate *state, bool forward, * Returns FALSE if no more datums. * * If the Datum is pass-by-ref type, the returned value is freshly palloc'd - * and is now owned by the caller. + * in caller's context, and is now owned by the caller (this differs from + * similar routines for other types of tuplesorts). * * Caller may optionally be passed back abbreviated value (on TRUE return * value) when abbreviation was used, which can be used to cheaply avoid @@ -2155,6 +2174,9 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward, return false; } + /* Ensure we copy into caller's memory context */ + MemoryContextSwitchTo(oldcontext); + /* Record abbreviated key for caller */ if (state->sortKeys->abbrev_converter && abbrev) *abbrev = stup.datum1; @@ -2166,17 +2188,27 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward, } else { - /* use stup.tuple because stup.datum1 may be an abbreviation */ + /* + * Callers rely on datum being in their own memory context, which is + * not guaranteed by tuplesort_gettuple_common(), even when should_free + * is set to TRUE. We must always copy here, since our interface does + * not allow callers to opt into arrangement where tuple memory can go + * away on the next call here, or after tuplesort_end() is called. + * + * Use stup.tuple because stup.datum1 may be an abbreviation. + */ + *val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen); + *isNull = false; + /* + * Free local copy if needed. It would be very invasive to get + * tuplesort_gettuple_common() to allocate tuple in caller's context + * for us, so we just do this instead. + */ if (should_free) - *val = PointerGetDatum(stup.tuple); - else - *val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen); - *isNull = false; + pfree(stup.tuple); } - MemoryContextSwitchTo(oldcontext); - return true; } |