Hi,
I tested this on master (50d6e533e4), on macOS arm64, with a debug
build with assertions enabled.
The patch applies cleanly and the regression tests pass. The two
EXPLAIN ANALYZE cases from the first message behave as described:
LIMIT query: still in progress / 0kB -> top-N heapsort / 25kB
plain ORDER BY: quicksort / 3073kB -> external merge / Disk: 3920kB
I also tried the other callers of tuplesort_putdatum() that go through
the datum path, using work_mem = 4MB, a 100k-row table of md5() values
and log_temp_files = 0 with log_statement = 'all'. On master,
count(DISTINCT h), array_agg(h ORDER BY h) and
percentile_disc(0.5) WITHIN GROUP (ORDER BY h) don't write any
temporary file. With the patch, each of them writes one (about 3.5MB),
so the bug was not limited to Sort nodes.
The new code follows what the other tuplesort_put*() functions do, and
the comment about GetMemoryChunkSpace() and bump contexts matches. I
have no concerns about the patch itself.
Two small questions: since 6ed83d5fa55 went into 17, are we planning
to backpatch this to 17 and 18? And since some queries that used to
stay in memory will now spill, would it be worth saying so in the
commit message?
Thanks,
João Marcelo
Hi hackers,
Memory allocated for copied pass-by-reference Datums was not accounted
against work_mem because tuplesort_putdatum() passed a hardcoded tuplen
of 0 to tuplesort_puttuple_common(). Function free_sort_tuple() adjusts
the accounting by the amount actually allocated, so freeing such a
tuple subtracts an amount that was never added.
This was introduced in 6ed83d5fa55, which switched non-bounded sorts to
bump contexts. That commit correctly changed the other tuplesort_put*()
functions to compute the size, leaving only this one passing hardcoded
0.
So currently:
1) Bounded datum sorts are misreported. With work_mem = 4MB:
EXPLAIN ANALYZE SELECT md5(i::text) AS hash
FROM generate_series(1,100000) i
ORDER BY hash LIMIT 5;
master: Sort Method: still in progress Memory: 0kB
patched: Sort Method: top-N heapsort Memory: 25kB
2) work_mem is not enforced against the tuple data, and hold more data
than allowed before spilling With work_mem = 4MB:
EXPLAIN ANALYZE SELECT md5(i::text) AS hash
FROM generate_series(1,100000) i
ORDER BY hash;
master: Sort Method: quicksort Memory: 3073kB
patched: Sort Method: external merge Disk: 3920kB
The attached patch computes tuplen the way the tuplesort_put*()
variants do.
Thanks,
Mario
