BUG #14210: filter by "=" constraint doesn't work when hash index is present on a column

danielnewman@umich.edu writes:

When I run the query `select * from hash_issue_table where hash_issue_column
like '2184';` I get 701 results. When I run the query
When I run the query `select * from hash_issue_table where hash_issue_column
= '2184';`, I get 0 results. However, if I drop the hash index and rerun the
second query, I get the same results.

Sounds like a corrupted hash index to me; which is not terribly surprising
given hash indexes' lack of WAL support. Can you reproduce this after
rebuilding the hash index?

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Yes. If i delete the index and recreate it, the bug is replicated. I have
uploaded a pg_dump .sql file to
https://www.dropbox.com/s/4dxuo2uthj721od/hash_issue_db.sql?dl=0 that you
can use to recreate the issue.

After creating the database when I run:

select * from hash_issue_table where hash_issue_column = '1';

I get no results.

When I run:

drop index hash_issue_index;
select * from hash_issue_table where hash_issue_column = '1';

I get 531 rows of results.

When I run:

create index hash_issue_index on hash_issue_table using

hash(hash_issue_column);

select * from hash_issue_table where hash_issue_column = '1';

I get 0 results again. I have repeated this successfully multiple times and
on a coworker's machine as well.

Interestingly, I modified the pg_dump file a bit. At the end, it says:

CREATE INDEX hash_issue_index ON hash_issue_table USING hash

(hash_issue_column);

DROP INDEX hash_issue_index;

CREATE INDEX hash_issue_index ON hash_issue_table USING hash
(hash_issue_column);

This is because the issue was not replicating (for some reason) when it
built the index the first time.

On Thu, Jun 23, 2016 at 1:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Newman <dtnewman@gmail.com> writes:

Yes. If i delete the index and recreate it, the bug is replicated.

So the answer is that this got broken by commit 9f03ca915196dfc8,
which appears to have imagined that _hash_form_tuple() is just an
alias for index_form_tuple(). But it ain't. As a result, construction
of hash indexes larger than shared_buffers is broken in 9.5 and up:
what gets entered into the index is garbage, because we are taking
raw data as if it were already hashed. (In fact, in this example,
we seem to be taking *pointers* to raw data as the hash values.)

Interestingly, I modified the pg_dump file a bit. At the end, it says:

CREATE INDEX hash_issue_index ON hash_issue_table USING hash
(hash_issue_column);
DROP INDEX hash_issue_index;
CREATE INDEX hash_issue_index ON hash_issue_table USING hash
(hash_issue_column);

This is because the issue was not replicating (for some reason) when it
built the index the first time.

I think what's happening there is that the first CREATE INDEX
underestimates the size of the table and decides it doesn't need to
use the _h_spool() code path. The other path isn't broken.

We can either revert the aforesaid commit so far as it affects hash,
or do something to break _hash_form_tuple's encapsulation of the
hash-value-for-data substitution. I don't immediately see a non-messy
way to do the latter.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Wow, thank you for such a quick response! And thanks for pointing out the
commit... I'm definitely interested in having a look at the code.

In the meantime, I can solve the specific problem I'm having by switching
to a btree index :)

Thank you for this and all the rest of your great work.

Best,
Daniel Newman

On Thu, Jun 23, 2016 at 7:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

So the answer is that this got broken by commit 9f03ca915196dfc8,
which appears to have imagined that _hash_form_tuple() is just an
alias for index_form_tuple(). But it ain't. As a result, construction
of hash indexes larger than shared_buffers is broken in 9.5 and up:

BTW, an easy way to prove that the _h_spool code path is broken is to
do this:

diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 49a6c81..162cb63 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -125,7 +125,7 @@ hashbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	 * NOTE: this test will need adjustment if a bucket is ever different from
 	 * one page.
 	 */
-	if (num_buckets >= (uint32) NBuffers)
+	if (1)
 		buildstate.spool = _h_spoolinit(heap, index, num_buckets);
 	else
 		buildstate.spool = NULL;

and then run the regression tests. I'm thinking it would be a good
idea to provide some less-painful way to force that code path to be
taken for testing purposes.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Jun 23, 2016 at 7:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Newman <dtnewman@gmail.com> writes:

Yes. If i delete the index and recreate it, the bug is replicated.

So the answer is that this got broken by commit 9f03ca915196dfc8,
which appears to have imagined that _hash_form_tuple() is just an
alias for index_form_tuple(). But it ain't. As a result, construction
of hash indexes larger than shared_buffers is broken in 9.5 and up:
what gets entered into the index is garbage, because we are taking
raw data as if it were already hashed. (In fact, in this example,
we seem to be taking *pointers* to raw data as the hash values.)

Oops.

Interestingly, I modified the pg_dump file a bit. At the end, it says:

CREATE INDEX hash_issue_index ON hash_issue_table USING hash
(hash_issue_column);
DROP INDEX hash_issue_index;
CREATE INDEX hash_issue_index ON hash_issue_table USING hash
(hash_issue_column);

