Обсуждение: range test for hash index?

Hello,

I noticed the tests for range types do this:

create table numrange_test2(nr numrange);
create index numrange_test2_hash_idx on numrange_test2 (nr);

Does that need a `using hash`? It seems like that's the intention. We
only use that table for equality comparisions. The script already
creates a table with a btree index further up. If I don't drop the
table I can see it's not a hash index:

regression=# \d numrange_test2
           Table "public.numrange_test2"
 Column |   Type   | Collation | Nullable | Default
--------+----------+-----------+----------+---------
 nr     | numrange |           |          |
Indexes:
    "numrange_test2_hash_idx" btree (nr)

Everything else passes if I change just that one line in the
{sql,expected} files.

Regards,
Paul

On Sat, Sep 14, 2019 at 12:48 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Hello,
>
> I noticed the tests for range types do this:
>
> create table numrange_test2(nr numrange);
> create index numrange_test2_hash_idx on numrange_test2 (nr);
>
> Does that need a `using hash`? It seems like that's the intention.
>

I also think so.  It appears to be added by commit 4429f6a9e3 which
has also added support for hash_range.  So ideally this index should
be there to cover hash_range.  I think you can once cross-check if by
default this test-file covers the case of hash_range?  If not and the
change you are proposing starts covering that code, then there is a
good chance that your finding is correct.

In general, the hash_range is covered by some of the existing test,
but I don't which test.  See the code coverage report here:
https://coverage.postgresql.org/src/backend/utils/adt/rangetypes.c.gcov.html

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Sep 14, 2019 at 5:13 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> In general, the hash_range is covered by some of the existing test,
> but I don't which test.  See the code coverage report here:
> https://coverage.postgresql.org/src/backend/utils/adt/rangetypes.c.gcov.html

Thanks! I did some experimenting, and the current test code *only*
calls `hash_range_internal` when we force it like this:

set enable_nestloop=f;
set enable_hashjoin=t;
set enable_mergejoin=f;
select * from numrange_test natural join numrange_test2 order by nr;

But if I create that index as a hash index instead, we also call it
for these inserts and selects (except for the empty ranges):

create table numrange_test2(nr numrange);
create index numrange_test2_hash_idx on numrange_test2 (nr);

INSERT INTO numrange_test2 VALUES('[, 5)');
INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));
INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));
INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2,'()'));
INSERT INTO numrange_test2 VALUES('empty');

select * from numrange_test2 where nr = 'empty'::numrange;
select * from numrange_test2 where nr = numrange(1.1, 2.2);
select * from numrange_test2 where nr = numrange(1.1, 2.3);

(None of that is surprising, right? :-)

So that seems like more confirmation that it was always intended to be
a hash index. Would you like a commit for that? Is it a small enough
change for a committer to just do it? The entire change is simply
(also attached as a file):

diff --git a/src/test/regress/expected/rangetypes.out
b/src/test/regress/expected/rangetypes.out
index 60d875e898..6fd16bddd1 100644
--- a/src/test/regress/expected/rangetypes.out
+++ b/src/test/regress/expected/rangetypes.out
@@ -519,7 +519,7 @@ select numrange(1.0, 2.0) * numrange(2.5, 3.0);
 (1 row)

 create table numrange_test2(nr numrange);
-create index numrange_test2_hash_idx on numrange_test2 (nr);
+create index numrange_test2_hash_idx on numrange_test2 using hash (nr);
 INSERT INTO numrange_test2 VALUES('[, 5)');
 INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));
 INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));
diff --git a/src/test/regress/sql/rangetypes.sql
b/src/test/regress/sql/rangetypes.sql
index 9fdb1953df..8960add976 100644
--- a/src/test/regress/sql/rangetypes.sql
+++ b/src/test/regress/sql/rangetypes.sql
@@ -119,7 +119,7 @@ select numrange(1.0, 2.0) * numrange(1.5, 3.0);
 select numrange(1.0, 2.0) * numrange(2.5, 3.0);

 create table numrange_test2(nr numrange);
