Обсуждение: move hash_any to utils/hash/hashfn.c

I just noticed (once again) that we have hash_any() declared in
src/access/hash.h (the header file for the hash index AM) rather than
somewhere in utils/, for no good reason other than perhaps there was no
better place when it was introduced.  This means that some files that
seem to just need hash_any end up pointlessly #including the whole AM
world upon themselves.

Would anybody object too hard if I move hash_any() and friends to
src/backend/utils/hash/hashfn.c and the declarations to
src/include/utils/hashutils.h?


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2019-01-25 16:35:18 -0300, Alvaro Herrera wrote:
> I just noticed (once again) that we have hash_any() declared in
> src/access/hash.h (the header file for the hash index AM) rather than
> somewhere in utils/, for no good reason other than perhaps there was no
> better place when it was introduced.  This means that some files that
> seem to just need hash_any end up pointlessly #including the whole AM
> world upon themselves.
> 
> Would anybody object too hard if I move hash_any() and friends to
> src/backend/utils/hash/hashfn.c and the declarations to
> src/include/utils/hashutils.h?

I hate the current split quite a bit, albeit for somewhat different
reasons. We make things like tag_hash, uint32_hash unnecessarily more
expensive by adding an entirely useless external function call. And
some of these can be fairly hot (e.g. for syscache).  So yea, let's
move this stuff together.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-01-25 16:35:18 -0300, Alvaro Herrera wrote:
>> Would anybody object too hard if I move hash_any() and friends to
>> src/backend/utils/hash/hashfn.c and the declarations to
>> src/include/utils/hashutils.h?

> I hate the current split quite a bit, albeit for somewhat different
> reasons. We make things like tag_hash, uint32_hash unnecessarily more
> expensive by adding an entirely useless external function call. And
> some of these can be fairly hot (e.g. for syscache).  So yea, let's
> move this stuff together.

+1

            regards, tom lane

Hello

On 2019-Jan-25, Andres Freund wrote:

> I hate the current split quite a bit, albeit for somewhat different
> reasons. We make things like tag_hash, uint32_hash unnecessarily more
> expensive by adding an entirely useless external function call. And
> some of these can be fairly hot (e.g. for syscache).  So yea, let's
> move this stuff together.

Here's a patch, but I haven't done anything that would change tag_hash
et al.  Looking at the new hashfn.s, it looks like this:

tag_hash:
.LFB42:
    .loc 1 681 0
    .cfi_startproc
.LVL310:
    .loc 1 682 0
    call    hash_any
.LVL311:
    .loc 1 684 0
    rep ret
    .cfi_endproc

In master, it looks like this instead:

tag_hash:
.LFB96:
    .loc 1 53 0
    .cfi_startproc
.LVL5:
    subq    $8, %rsp
    .cfi_def_cfa_offset 16
    .loc 1 54 0
    call    hash_any@PLT
.LVL6:
    .loc 1 56 0
    addq    $8, %rsp
    .cfi_def_cfa_offset 8
    ret
    .cfi_endproc


I don't know if the change is significant.  Obviously we lost the
subq/addq (probably nothing to be excited about) but there's also the
@PLT in the call ... I don't know what that means.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Jan-25, Alvaro Herrera wrote:

> Would anybody object too hard if I move hash_any() and friends to
> src/backend/utils/hash/hashfn.c and the declarations to
> src/include/utils/hashutils.h?

Pushed this.  I ended up adding an #include of utils/hashutils.h to
access/hash.h, so that any third-party code using hash_any() from the
latter continues to build unbroken.  I don't think this is terribly
problematic -- the only issue is that those projects won't become
updated to use the leaner file, but I don't think we care too much about
that.  If I do get pushback about this change I can definitely remove
that line and watch the fireworks.

Anyway, let's see what *else* breaks now ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services