Horton

Thread

Commits

  1. Add proargnames to multi-argument aggregate functions

  1. Adding argument names to aggregate functions

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-02-27T13:22:53Z

    Hi hackers,

I'm sure I'm not the only one who can never remember which way around
the value and delimiter arguments go for string_agg() and has to look it
up in the manual every time. To make it more convenient, here's a patch
that adds proargnames to its pg_proc entries so that it can be seen with
a quick \df in psql.

I also added names to json(b)_object_agg() for good measure, even though
they're more obvious. The remaining built-in multi-argument aggregate
functions are the stats-related ones, where it's all just Y/X (but why
in that order?), so I didn't think it was necessary. If others feel more
strongly, I can add those too.

- ilmari

  2. Re: Adding argument names to aggregate functions

    Vik Fearing <vik@postgresfriends.org> — 2023-02-27T23:38:21Z

    On 2/27/23 14:22, Dagfinn Ilmari Mannsåker wrote:
> Hi hackers,
> 
> I'm sure I'm not the only one who can never remember which way around
> the value and delimiter arguments go for string_agg() and has to look it
> up in the manual every time. To make it more convenient, here's a patch
> that adds proargnames to its pg_proc entries so that it can be seen with
> a quick \df in psql.
> 
> I also added names to json(b)_object_agg() for good measure, even though
> they're more obvious. The remaining built-in multi-argument aggregate
> functions are the stats-related ones, where it's all just Y/X (but why
> in that order?), so I didn't think it was necessary. If others feel more
> strongly, I can add those too.

No comment on adding names for everything, but a big +1 for the ones 
included here.
-- 
Vik Fearing

  3. Re: Adding argument names to aggregate functions

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-04-12T17:53:54Z

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Hi hackers,
>
> I'm sure I'm not the only one who can never remember which way around
> the value and delimiter arguments go for string_agg() and has to look it
> up in the manual every time. To make it more convenient, here's a patch
> that adds proargnames to its pg_proc entries so that it can be seen with
> a quick \df in psql.

Added to the 2023-07 commitfest:

https://commitfest.postgresql.org/43/4275/

- ilmari

  4. Re: Adding argument names to aggregate functions

    Jim Jones <jim.jones@uni-muenster.de> — 2023-04-14T09:12:24Z

    On 12.04.23 19:53, Dagfinn Ilmari Mannsåker wrote:
> Dagfinn Ilmari Mannsåker<ilmari@ilmari.org>  writes:
>
>> Hi hackers,
>>
>> I'm sure I'm not the only one who can never remember which way around
>> the value and delimiter arguments go for string_agg() and has to look it
>> up in the manual every time. To make it more convenient, here's a patch
>> that adds proargnames to its pg_proc entries so that it can be seen with
>> a quick \df in psql.
> Added to the 2023-07 commitfest:
>
> https://commitfest.postgresql.org/43/4275/
>
> - ilmari

+1 for adding the argument names.

The patch needs a rebase though.. it no longer applies :

$ git apply 
~/Downloads/0001-Add-argument-names-to-multi-argument-aggregates.patch
error: patch failed: src/include/catalog/pg_proc.dat:8899
error: src/include/catalog/pg_proc.dat: patch does not apply

Jim

  5. Re: Adding argument names to aggregate functions

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-04-14T10:03:03Z

    Jim Jones <jim.jones@uni-muenster.de> writes:

> On 12.04.23 19:53, Dagfinn Ilmari Mannsåker wrote:
>> Dagfinn Ilmari Mannsåker<ilmari@ilmari.org>  writes:
>>
>>> Hi hackers,
>>>
>>> I'm sure I'm not the only one who can never remember which way around
>>> the value and delimiter arguments go for string_agg() and has to look it
>>> up in the manual every time. To make it more convenient, here's a patch
>>> that adds proargnames to its pg_proc entries so that it can be seen with
>>> a quick \df in psql.
>> Added to the 2023-07 commitfest:
>>
>> https://commitfest.postgresql.org/43/4275/
>>
>> - ilmari
>
> +1 for adding the argument names.
>
> The patch needs a rebase though.. it no longer applies :
>
> $ git apply
> ~/Downloads/0001-Add-argument-names-to-multi-argument-aggregates.patch
> error: patch failed: src/include/catalog/pg_proc.dat:8899
> error: src/include/catalog/pg_proc.dat: patch does not apply

Thanks for the heads-up, here's a rebased patch.  I've also formatted
the lines to match what reformat_dat_file.pl wants.  It also wanted to
reformat a bunch of other entries, but I left those alone.

- ilmari

  6. Re: Adding argument names to aggregate functions

    Jim Jones <jim.jones@uni-muenster.de> — 2023-04-18T08:58:11Z

    On 14.04.23 12:03, Dagfinn Ilmari Mannsåker wrote:
> Thanks for the heads-up, here's a rebased patch. I've also formatted
> the lines to match what reformat_dat_file.pl wants.  It also wanted to
> reformat a bunch of other entries, but I left those alone.
>
> - ilmari

