On Fri, 6 Sept 2024 at 16:21, Tatsuo Ishii <[email protected]> wrote:
> However, for 10, 2, 1 partitions. I see large performance
> degradation with the patched version: patched is slower than stock
> master in 1.5% (10 partitions), 41% (2 partitions) and 55.7% (1
> partition). See the attached graph.
Thanks for making the adjustments to this.
I don't think there is any need to call tuplestore_updatemax() from
within writetup_heap(). That means having to update the maximum space
used every time a tuple is written to disk. That's a fairly massive
overhead.
Instead, it should be fine to modify tuplestore_updatemax() to set a
flag to true if state->status != TSS_INMEM and then record the disk
space used. That flag won't ever be set to false again.
tuplestore_storage_type_name() should just return "Disk" if the new
disk flag is set, even if state->status == TSS_INMEM. Since the
work_mem size won't change between tuplestore_clear() calls, if we've
once spilt to disk, then we shouldn't care about the memory used for
runs that didn't. Those will always have used less memory.
I did this quickly, but playing around with the attached, I didn't see
any slowdown.
Here's the results I got on my Zen2 AMD machine:
parts master yours mine mine_v_master
10000 5.01 5.12 5.09 99%
1000 4.30 4.25 4.24 101%
100 4.17 4.13 4.12 101%
10 4.16 4.12 4.10 101%
2 4.75 7.64 4.73 100%
1 4.75 8.57 4.73 100%
David
diff --git a/src/backend/utils/sort/tuplestore.c
b/src/backend/utils/sort/tuplestore.c
index 444c8e25c2..03d745c609 100644
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -107,9 +107,10 @@ struct Tuplestorestate
bool backward; /* store extra length words in
file? */
bool interXact; /* keep open through
transactions? */
bool truncated; /* tuplestore_trim has removed
tuples? */
+ bool usedDisk; /* true if tuple store has ever
gone to disk */
int64 availMem; /* remaining memory available,
in bytes */
int64 allowedMem; /* total memory allowed, in
bytes */
- int64 maxSpace; /* maximum space used in memory
*/
+ int64 maxSpace; /* maximum space used in memory
or disk */
int64 tuples; /* number of tuples added */
BufFile *myfile; /* underlying file, or NULL if
none */
MemoryContext context; /* memory context for holding tuples */
@@ -262,6 +263,7 @@ tuplestore_begin_common(int eflags, bool interXact, int
maxKBytes)
state->eflags = eflags;
state->interXact = interXact;
state->truncated = false;
+ state->usedDisk = false;
state->allowedMem = maxKBytes * 1024L;
state->availMem = state->allowedMem;
state->maxSpace = 0;
@@ -1499,17 +1501,25 @@ tuplestore_updatemax(Tuplestorestate *state)
if (state->status == TSS_INMEM)
state->maxSpace = Max(state->maxSpace,
state->allowedMem -
state->availMem);
+ else
+ {
+ state->maxSpace = Max(state->maxSpace,
+
BufFileSize(state->myfile));
+ state->usedDisk = true;
+ }
}
/*
* tuplestore_storage_type_name
* Return a string description of the storage method used to store
the
- * tuples.
+ * tuples. We don't use the current status as if the tuplestore
went to
+ * disk and a tuplestore_clear() allowed it to switch back to
memory
+ * again, we still want to know that it has spilled to disk.
*/
const char *
tuplestore_storage_type_name(Tuplestorestate *state)
{
- if (state->status == TSS_INMEM)
+ if (!state->usedDisk)
return "Memory";
else
return "Disk";
@@ -1517,8 +1527,7 @@ tuplestore_storage_type_name(Tuplestorestate *state)
/*
* tuplestore_space_used
- * Return the maximum space used in memory unless the tuplestore
has spilled
- * to disk, in which case, return the disk space used.
+ * Return the maximum space used in memory or disk.
*/
int64
tuplestore_space_used(Tuplestorestate *state)
@@ -1526,10 +1535,7 @@ tuplestore_space_used(Tuplestorestate *state)
/* First, update the maxSpace field */
tuplestore_updatemax(state);
- if (state->status == TSS_INMEM)
- return state->maxSpace;
- else
- return BufFileSize(state->myfile);
+ return state->maxSpace;
}
/*
@@ -1601,7 +1607,6 @@ writetup_heap(Tuplestorestate *state, void *tup)
if (state->backward) /* need trailing length word? */
BufFileWrite(state->myfile, &tuplen, sizeof(tuplen));
- /* no need to call tuplestore_updatemax() when not in TSS_INMEM */
FREEMEM(state, GetMemoryChunkSpace(tuple));
heap_free_minimal_tuple(tuple);
}
