Re: [PATCH] Add memory/disk usage for Function Scan nodes in EXPLAIN

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