From 7b67ec71b8291fb3051a924919f2f1c82e8fe121 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 8 Dec 2016 14:37:54 -0800 Subject: [PATCH 1/3] Remove should_free tuplesort routine arguments Tuplesort routine should_free status arguments are redundant, as memory for tuples returned from a sort are always owned by tuplesort in practice. This has been the case since commit e94568ecc removed any instances of caller tuples being returned as anything other than pointers into tuplesort-owned buffers. Refactor interface of certain tuplesort routines to reflect this general state of affairs. There are two remaining cases where we must always copy caller tuples because callers are likely to require ownership of memory: when the caller passes a TupleTableSlot which tuplesort stores a heap tuple in for caller, and when the caller gets a pass-by-reference Datum tuple from tuplesort. In the future, it may be possible for some of the callers of these two remaining functions to opt out of having tuplesort produce a new copy for them, as a performance optimization, but that has nothing to do with tuplesort memory management, and everything to do with the requirements of those callers. --- src/backend/access/hash/hashsort.c | 6 +--- src/backend/access/nbtree/nbtsort.c | 24 ++++---------- src/backend/commands/cluster.c | 6 +--- src/backend/utils/sort/tuplesort.c | 62 +++++++++++++------------------------ src/include/utils/tuplesort.h | 6 ++-- 5 files changed, 31 insertions(+), 73 deletions(-) diff --git a/src/backend/access/hash/hashsort.c b/src/backend/access/hash/hashsort.c index 8938ab5..12fb78a 100644 --- a/src/backend/access/hash/hashsort.c +++ b/src/backend/access/hash/hashsort.c @@ -104,15 +104,13 @@ void _h_indexbuild(HSpool *hspool) { IndexTuple itup; - bool should_free; #ifdef USE_ASSERT_CHECKING uint32 hashkey = 0; #endif tuplesort_performsort(hspool->sortstate); - while ((itup = tuplesort_getindextuple(hspool->sortstate, - true, &should_free)) != NULL) + while ((itup = tuplesort_getindextuple(hspool->sortstate, true)) != NULL) { /* * Technically, it isn't critical that hash keys be found in sorted @@ -129,7 +127,5 @@ _h_indexbuild(HSpool *hspool) #endif _hash_doinsert(hspool->index, itup); - if (should_free) - pfree(itup); } } diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index 99a014e..7f65b5a 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -680,9 +680,7 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2) bool merge = (btspool2 != NULL); IndexTuple itup, itup2 = NULL; - bool should_free, - should_free2, - load1; + bool load1; TupleDesc tupdes = RelationGetDescr(wstate->index); int i, keysz = RelationGetNumberOfAttributes(wstate->index); @@ -697,10 +695,8 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2) */ /* the preparation of merge */ - itup = tuplesort_getindextuple(btspool->sortstate, - true, &should_free); - itup2 = tuplesort_getindextuple(btspool2->sortstate, - true, &should_free2); + itup = tuplesort_getindextuple(btspool->sortstate, true); + itup2 = tuplesort_getindextuple(btspool2->sortstate, true); indexScanKey = _bt_mkscankey_nodata(wstate->index); /* Prepare SortSupport data for each column */ @@ -775,18 +771,12 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2) if (load1) { _bt_buildadd(wstate, state, itup); - if (should_free) - pfree(itup); - itup = tuplesort_getindextuple(btspool->sortstate, - true, &should_free); + itup = tuplesort_getindextuple(btspool->sortstate, true); } else { _bt_buildadd(wstate, state, itup2); - if (should_free2) - pfree(itup2); - itup2 = tuplesort_getindextuple(btspool2->sortstate, - true, &should_free2); + itup2 = tuplesort_getindextuple(btspool2->sortstate, true); } } pfree(sortKeys); @@ -795,15 +785,13 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2) { /* merge is unnecessary */ while ((itup = tuplesort_getindextuple(btspool->sortstate, - true, &should_free)) != NULL) + true)) != NULL) { /* When we see first tuple, create first index page */ if (state == NULL) state = _bt_pagestate(wstate, 0); _bt_buildadd(wstate, state, itup); - if (should_free) - pfree(itup); } } diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index dc1f79f..2131226 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1057,11 +1057,10 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, for (;;) { HeapTuple tuple; - bool shouldfree; CHECK_FOR_INTERRUPTS(); - tuple = tuplesort_getheaptuple(tuplesort, true, &shouldfree); + tuple = tuplesort_getheaptuple(tuplesort, true); if (tuple == NULL) break; @@ -1069,9 +1068,6 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, oldTupDesc, newTupDesc, values, isnull, NewHeap->rd_rel->relhasoids, rwstate); - - if (shouldfree) - heap_freetuple(tuple); } tuplesort_end(tuplesort); diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index baf87b3..71524c2 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -1841,12 +1841,12 @@ tuplesort_performsort(Tuplesortstate *state) /* * Internal routine to fetch the next tuple in either forward or back * 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. + * Returned tuple belongs to tuplesort memory context, and must not be freed + * by caller. Caller should not use tuple following next call here. */ static bool tuplesort_gettuple_common(Tuplesortstate *state, bool forward, - SortTuple *stup, bool *should_free) + SortTuple *stup) { unsigned int tuplen; @@ -1855,7 +1855,6 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward, case TSS_SORTEDINMEM: Assert(forward || state->randomAccess); Assert(!state->slabAllocatorUsed); - *should_free = false; if (forward) { if (state->current < state->memtupcount) @@ -1927,7 +1926,6 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward, */ state->lastReturnedTuple = stup->tuple; - *should_free = false; return true; } else @@ -2008,14 +2006,12 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward, */ state->lastReturnedTuple = stup->tuple; - *should_free = false; return true; case TSS_FINALMERGE: Assert(forward); /* We are managing memory ourselves, with the slab allocator. */ Assert(state->slabAllocatorUsed); - *should_free = false; /* * The slab slot holding the tuple that we returned in previous @@ -2097,9 +2093,8 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward, { MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext); SortTuple stup; - bool should_free; - if (!tuplesort_gettuple_common(state, forward, &stup, &should_free)) + if (!tuplesort_gettuple_common(state, forward, &stup)) stup.tuple = NULL; MemoryContextSwitchTo(oldcontext); @@ -2110,12 +2105,8 @@ 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); + stup.tuple = heap_copy_minimal_tuple((MinimalTuple) stup.tuple); + ExecStoreMinimalTuple((MinimalTuple) stup.tuple, slot, true); return true; } else @@ -2127,18 +2118,17 @@ tuplesort_gettupleslot(Tuplesortstate *state, bool forward, /* * Fetch the next tuple in either forward or back direction. - * 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. + * Returns NULL if no more tuples. Returned tuple belongs to tuplesort memory + * context, and must not be freed by caller. Caller should not use tuple + * following next call here. */ HeapTuple -tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free) +tuplesort_getheaptuple(Tuplesortstate *state, bool forward) { MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext); SortTuple stup; - if (!tuplesort_gettuple_common(state, forward, &stup, should_free)) + if (!tuplesort_gettuple_common(state, forward, &stup)) stup.tuple = NULL; MemoryContextSwitchTo(oldcontext); @@ -2148,19 +2138,17 @@ tuplesort_getheaptuple(Tuplesortstate *state, bool forward, bool *should_free) /* * Fetch the next index tuple in either forward or back direction. - * 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. + * Returns NULL if no more tuples. Returned tuple belongs to tuplesort memory + * context, and must not be freed by caller. Caller should not use tuple + * following next call here. */ IndexTuple -tuplesort_getindextuple(Tuplesortstate *state, bool forward, - bool *should_free) +tuplesort_getindextuple(Tuplesortstate *state, bool forward) { MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext); SortTuple stup; - if (!tuplesort_gettuple_common(state, forward, &stup, should_free)) + if (!tuplesort_gettuple_common(state, forward, &stup)) stup.tuple = NULL; MemoryContextSwitchTo(oldcontext); @@ -2173,7 +2161,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. + * 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 @@ -2188,9 +2177,8 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward, { MemoryContext oldcontext = MemoryContextSwitchTo(state->sortcontext); SortTuple stup; - bool should_free; - if (!tuplesort_gettuple_common(state, forward, &stup, &should_free)) + if (!tuplesort_gettuple_common(state, forward, &stup)) { MemoryContextSwitchTo(oldcontext); return false; @@ -2208,11 +2196,7 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward, else { /* use stup.tuple because stup.datum1 may be an abbreviation */ - - if (should_free) - *val = PointerGetDatum(stup.tuple); - else - *val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen); + *val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen); *isNull = false; } @@ -2270,16 +2254,12 @@ tuplesort_skiptuples(Tuplesortstate *state, int64 ntuples, bool forward) while (ntuples-- > 0) { SortTuple stup; - bool should_free; - if (!tuplesort_gettuple_common(state, forward, - &stup, &should_free)) + if (!tuplesort_gettuple_common(state, forward, &stup)) { MemoryContextSwitchTo(oldcontext); return false; } - if (should_free && stup.tuple) - pfree(stup.tuple); CHECK_FOR_INTERRUPTS(); } MemoryContextSwitchTo(oldcontext); diff --git a/src/include/utils/tuplesort.h b/src/include/utils/tuplesort.h index 5cecd6d..38af746 100644 --- a/src/include/utils/tuplesort.h +++ b/src/include/utils/tuplesort.h @@ -94,10 +94,8 @@ extern void tuplesort_performsort(Tuplesortstate *state); extern bool tuplesort_gettupleslot(Tuplesortstate *state, bool forward, TupleTableSlot *slot, Datum *abbrev); -extern HeapTuple tuplesort_getheaptuple(Tuplesortstate *state, bool forward, - bool *should_free); -extern IndexTuple tuplesort_getindextuple(Tuplesortstate *state, bool forward, - bool *should_free); +extern HeapTuple tuplesort_getheaptuple(Tuplesortstate *state, bool forward); +extern IndexTuple tuplesort_getindextuple(Tuplesortstate *state, bool forward); extern bool tuplesort_getdatum(Tuplesortstate *state, bool forward, Datum *val, bool *isNull, Datum *abbrev); -- 2.7.4