Date: Wed, 2 Mar 2022 19:04:17 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pg_stop_backup() v2 incorrectly marked as proretset
Message-ID: <Yh9BIWEcWVZJUQm1@paquier.xyz>
References: <Yh8guT78f1Ercfzw@paquier.xyz>
 <20220302.172235.1181597714315490061.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="BtdqSWtjo5FHClBT"
Content-Disposition: inline
In-Reply-To: <20220302.172235.1181597714315490061.horikyota.ntt@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Yh9BIWEcWVZJUQm1%40paquier.xyz>
Precedence: bulk


--BtdqSWtjo5FHClBT
Content-Type: multipart/mixed; boundary="RUlzhJ/egrKnO5qp"
Content-Disposition: inline


--RUlzhJ/egrKnO5qp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 02, 2022 at 05:22:35PM +0900, Kyotaro Horiguchi wrote:
> But the patch forgets to remove an useless variable.

Indeed.  I forgot to look at stderr.

>>	/* Initialise attributes information in the tuple descriptor */
>>	tupdesc =3D CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS);
>>	TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn",
>>					   PG_LSNOID, -1, 0);
>=20
> I think we can use get_call_resuilt_type here.

Yes, I don't mind doing so here.
--
Michael

--RUlzhJ/egrKnO5qp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="stopbackup-no-srf-2.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..9f7a282ed2 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -165,43 +165,20 @@ pg_stop_backup(PG_FUNCTION_ARGS)
 Datum
 pg_stop_backup_v2(PG_FUNCTION_ARGS)
 {
-	ReturnSetInfo *rsinfo =3D (ReturnSetInfo *) fcinfo->resultinfo;
+#define PG_STOP_BACKUP_V2_COLS 3
 	TupleDesc	tupdesc;
-	Tuplestorestate *tupstore;
-	MemoryContext per_query_ctx;
-	MemoryContext oldcontext;
-	Datum		values[3];
-	bool		nulls[3];
+	Datum		values[PG_STOP_BACKUP_V2_COLS];
+	bool		nulls[PG_STOP_BACKUP_V2_COLS];
=20
 	bool		exclusive =3D PG_GETARG_BOOL(0);
 	bool		waitforarchive =3D PG_GETARG_BOOL(1);
 	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 */
+	/* Initialise attributes information in the tuple descriptor */
 	if (get_call_result_type(fcinfo, NULL, &tupdesc) !=3D TYPEFUNC_COMPOSITE)
 		elog(ERROR, "return type must be a row type");
=20
-	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);
-
 	MemSet(values, 0, sizeof(values));
 	MemSet(nulls, 0, sizeof(nulls));
=20
@@ -251,9 +228,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

--RUlzhJ/egrKnO5qp--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmIfQSEACgkQnvQgOdby
QH0Q2Q/8D50u/tWCullg8kAfU/alF98h79eStRhLaHsjnn1p1RE2pgLHWYwXR1fo
kCfgcTh3nsA8kJXp3CVXjR0KrmYUPc6NKK50uhasAph39lQeGkn2P7zhlexDR70n
A4zkxRbGfEsZyArxuaanPazIcGYz/T8TD5jxxI5tcMAZGsA6fVDMGAw7rBzXJXEe
3r4ZjuehZ8AWKgokbOrNeMCysGjbORlcQj7TpnlPbMlHcuJQ7FKCZe2gC6AG9WGZ
a1EixLpoxRM53FOSH4Mmq2R8gj7SUtWCSHXReqNWSU19xLGJjWBCXQfVpYS2KdM6
1ftWNRhCnPUXLWGYkCVWB8Ir5FcBJSckW72jKakNn4GS/Pmy1EsuaOZIotOK7ZBj
x2dLNTPyPqUaSOTeb6YcDH9sPDfiWpBezDv2bff36UKIf/uNM3SDc5oxn7mKV0JB
2rcZCnNHMEVSiZRfEUZmoa1WvZL/vOyd1rJYxxY1WIYspp6ie3cQZ+QWJ28cVQ4h
v4m6AwIoB2KFibQj9BwTjkP6rbrONLY1SPmrwzxyiv3RGx9t5oXzQ3hjTjU4xapW
zex9tGLrHywhfct0DugIX5qY21YD4scBVyuRLa1r/kDv18SAHvTRiUKwmBc/HdQM
E/tOJbnmDfF/8Be7FFyvhz85Wgi0AQPvLWYUHZ4dMJEv/CXnDQw=
=KvIc
-----END PGP SIGNATURE-----

--BtdqSWtjo5FHClBT--