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
Attachments:
hash_range_test_v0001.patchapplication/octet-stream; name=hash_range_test_v0001.patchDownload
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));
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.htmlThanks! 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
Hello,
I've done some code coverage testing by running make check-world. It
doesn't show any difference in the test coverage. The patch looks good to
me.
--
Thanks & Regards,
Mahendra Thalor
EnterpriseDB: http://www.enterprisedb.com
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
Attachments:
0001-Fix-oversight-in-commit-4429f6a9e3e12bb4af6e3677fbc7.patchapplication/octet-stream; name=0001-Fix-oversight-in-commit-4429f6a9e3e12bb4af6e3677fbc7.patchDownload
From 445f6aa2aeac573d6be94c5de0f3410294dfa278 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Wed, 18 Sep 2019 09:14:26 +0530
Subject: [PATCH] Fix oversight in commit
4429f6a9e3e12bb4af6e3677fbc78cd80f160252.
The test name and the following test cases suggest the index created
should be hash index, but it forgot to add 'using hash' in the test case.
This in itself won't improve code coverage as there were some other tests
which were covering the corresponding code. However, it is better if the
added tests serve their actual purpose.
Reported-by: Paul A Jungwirth
Author: Paul A Jungwirth
Reviewed-by: Mahendra Singh
Backpatch-through: 9.4
Discussion: https://postgr.es/m/CA+renyV=Us-5XfMC25bNp-uWSj39XgHHmGE9Rh2cQKMegSj52g@mail.gmail.com
---
src/test/regress/expected/rangetypes.out | 2 +-
src/test/regress/sql/rangetypes.sql | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/test/regress/expected/rangetypes.out b/src/test/regress/expected/rangetypes.out
index 60d875e..6fd16bd 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 9fdb195..8960add 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));
--
1.8.3.1
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