Thread: 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
Attachment
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
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
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
Attachment
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