This is because the issue was not replicating (for some reason) when it
built the index the first time.

I think what's happening there is that the first CREATE INDEX
underestimates the size of the table and decides it doesn't need to
use the _h_spool() code path. The other path isn't broken.

We can either revert the aforesaid commit so far as it affects hash,
or do something to break _hash_form_tuple's encapsulation of the
hash-value-for-data substitution. I don't immediately see a non-messy
way to do the latter.

Would it work to have something like _hash_form_tuple() except that
instead of returning an index tuple it would just return the Datum and
isnull flags and let the caller decide what to do with them?

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Jun 23, 2016 at 7:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

We can either revert the aforesaid commit so far as it affects hash,
or do something to break _hash_form_tuple's encapsulation of the
hash-value-for-data substitution. I don't immediately see a non-messy
way to do the latter.

Would it work to have something like _hash_form_tuple() except that
instead of returning an index tuple it would just return the Datum and
isnull flags and let the caller decide what to do with them?

Yeah. Let's define it as a function that transforms input values/isnull
arrays into output values/isnull arrays. It seems all right for the
callers to know the latter are always of length 1, so they can be local
variables in the callers. I'll go make it so.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Jun 23, 2016 at 4:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So the answer is that this got broken by commit 9f03ca915196dfc8,
which appears to have imagined that _hash_form_tuple() is just an
alias for index_form_tuple(). But it ain't.

Can we add test coverage that will detect when comparetup_index_hash()
gives wrong answers, please? It's been broken at least 2 times, that
I'm aware of. I'll write a patch, if that helps.

--
Peter Geoghegan

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Peter Geoghegan <pg@heroku.com> writes:

On Thu, Jun 23, 2016 at 4:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So the answer is that this got broken by commit 9f03ca915196dfc8,
which appears to have imagined that _hash_form_tuple() is just an
alias for index_form_tuple(). But it ain't.

Can we add test coverage that will detect when comparetup_index_hash()
gives wrong answers, please? It's been broken at least 2 times, that
I'm aware of. I'll write a patch, if that helps.

I agree that we need to do something, but I'm not very clear on what
the something is. I considered inventing a debug #define that would
reduce the _h_spool threshold to the minimum workable amount (2, looks
like), but I'm not sure how much that will help. What did you have
in mind?

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Jun 24, 2016 at 2:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I agree that we need to do something, but I'm not very clear on what
the something is. I considered inventing a debug #define that would
reduce the _h_spool threshold to the minimum workable amount (2, looks
like), but I'm not sure how much that will help. What did you have
in mind?

I think we should do that. Why do you say that you're not sure how
much it will help?

It may have escaped your attention, so I'll point out that amcheck
regression tests are my solution to testing external sorting, an area
which is similarly sorely lacking.

--
Peter Geoghegan

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Peter Geoghegan <pg@heroku.com> writes:

On Fri, Jun 24, 2016 at 2:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I agree that we need to do something, but I'm not very clear on what
the something is. I considered inventing a debug #define that would
reduce the _h_spool threshold to the minimum workable amount (2, looks
like), but I'm not sure how much that will help. What did you have
in mind?

I think we should do that. Why do you say that you're not sure how
much it will help?

Well, it won't run automatically --- unless someone spins up a buildfarm
machine that adds that symbol to CPPFLAGS, and even then we'd only have
coverage on one platform.

However, the next step up would be to invent an index storage parameter or
some such to force the decision, letting the standard regression tests
include a reasonably-cheap case that would exercise _h_spool. But that
seems like rather a large amount of work compared to the likely benefits.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Fri, Jun 24, 2016 at 3:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think we should do that. Why do you say that you're not sure how
much it will help?

Well, it won't run automatically --- unless someone spins up a buildfarm
machine that adds that symbol to CPPFLAGS, and even then we'd only have
coverage on one platform.

I think that this is an argument against further proliferation of
#define's for this kind of thing, not an argument against adding a way
to force tuplesort based hash index builds.

--
Peter Geoghegan

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Peter Geoghegan <pg@heroku.com> writes:

On Fri, Jun 24, 2016 at 3:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Well, it won't run automatically --- unless someone spins up a buildfarm
machine that adds that symbol to CPPFLAGS, and even then we'd only have
coverage on one platform.

I think that this is an argument against further proliferation of
#define's for this kind of thing, not an argument against adding a way
to force tuplesort based hash index builds.

So ... what's your point? This statement doesn't seem to lead to a
conclusion in favor of either of the alternatives I mentioned.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Sat, Jun 25, 2016 at 8:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think that this is an argument against further proliferation of
#define's for this kind of thing, not an argument against adding a way
to force tuplesort based hash index builds.

So ... what's your point? This statement doesn't seem to lead to a
conclusion in favor of either of the alternatives I mentioned.

I'm in favor of just adding a debug option. We should introduce a more
general idea of a #define that enables a variety of debugging options,
because it's silly to have to worry about buildfarm coverage each and
every time this crops up. I'm not entirely sure what level that should
be scoped at. The status quo is impractical.

