Horton

Thread

Commits

  1. Add min and max aggregates for bytea type.

  1. [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-03T13:03:38Z

    Hello. BYTEA type has the ability to use comparison operations. But it
is absent of min/max aggregate functions. They are nice to have to
provide consistency with the TEXT type.


--

With best regards,
Marat Bukharov

  2. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-03T14:17:39Z

    V2 patch with fixed tests

--

With best regards,
Marat Bukharov

ср, 3 июл. 2024 г. в 16:03, Marat Buharov <marat.buharov@gmail.com>:
>
> Hello. BYTEA type has the ability to use comparison operations. But it
> is absent of min/max aggregate functions. They are nice to have to
> provide consistency with the TEXT type.
>
>
> --
>
> With best regards,
> Marat Bukharov

  3. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-03T14:54:24Z

    V3 patch with fixed length comparison

--

With best regards,
Marat Bukharov

>
> V2 patch with fixed tests
>
> >
> > Hello. BYTEA type has the ability to use comparison operations. But it
> > is absent of min/max aggregate functions. They are nice to have to
> > provide consistency with the TEXT type.
> >

  4. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-03T14:55:54Z

    V3 patch with fixed length comparison


--

With best regards,
Marat Bukharov

>
> V2 patch with fixed tests
>
> >
> > Hello. BYTEA type has the ability to use comparison operations. But it
> > is absent of min/max aggregate functions. They are nice to have to
> > provide consistency with the TEXT type.
> >

  5. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-03T16:04:40Z

    V4 path with fixed usage PG_GETARG_BYTEA_PP instead of PG_GETARG_TEXT_PP

--

With best regards,
Marat Bukharov

>
> V3 patch with fixed length comparison
>
> >
> > V2 patch with fixed tests
> >
> > >
> > > Hello. BYTEA type has the ability to use comparison operations. But it
> > > is absent of min/max aggregate functions. They are nice to have to
> > > provide consistency with the TEXT type.
> > >

  6. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Aleksander Alekseev <aleksander@timescale.com> — 2024-07-04T12:29:09Z

    Hi Marat,

> V4 path with fixed usage PG_GETARG_BYTEA_PP instead of PG_GETARG_TEXT_PP

Thanks for the patch.

Please add it to the nearest open commitfest [1].

```
+select min(v) from bytea_test_table;
+ min
+------
+ \xaa
+(1 row)
+
+select max(v) from bytea_test_table;
+ max
+------
+ \xff
+(1 row)
```

If I understand correctly, all the v's are of the same size. If this
is the case you should add more test cases.

[1]: https://commitfest.postgresql.org/

-- 
Best regards,
Aleksander Alekseev

  7. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-05T19:43:56Z

    What part of commitfest should I put the current patch to: "SQL
Commands", "Miscellaneous" or something else? I can't figure it out.

--

With best regards,
Marat Bukharov

> Hi Marat,
>
> > V4 path with fixed usage PG_GETARG_BYTEA_PP instead of PG_GETARG_TEXT_PP
>
> Thanks for the patch.
>
> Please add it to the nearest open commitfest [1].
>
> ```
> +select min(v) from bytea_test_table;
> + min
> +------
> + \xaa
> +(1 row)
> +
> +select max(v) from bytea_test_table;
> + max
> +------
> + \xff
> +(1 row)
> ```
>
> If I understand correctly, all the v's are of the same size. If this
> is the case you should add more test cases.
>
> [1]: https://commitfest.postgresql.org/
>
> --
> Best regards,
> Aleksander Alekseev

  8. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Aleksander Alekseev <aleksander@timescale.com> — 2024-07-08T09:06:29Z

    Hi,

> What part of commitfest should I put the current patch to: "SQL
> Commands", "Miscellaneous" or something else? I can't figure it out.

Personally I qualified a similar patch [1] as "Server Features",
although I'm not 100% sure if this was the best choice.

[1]: https://commitfest.postgresql.org/48/4905/

-- 
Best regards,
Aleksander Alekseev

  9. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-24T14:42:11Z

    V5 patch. I've added more tests with different bytea sizes

--

With best regards,
Marat Bukharov

чт, 4 июл. 2024 г. в 15:29, Aleksander Alekseev <aleksander@timescale.com>:
>
> Hi Marat,
>
> > V4 path with fixed usage PG_GETARG_BYTEA_PP instead of PG_GETARG_TEXT_PP
>
> Thanks for the patch.
>
> Please add it to the nearest open commitfest [1].
>
> ```
> +select min(v) from bytea_test_table;
> + min
> +------
> + \xaa
> +(1 row)
> +
> +select max(v) from bytea_test_table;
> + max
> +------
> + \xff
> +(1 row)
> ```
>
> If I understand correctly, all the v's are of the same size. If this
> is the case you should add more test cases.
>
> [1]: https://commitfest.postgresql.org/
>
> --
> Best regards,
> Aleksander Alekseev

  10. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-07-24T14:47:01Z

    Added patch to commitfest https://commitfest.postgresql.org/49/5138/

--

With best regards,
Marat Bukharov

>
> Hi,
>
> > What part of commitfest should I put the current patch to: "SQL
> > Commands", "Miscellaneous" or something else? I can't figure it out.
>
> Personally I qualified a similar patch [1] as "Server Features",
> although I'm not 100% sure if this was the best choice.
>
> [1]: https://commitfest.postgresql.org/48/4905/
>
> --
> Best regards,
> Aleksander Alekseev

  11. Re: [PATCH] Add min/max aggregate functions to BYTEA

    x4mmm@yandex-team.ru — 2024-08-02T09:20:04Z

    
> On 24 Jul 2024, at 17:42, Marat Bukharov <marat.buharov@gmail.com> wrote:
> 
> V5 patch. I've added more tests with different bytea sizes

Hi Marat!

This looks like a nice feature to have.

I’ve took a look into the patch and have few suggestions:
0. Please write more descriptive commit message akin to [0]
1. Use oids from development range 8000-9999
2. Replace VARDATA_ANY\memcmp dance with a call to varstrfastcmp_c().

Thank you!


Best regards, Andrey Borodin.

[0] https://github.com/postgres/postgres/commit/a0f1fce80c03

  12. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-03T21:19:14Z

    "Andrey M. Borodin" <x4mmm@yandex-team.ru> writes:
> 0. Please write more descriptive commit message akin to [0]
> 1. Use oids from development range 8000-9999

Yeah, we don't try anymore to manually select permanent oids [1].

> 2. Replace VARDATA_ANY\memcmp dance with a call to varstrfastcmp_c().

I don't agree with that recommendation in the slightest: it's a
fundamental type pun for bytea to piggyback on text/varchar functions.
It risks bugs in bytea due to somebody inserting collation dependencies
into those functions.  It also creates special cases that those
functions shouldn't have to cope with, see e.g. comment for
varstr_sortsupport about how we have to allow NUL bytes there but
only if it's C locale.  That's a laughably rickety bit of coding.

I see that somebody already made such a pun in bytea_sortsupport,
but that was a bad idea that we should undo not double down on.

I wonder if we shouldn't pull all the bytea support functions out
of varlena.c (say into a new file bytea.c), to discourage such
gamesmanship in the future.

			regards, tom lane

[1] https://www.postgresql.org/docs/devel/system-catalog-initial-data.html#SYSTEM-CATALOG-OID-ASSIGNMENT

  13. Re: [PATCH] Add min/max aggregate functions to BYTEA

    David Rowley <dgrowleyml@gmail.com> — 2024-09-23T01:05:37Z

    On Thu, 25 Jul 2024 at 02:42, Marat Bukharov <marat.buharov@gmail.com> wrote:
> V5 patch. I've added more tests with different bytea sizes

I just glanced over this patch. Are you still planning on working on
it? There's been no adjustments made since the last feedback you got
in early August.

Can you address Andrey's feedback on point #1?

Also, for bytea_larger() and bytea_smaller(), I suggest copying what's
been done in record_larger() and record_smaller() except use
byteacmp(). That'll remove all the duplicated code.

If you fix those up, I see no reason not to commit the patch.

David

  14. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Aleksander Alekseev <aleksander@timescale.com> — 2024-10-08T14:23:54Z

    Hi,

> I just glanced over this patch. Are you still planning on working on
> it? There's been no adjustments made since the last feedback you got
> in early August.
>
> Can you address Andrey's feedback on point #1?
>
> Also, for bytea_larger() and bytea_smaller(), I suggest copying what's
> been done in record_larger() and record_smaller() except use
> byteacmp(). That'll remove all the duplicated code.
>
> If you fix those up, I see no reason not to commit the patch.

Since we haven't heard from Marat since July I decided to rebase the
patch and address the feedback received so far. PFA patch v6.

-- 
Best regards,
Aleksander Alekseev

  15. Re: [PATCH] Add min/max aggregate functions to BYTEA

    x4mmm@yandex-team.ru — 2024-10-08T14:33:03Z

    
> On 8 Oct 2024, at 19:23, Aleksander Alekseev <aleksander@timescale.com> wrote:
> 
> PFA patch v6.

IMO the patch looks RfC.


Best regards, Andrey Borodin.

  16. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-08T17:52:49Z

    "Andrey M. Borodin" <x4mmm@yandex-team.ru> writes:
> IMO the patch looks RfC.

LGTM too.  Pushed.

			regards, tom lane

  17. Re: [PATCH] Add min/max aggregate functions to BYTEA

    Marat Bukharov <marat.buharov@gmail.com> — 2024-10-08T18:12:22Z

    Thank you for the corrections. I was busy recently and did not follow
conversations in the mailing list. Sorry about that.

--

With best regards,
Marat Bukharov

вт, 8 окт. 2024 г. в 17:24, Aleksander Alekseev <aleksander@timescale.com>:
>
> Hi,
>
> > I just glanced over this patch. Are you still planning on working on
> > it? There's been no adjustments made since the last feedback you got
> > in early August.
> >
> > Can you address Andrey's feedback on point #1?
> >
> > Also, for bytea_larger() and bytea_smaller(), I suggest copying what's
> > been done in record_larger() and record_smaller() except use
> > byteacmp(). That'll remove all the duplicated code.
> >
> > If you fix those up, I see no reason not to commit the patch.
>
> Since we haven't heard from Marat since July I decided to rebase the
> patch and address the feedback received so far. PFA patch v6.
>
> --
> Best regards,
> Aleksander Alekseev