The patch applies cleanly now and \df shows the argument names:

postgres=# \df string_agg
                                 List of functions
    Schema   |    Name    | Result data type |     Argument data 
types      | Type
------------+------------+------------------+------------------------------+------
  pg_catalog | string_agg | bytea            | value bytea, delimiter 
bytea | agg
  pg_catalog | string_agg | text             | value text, delimiter 
text   | agg
(2 rows)

postgres=# \df json_object_agg
                                 List of functions
    Schema   |      Name       | Result data type |  Argument data 
types   | Type
------------+-----------------+------------------+------------------------+------
  pg_catalog | json_object_agg | json             | key "any", value 
"any" | agg
(1 row)


I'm wondering if there are some sort of guidelines that dictate when to 
name an argument or not. It would be nice to have one for future reference.

I will mark the CF entry as "Read for Committer" and let the committers 
decide if it's best to first create a guideline for that or not.

Best, Jim

  7. Re: Adding argument names to aggregate functions

    Jim Jones <jim.jones@uni-muenster.de> — 2023-04-18T09:16:46Z

    On 18.04.23 10:58, I wrote:
> On 14.04.23 12:03, Dagfinn Ilmari Mannsåker wrote:
>> Thanks for the heads-up, here's a rebased patch. I've also formatted
>> the lines to match what reformat_dat_file.pl wants.  It also wanted to
>> reformat a bunch of other entries, but I left those alone.
>>
>> - ilmari
>
> The patch applies cleanly now and \df shows the argument names:
>
> postgres=# \df string_agg
>                                 List of functions
>    Schema   |    Name    | Result data type |     Argument data 
> types      | Type
> ------------+------------+------------------+------------------------------+------ 
>
>  pg_catalog | string_agg | bytea            | value bytea, delimiter 
> bytea | agg
>  pg_catalog | string_agg | text             | value text, delimiter 
> text   | agg
> (2 rows)
>
> postgres=# \df json_object_agg
>                                 List of functions
>    Schema   |      Name       | Result data type |  Argument data 
> types   | Type
> ------------+-----------------+------------------+------------------------+------ 
>
>  pg_catalog | json_object_agg | json             | key "any", value 
> "any" | agg
> (1 row)
>
>
> I'm wondering if there are some sort of guidelines that dictate when 
> to name an argument or not. It would be nice to have one for future 
> reference.
>
> I will mark the CF entry as "Read for Committer" and let the 
> committers decide if it's best to first create a guideline for that or 
> not.
>
> Best, Jim
>
I just saw that the patch is failing[1] on "macOS - Ventura - Meson". 
Not sure if it is related to this patch though ..

[1] 
https://api.cirrus-ci.com/v1/artifact/task/5881376021413888/meson_log/build/meson-logs/meson-log.txt

  8. Re: Adding argument names to aggregate functions

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-04-18T10:27:54Z

    Jim Jones <jim.jones@uni-muenster.de> writes:

> On 18.04.23 10:58, I wrote:
>> On 14.04.23 12:03, Dagfinn Ilmari Mannsåker wrote:
>>> Thanks for the heads-up, here's a rebased patch. I've also formatted
>>> the lines to match what reformat_dat_file.pl wants.  It also wanted to
>>> reformat a bunch of other entries, but I left those alone.
>>>
>>> - ilmari
>>
>> The patch applies cleanly now and \df shows the argument names:
>>
>> postgres=# \df string_agg
>>                                 List of functions
>>    Schema   |    Name    | Result data type |     Argument data
>> types      | Type
>> ------------+------------+------------------+------------------------------+------ 
>>  pg_catalog | string_agg | bytea            | value bytea, delimiter bytea | agg
>>  pg_catalog | string_agg | text             | value text, delimiter text   | agg
>> (2 rows)
>>
>> postgres=# \df json_object_agg
>>                                 List of functions
>>    Schema   |      Name       | Result data type |  Argument data
>> types   | Type
>> ------------+-----------------+------------------+------------------------+------ 
>>  pg_catalog | json_object_agg | json             | key "any", value "any" | agg
>> (1 row)
>>
>>
>> I'm wondering if there are some sort of guidelines that dictate when
>> to name an argument or not. It would be nice to have one for future 
>> reference.

I seemed to recall a patch to add arugment names to a bunch of functions
in the past, thinking that might have some guidance, but can't for the
life of me find it now.

>> I will mark the CF entry as "Read for Committer" and let the
>> committers decide if it's best to first create a guideline for that or 
>> not.
>>
>> Best, Jim
>>
> I just saw that the patch is failing[1] on "macOS - Ventura -
> Meson". Not sure if it is related to this patch though ..
>
> [1]
> https://api.cirrus-ci.com/v1/artifact/task/5881376021413888/meson_log/build/meson-logs/meson-log.txt

Link to the actual job:

https://cirrus-ci.com/task/5881376021413888

The failure was:

[09:54:38.727] 216/262 postgresql:recovery / recovery/031_recovery_conflict ERROR 198.73s exit status 60