Incidentally, did you see to it that there is buildfarm coverage for
RAW_EXPRESSION_COVERAGE_TEST?

--
Peter Geoghegan

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Peter Geoghegan <pg@heroku.com> writes:

On Sat, Jun 25, 2016 at 8:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So ... what's your point? This statement doesn't seem to lead to a
conclusion in favor of either of the alternatives I mentioned.

I'm in favor of just adding a debug option. We should introduce a more
general idea of a #define that enables a variety of debugging options,
because it's silly to have to worry about buildfarm coverage each and
every time this crops up. I'm not entirely sure what level that should
be scoped at. The status quo is impractical.

I don't exactly see how that leads to any improvement --- you still end
up having to manually configure some buildfarm critters for coverage, no?

Incidentally, did you see to it that there is buildfarm coverage for
RAW_EXPRESSION_COVERAGE_TEST?

Yes, see dromedary.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Sat, Jun 25, 2016 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm in favor of just adding a debug option. We should introduce a more
general idea of a #define that enables a variety of debugging options,
because it's silly to have to worry about buildfarm coverage each and
every time this crops up. I'm not entirely sure what level that should
be scoped at. The status quo is impractical.

I don't exactly see how that leads to any improvement --- you still end
up having to manually configure some buildfarm critters for coverage, no?

Yes, but you only have to do it once. I don't see the problem with
creating a #define that represents a grab-bag of debug options that
should run on one or two buildfarm animals. That doesn't preclude also
allowing things like RAW_EXPRESSION_COVERAGE_TEST to be enabled more
selectively.

In short, I want to make it easier to add custom debugging options
that could trip a regression test failure.

--
Peter Geoghegan

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Peter Geoghegan <pg@heroku.com> writes:

On Sat, Jun 25, 2016 at 12:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't exactly see how that leads to any improvement --- you still end
up having to manually configure some buildfarm critters for coverage, no?

Yes, but you only have to do it once. I don't see the problem with
creating a #define that represents a grab-bag of debug options that
should run on one or two buildfarm animals. That doesn't preclude also
allowing things like RAW_EXPRESSION_COVERAGE_TEST to be enabled more
selectively.

Meh. I do not really think that dromedary's test options
(-DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST) represent a model
to follow for this problem. In both of those cases, the value of having
the option turned on is that it will exercise code that hasn't even been
written yet. So those options have to be enabled across the entire test
suite in order to be of much use. Furthermore, there's no particular
reason to think that they'd expose platform-dependent behavior, so there's
no real downside to having just one or two buildfarm critters using them.

But exercising a non-default code path in hash index build is something we
only need in one place in the tests, and at least in my judgment there is
a possibility for platform-specific behavior. So I think some way of
turning it on dynamically, and then adding that as a standard test case,
would be a considerable improvement. I just don't quite want to go to the
effort of inventing a GUC or reloption. Is there some intermediate
answer?

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

I wrote:

But exercising a non-default code path in hash index build is something we
only need in one place in the tests, and at least in my judgment there is
a possibility for platform-specific behavior. So I think some way of
turning it on dynamically, and then adding that as a standard test case,
would be a considerable improvement. I just don't quite want to go to the
effort of inventing a GUC or reloption. Is there some intermediate
answer?

A small-footprint answer that just occurred to me is to redefine the
threshold as being, not NBuffers, but maintenance_work_mem. Then a
reasonably-sized test case could be made by reducing that to its minimum
allowed value. This seems like it'd be fairly unsurprising for users
since there's lots of precedent for maintenance_work_mem affecting the
behavior of CREATE INDEX.

If maintenance_work_mem exceeds shared_buffers then you'd get a certain
amount of thrashing between shared buffers and kernel disk cache, but it
wouldn't be really awful unless maintenance_work_mem exceeded available
RAM, which would be a certifiably Bad Idea anyway. I guess we could
forestall the buffer-thrashing scenario and preserve testability by
setting the sort threshold to min(NBuffers, maintenance_work_mem).

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Mon, Jun 27, 2016 at 7:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Meh. I do not really think that dromedary's test options
(-DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST) represent a model
to follow for this problem. In both of those cases, the value of having
the option turned on is that it will exercise code that hasn't even been
written yet. So those options have to be enabled across the entire test
suite in order to be of much use. Furthermore, there's no particular
reason to think that they'd expose platform-dependent behavior, so there's
no real downside to having just one or two buildfarm critters using them.

You think that the hash index tuplesort code should be possible to
exercise in the regression tests dynamically, without any new #define
whatsoever. Fair enough, but I still think that enabling debug #define
options should be possible at a coarser granularity, even if that ends
up covering a set of cases that are not very crisply defined. Maybe
that's a discussion for the next time something like
RAW_EXPRESSION_COVERAGE_TEST is proposed, though.

--
Peter Geoghegan

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs