Re: pg_stop_backup() v2 incorrectly marked as proretset
At Fri, 4 Mar 2022 10:09:19 +0900, Michael Paquier wrote in > On Thu, Mar 03, 2022 at 04:40:42PM -0500, Tom Lane wrote: > > The point is to make it clear that the macro isn't intended to affect > > code outside the function. Since C lacks block-scoped macros, > > there's no other way to do that. > > > > I concede that a lot of our code is pretty sloppy about this, but > > that doesn't make it a good practice. > > Well, if we change that, better to do that in all the places where > this would be affected, but I am not sure to see a style appealing > enough on this thread. > > From what I can see, history shows that the style of using a #define > for the number of columns originates from da2c1b8, aka 9.0. Its use > inside a function originates from a755ea3 as of 9.1 and then it has > just spread around without any undefs, so it looks like people like > that way of doing things. I'm one of them. Not unliking #undef, though. It seems to me the name "PG_STOP_BACKUP_V2_COLS" alone is specific enough for the purpose of avoiding misuse. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Thu, Mar 03, 2022 at 04:40:42PM -0500, Tom Lane wrote: > The point is to make it clear that the macro isn't intended to affect > code outside the function. Since C lacks block-scoped macros, > there's no other way to do that. > > I concede that a lot of our code is pretty sloppy about this, but > that doesn't make it a good practice. Well, if we change that, better to do that in all the places where this would be affected, but I am not sure to see a style appealing enough on this thread. From what I can see, history shows that the style of using a #define for the number of columns originates from da2c1b8, aka 9.0. Its use inside a function originates from a755ea3 as of 9.1 and then it has just spread around without any undefs, so it looks like people like that way of doing things. -- Michael signature.asc Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
Chapman Flack writes: > On 03/03/22 16:40, Tom Lane wrote: >> The point is to make it clear that the macro isn't intended to affect >> code outside the function. Since C lacks block-scoped macros, >> there's no other way to do that. > Would the > Datum values[3]; > bool nulls[ lengthof(values) ]; > pattern be more notationally tidy? Hm, I don't care for that particularly. (1) It *looks* asymmetrical, even if it isn't. (2) I think a lot of the benefit of the macro approach is to give a name (and thereby some free documentation, assuming you take some care in choosing the name) to what would otherwise be a very anonymous constant. There's an actual practical problem with the anonymous-constant approach, which is that if you have some other occurrence of "3" in the function, it's very hard to tell if that's indeed an independent value or it's something that should have been replaced by lengthof(values). Now admittedly the same complaint can be made against the macro approach, but at least there, you have some chance of the macro's name providing enough docs to make it clear what the proper uses of it are. (I'd suggest that that other "3" should also have been made a named constant in many cases.) regards, tom lane
Re: pg_stop_backup() v2 incorrectly marked as proretset
On 03/03/22 16:40, Tom Lane wrote: > The point is to make it clear that the macro isn't intended to affect > code outside the function. Since C lacks block-scoped macros, > there's no other way to do that. > > I concede that a lot of our code is pretty sloppy about this, but > that doesn't make it a good practice. Would the Datum values[3]; bool nulls[ lengthof(values) ]; pattern be more notationally tidy? No macro to define or undefine, we already define lengthof() in c.h, and it seems pretty much made for the purpose, if the objective is to have just one 3 to change if it someday becomes not-3. Regards, -Chap
Re: pg_stop_backup() v2 incorrectly marked as proretset
Robert Haas writes: > On Wed, Mar 2, 2022 at 9:35 AM Tom Lane wrote: >> I've not read the whole patch, but this snippet seems fine to me >> if there's also an #undef at the end of the function. >> From later emails, it sounds like that's not the common practice in > similar cases, and I don't personally see the point. The point is to make it clear that the macro isn't intended to affect code outside the function. Since C lacks block-scoped macros, there's no other way to do that. I concede that a lot of our code is pretty sloppy about this, but that doesn't make it a good practice. regards, tom lane
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Wed, Mar 2, 2022 at 9:35 AM Tom Lane wrote: > Yeah, there's plenty of precedent for that coding if you look around. > I've not read the whole patch, but this snippet seems fine to me > if there's also an #undef at the end of the function. >From later emails, it sounds like that's not the common practice in similar cases, and I don't personally see the point. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Wed, Mar 02, 2022 at 12:04:59PM -0500, Chapman Flack wrote: > I had just recently noticed that while reviewing [0], but shrugged, > as I didn't know what the history was. Okay. I did not see you mention it on the thread, but the discussion is long so it is easy to miss some of its details. > Is this best handled as a separate patch, or folded into [0], which is > going to be altering and renaming that function anyway? No idea where this is leading, but I'd rather fix what is at hands now rather than assuming that something may or may not happen. If, as you say, this code gets removed, rebasing this conflict is just a matter of removing the existing code again so that's trivial. -- Michael signature.asc Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Thu, Mar 03, 2022 at 12:36:32AM +0800, Julien Rouhaud wrote: > I don't see strong evidence for that pattern being wildly used with some naive > grepping: Yes, I don't recall either seeing the style with an undef a lot when it came to system functions. I'll move on and apply the fix in a minute using this style. -- Michael signature.asc Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
On 3/2/22 11:04, Chapman Flack wrote: On 03/02/22 02:46, Michael Paquier wrote: system function marked as proretset while it builds and returns only one record. And this is a popular one: pg_stop_backup(), labelled v2. I had just recently noticed that while reviewing [0], but shrugged, as I didn't know what the history was. Is this best handled as a separate patch, or folded into [0], which is going to be altering and renaming that function anyway? On 03/02/22 09:31, Robert Haas wrote: On Wed, Mar 2, 2022 at 5:25 AM Aleksander Alekseev Since it doesn't seem to be used for anything except these two array declarations I suggest keeping simply "3" here. I think we do this kind of thing in various places in similar situations, and I think it is good style. It makes it easier to catch everything if you ever need to update the code. I've been known (in other projects) to sometimes accomplish the same thing with, e.g., Datum values[3]; boolnulls[sizeof values / sizeof *values]; I also use this pattern, though I would generally write it as: bool nulls[sizeof(values) / sizeof(Datum)]; Chap's way makes it possible to use a macro, though, so that's a plus. Regards, -David
Re: pg_stop_backup() v2 incorrectly marked as proretset
On 03/02/22 02:46, Michael Paquier wrote: > system function marked as proretset while it builds and returns only > one record. And this is a popular one: pg_stop_backup(), labelled > v2. I had just recently noticed that while reviewing [0], but shrugged, as I didn't know what the history was. Is this best handled as a separate patch, or folded into [0], which is going to be altering and renaming that function anyway? On 03/02/22 09:31, Robert Haas wrote: > On Wed, Mar 2, 2022 at 5:25 AM Aleksander Alekseev >> Since it doesn't seem to be used for anything except these two array >> declarations I suggest keeping simply "3" here. > > I think we do this kind of thing in various places in similar > situations, and I think it is good style. It makes it easier to catch > everything if you ever need to update the code. I've been known (in other projects) to sometimes accomplish the same thing with, e.g., Datum values[3]; boolnulls[sizeof values / sizeof *values]; Doesn't win any beauty contests, but just one place to change the length if it needs changing. I see we define a lengthof in c.h, so could use: Datum values[3]; boolnulls[lengthof(values)]; Regards, -Chap [0] https://commitfest.postgresql.org/37/3436/
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Wed, Mar 02, 2022 at 05:40:00PM +0300, Aleksander Alekseev wrote: > Hi Tom. > > Yeah, there's plenty of precedent for that coding if you look around. > > I've not read the whole patch, but this snippet seems fine to me > > if there's also an #undef at the end of the function. > > No, there is no #undef. With #undef I don't mind it either. I don't see strong evidence for that pattern being wildly used with some naive grepping: #define for such use without undef: POSTGRES_FDW_GET_CONNECTIONS_COLS HEAP_TUPLE_INFOMASK_COLS CONNECTBY_NCOLS DBLINK_NOTIFY_COLS PG_STAT_STATEMENTS_COLS PG_STAT_STATEMENTS_INFO_COLS HEAPCHECK_RELATION_COLS PG_PARTITION_TREE_COLS PG_STAT_GET_ACTIVITY_COLS PG_STAT_GET_WAL_COLS PG_STAT_GET_SLRU_COLS PG_STAT_GET_REPLICATION_SLOT_COLS PG_STAT_GET_SUBSCRIPTION_STATS_COLS PG_GET_BACKEND_MEMORY_CONTEXTS_COLS PG_GET_SHMEM_SIZES_COLS PG_GET_REPLICATION_SLOTS_COLS READ_REPLICATION_SLOT_COLS PG_STAT_GET_WAL_SENDERS_COLS PG_STAT_GET_SUBSCRIPTION_COLS With an undef: REPLICATION_ORIGIN_PROGRESS_COLS
Re: pg_stop_backup() v2 incorrectly marked as proretset
Hi Tom. Yeah, there's plenty of precedent for that coding if you look around. > I've not read the whole patch, but this snippet seems fine to me > if there's also an #undef at the end of the function. > No, there is no #undef. With #undef I don't mind it either. -- Best regards, Aleksander Alekseev
Re: pg_stop_backup() v2 incorrectly marked as proretset
Robert Haas writes: > On Wed, Mar 2, 2022 at 5:25 AM Aleksander Alekseev > wrote: >> Declaring a macro inside the procedure body is a bit unconventional. >> Since it doesn't seem to be used for anything except these two array >> declarations I suggest keeping simply "3" here. > I think we do this kind of thing in various places in similar > situations, and I think it is good style. It makes it easier to catch > everything if you ever need to update the code. Yeah, there's plenty of precedent for that coding if you look around. I've not read the whole patch, but this snippet seems fine to me if there's also an #undef at the end of the function. regards, tom lane
Re: pg_stop_backup() v2 incorrectly marked as proretset
On Wed, Mar 2, 2022 at 5:25 AM Aleksander Alekseev
wrote:
> ```
> Datum
> pg_stop_backup_v2(PG_FUNCTION_ARGS)
> {
> -ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
> +#define PG_STOP_BACKUP_V2_COLS 3
> TupleDesctupdesc;
> -Tuplestorestate *tupstore;
> -MemoryContext per_query_ctx;
> -MemoryContext oldcontext;
> -Datumvalues[3];
> -boolnulls[3];
> +Datumvalues[PG_STOP_BACKUP_V2_COLS];
> +boolnulls[PG_STOP_BACKUP_V2_COLS];
> ```
>
> Declaring a macro inside the procedure body is a bit unconventional.
> Since it doesn't seem to be used for anything except these two array
> declarations I suggest keeping simply "3" here.
I think we do this kind of thing in various places in similar
situations, and I think it is good style. It makes it easier to catch
everything if you ever need to update the code.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: pg_stop_backup() v2 incorrectly marked as proretset
Hi Michael,
```
Datum
pg_stop_backup_v2(PG_FUNCTION_ARGS)
{
-ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+#define PG_STOP_BACKUP_V2_COLS 3
TupleDesctupdesc;
-Tuplestorestate *tupstore;
-MemoryContext per_query_ctx;
-MemoryContext oldcontext;
-Datumvalues[3];
-boolnulls[3];
+Datumvalues[PG_STOP_BACKUP_V2_COLS];
+boolnulls[PG_STOP_BACKUP_V2_COLS];
```
Declaring a macro inside the procedure body is a bit unconventional.
Since it doesn't seem to be used for anything except these two array
declarations I suggest keeping simply "3" here.
--
Best regards,
Aleksander Alekseev
Re: pg_stop_backup() v2 incorrectly marked as proretset
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 = CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS);
>> TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn",
>> PG_LSNOID, -1, 0);
>
> I think we can use get_call_resuilt_type here.
Yes, I don't mind doing so here.
--
Michael
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..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 = (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];
bool exclusive = PG_GETARG_BOOL(0);
bool waitforarchive = PG_GETARG_BOOL(1);
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 */
+ /* Initialise attributes information in the tuple descriptor */
if (get_call_result_type(fcinfo, NULL, ) != 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);
-
MemSet(values, 0, sizeof(values));
MemSet(nulls, 0, sizeof(nulls));
@@ -251,9 +228,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
signature.asc
Description: PGP signature
Re: pg_stop_backup() v2 incorrectly marked as proretset
At Wed, 2 Mar 2022 16:46:01 +0900, Michael Paquier wrote in > 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? That direction seems find to me. But the patch forgets to remove an useless variable. > /* Initialise attributes information in the tuple descriptor */ > tupdesc = CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS); > TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn", > PG_LSNOID, -1, 0); I think we can use get_call_resuilt_type here. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
