[PATCH] Add memory/disk usage for Function Scan nodes in EXPLAIN
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253428psql -h localhost -U postgresBuilt from patchset v20 (message #20), October 24, 2026 at 02:55 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253428_20 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253428_20 && git checkout t253428_20Patchset v20 (message #20) is on t253428_20
Hi,
1eff8279d4, 95d6e9af07 and 40708acd65 added memory/disk usage for
Materialize, WindowAgg, CTE Scan, Table Function Scan and Recursive
Union in EXPLAIN ANALYZE. [1]Discussion for 40708acd65: /messages/by-id/20240918.211246.1127161704188186085.ishii@postgresql.org
So I wanted to add memory/disk usage for Function Scan.
## The patch
It follows the shape of the existing five nodes, so there are only two
things worth mentioning.
1. Handling of multiple tuplestores
A FunctionScan uses one tuplestore per function, so there can be more
than one when ROWS FROM is used. I used the same rule as Recursive
Union: the storage type of whichever one consumed the most
memory/disk, and the sum of the sizes of them all.
2. Moving FunctionScanPerFuncState to execnodes.h
Its definition lives in nodeFunctionscan.c and execnodes.h only has a
forward declaration, so explain.c cannot reach funcstates[i].tstore.
The state structs of the other five nodes are all in execnodes.h, so
I moved this one there too.
## Behavior
Measured with work_mem = 64kB. On its own, 1000 rows gives
"Memory 56kB" and 500000 rows gives "Disk 6836kB".
-- two identical functions: exactly twice the single-function figure
SELECT count(*) FROM ROWS FROM (generate_series(1,500000),
generate_series(1,500000)) g;
Storage: Disk Maximum Storage: 13672kB
-- a small one and a large one: type from the larger, size is the sum
SELECT count(*) FROM ROWS FROM (generate_series(1,10),
generate_series(1,500000)) g;
Storage: Disk Maximum Storage: 6853kB
## What is reported when loops > 1
The Storage line follows the same policy as the Sort Method line of
Sort, that is, it reports the peak recorded by whichever object is still
around at EXPLAIN time. The statistics live inside the Tuplestorestate
(or Tuplesortstate) and are lost along with it when rescan calls end().
ExecReScanFunctionScan() has the same shape as
ExecReScanTableFuncScan(), and on master both Sort and Table Function
Scan already change what they report if you reorder the rows.
When loops is 1 the value is of course exact.
I could not find a settled policy for how this kind of per-node resource
statistic should be aggregated when loops > 1. So this patch follows
Sort. If the consensus is that the maximum across all loops should be
reported instead, that would be a separate change spanning Sort,
Incremental Sort, Material, Table Function Scan and Function Scan, and
I would be happy to work on it separately.
make check passes all 245 tests.
Patch attached.
Regards,
Tatsuya Kawata
[1]: Discussion for 40708acd65: /messages/by-id/20240918.211246.1127161704188186085.ishii@postgresql.org
/messages/by-id/20240918.211246.1127161704188186085.ishii@postgresql.org
Hi Tatsuya,
I reviewed v1 of this patch on PostgreSQL 20devel on macOS ARM64.
The patch applied cleanly and built successfully. I also ran the full
regression suite using an out-of-tree build configured with --enable-cassert
--enable-debug; all 243 tests passed in my environment.
I tested the Function Scan storage reporting under several scenarios,
including memory-backed and disk-backed tuplestores by varying work_mem, ROWS
FROM with multiple functions, zero-row execution, WITH ORDINALITY, plans
containing both Sort and Function Scan, and user-defined set-returning
functions.
As a basic disk-backed case, with work_mem = '64kB':
EXPLAIN (ANALYZE)SELECT * FROM generate_series(1,10000);
reported:
Storage: Disk Maximum Storage: 137kB
Buffers: temp read=18 written=18
I also tested a SQL-language SRF whose underlying query performs an
internal ORDER BY. With work_mem = '64kB', the Function Scan reported:
Storage: Disk Maximum Storage: 1758kB
Buffers: shared hit=19, temp read=843 written=879
With the same function and work_mem = '16MB', it reported:
Storage: Memory Maximum Storage: 4150kB
with no temporary-buffer activity reported by the outer EXPLAIN.
The Storage / Maximum Storage reporting behaved consistently across the
cases I tested. I did not find a correctness issue with v1.
Regards,
Shashishekar Hullahally Anantharamu
On Thu, Sep 3, 2026 at 10:21 PM Tatsuya Kawata <kawatatatsuya0913@gmail.com>
wrote:
Show quoted text
Hi,
1eff8279d4, 95d6e9af07 and 40708acd65 added memory/disk usage for
Materialize, WindowAgg, CTE Scan, Table Function Scan and Recursive
Union in EXPLAIN ANALYZE. [1]
So I wanted to add memory/disk usage for Function Scan.## The patch
It follows the shape of the existing five nodes, so there are only two
things worth mentioning.1. Handling of multiple tuplestores
A FunctionScan uses one tuplestore per function, so there can be more
than one when ROWS FROM is used. I used the same rule as Recursive
Union: the storage type of whichever one consumed the most
memory/disk, and the sum of the sizes of them all.2. Moving FunctionScanPerFuncState to execnodes.h
Its definition lives in nodeFunctionscan.c and execnodes.h only has a
forward declaration, so explain.c cannot reach funcstates[i].tstore.
The state structs of the other five nodes are all in execnodes.h, so
I moved this one there too.## Behavior
Measured with work_mem = 64kB. On its own, 1000 rows gives
"Memory 56kB" and 500000 rows gives "Disk 6836kB".-- two identical functions: exactly twice the single-function figure
SELECT count(*) FROM ROWS FROM (generate_series(1,500000),
generate_series(1,500000)) g;
Storage: Disk Maximum Storage: 13672kB-- a small one and a large one: type from the larger, size is the sum
SELECT count(*) FROM ROWS FROM (generate_series(1,10),
generate_series(1,500000)) g;
Storage: Disk Maximum Storage: 6853kB## What is reported when loops > 1
The Storage line follows the same policy as the Sort Method line of
Sort, that is, it reports the peak recorded by whichever object is still
around at EXPLAIN time. The statistics live inside the Tuplestorestate
(or Tuplesortstate) and are lost along with it when rescan calls end().
ExecReScanFunctionScan() has the same shape as
ExecReScanTableFuncScan(), and on master both Sort and Table Function
Scan already change what they report if you reorder the rows.
When loops is 1 the value is of course exact.I could not find a settled policy for how this kind of per-node resource
statistic should be aggregated when loops > 1. So this patch follows
Sort. If the consensus is that the maximum across all loops should be
reported instead, that would be a separate change spanning Sort,
Incremental Sort, Material, Table Function Scan and Function Scan, and
I would be happy to work on it separately.make check passes all 245 tests.
Patch attached.Regards,
Tatsuya Kawata[1] Discussion for 40708acd65:
/messages/by-id/20240918.211246.1127161704188186085.ishii@postgresql.org
Hi Shashishekar,
Thank you very much for the thorough review and testing!
Regards,
Tatsuya Kawata
Hi Tatsuya,
You are very welcome! Glad I could help with the review.
Regards,
Shashishekar
On Tue, Sep 22, 2026 at 2:16 AM Tatsuya Kawata <kawatatatsuya0913@gmail.com>
wrote:
Show quoted text
Hi Shashishekar,
Thank you very much for the thorough review and testing!
Regards,
Tatsuya Kawata
On 4 Sep 2026, at 08:37, Shashishekar Hullahally Anantharamu <shashi.h.ananth@gmail.com> wrote:
I did not find a correctness issue with v1.
Shashishekar, do you think commitfest entry [0]https://commitfest.postgresql.org/patch/7153/ should be marked Ready For Committer?
If so - flip the patch state in the commitfest. Thank you!
Best regards, Andrey Borodin.
It's done!
Regards,
Shashi A
On Sun, Sep 27, 2026 at 12:26 PM Andrey Borodin <x4mmm@yandex-team.ru>
wrote:
Show quoted text
On 4 Sep 2026, at 08:37, Shashishekar Hullahally Anantharamu <
shashi.h.ananth@gmail.com> wrote:
I did not find a correctness issue with v1.
Shashishekar, do you think commitfest entry [0] should be marked Ready For
Committer?
If so - flip the patch state in the commitfest. Thank you!Best regards, Andrey Borodin.
On Sun, 16 Aug 2026 at 18:14, Tatsuya Kawata
<kawatatatsuya0913@gmail.com> wrote:
The Storage line follows the same policy as the Sort Method line of
Sort, that is, it reports the peak recorded by whichever object is still
around at EXPLAIN time. The statistics live inside the Tuplestorestate
(or Tuplesortstate) and are lost along with it when rescan calls end().
ExecReScanFunctionScan() has the same shape as
ExecReScanTableFuncScan(), and on master both Sort and Table Function
Scan already change what they report if you reorder the rows.
When loops is 1 the value is of course exact.
I understand that's what a few other nodes do, but I don't think
that's a great example to follow, especially given that the EXPLAIN
text claims the value is "Maximum Storage".
Looking at the output of the following, I've only swapped the VALUES
order. The memory reported by the patch is quite different in each
case.
explain analyze select a,g.s from (values(10000000),(1)) a(a), lateral
generate_series(1,a.a) g(s);
-> Function Scan on generate_series g (cost=0.00..10.00 rows=1000
width=4) (actual time=852.914..1477.110 rows=5000000.50 loops=2)
Storage: Memory Maximum Storage: 17kB
explain analyze select a,g.s from (values(1),(10000000)) a(a), lateral
generate_series(1,a.a) g(s);
-> Function Scan on generate_series g (cost=0.00..10.00 rows=1000
width=4) (actual time=838.233..1448.676 rows=5000000.50 loops=2)
Storage: Disk Maximum Storage: 136719kB
Can you prepare an initial patch that swaps tuplestore_end() for
tuplestore_clear() in the relevant locations (similar to what
908a96861 did). This can go in separately on the justification that
it's an optimisation to avoid the reallocation of fields that are
pfree'd in tuplestore_end().
I'll look at doing this for nodeMaterial.c. It might be somewhat
harder to get a plan with a parameterised Material node, however, but
it should be possible.
David
Hi David,
Thank you for the review!
Can you prepare an initial patch that swaps tuplestore_end() for
tuplestore_clear() in the relevant locations (similar to what
908a96861 did). This can go in separately on the justification that
it's an optimisation to avoid the reallocation of fields that are
pfree'd in tuplestore_end().
Sure! Happy to work on this. I'll prepare a separate patch and
post it once it's ready.
I'll look at doing this for nodeMaterial.c. It might be somewhat
harder to get a plan with a parameterised Material node, however, but
it should be possible.
I noticed you've already posted the nodeMaterial.c patch [1]/messages/by-id/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com, so I'll
use it as a reference.
Regards,
Tatsuya Kawata
[1]: /messages/by-id/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com
/messages/by-id/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com
Hi David,
Can you prepare an initial patch that swaps tuplestore_end() for
tuplestore_clear() in the relevant locations (similar to what
908a96861 did).
I took another look at whether the change you suggested can be applied
to Function Scan. There are differences from [1]/messages/by-id/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com and from commit
908a96861, and I've come to the conclusion that just changing
tuplestore_end() to tuplestore_clear() won't work here.
When a Function Scan is rescanned with changed parameters, the function
is called again and the tuplestore is created anew. With
rsinfo.returnMode == SFRM_ValuePerCall it's created on the executor side
by ExecMakeTableFunctionResult(), but with SFRM_Materialize it's created
by the function itself, so the functions would be affected as well.
Fixing the SFRM_Materialize case would be possible by allowing the
tuplestore to be passed to the function and calling tuplestore_clear()
on it, but that would change the SRF calling convention, which looks
like a fairly large change, extensions included.
So I'd like to split this into several steps:
1. Add fields to FunctionScanPerFuncState to hold the storage
statistics, along with a function to update them and the calls to
it.
2. Mostly the same as the v1 patch, except that EXPLAIN reports the
larger of the statistics saved in 1 and those of the final
tuplestore. tuplestore_end() is still called at this point.
3. For value-per-call mode only, stop calling tuplestore_end() and allow
the tuplestore to be passed in, as a performance improvement. This
is something I'd like to look into.
Whether to change the SRF calling convention is an entirely separate
question, and I'd like to keep it apart from this work.
Roughly, the split is this: since we can't avoid getting a new
tuplestore back in materialize mode, a way to save the statistics
before the tuplestore is discarded (1 and 2) is needed anyway for the
statistics to be correct, so I'd do that first, while 3 is a performance
optimisation.
What do you think? I've attached 1 and 2 as v2.
This can go in separately on the justification that
it's an optimisation to avoid the reallocation of fields that are
pfree'd in tuplestore_end().
In nodeTableFuncscan.c the node creates the tuplestore itself, so
I think it can be changed to use tuplestore_clear() in the same way as
908a96861. I'll start a separate thread and post a patch for that.
Regards,
Tatsuya Kawata
[1]: /messages/by-id/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com
/messages/by-id/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5=Z+_UXTUzUxdQg@mail.gmail.com
Attachments:
t253428_9v2-0001-Keep-Function-Scan-tuplestore-statistics-across-r.patchapplication/octet-stream; name=v2-0001-Keep-Function-Scan-tuplestore-statistics-across-r.patchDownload+38-1
v2-0002-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchapplication/octet-stream; name=v2-0002-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchDownload+161-29
On Mon, 5 Oct 2026 at 00:14, Tatsuya Kawata <kawatatatsuya0913@gmail.com> wrote:
Roughly, the split is this: since we can't avoid getting a new
tuplestore back in materialize mode, a way to save the statistics
before the tuplestore is discarded (1 and 2) is needed anyway for the
statistics to be correct, so I'd do that first, while 3 is a performance
optimisation.What do you think? I've attached 1 and 2 as v2.
I've read through the patch. Here's my review:
1. I'm confused by the following in explain.sql. We don't support
parallel function scans, so the comments and setting of
max_parallel_workers_per_gather seem bogus:
-- Ensure the Function Scan runs in the leader. The storage information is
-- taken from the leader's tuplestore, so a parallel plan would report
-- nothing here.
set max_parallel_workers_per_gather to 0;
2. In the following, 10000 tuples seems a little excessive to spill 64
kilobytes of work_mem. I checked on a 32-bit build without memory
context checking and 2500 row was enough. 2047 was the most I could
get without spilling, so the extra ~450 seems like a good enough
margin.
set work_mem to 64;
select explain_filter('explain (analyze,buffers off,costs off) select
count(*) from generate_series(1,10000) a(n)');
-- Test tuplestore storage usage in Function Scan with ROWS FROM, which uses
-- one tuplestore per function
select explain_filter('explain (analyze,buffers off,costs off) select
count(*) from rows from (generate_series(1,10000),
generate_series(1,10000)) a(n,m)');
You could also move the existing "reset work_mem;" down after your
tests. Your 10-row test won't spill with work_mem set to 64.
David
Hi David,
Thanks for the review!
1. I'm confused by the following in explain.sql. We don't support
parallel function scans, so the comments and setting of
max_parallel_workers_per_gather seem bogus:
Function Scan isn't parallel-aware, but generate_series() is parallel
safe, so with debug_parallel_query = regress the whole plan runs in a
parallel worker. EXPLAIN only reads the storage information from the
leader's tuplestore, so the Storage line doesn't appear in that case.
Indeed, when I removed the setting and ran make check with
debug_parallel_query = regress, the Storage line disappeared from all
three of the new tests and the explain test failed.
So I kept the setting and rewrote the comment to make the reason
clearer.
2. In the following, 10000 tuples seems a little excessive to spill 64
...
You could also move the existing "reset work_mem;" down after your
tests. Your 10-row test won't spill with work_mem set to 64.
As you suggested, I changed it to 2500 rows, moved the existing
"reset work_mem;" down after the new tests, and removed the
"set work_mem to 64;" that I had added.
In v3, only the tests in 0002 have changed; 0001 is unchanged.
Regards,
Tatsuya Kawata
Attachments:
t253428_11v3-0001-Keep-Function-Scan-tuplestore-statistics-across-r.patchapplication/octet-stream; name=v3-0001-Keep-Function-Scan-tuplestore-statistics-across-r.patchDownload+38-1
v3-0002-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchapplication/octet-stream; name=v3-0002-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchDownload+159-29
On Tue, 6 Oct 2026 at 03:09, Tatsuya Kawata <kawatatatsuya0913@gmail.com> wrote:
1. I'm confused by the following in explain.sql. We don't support
parallel function scans, so the comments and setting of
max_parallel_workers_per_gather seem bogus:Function Scan isn't parallel-aware, but generate_series() is parallel
safe, so with debug_parallel_query = regress the whole plan runs in a
parallel worker. EXPLAIN only reads the storage information from the
leader's tuplestore, so the Storage line doesn't appear in that case.Indeed, when I removed the setting and ran make check with
debug_parallel_query = regress, the Storage line disappeared from all
three of the new tests and the explain test failed.So I kept the setting and rewrote the comment to make the reason
clearer.
That doesn't seem great. On checking, I see Material has the same
issue, but that's not been highlighted by the buildfarm members
running debug_parallel_query = regress due to lack of EXPLAIN ANALYZE
test that has a Material node. I've just posted a patch to fix that in
[1]: /messages/by-id/CAApHDvoUWCms-b27MpDKd5_RUFtAnVrxPgN9u3QoQbBihk+S6g@mail.gmail.com
David
[1]: /messages/by-id/CAApHDvoUWCms-b27MpDKd5_RUFtAnVrxPgN9u3QoQbBihk+S6g@mail.gmail.com
Hi David,
Thank you for the comments and the patch!
That doesn't seem great. On checking, I see Material has the same
issue, but that's not been highlighted by the buildfarm members
running debug_parallel_query = regress due to lack of EXPLAIN ANALYZE
test that has a Material node. I've just posted a patch to fix that in
[1]. Can you follow that code and resolve this issue in your patch?
Understood. I haven't looked at your patch in [1] closely yet, but
I'll go through it and update my patch so that Function Scan also
reports the statistics from parallel workers.
I'll post a new version once it's ready.
Regards,
Tatsuya Kawata
Hi, I have been studying this thread for the past few days now and
tried to understand the problem,
I was interested in investigating the part of swapping
tuplestore_end() for tuplestore_clear() for function scan.
When a Function Scan is rescanned with changed parameters, the function
is called again and the tuplestore is created anew. With
rsinfo.returnMode == SFRM_ValuePerCall it's created on the executor side
by ExecMakeTableFunctionResult(), but with SFRM_Materialize it's created
by the function itself, so the functions would be affected as well.
Fixing the SFRM_Materialize case would be possible by allowing the
tuplestore to be passed to the function and calling tuplestore_clear()
on it, but that would change the SRF calling convention, which looks
like a fairly large change, extensions included.
For now I understand that for each rerun, a new tuple store would be
obtained along with new stats, and that it would mean changing srf
calling convention in order for the swap to work, but that would mean
a big change. I have also tried to understand the relevant code path.
So I wanted to try if I could find a way to solve this, as a
first-time contributor, I'd appreciate any feedback on whether my
understanding is correct, and whether changing the calling convention
is the only way here.
Thank You. Regards,
Yash Jadhav
On Fri, 9 Oct 2026 at 08:00, Yash Jadhav <yash.jadhav8008@gmail.com> wrote:
For now I understand that for each rerun, a new tuple store would be
obtained along with new stats, and that it would mean changing srf
calling convention in order for the swap to work, but that would mean
a big change. I have also tried to understand the relevant code path.So I wanted to try if I could find a way to solve this, as a
first-time contributor, I'd appreciate any feedback on whether my
understanding is correct, and whether changing the calling convention
is the only way here.
Switching to tuplestore_clear() is welcome for other reasons. There's
been no benchmarking done on this thread for it, but see the reported
performance gains mentioned in the commit message [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=908a9686.
This may be an argument for doing the tuplestore_clear() change as a
separate commit.
David
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=908a9686
Hi David,
That doesn't seem great. On checking, I see Material has the same
issue, but that's not been highlighted by the buildfarm members
running debug_parallel_query = regress due to lack of EXPLAIN ANALYZE
test that has a Material node. I've just posted a patch to fix that in
[1]. Can you follow that code and resolve this issue in your patch?
Thanks for committing the Material patch! v4 Attached.
The main changes from v3 are:
- As is done for Material, each worker now copies its statistics into
shared memory in ExecEndFunctionScan(), and EXPLAIN ANALYZE shows them
per worker.
- I changed how the statistics are kept, so that the workers and EXPLAIN
use the same values. The tuplestore returned by
ExecMakeTableFunctionResult() already holds the whole result set of
the function and doesn't grow after that, so its statistics are taken
right after it's returned, and used to update the per-function maximum
and the node-level values. EXPLAIN just reads the leader's values, and
each worker copies its own into shared memory when it ends.
This made v3-0001's saving of the statistics before a rescan discards
the tuplestore, and moving FunctionScanPerFuncState into execnodes.h,
unnecessary, so I've merged the patches into one. The statistics are
only collected for EXPLAIN ANALYZE.
- Removed max_parallel_workers_per_gather = 0 from the tests. I've
confirmed make check passes with debug_parallel_query = regress too.
Regards,
Tatsuya Kawata
Attachments:
t253428_16v4-0001-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchapplication/x-patch; name=v4-0001-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchDownload+325-4
On Wed, 14 Oct 2026 at 03:38, Tatsuya Kawata
<kawatatatsuya0913@gmail.com> wrote:
- I changed how the statistics are kept, so that the workers and EXPLAIN
use the same values. The tuplestore returned by
ExecMakeTableFunctionResult() already holds the whole result set of
the function and doesn't grow after that, so its statistics are taken
right after it's returned, and used to update the per-function maximum
and the node-level values. EXPLAIN just reads the leader's values, and
each worker copies its own into shared memory when it ends.
I don't think reporting the sum of the maximum space used by each
function is an accurate way to report the storage space used. I
believe what we should be showing is the "high water mark" of *total
storage* used per rescan, not the sum of high water marks per
function. With what you have, we could report a storage amount that
was never used by the Function Scan at any single moment in time.
To look at it another way: if you have two bank accounts with ¥1000 in
one and ¥0 in the other. If today you transfer the ¥1000 into the
account with ¥0, would you report your total net worth as the sum of
the maximum balance of the accounts on that day? Surely the answer is
¥1000 and not ¥2000, as at no point in time in this scenario did you
have ¥2000.
Effectively, the patch is reporting the ¥2000 scenario:
explain (analyze, costs off, timing off, summary off) select * from
(values(1),(2000)) v(i), lateral (select * from rows from
(generate_series(1, v.i), generate_series(2000, v.i, -1)));
Nested Loop (actual rows=4000.00 loops=1)
-> Values Scan on "*VALUES*" (actual rows=2.00 loops=1)
-> Function Scan on generate_series (actual rows=2000.00 loops=2)
Storage: Memory Maximum Storage: 189kB
In each rescan there's a function with a 1-row series and the other
has a 2000-row series. The patch is reporting the memory used by 4000
rows instead of 2001 rows.
I'd expect to see this report 112kB, effectively 17kB + 95kB per:
explain (analyze, costs off, timing off, summary off) select * from
generate_Series(1,2000);
Function Scan on generate_series (actual rows=2000.00 loops=1)
Storage: Memory Maximum Storage: 95kB
explain (analyze, costs off, timing off, summary off) select * from
generate_Series(2000,2000,-1);
Function Scan on generate_series (actual rows=1.00 loops=1)
Storage: Memory Maximum Storage: 17kB
David
Hi David,
I don't think reporting the sum of the maximum space used by each
function is an accurate way to report the storage space used. I
believe what we should be showing is the "high water mark" of *total
storage* used per rescan, not the sum of high water marks per
function. With what you have, we could report a storage amount that
was never used by the Function Scan at any single moment in time.To look at it another way: if you have two bank accounts with ¥1000 in
one and ¥0 in the other. If today you transfer the ¥1000 into the
account with ¥0, would you report your total net worth as the sum of
the maximum balance of the accounts on that day? Surely the answer is
¥1000 and not ¥2000, as at no point in time in this scenario did you
have ¥2000.
Thanks for pointing that out! That makes sense. Attached is v5.
v5 now computes the total space used by the tuplestores that exist at
the same time, and reports the maximum of that.
explain (analyze, costs off, timing off, summary off) select * from
(values(1),(2000)) v(i), lateral (select * from rows from
(generate_series(1, v.i), generate_series(2000, v.i, -1)));
Your example query now shows 111kB, which is almost what you expected.
Regards,
Tatsuya Kawata
Attachments:
t253428_18v5-0001-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchapplication/octet-stream; name=v5-0001-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patchDownload+309-4
On Fri, 16 Oct 2026 at 04:01, Tatsuya Kawata
<kawatatatsuya0913@gmail.com> wrote:
Thanks for pointing that out! That makes sense. Attached is v5.
I looked at v5. Can we not switch back to how you had it in v2 by
moving the FunctionScanPerFuncState into execnodes.h and using that
directly in explain.c? It's the FunctionNext() changes that I'm not
keen on. I feel that's adding too much code to a function that
executes on every row. I quickly put that together in the attached,
which applies on top of v5. It seems to pass the tests. I didn't look
further than that, so please check.
David
Attachments:
nocfbot-fixes.diff.txttext/plain; charset=US-ASCII; name=nocfbot-fixes.diff.txtDownload+85-71
Hi David,
It's the FunctionNext() changes that I'm not keen on. I feel that's
adding too much code to a function that executes on every row.
Thanks for the review and the patch! I'll keep that in mind when
touching the executor's hot paths.
Attached is v6, based on your patch, which leaves FunctionNext() as is.
0001 is v5 as is, and 0002 is your patch with my changes on top. I kept
them separate so that the changes from your patch are easy to see, but
0002 should be squashed into 0001.
The main change is in the total taken at rescan. The statistics were
only collected when bms_overlap() returned true, so the tuplestores of
the functions whose parameters didn't change weren't included. I've
changed that and moved the code into update_storage_stats(). For example,
the following query showed 111kB, whereas the correct value is 189kB:
explain (analyze, costs off, timing off, summary off) select * from
(values(2000),(1)) v(i), lateral (select * from rows from
(generate_series(1, v.i), generate_series(1, 2000)));
Other than that, I've put the number of rows in the ROWS FROM test back
to 2500, and fixed compiler warnings and a typo in a comment. With 1250
rows, neither tuplestore spilled to disk on a build without
--enable-cassert, so the test failed with "Memory".
Regards,
Tatsuya Kawata