-create index numrange_test2_hash_idx on numrange_test2 (nr);
+create index numrange_test2_hash_idx on numrange_test2 using hash (nr);

 INSERT INTO numrange_test2 VALUES('[, 5)');
 INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));

Yours,
Paul

On Mon, Sep 16, 2019 at 7:23 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Sat, Sep 14, 2019 at 5:13 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > In general, the hash_range is covered by some of the existing test,
> > but I don't which test.  See the code coverage report here:
> > https://coverage.postgresql.org/src/backend/utils/adt/rangetypes.c.gcov.html
>
> Thanks! I did some experimenting, and the current test code *only*
> calls `hash_range_internal` when we force it like this:
>

I don't see this function on the master branch.  Is this function name
correct?  Are you looking at some different branch?

> set enable_nestloop=f;
> set enable_hashjoin=t;
> set enable_mergejoin=f;
> select * from numrange_test natural join numrange_test2 order by nr;
>
> But if I create that index as a hash index instead, we also call it
> for these inserts and selects (except for the empty ranges):
>
> create table numrange_test2(nr numrange);
> create index numrange_test2_hash_idx on numrange_test2 (nr);
>
> INSERT INTO numrange_test2 VALUES('[, 5)');
> INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));
> INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2));
> INSERT INTO numrange_test2 VALUES(numrange(1.1, 2.2,'()'));
> INSERT INTO numrange_test2 VALUES('empty');
>
> select * from numrange_test2 where nr = 'empty'::numrange;
> select * from numrange_test2 where nr = numrange(1.1, 2.2);
> select * from numrange_test2 where nr = numrange(1.1, 2.3);
>
> (None of that is surprising, right? :-)
>
> So that seems like more confirmation that it was always intended to be
> a hash index.

Yes, it indicates that.

Jeff/Heikki, to me the issue pointed by Paul looks like an oversight
in commit 4429f6a9e3.  Can you think of any other reason?  If not, I
can commit this patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Sep 16, 2019 at 5:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I don't see this function on the master branch.  Is this function name
> correct?  Are you looking at some different branch?

Sorry about that! You're right, I was on my multirange branch. But I
see the same thing on latest master (but calling hash_range instead of
hash_range_internal).

Paul

On Mon, Sep 16, 2019 at 11:24 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Mon, Sep 16, 2019 at 5:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I don't see this function on the master branch.  Is this function name
> > correct?  Are you looking at some different branch?
>
> Sorry about that! You're right, I was on my multirange branch. But I
> see the same thing on latest master (but calling hash_range instead of
> hash_range_internal).
>

No problem, attached is a patch with a proposed commit message.  I
will wait for a few days to see if Heikki/Jeff or anyone else responds
back, otherwise will commit and backpatch this early next week.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 18, 2019 at 9:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Sep 16, 2019 at 11:24 PM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > On Mon, Sep 16, 2019 at 5:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > I don't see this function on the master branch.  Is this function name
> > > correct?  Are you looking at some different branch?
> >
> > Sorry about that! You're right, I was on my multirange branch. But I
> > see the same thing on latest master (but calling hash_range instead of
> > hash_range_internal).
> >
>
> No problem, attached is a patch with a proposed commit message.  I
> will wait for a few days to see if Heikki/Jeff or anyone else responds
> back, otherwise will commit and backpatch this early next week.
>

