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] 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] https://www.postgresql.org/message-id/flat/CAApHDvoa55vcRth05Ozu5be4FawgTH-aCsZ5%3DZ%2B_UXTUzUxdQg%40mail.gmail.com
v2-0001-Keep-Function-Scan-tuplestore-statistics-across-r.patch
Description: Binary data
v2-0002-Add-memory-disk-usage-for-Function-Scan-nodes-in-.patch
Description: Binary data
