Horton

Thread

Commits

  1. Complain if a function-in-FROM returns a set when it shouldn't.

  2. Properly mark pg_stat_get_subscription() as returning a set.

  1. Why isn't pg_stat_get_subscription() marked as proretset?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-07T18:29:42Z

    The code in pg_stat_get_subscription() appears to believe that it
can return a set of rows, but its pg_proc entry does not have
proretset set.  It may be that this somehow accidentally fails
to malfunction when the function is used via the system views,
but if you try to call it directly it falls over:

regression=# select pg_stat_get_subscription(0);
ERROR:  set-valued function called in context that cannot accept a set

			regards, tom lane

  2. Re: Why isn't pg_stat_get_subscription() marked as proretset?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-08T19:25:22Z

    I wrote:
> The code in pg_stat_get_subscription() appears to believe that it
> can return a set of rows, but its pg_proc entry does not have
> proretset set.  It may be that this somehow accidentally fails
> to malfunction when the function is used via the system views,
> but if you try to call it directly it falls over:
> regression=# select pg_stat_get_subscription(0);
> ERROR:  set-valued function called in context that cannot accept a set

Indeed, the reason we have not noticed this mistake is that
nodeFunctionscan.c and execSRF.c do not complain if a function-in-FROM
returns a tuplestore without having been marked as proretset.
That seems like a bad idea: it only accidentally works at all,
I think, and we might break such cases unknowingly via future code
rearrangement in that area.  There are also bad consequences
elsewhere, such as that the planner mistakenly expects the function
to return just one row, which could result in poor plan choices.

So I think we should not just fix the bogus pg_proc marking, but
also change the executor to complain if a non-SRF tries to return
a set.  I propose the attached.

(I initially had it complain if a non-SRF returns returnMode ==
SFRM_ValuePerCall and isDone == ExprEndResult, but it turns out that
plperl sometimes does that as a way of returning NULL.  I'm not
sufficiently excited about this to go change that, so the patch lets
that case pass.)

			regards, tom lane