Today, while I was trying to backpatch, I realized that hash indexes
were not WAL-logged before 10 and they give warning "WARNING:  hash
indexes are not WAL-logged and their use is discouraged".  However,
this test has nothing to do with the durability of hash-indexes, so I
think we can safely backpatch, but still, I thought it is better to
check if anybody thinks that is not a good idea.   In back-branches,
we are already using hash-index in regression tests in some cases like
enum.sql, macaddr.sql, etc., so adding for one more genuine case
should be fine.  OTOH, we can back-patch till 10, but the drawback is
the tests will be inconsistent across branches.  Does anyone think it
is not a good idea to backpatch this till 9.4?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 25, 2019 at 09:07:13AM +0530, Amit Kapila wrote:
>On Wed, Sep 18, 2019 at 9:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Mon, Sep 16, 2019 at 11:24 PM Paul A Jungwirth
>> <pj@illuminatedcomputing.com> wrote:
>> >
>> > On Mon, Sep 16, 2019 at 5:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > > I don't see this function on the master branch.  Is this function name
>> > > correct?  Are you looking at some different branch?
>> >
>> > Sorry about that! You're right, I was on my multirange branch. But I
>> > see the same thing on latest master (but calling hash_range instead of
>> > hash_range_internal).
>> >
>>
>> No problem, attached is a patch with a proposed commit message.  I
>> will wait for a few days to see if Heikki/Jeff or anyone else responds
>> back, otherwise will commit and backpatch this early next week.
>>
>
>Today, while I was trying to backpatch, I realized that hash indexes
>were not WAL-logged before 10 and they give warning "WARNING:  hash
>indexes are not WAL-logged and their use is discouraged".  However,
>this test has nothing to do with the durability of hash-indexes, so I
>think we can safely backpatch, but still, I thought it is better to
>check if anybody thinks that is not a good idea.   In back-branches,
>we are already using hash-index in regression tests in some cases like
>enum.sql, macaddr.sql, etc., so adding for one more genuine case
>should be fine.  OTOH, we can back-patch till 10, but the drawback is
>the tests will be inconsistent across branches.  Does anyone think it
>is not a good idea to backpatch this till 9.4?
>

By "inconsistent" you mean that pre-10 versions will have different
expected output than versions with WAL-logged hash indexes? I don't see
why that would be a reason not to backpatch to all supported versions,
considering we already have the same difference for other test suites.

regards

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

On Fri, Sep 27, 2019 at 4:03 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Wed, Sep 25, 2019 at 09:07:13AM +0530, Amit Kapila wrote:
> >On Wed, Sep 18, 2019 at 9:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> On Mon, Sep 16, 2019 at 11:24 PM Paul A Jungwirth
> >> <pj@illuminatedcomputing.com> wrote:
> >> >
> >> > On Mon, Sep 16, 2019 at 5:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> > > I don't see this function on the master branch.  Is this function name
> >> > > correct?  Are you looking at some different branch?
> >> >
> >> > Sorry about that! You're right, I was on my multirange branch. But I
> >> > see the same thing on latest master (but calling hash_range instead of
> >> > hash_range_internal).
> >> >
> >>
> >> No problem, attached is a patch with a proposed commit message.  I
> >> will wait for a few days to see if Heikki/Jeff or anyone else responds
> >> back, otherwise will commit and backpatch this early next week.
> >>
> >
> >Today, while I was trying to backpatch, I realized that hash indexes
> >were not WAL-logged before 10 and they give warning "WARNING:  hash
> >indexes are not WAL-logged and their use is discouraged".  However,
> >this test has nothing to do with the durability of hash-indexes, so I
> >think we can safely backpatch, but still, I thought it is better to
> >check if anybody thinks that is not a good idea.   In back-branches,
> >we are already using hash-index in regression tests in some cases like
> >enum.sql, macaddr.sql, etc., so adding for one more genuine case
> >should be fine.  OTOH, we can back-patch till 10, but the drawback is
> >the tests will be inconsistent across branches.  Does anyone think it
> >is not a good idea to backpatch this till 9.4?
> >
>
> By "inconsistent" you mean that pre-10 versions will have different
> expected output than versions with WAL-logged hash indexes?
>

Yes.

> I don't see
> why that would be a reason not to backpatch to all supported versions,
> considering we already have the same difference for other test suites.
>

Yeah, I also think so.  I will do this today.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Sep 27, 2019 at 6:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Sep 27, 2019 at 4:03 AM Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
> >
> >
> > By "inconsistent" you mean that pre-10 versions will have different
> > expected output than versions with WAL-logged hash indexes?
> >
>
> Yes.
>
> > I don't see
> > why that would be a reason not to backpatch to all supported versions,
> > considering we already have the same difference for other test suites.
> >
>
> Yeah, I also think so.  I will do this today.
>

Pushed.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com