From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: Re: Why isn't pg_stat_get_subscription() marked as proretset?
In-reply-to: <1636062.1615141782@sss.pgh.pa.us>
References: <1636062.1615141782@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sun, 07 Mar 2021 13:29:42 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1839465.1615231362.0@sss.pgh.pa.us>
Date: Mon, 08 Mar 2021 14:25:22 -0500
Message-ID: <1839767.1615231522@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1839767.1615231522%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1839465.1615231362.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="tighten-up-SRF-protocol-checks.patch";
	charset="us-ascii"
Content-ID: <1839465.1615231362.2@sss.pgh.pa.us>
Content-Description: tighten-up-SRF-protocol-checks.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF=
.c
index 8aec3b549b..545b6c19da 100644
--- a/src/backend/executor/execSRF.c
+++ b/src/backend/executor/execSRF.c
@@ -353,11 +353,21 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
 			 */
 			if (rsinfo.isDone !=3D ExprMultipleResult)
 				break;
+
+			/*
+			 * Check that set-returning functions were properly declared.
+			 * (Note: for historical reasons, we don't complain if a non-SRF
+			 * returns ExprEndResult; that's treated as returning NULL.)
+			 */
+			if (!returnsSet)
+				ereport(ERROR,
+						(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
+						 errmsg("table-function protocol for value-per-call mode was not fo=
llowed")));
 		}
 		else if (rsinfo.returnMode =3D=3D SFRM_Materialize)
 		{
 			/* check we're on the same page as the function author */
-			if (!first_time || rsinfo.isDone !=3D ExprSingleResult)
+			if (!first_time || rsinfo.isDone !=3D ExprSingleResult || !returnsSet)
 				ereport(ERROR,
 						(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
 						 errmsg("table-function protocol for materialize mode was not follo=
wed")));
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc=
.dat
index 506689d8ac..a0fe0851c4 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5287,8 +5287,9 @@
   proargnames =3D> '{slot_name,spill_txns,spill_count,spill_bytes,stream_=
txns,stream_count,stream_bytes,stats_reset}',
   prosrc =3D> 'pg_stat_get_replication_slots' },
 { oid =3D> '6118', descr =3D> 'statistics: information about subscription=
',
-  proname =3D> 'pg_stat_get_subscription', proisstrict =3D> 'f', provolat=
ile =3D> 's',
-  proparallel =3D> 'r', prorettype =3D> 'record', proargtypes =3D> 'oid',
+  proname =3D> 'pg_stat_get_subscription', prorows =3D> '10', proisstrict=
 =3D> 'f',
+  proretset =3D> 't', provolatile =3D> 's', proparallel =3D> 'r',
+  prorettype =3D> 'record', proargtypes =3D> 'oid',
   proallargtypes =3D> '{oid,oid,oid,int4,pg_lsn,timestamptz,timestamptz,p=
g_lsn,timestamptz}',
   proargmodes =3D> '{i,o,o,o,o,o,o,o,o}',
   proargnames =3D> '{subid,subid,relid,pid,received_lsn,last_msg_send_tim=
e,last_msg_receipt_time,latest_end_lsn,latest_end_time}',

------- =_aaaaaaaaaa0--