public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Postgres hackers <[email protected]>
Subject: pg_stop_backup() v2 incorrectly marked as proretset
Date: Wed, 2 Mar 2022 16:46:01 +0900
Message-ID: <[email protected]> (raw)
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
Attachments:
[text/x-diff] stopbackup-no-srf.patch (4.0K, ../[email protected]/2-stopbackup-no-srf.patch)
download | inline diff:
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 => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
prorettype => 'pg_lsn', proargtypes => '', prosrc => 'pg_stop_backup' },
{ oid => '2739', descr => 'finish taking an online backup',
- proname => 'pg_stop_backup', prorows => '1', proretset => 't',
- provolatile => 'v', proparallel => 'r', prorettype => 'record',
- proargtypes => 'bool bool', proallargtypes => '{bool,bool,pg_lsn,text,text}',
+ proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r',
+ prorettype => 'record', proargtypes => 'bool bool',
+ proallargtypes => '{bool,bool,pg_lsn,text,text}',
proargmodes => '{i,i,o,o,o}',
proargnames => '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}',
prosrc => 'pg_stop_backup_v2' },
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/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 = (ReturnSetInfo *) fcinfo->resultinfo;
TupleDesc tupdesc;
- Tuplestorestate *tupstore;
- MemoryContext per_query_ctx;
- MemoryContext oldcontext;
Datum values[3];
bool nulls[3];
@@ -178,29 +176,15 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
XLogRecPtr stoppoint;
SessionBackupState status = get_backup_status();
- /* check to see if caller supports us returning a tuplestore */
- if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("set-valued function called in context that cannot accept a set")));
- if (!(rsinfo->allowedModes & SFRM_Materialize))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("materialize mode required, but it is not allowed in this context")));
-
- /* Build a tuple descriptor for our result type */
- if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
- elog(ERROR, "return type must be a row type");
-
- per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
- oldcontext = MemoryContextSwitchTo(per_query_ctx);
-
- tupstore = tuplestore_begin_heap(true, false, work_mem);
- rsinfo->returnMode = SFRM_Materialize;
- rsinfo->setResult = tupstore;
- rsinfo->setDesc = tupdesc;
-
- MemoryContextSwitchTo(oldcontext);
+ /* Initialise attributes information in the tuple descriptor */
+ tupdesc = 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);
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] = LSNGetDatum(stoppoint);
- tuplestore_putvalues(tupstore, tupdesc, values, nulls);
-
- return (Datum) 0;
+ /* Returns the record as Datum */
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls)));
}
/*
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_backup_v2'
+ RETURNS record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
PARALLEL RESTRICTED;
CREATE OR REPLACE FUNCTION
[application/pgp-signature] signature.asc (833B, ../[email protected]/3-signature.asc)
download
view thread (2+ messages)
reply
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: [email protected]
Cc: [email protected], [email protected]
Subject: Re: pg_stop_backup() v2 incorrectly marked as proretset
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox