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?
Date: Mon, 08 Mar 2021 14:25:22 -0500
Message-ID: <1839767.1615231522@sss.pgh.pa.us> (raw)
In-Reply-To: <1636062.1615141782@sss.pgh.pa.us>
References: <1636062.1615141782@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
Attachments:tighten-up-SRF-protocol-checks.patch (text/x-diff, 2.1K) downloaddiff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF.cindex 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 != 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 followed")));
}
else if (rsinfo.returnMode == SFRM_Materialize)
{
/* check we're on the same page as the function author */
- if (!first_time || rsinfo.isDone != ExprSingleResult)+ if (!first_time || rsinfo.isDone != ExprSingleResult || !returnsSet)
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
errmsg("table-function protocol for materialize mode was not followed")));
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.datindex 506689d8ac..a0fe0851c4 100644--- a/src/include/catalog/pg_proc.dat+++ b/src/include/catalog/pg_proc.dat@@ -5287,8 +5287,9 @@
proargnames => '{slot_name,spill_txns,spill_count,spill_bytes,stream_txns,stream_count,stream_bytes,stats_reset}',
prosrc => 'pg_stat_get_replication_slots' },
{ oid => '6118', descr => 'statistics: information about subscription',
- proname => 'pg_stat_get_subscription', proisstrict => 'f', provolatile => 's',- proparallel => 'r', prorettype => 'record', proargtypes => 'oid',+ proname => 'pg_stat_get_subscription', prorows => '10', proisstrict => 'f',+ proretset => 't', provolatile => 's', proparallel => 'r',+ prorettype => 'record', proargtypes => 'oid',
proallargtypes => '{oid,oid,oid,int4,pg_lsn,timestamptz,timestamptz,pg_lsn,timestamptz}',
proargmodes => '{i,o,o,o,o,o,o,o,o}',
proargnames => '{subid,subid,relid,pid,received_lsn,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time}',
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Why isn't pg_stat_get_subscription() marked as proretset?
In-Reply-To: <1839767.1615231522@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox