Re: Changed SRF in targetlist handling

On 2016-08-17 17:41:28 -0700, Andres Freund wrote:
> a) Allow to avoid using a tuplestore for SRF_PERCALL SRFs in ROWS FROM -
> otherwise our performance would regress noticeably in some cases.

To demonstrate the problem:

master:
=# COPY (SELECT generate_series(1, 50000000)) TO '/dev/null';
COPY 50000000
Time: 6859.830 ms
=# COPY (SELECT * FROM generate_series(1, 50000000)) TO '/dev/null';
COPY 50000000
Time: 11314.507 ms

getting rid of the materialization indeed fixes the problem:

dev:
=# COPY (SELECT generate_series(1, 50000000)) TO
'/dev/null';
COPY 50000000
Time: 5757.547 ms

=# COPY (SELECT * FROM generate_series(1, 50000000)) TO
'/dev/null';
COPY 50000000
Time: 5842.524 ms

I've currently implemented this by having nodeFunctionscan.c store
enough state in FunctionScanPerFuncState to continue the ValuePerCall
protocol. That all seems to work well, without big problems.

The open issue here is whether / how we want to deal with
EXEC_FLAG_REWIND and EXEC_FLAG_BACKWARD. Currently that, with some added
complications, is implemented in nodeFunctionscan.c itself. But for
ValuePerCall SRFs that doesn't directly work anymore.

ISTM that the easiest way here is actually to rip out support for
EXEC_FLAG_REWIND/EXEC_FLAG_BACKWARD from nodeFunctionscan.c. If the plan
requires that, the planner will slap a Material node on-top. Which will
even be more efficient when ROWS FROM for multiple SRFs, or WITH
ORDINALITY, are used. Alternatively we can continue to create a
tuplestore for ValuePerCall when eflags indicates that's required. But
personally I don't see that as an advantageous course.

Comments?

Andres