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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Why isn't pg_stat_get_subscription() marked as proretset?
Date: 2021-03-08 19:25:22
Message-ID: 1839767.1615231522@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
tighten-up-SRF-protocol-checks.patch text/x-diff 2.1 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Joel Jacobson 2021-03-08 19:41:03 Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[]
Previous Message Robert Haas 2021-03-08 18:57:01 Re: New IndexAM API controlling index vacuum strategies