pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: logtape.c stats don't account for unused "prefetched" block numbers
5+ messages / 3 participants
[nested] [flat]
* Re: logtape.c stats don't account for unused "prefetched" block numbers
@ 2020-09-01 23:36 Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-02 00:24 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 5+ messages in thread
From: Alvaro Herrera @ 2020-09-01 23:36 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-hackers
On 2020-Jul-30, Peter Geoghegan wrote:
> Commit 896ddf9b added prefetching to logtape.c to avoid excessive
> fragmentation in the context of hash aggs that spill and have many
> batches/tapes. Apparently the preallocation doesn't actually perform
> any filesystem operations, so the new mechanism should be zero
> overhead when "preallocated" blocks aren't actually used after all
> (right?). However, I notice that this breaks the statistics shown by
> things like trace_sort, and even EXPLAIN ANALYZE.
> LogicalTapeSetBlocks() didn't get the memo about preallocation.
This open item hasn't received any replies. I think Peter knows how to
fix it already, but no patch has been posted ... It'd be good to get a
move on it.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: logtape.c stats don't account for unused "prefetched" block numbers
2020-09-01 23:36 Re: logtape.c stats don't account for unused "prefetched" block numbers Alvaro Herrera <alvherre@2ndquadrant.com>
@ 2020-09-02 00:24 ` Peter Geoghegan <pg@bowt.ie>
2020-09-05 19:03 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 5+ messages in thread
From: Peter Geoghegan @ 2020-09-02 00:24 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-hackers
On Tue, Sep 1, 2020 at 4:36 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> This open item hasn't received any replies. I think Peter knows how to
> fix it already, but no patch has been posted ... It'd be good to get a
> move on it.
I picked this up again today.
It's not obvious what we should do. It's true that the instrumentation
doesn't accurately reflect the on-disk temp file overhead. That is, it
doesn't agree with the high watermark temp file size I see in the
pgsql_tmp directory, which is a clear regression compared to earlier
releases (where tuplesort was the only user of logtape.c). But it's
also true that we need to use somewhat more temp file space for a
tuplesort in Postgres 13, because we use the preallocation stuff for
tuplesort -- though probably without getting any benefit for it.
I haven't figured out how to correct the accounting just yet. In fact,
I'm not sure that this isn't some kind of leak of blocks from the
freelist, which shouldn't happen at all. The code is complicated
enough that I wasn't able to work that out in the couple of hours I
spent on it today. I can pick it up again tomorrow.
BTW, this MaxAllocSize freeBlocksLen check is wrong -- doesn't match
the later repalloc allocation:
if (lts->nFreeBlocks >= lts->freeBlocksLen)
{
/*
* If the freelist becomes very large, just return and leak this free
* block.
*/
if (lts->freeBlocksLen * 2 > MaxAllocSize)
return;
lts->freeBlocksLen *= 2;
lts->freeBlocks = (long *) repalloc(lts->freeBlocks,
lts->freeBlocksLen * sizeof(long));
}
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: logtape.c stats don't account for unused "prefetched" block numbers
2020-09-01 23:36 Re: logtape.c stats don't account for unused "prefetched" block numbers Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-02 00:24 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
@ 2020-09-05 19:03 ` Peter Geoghegan <pg@bowt.ie>
2020-09-08 17:27 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Jeff Davis <pgsql@j-davis.com>
2020-09-09 06:28 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Jeff Davis <pgsql@j-davis.com>
0 siblings, 2 replies; 5+ messages in thread
From: Peter Geoghegan @ 2020-09-05 19:03 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; pgsql-hackers
On Tue, Sep 1, 2020 at 5:24 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Tue, Sep 1, 2020 at 4:36 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > This open item hasn't received any replies. I think Peter knows how to
> > fix it already, but no patch has been posted ... It'd be good to get a
> > move on it.
>
> I picked this up again today.
One easy way to get logtape.c to behave in the same way as Postgres 12
for a multi-pass external sort (i.e. to use fewer blocks and to report
the number of blocks used accurately) is to #define
TAPE_WRITE_PREALLOC_MIN and TAPE_WRITE_PREALLOC_MAX to 1. So it looks
like the problem is in the preallocation stuff added by commit
896ddf9b3cd, and not the new heap-based free list logic added by
commit c02fdc92230. That's good news, because it means that the
problem may be fairly well isolated -- commit 896ddf9b3cd was a pretty
small and isolated thing.
The comments in ltsWriteBlock() added by the 2017 bugfix commit
7ac4a389a7d clearly say that the zero block writing stuff is only
supposed to happen at the edge of a tape boundary, which ought to be
rare -- see the comment block in ltsWriteBlock(). And yet the new
preallocation stuff explicitly relies on that it writing zero blocks
much more frequently. I'm concerned that that can result in increased
and unnecessary I/O, especially for sorts, but also for hash aggs that
spill. I'm also concerned that having preallocated-but-allocated
blocks confuses the accounting used by
trace_sort/LogicalTapeSetBlocks().
Separately, it's possible to make the
trace_sort/LogicalTapeSetBlocks() instrumentation agree with the
filesystem by replacing the use of nBlocksAllocated within
LogicalTapeSetBlocks() with nBlocksWritten -- that seems to make the
instrumentation correct without changing the current behavior at all.
But I'm not ready to endorse that approach, since it's not quite clear
what nBlocksAllocated and nBlocksWritten mean right now -- those two
fields were both added by the aforementioned 2017 bugfix commit, which
introduced the "allocated vs written" distinction in the first place.
We should totally disable the preallocation stuff for external sorts
in any case. External sorts are naturally characterized by relatively
large, distinct batching of reads and writes -- preallocation cannot
help.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: logtape.c stats don't account for unused "prefetched" block numbers
2020-09-01 23:36 Re: logtape.c stats don't account for unused "prefetched" block numbers Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-02 00:24 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
2020-09-05 19:03 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
@ 2020-09-08 17:27 ` Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 5+ messages in thread
From: Jeff Davis @ 2020-09-08 17:27 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
On Sat, 2020-09-05 at 12:03 -0700, Peter Geoghegan wrote:
> We should totally disable the preallocation stuff for external sorts
> in any case. External sorts are naturally characterized by relatively
> large, distinct batching of reads and writes -- preallocation cannot
> help.
Patch attached to disable preallocation for Sort.
I'm still looking into the other concerns.
Regards,
Jeff Davis
Attachments:
[text/x-patch] sort-no-prealloc.patch (5.7K, ../../0383d21d58c6fd7d2426d7730c50c6743a5ba303.camel@j-davis.com/2-sort-no-prealloc.patch)
download | inline diff:
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 9776263ae75..f74d4841f17 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2882,7 +2882,7 @@ hashagg_tapeinfo_init(AggState *aggstate)
HashTapeInfo *tapeinfo = palloc(sizeof(HashTapeInfo));
int init_tapes = 16; /* expanded dynamically */
- tapeinfo->tapeset = LogicalTapeSetCreate(init_tapes, NULL, NULL, -1);
+ tapeinfo->tapeset = LogicalTapeSetCreate(init_tapes, true, NULL, NULL, -1);
tapeinfo->ntapes = init_tapes;
tapeinfo->nfreetapes = init_tapes;
tapeinfo->freetapes_alloc = init_tapes;
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index bbb01f6d337..8ec224b62de 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -212,6 +212,7 @@ struct LogicalTapeSet
long *freeBlocks; /* resizable array holding minheap */
long nFreeBlocks; /* # of currently free blocks */
Size freeBlocksLen; /* current allocated length of freeBlocks[] */
+ bool enable_prealloc; /* preallocate write blocks? */
/* The array of logical tapes. */
int nTapes; /* # of logical tapes in set */
@@ -220,6 +221,7 @@ struct LogicalTapeSet
static void ltsWriteBlock(LogicalTapeSet *lts, long blocknum, void *buffer);
static void ltsReadBlock(LogicalTapeSet *lts, long blocknum, void *buffer);
+static long ltsGetBlock(LogicalTapeSet *lts, LogicalTape *lt);
static long ltsGetFreeBlock(LogicalTapeSet *lts);
static long ltsGetPreallocBlock(LogicalTapeSet *lts, LogicalTape *lt);
static void ltsReleaseBlock(LogicalTapeSet *lts, long blocknum);
@@ -373,8 +375,20 @@ parent_offset(unsigned long i)
}
/*
- * Select the lowest currently unused block by taking the first element from
- * the freelist min heap.
+ * Get the next block for writing.
+ */
+static long
+ltsGetBlock(LogicalTapeSet *lts, LogicalTape *lt)
+{
+ if (lts->enable_prealloc)
+ return ltsGetPreallocBlock(lts, lt);
+ else
+ return ltsGetFreeBlock(lts);
+}
+
+/*
+ * Select the lowest currently unused block from the tape set's global free
+ * list min heap.
*/
static long
ltsGetFreeBlock(LogicalTapeSet *lts)
@@ -430,7 +444,8 @@ ltsGetFreeBlock(LogicalTapeSet *lts)
/*
* Return the lowest free block number from the tape's preallocation list.
- * Refill the preallocation list if necessary.
+ * Refill the preallocation list with blocks from the tape set's free list if
+ * necessary.
*/
static long
ltsGetPreallocBlock(LogicalTapeSet *lts, LogicalTape *lt)
@@ -671,8 +686,8 @@ ltsInitReadBuffer(LogicalTapeSet *lts, LogicalTape *lt)
* infrastructure that may be lifted in the future.
*/
LogicalTapeSet *
-LogicalTapeSetCreate(int ntapes, TapeShare *shared, SharedFileSet *fileset,
- int worker)
+LogicalTapeSetCreate(int ntapes, bool preallocate, TapeShare *shared,
+ SharedFileSet *fileset, int worker)
{
LogicalTapeSet *lts;
int i;
@@ -689,6 +704,7 @@ LogicalTapeSetCreate(int ntapes, TapeShare *shared, SharedFileSet *fileset,
lts->freeBlocksLen = 32; /* reasonable initial guess */
lts->freeBlocks = (long *) palloc(lts->freeBlocksLen * sizeof(long));
lts->nFreeBlocks = 0;
+ lts->enable_prealloc = preallocate;
lts->nTapes = ntapes;
lts->tapes = (LogicalTape *) palloc(ntapes * sizeof(LogicalTape));
@@ -782,7 +798,7 @@ LogicalTapeWrite(LogicalTapeSet *lts, int tapenum,
Assert(lt->firstBlockNumber == -1);
Assert(lt->pos == 0);
- lt->curBlockNumber = ltsGetPreallocBlock(lts, lt);
+ lt->curBlockNumber = ltsGetBlock(lts, lt);
lt->firstBlockNumber = lt->curBlockNumber;
TapeBlockGetTrailer(lt->buffer)->prev = -1L;
@@ -806,7 +822,7 @@ LogicalTapeWrite(LogicalTapeSet *lts, int tapenum,
* First allocate the next block, so that we can store it in the
* 'next' pointer of this block.
*/
- nextBlockNumber = ltsGetPreallocBlock(lts, lt);
+ nextBlockNumber = ltsGetBlock(lts, lt);
/* set the next-pointer and dump the current block. */
TapeBlockGetTrailer(lt->buffer)->next = nextBlockNumber;
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 3c49476483b..cbda911f465 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2591,7 +2591,7 @@ inittapes(Tuplesortstate *state, bool mergeruns)
/* Create the tape set and allocate the per-tape data arrays */
inittapestate(state, maxTapes);
state->tapeset =
- LogicalTapeSetCreate(maxTapes, NULL,
+ LogicalTapeSetCreate(maxTapes, false, NULL,
state->shared ? &state->shared->fileset : NULL,
state->worker);
@@ -4657,8 +4657,9 @@ leader_takeover_tapes(Tuplesortstate *state)
* randomAccess is disallowed for parallel sorts.
*/
inittapestate(state, nParticipants + 1);
- state->tapeset = LogicalTapeSetCreate(nParticipants + 1, shared->tapes,
- &shared->fileset, state->worker);
+ state->tapeset = LogicalTapeSetCreate(nParticipants + 1, false,
+ shared->tapes, &shared->fileset,
+ state->worker);
/* mergeruns() relies on currentRun for # of runs (in one-pass cases) */
state->currentRun = nParticipants;
diff --git a/src/include/utils/logtape.h b/src/include/utils/logtape.h
index 39a99174afe..da5159e4c6c 100644
--- a/src/include/utils/logtape.h
+++ b/src/include/utils/logtape.h
@@ -54,7 +54,8 @@ typedef struct TapeShare
* prototypes for functions in logtape.c
*/
-extern LogicalTapeSet *LogicalTapeSetCreate(int ntapes, TapeShare *shared,
+extern LogicalTapeSet *LogicalTapeSetCreate(int ntapes, bool preallocate,
+ TapeShare *shared,
SharedFileSet *fileset, int worker);
extern void LogicalTapeSetClose(LogicalTapeSet *lts);
extern void LogicalTapeSetForgetFreeSpace(LogicalTapeSet *lts);
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: logtape.c stats don't account for unused "prefetched" block numbers
2020-09-01 23:36 Re: logtape.c stats don't account for unused "prefetched" block numbers Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-02 00:24 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
2020-09-05 19:03 ` Re: logtape.c stats don't account for unused "prefetched" block numbers Peter Geoghegan <pg@bowt.ie>
@ 2020-09-09 06:28 ` Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 5+ messages in thread
From: Jeff Davis @ 2020-09-09 06:28 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
On Sat, 2020-09-05 at 12:03 -0700, Peter Geoghegan wrote:
> The comments in ltsWriteBlock() added by the 2017 bugfix commit
> 7ac4a389a7d clearly say that the zero block writing stuff is only
> supposed to happen at the edge of a tape boundary, which ought to be
> rare -- see the comment block in ltsWriteBlock(). And yet the new
> preallocation stuff explicitly relies on that it writing zero blocks
> much more frequently. I'm concerned that that can result in increased
> and unnecessary I/O, especially for sorts, but also for hash aggs
> that
> spill. I'm also concerned that having preallocated-but-allocated
> blocks confuses the accounting used by
> trace_sort/LogicalTapeSetBlocks().
Preallocation showed significant gains for HashAgg, and BufFile doesn't
support sparse writes. So, for HashAgg, it seems like we should just
update the comment and consider it the price of using BufFile.
(Aside: is there a reason why BufFile doesn't support sparse writes, or
is it just a matter of implementation?)
For Sort, we can just disable preallocation.
> Separately, it's possible to make the
> trace_sort/LogicalTapeSetBlocks() instrumentation agree with the
> filesystem by replacing the use of nBlocksAllocated within
> LogicalTapeSetBlocks() with nBlocksWritten -- that seems to make the
> instrumentation correct without changing the current behavior at all.
> But I'm not ready to endorse that approach, since it's not quite
> clear
> what nBlocksAllocated and nBlocksWritten mean right now -- those two
> fields were both added by the aforementioned 2017 bugfix commit,
> which
> introduced the "allocated vs written" distinction in the first place
Right now, it seems nBlocksAllocated means "number of blocks returned
by ltsGetFreeBlock(), plus nHoleBlocks".
nBlocksWritten seems to mean "the logical size of the BufFile". The
BufFile can have holes in it after concatenation, but from the
perspective of logtape.c, nBlocksWritten seems like a better fit for
instrumentation purposes. So I'd be inclined to return (nBlocksWritten
- nHoleBlocks).
The only thing I can think of that would be better is if BufFile
tracked for itself the logical vs. physical size, which might be a good
improvement to make (and would mean that logtape.c wouldn't be
responsible for tracking the holes itself).
Thoughts?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2020-09-09 06:28 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 23:36 Re: logtape.c stats don't account for unused "prefetched" block numbers Alvaro Herrera <alvherre@2ndquadrant.com>
2020-09-02 00:24 ` Peter Geoghegan <pg@bowt.ie>
2020-09-05 19:03 ` Peter Geoghegan <pg@bowt.ie>
2020-09-08 17:27 ` Jeff Davis <pgsql@j-davis.com>
2020-09-09 06:28 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox