Hash support for row types

In [0] it was discussed that hash support for row types/record would be
handy. So I implemented that.

The implementation hashes each field and combines the hash values. Most
of the code structure can be borrowed from the record comparison
functions/btree support. To combine the hash values, I adapted the code
from the array hashing functions. (The hash_combine()/hash_combine64()
functions also looked sensible, but they don't appear to work in a way
that satisfies the hash_func regression test. Could be documented better.)

The main motivation is to support UNION [DISTINCT] as discussed in [0],
but this also enables other hash-related functionality such as hash
joins (as one regression test accidentally revealed) and hash partitioning.

[0]:
https://www.postgresql.org/message-id/flat/52beaf44-ccc3-0ba1-45c7-74aa251cd6ab%402ndquadrant.com#9559845e0ee2129c483b745b9843c571

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2020-10-19 10:01:14 +0200, Peter Eisentraut wrote:
> In [0] it was discussed that hash support for row types/record would be
> handy. So I implemented that.

> The implementation hashes each field and combines the hash values. Most of
> the code structure can be borrowed from the record comparison
> functions/btree support. To combine the hash values, I adapted the code
> from the array hashing functions. (The hash_combine()/hash_combine64()
> functions also looked sensible, but they don't appear to work in a way that
> satisfies the hash_func regression test. Could be documented better.)
>
> The main motivation is to support UNION [DISTINCT] as discussed in [0], but
> this also enables other hash-related functionality such as hash joins (as
> one regression test accidentally revealed) and hash partitioning.

How does this deal with row types with a field that doesn't have a hash
function? Erroring out at runtime could cause queries that used to
succeed, e.g. because all fields have btree ops, to fail, if we just have
a generic unconditionally present hash opclass? Is that an OK
"regression"?

Greetings,

Andres Freund

On 2020-10-20 01:32, Andres Freund wrote:
> How does this deal with row types with a field that doesn't have a hash
> function? Erroring out at runtime could cause queries that used to
> succeed, e.g. because all fields have btree ops, to fail, if we just have
> a generic unconditionally present hash opclass? Is that an OK
> "regression"?

Good point. There is actually code in the type cache that is supposed
to handle that, so I'll need to adjust that.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Oct 20, 2020 at 11:10 AM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> On 2020-10-20 01:32, Andres Freund wrote:
> > How does this deal with row types with a field that doesn't have a hash
> > function? Erroring out at runtime could cause queries that used to
> > succeed, e.g. because all fields have btree ops, to fail, if we just have
> > a generic unconditionally present hash opclass? Is that an OK
> > "regression"?
>
> Good point. There is actually code in the type cache that is supposed
> to handle that, so I'll need to adjust that.

Do we need to worry about what happens if somebody modifies the
opclass/opfamily definitions?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Do we need to worry about what happens if somebody modifies the
> opclass/opfamily definitions?

There's a lot of places that you can break by doing that. I'm not
too concerned about it.

regards, tom lane

On 2020-10-20 17:10, Peter Eisentraut wrote:
> On 2020-10-20 01:32, Andres Freund wrote:
>> How does this deal with row types with a field that doesn't have a hash
>> function? Erroring out at runtime could cause queries that used to
>> succeed, e.g. because all fields have btree ops, to fail, if we just have
>> a generic unconditionally present hash opclass? Is that an OK
>> "regression"?
>
> Good point. There is actually code in the type cache that is supposed
> to handle that, so I'll need to adjust that.

Here is an updated patch with the type cache integration added.

To your point, this now checks each fields hashability before
considering a row type as hashable. It can still have run-time errors
for untyped record datums, but that's not something we can do anything
about.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> Here is an updated patch with the type cache integration added.

> To your point, this now checks each fields hashability before
> considering a row type as hashable. It can still have run-time errors
> for untyped record datums, but that's not something we can do anything
> about.

This looks good code-wise. A couple small niggles on the tests:

* The new test in with.sql claims to be testing row hashing, but
it's not very apparent that any such thing actually happens. Maybe
EXPLAIN the query, as well as execute it, to confirm that a
hash-based plan is used.

* Is it worth devising a test case in which hashing is not possible
because one of the columns isn't hashable? I have mixed feelings
about this because the set of suitable column types may decrease
to empty over time, making it hard to maintain the test case.

I marked it RFC.

regards, tom lane

I wrote a new patch to add a lot more tests around hash-based plans.
This is intended to apply separately from the other patch, and the other
patch would then "flip" some of the test cases.

On 2020-11-13 20:51, Tom Lane wrote:
> * The new test in with.sql claims to be testing row hashing, but
> it's not very apparent that any such thing actually happens. Maybe
> EXPLAIN the query, as well as execute it, to confirm that a
> hash-based plan is used.

The recursive union requires hashing, but this is not visible in the
plan. You only get an error if there is no hashing support for a type.
I have added a test for this.

For the non-recursive union, I have added more tests that show this in
the plans.

> * Is it worth devising a test case in which hashing is not possible
> because one of the columns isn't hashable? I have mixed feelings
> about this because the set of suitable column types may decrease
> to empty over time, making it hard to maintain the test case.

I used the money type for now. If someone adds hash support for that,
we'll change it. I don't think this will change too rapidly, though.

--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> I wrote a new patch to add a lot more tests around hash-based plans.
> This is intended to apply separately from the other patch, and the other
> patch would then "flip" some of the test cases.

OK, that addresses my concerns.

regards, tom lane

On 2020-11-17 20:33, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> I wrote a new patch to add a lot more tests around hash-based plans.
>> This is intended to apply separately from the other patch, and the other
>> patch would then "flip" some of the test cases.
>
> OK, that addresses my concerns.

Thanks. I have committed the tests and then subsequently the feature patch.

--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/