Looking at its log:

https://api.cirrus-ci.com/v1/artifact/task/5881376021413888/testrun/build/testrun/recovery/031_recovery_conflict/log/regress_log_031_recovery_conflict

we see:

timed out waiting for match: (?^:User was holding a relation lock for too long) at /Users/admin/pgsql/src/test/recovery/t/031_recovery_conflict.pl line 311.

That looks indeed completely unrelated to this patch.

- ilmari

  9. Re: Adding argument names to aggregate functions

    Jim Jones <jim.jones@uni-muenster.de> — 2023-04-18T10:39:33Z

    On 18.04.23 12:27, Dagfinn Ilmari Mannsåker wrote:
> Link to the actual job:
> https://cirrus-ci.com/task/5881376021413888
>
> The failure was:
>
> [09:54:38.727] 216/262 postgresql:recovery / recovery/031_recovery_conflict ERROR 198.73s exit status 60
>
> Looking at its log:
>
> https://api.cirrus-ci.com/v1/artifact/task/5881376021413888/testrun/build/testrun/recovery/031_recovery_conflict/log/regress_log_031_recovery_conflict
>
> we see:
>
> timed out waiting for match: (?^:User was holding a relation lock for too long) at /Users/admin/pgsql/src/test/recovery/t/031_recovery_conflict.pl line 311.
>
> That looks indeed completely unrelated to this patch.

Yes, that's what I suspected. The patch passes all tests now :)

I've marked the CF entry as "Ready for Committer".

Jim

  10. Re: Adding argument names to aggregate functions

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-19T07:56:29Z

    This patch no longer applied but had a fairly trivial conflict so I've attached
a rebased v3 addressing the conflict in the hopes of getting this further.

--
Daniel Gustafsson

  11. Re: Adding argument names to aggregate functions

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-07-19T17:32:16Z

    Daniel Gustafsson <daniel@yesql.se> writes:

> This patch no longer applied but had a fairly trivial conflict so I've attached
> a rebased v3 addressing the conflict in the hopes of getting this further.

Thanks for the heads-up!  Turns out the conflict was due to the new
json(b)_object_agg(_unique)(_strict) functions, which should also have
proargnames added.  Here's an updated patch that does that.

- ilmari

  12. Re: Adding argument names to aggregate functions

    Daniel Gustafsson <daniel@yesql.se> — 2023-07-19T19:38:12Z

    > On 19 Jul 2023, at 19:32, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
> 
>> This patch no longer applied but had a fairly trivial conflict so I've attached
>> a rebased v3 addressing the conflict in the hopes of getting this further.
> 
> Thanks for the heads-up!  Turns out the conflict was due to the new
> json(b)_object_agg(_unique)(_strict) functions, which should also have
> proargnames added.  Here's an updated patch that does that.

Great, thanks!  I had a quick look at this while rebasing (as well as your
updated patch) and it seems like a good idea to add this.  Unless there are
objections I will look at getting this in.

--
Daniel Gustafsson

  13. Re: Adding argument names to aggregate functions

    Nathan Bossart <nathandbossart@gmail.com> — 2023-08-03T23:36:01Z

    On Wed, Jul 19, 2023 at 09:38:12PM +0200, Daniel Gustafsson wrote:
> Great, thanks!  I had a quick look at this while rebasing (as well as your
> updated patch) and it seems like a good idea to add this.  Unless there are
> objections I will look at getting this in.

Hey Daniel, are you still planning on committing this?  I can pick it up if
you are busy.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  14. Re: Adding argument names to aggregate functions

    Daniel Gustafsson <daniel@yesql.se> — 2023-08-24T10:05:17Z

    > On 4 Aug 2023, at 01:36, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
> On Wed, Jul 19, 2023 at 09:38:12PM +0200, Daniel Gustafsson wrote:
>> Great, thanks!  I had a quick look at this while rebasing (as well as your
>> updated patch) and it seems like a good idea to add this.  Unless there are
>> objections I will look at getting this in.
> 
> Hey Daniel, are you still planning on committing this?  I can pick it up if
> you are busy.

Finally unburied this from the post-vacation pile on the TODO list and pushed
it after another once-over.

--
Daniel Gustafsson

  15. Re: Adding argument names to aggregate functions

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-08-24T12:38:59Z

    On Thu, 24 Aug 2023, at 11:05, Daniel Gustafsson wrote:
>> On 4 Aug 2023, at 01:36, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> 
>> On Wed, Jul 19, 2023 at 09:38:12PM +0200, Daniel Gustafsson wrote:
>>> Great, thanks!  I had a quick look at this while rebasing (as well as your
>>> updated patch) and it seems like a good idea to add this.  Unless there are
>>> objections I will look at getting this in.
>> 
>> Hey Daniel, are you still planning on committing this?  I can pick it up if
>> you are busy.
>
> Finally unburied this from the post-vacation pile on the TODO list and pushed
> it after another once-over.

Thanks!

-- 
- ilmari