Re: Sort functions with specialized comparators

From: "Andrey M(dot) Borodin" <x4mmm(at)yandex-team(dot)ru>
To: John Naylor <johncnaylorls(at)gmail(dot)com>
Cc: David Rowley <dgrowleyml(at)gmail(dot)com>, Антуан Виолин <violin(dot)antuan(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Sort functions with specialized comparators
Date: 2025-01-06 15:50:37
Message-ID: BEAA383A-0165-49F6-88F0-AA0A2B1EDD53@yandex-team.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 6 Jan 2025, at 15:54, John Naylor <johncnaylorls(at)gmail(dot)com> wrote:
>>
>> I thought about it, but decided to rename the routine.
>> Here's a version 7 with compASC().
>
> I had the right idea, but the wrong function -- HEAD already had a
> suitable comp function, and one that works well with inlined
> specialized sorts: isort_cmp(). We just need to remove the extra
> argument. Like some other patches in this series, this does have the
> side effect of removing the ability to skip quinique(), so that should
> be benchmarked (I can do that this week unless you beat me to it).

With the same setup as in the first message of this thread we can do:

postgres=# SELECT _int_contains(arr,ARRAY[1]) FROM arrays_to_sort;

before patch patch
Time: 567.928 ms
after patch
Time: 890.297 ms
timing of this function is dominated by PREPAREARR(a);

What bothers me is that PREPAREARR(a); is returning new array in case of empty input. That's why I considered little refactoring of resize_intArrayType(): reorder cases so that if (num == ARRNELEMS(a)) was first.

> We
> can specialize quinique too, as mentioned upthread, but I'm not sure
> we need to.
>
>> And, just in case, if we already have ASC, why not keep DESC too instead of newly invented cmp function :) PFA v8.
>
> Those functions from common/int.h are probably not good when inlined
> (see comment there). If we really want to keep the branch for asc/desc
> out of the comparison, it makes more sense to add a function to
> reverse the elements after sorting. That allows using the same cmp
> function for everything, thus removing the apparent need for a global
> wrapper around the static sort function.
>
> I've done both ideas in v9, which also tries to improve patch
> legibility by keeping things in the same place they were before. It
> also removes the internal "n > 1" checks that you mentioned earlier --
> after thinking about it that seems better than adding braces to one
> macro to keep it functional.

LGTM.

Thanks!

Best regards, Andrey Borodin.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2025-01-06 15:52:48 Re: allow changing autovacuum_max_workers without restarting
Previous Message Robert Haas 2025-01-06 15:42:16 Re: Adjusting hash join memory limit to handle batch explosion