Thread: pg_stop_backup() v2 incorrectly marked as proretset
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
Attachment
At Wed, 2 Mar 2022 16:46:01 +0900, Michael Paquier <michael@paquier.xyz> 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
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
Attachment
Hi Michael,
```
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];
```
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
On Wed, Mar 2, 2022 at 5:25 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> ```
> 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];
> ```
>
> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 2, 2022 at 5:25 AM Aleksander Alekseev
> <aleksander@timescale.com> 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
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
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
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]; bool nulls[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]; bool nulls[lengthof(values)]; Regards, -Chap [0] https://commitfest.postgresql.org/37/3436/
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]; > bool nulls[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
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
Attachment
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
Attachment
On Wed, Mar 2, 2022 at 9:35 AM Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 2, 2022 at 9:35 AM Tom Lane <tgl@sss.pgh.pa.us> 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
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
Chapman Flack <chap@anastigmatix.net> 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
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
Attachment
At Fri, 4 Mar 2022 10:09:19 +0900, Michael Paquier <michael@paquier.xyz> 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