Date: Wed, 2 Mar 2022 16:46:01 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: pg_stop_backup() v2 incorrectly marked as proretset
Message-ID: <Yh8guT78f1Ercfzw@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2DB4DILVuGasknAX"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/Yh8guT78f1Ercfzw%40paquier.xyz>
Precedence: bulk


--2DB4DILVuGasknAX
Content-Type: multipart/mixed; boundary="UeDy/9K1fvtFzBXq"
Content-Disposition: inline


--UeDy/9K1fvtFzBXq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi all,

In my hunt looking for incorrect SRFs, I have noticed a new case of a
system function marked as proretset while it builds and returns only
one record.  And this is a popular one: pg_stop_backup(), labelled
v2.

This leads to a lot of unnecessary work, as the function creates a
tuplestore it has no need for with the usual set of checks related to
SRFs.  The logic can be be simplified as of the attached.

Thoughts?
--
Michael

--UeDy/9K1fvtFzBXq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="stopbackup-no-srf.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.=
dat
index bf88858171..d8e8715ed1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6275,9 +6275,9 @@
   proname =3D> 'pg_stop_backup', provolatile =3D> 'v', proparallel =3D> 'r=
',
   prorettype =3D> 'pg_lsn', proargtypes =3D> '', prosrc =3D> 'pg_stop_back=
up' },
 { oid =3D> '2739', descr =3D> 'finish taking an online backup',
-  proname =3D> 'pg_stop_backup', prorows =3D> '1', proretset =3D> 't',
-  provolatile =3D> 'v', proparallel =3D> 'r', prorettype =3D> 'record',
-  proargtypes =3D> 'bool bool', proallargtypes =3D> '{bool,bool,pg_lsn,tex=
t,text}',
+  proname =3D> 'pg_stop_backup', provolatile =3D> 'v', proparallel =3D> 'r=
',
+  prorettype =3D> 'record', proargtypes =3D> 'bool bool',
+  proallargtypes =3D> '{bool,bool,pg_lsn,text,text}',
   proargmodes =3D> '{i,i,o,o,o}',
   proargnames =3D> '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}',
   prosrc =3D> 'pg_stop_backup_v2' },
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/tr=
ansam/xlogfuncs.c
index 12e2bf4135..2f292e2bd8 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -165,11 +165,9 @@ pg_stop_backup(PG_FUNCTION_ARGS)
 Datum
 pg_stop_backup_v2(PG_FUNCTION_ARGS)
 {
+#define PG_STOP_BACKUP_V2_COLS 3
 	ReturnSetInfo *rsinfo =3D (ReturnSetInfo *) fcinfo->resultinfo;
 	TupleDesc	tupdesc;
-	Tuplestorestate *tupstore;
-	MemoryContext per_query_ctx;
-	MemoryContext oldcontext;
 	Datum		values[3];
 	bool		nulls[3];
=20
@@ -178,29 +176,15 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 	XLogRecPtr	stoppoint;
 	SessionBackupState status =3D get_backup_status();
=20
-	/* check to see if caller supports us returning a tuplestore */
-	if (rsinfo =3D=3D NULL || !IsA(rsinfo, ReturnSetInfo))
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("set-valued function called in context that cannot accept a se=
t")));
-	if (!(rsinfo->allowedModes & SFRM_Materialize))
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("materialize mode required, but it is not allowed in this cont=
ext")));
-
-	/* Build a tuple descriptor for our result type */
-	if (get_call_result_type(fcinfo, NULL, &tupdesc) !=3D TYPEFUNC_COMPOSITE)
-		elog(ERROR, "return type must be a row type");
-
-	per_query_ctx =3D rsinfo->econtext->ecxt_per_query_memory;
-	oldcontext =3D MemoryContextSwitchTo(per_query_ctx);
-
-	tupstore =3D tuplestore_begin_heap(true, false, work_mem);
-	rsinfo->returnMode =3D SFRM_Materialize;
-	rsinfo->setResult =3D tupstore;
-	rsinfo->setDesc =3D tupdesc;
-
-	MemoryContextSwitchTo(oldcontext);
+	/* Initialise attributes information in the tuple descriptor */
+	tupdesc =3D CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn",
+					   PG_LSNOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 2, "labelfile",
+					   TEXTOID, -1, 0);
+	TupleDescInitEntry(tupdesc, (AttrNumber) 3, "spcmapfile",
+					   TEXTOID, -1, 0);
+	BlessTupleDesc(tupdesc);
=20
 	MemSet(values, 0, sizeof(values));
 	MemSet(nulls, 0, sizeof(nulls));
@@ -251,9 +235,8 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 	/* Stoppoint is included on both exclusive and nonexclusive backups */
 	values[0] =3D LSNGetDatum(stoppoint);
=20
-	tuplestore_putvalues(tupstore, tupdesc, values, nulls);
-
-	return (Datum) 0;
+	/* Returns the record as Datum */
+	PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)=
));
 }
=20
 /*
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog=
/system_functions.sql
index 758ab6e25a..81bac6f581 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -384,7 +384,7 @@ CREATE OR REPLACE FUNCTION
 CREATE OR REPLACE FUNCTION pg_stop_backup (
         exclusive boolean, wait_for_archive boolean DEFAULT true,
         OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
-  RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backu=
p_v2'
+  RETURNS record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
   PARALLEL RESTRICTED;
=20
 CREATE OR REPLACE FUNCTION

--UeDy/9K1fvtFzBXq--

--2DB4DILVuGasknAX
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmIfILkACgkQnvQgOdby
QH1wFA/9H9RR5qa0M74iuY15uvX+7okdZl3wuD+J0vxaiJ3IP1pY/i1qofK81Jfq
NuX3JzsfQQ4ZyhXq+WRS4M29rgHnQNfZAbZBSTo4fvD7q6fqLHFSV9QnfEgvB6/l
m65UvFFdAJRLTTPTPcFeMnCGH+e+x5EuW8sr6eI5oDTmGt24oOLFEtZUQUamRP4m
Jpa390cbNCMrkldMWLCPwh/a2v96RXQJJBcqSRV9uabRuDQfYjQjBhI4xd6EcbNQ
uz/C78V+nGii9nc2oW2j4k4yUUxqSb1Euxo9hVQ3KPt7xguj8YKqljY972hJ8La2
kE6TXkGSCe8we0oertvKD3AbyQHwQC0QWy05nWw5L+tQZ0uKAS4CmB/MPdTSTTvP
hrNHZG2xp0sL5ed5UwtYjuHHDOyrIZhJ/tztuAZSxMHuzAc5blvf7j2xEASZtu7Q
zrAUu+RQI+u2x6G4LBp+ib2VYTylc4rLUwD05sknF/pZhbABbO7u0gS8pUtusKpj
lS0l9iBmliUd4lnjr9FYDtNkKkQ3I/hboLCUDTBm03kTtzdMLaspKOyJj/bDXAoA
OS2YdumKuR/ftNVk/Fdb6w6VNkczlhyAGJkrjFD0+EGtL4LtzI6QJzjFTqD/otIR
d04QESF4Qx0ez+UYiZ2Gb+O0GlvXMXUAQq5n7JCPYY0GwRkHI+8=
=SfvS
-----END PGP SIGNATURE-----

--2DB4DILVuGasknAX--