From 3856cf9607f41245ec9462519c53f1109e781fc5 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 12 Dec 2016 15:57:35 -0500 Subject: [PATCH] Remove should_free arguments to tuplesort routines. Since commit e94568ecc10f2638e542ae34f2990b821bbf90ac, the answer is always "false", and we do not need to complicate the API by arranging to return a constant value. Peter Geoghegan Discussion: http://postgr.es/m/CAM3SWZQWZZ_N=DmmL7tKy_OUjGH_5mN=N=A6h7kHyyDvEhg2DA@mail.gmail.com --- 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 8938ab5b24..12fb78a5c0 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 99a014e8f4..7f65b5ad6f 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 dc1f79f594..21312268a0 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 baf87b3cdf..71524c267f 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 5cecd6d1b8..38af746c85 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.40.0