On Tue, 2020-02-04 at 18:10 +0200, Heikki Linnakangas wrote:
> I'd love to change the LogicalTape API so that you could allocate
> and
> free tapes more freely. I wrote a patch to do that, as part of
> replacing
> tuplesort.c's polyphase algorithm with a simpler one (see [1]), but
> I
> never got around to committing it. Maybe the time is ripe to do that
> now?
It's interesting that you wrote a patch to pause the tapes a while ago.
Did it just fall through the cracks or was there a problem with it?
Is pause/resume functionality required, or is it good enough that
rewinding a tape frees the buffer, to be lazily allocated later?
Regarding the API, I'd like to change it, but I'm running into some
performance challenges when adding a layer of indirection. If I apply
the very simple attached patch, which simply makes a separate
allocation for the tapes array, it seems to slow down sort by ~5%.
Regards,
Jeff Davis
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index 42cfb1f9f98..5a47835024e 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -202,7 +202,7 @@ struct LogicalTapeSet
/* The array of logical tapes. */
int nTapes; /* # of logical tapes in set */
- LogicalTape tapes[FLEXIBLE_ARRAY_MEMBER]; /* has nTapes nentries */
+ LogicalTape *tapes; /* has nTapes nentries */
};
static void ltsWriteBlock(LogicalTapeSet *lts, long blocknum, void *buffer);
@@ -518,8 +518,7 @@ LogicalTapeSetCreate(int ntapes, TapeShare *shared, SharedFileSet *fileset,
* Create top-level struct including per-tape LogicalTape structs.
*/
Assert(ntapes > 0);
- lts = (LogicalTapeSet *) palloc(offsetof(LogicalTapeSet, tapes) +
- ntapes * sizeof(LogicalTape));
+ lts = (LogicalTapeSet *) palloc(sizeof(LogicalTapeSet));
lts->nBlocksAllocated = 0L;
lts->nBlocksWritten = 0L;
lts->nHoleBlocks = 0L;
@@ -529,6 +528,7 @@ LogicalTapeSetCreate(int ntapes, TapeShare *shared, SharedFileSet *fileset,
lts->freeBlocks = (long *) palloc(lts->freeBlocksLen * sizeof(long));
lts->nFreeBlocks = 0;
lts->nTapes = ntapes;
+ lts->tapes = (LogicalTape *) palloc(ntapes * sizeof(LogicalTape));
/*
* Initialize per-tape structs. Note we allocate the I/O buffer and the
