Horton

Thread

Commits

  1. Fix catalog data of pg_stop_backup(), labelled v2

  1. pg_stop_backup() v2 incorrectly marked as proretset

    Michael Paquier <michael@paquier.xyz> — 2022-03-02T07:46:01Z

    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

  2. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-02T08:22:35Z

    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

  3. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Michael Paquier <michael@paquier.xyz> — 2022-03-02T10:04:17Z

    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

  4. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-02T10:25:17Z

    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

  5. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Robert Haas <robertmhaas@gmail.com> — 2022-03-02T14:31:49Z

    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

  6. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-02T14:35:44Z

    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

  7. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Aleksander Alekseev <aleksander@timescale.com> — 2022-03-02T14:40:00Z

    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

  8. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Julien Rouhaud <rjuju123@gmail.com> — 2022-03-02T16:36:32Z

    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

  9. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Chapman Flack <chap@anastigmatix.net> — 2022-03-02T17:04:59Z

    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/

  10. Re: pg_stop_backup() v2 incorrectly marked as proretset

    David Steele <david@pgmasters.net> — 2022-03-02T17:24:24Z

    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

  11. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Michael Paquier <michael@paquier.xyz> — 2022-03-03T01:10:36Z

    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

  12. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Michael Paquier <michael@paquier.xyz> — 2022-03-03T01:17:10Z

    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

  13. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Robert Haas <robertmhaas@gmail.com> — 2022-03-03T21:09:39Z

    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

  14. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-03T21:40:42Z

    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

  15. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Chapman Flack <chap@anastigmatix.net> — 2022-03-03T22:00:23Z

    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

  16. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-03T22:17:56Z

    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

  17. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Michael Paquier <michael@paquier.xyz> — 2022-03-04T01:09:19Z

    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

  18. Re: pg_stop_backup() v2 incorrectly marked as proretset

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-03-04T01:32:27Z

    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