From: | Kevin Grittner <kgrittn(at)ymail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Bug? ExecChooseHashTableSize() got assertion failed with crazy number of rows |
Date: | 2015-08-19 15:06:08 |
Message-ID: | 346603214.4643463.1439996768358.JavaMail.yahoo@mail.yahoo.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>>> we may need a couple of overhaul around HashJoin to support large
>>> size of data, not only nbuckets around 0x80000000.
>> Perhaps, but this is a clear bug, introduced to the 9.5 code, with
>> an obvious fix; so I've pushed the change from 1 to 1L on that left
>> shift.
> I don't think it's anywhere near as clear as you think. The preceding
> lines should have limited nbuckets to be no more than INT_MAX/2, so how
> could an overflow occur there?
There is a 1 shifted for nbuckets that I didn't touch because it
could not. The limits on nbuckets are not applied at the point of
the bug.
> (The result of 1<<mylog() should be
> at most 0x40000000 AFAICS.) If overflow is possible, how will s/1/1L/
> make it better on machines where int and long are the same size?
In such cases there is not a bug -- at least not with my_log
returning less than 63. In those cases the change will make no
difference.
> And on machines where long is wider than int, you've still got a bug
> if the result of the left shift somehow overflowed an int, because
> it's going to be assigned to nbuckets which is an int.
You seem to have missed reading the line in between:
| lbuckets = 1L << my_log2(hash_table_bytes / bucket_size);
| lbuckets = Min(lbuckets, max_pointers);
| nbuckets = (int) lbuckets;
There is a Min() and a cast to sort that out.
> So I think the "1" coding was just fine in context.
I disagree.
> If there is an overflow in ExecChooseHashTableSize, it's
> somewhere else.
I do believe that there are other places we need to place limits,
to avoid attempts to allocate more than 1GB, but I think patches to
address that should be separate from fixing this particular bug.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2015-08-19 15:06:19 | Re: DBT-3 with SF=20 got failed |
Previous Message | Andres Freund | 2015-08-19 15:00:39 | Re: Make HeapTupleSatisfiesMVCC more concurrent |