On Thu, Nov 15, 2012 at 12:14 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> On 15 November 2012 16:09, Robert Haas <robertmhaas@gmail.com> wrote:
> [Lots of complicated commentary on tuplesort variables]
>> Whew. In the attached version, I
>> updated the comment to reflect this, and also reversed the order of
>> the if/else block to put the shorter and more common case first, which
>> I think is better style.
>
> Fine by me.
>
> In conversation with Greg Stark in Prague, he seemed to think that
> there was an integer overflow hazard in v3, which is, in terms of the
> machine code it will produce, identical to your revision. He pointed
> this out to me when we were moving between sessions, and I only
> briefly looked over his shoulder, so while I don't want to
> misrepresent what he said, I *think* he said that this could overflow:
>
> + newmemtupsize = memtupsize * allowedMem / memNowUsed;
Ah, yeah. I wondered in passing about that but forgot to follow up on
it. The problem specifically is that the intermediate result
memtupsize * newmemtuples might overflow. I believe that the old
memtupsize can never be more than 2^26 bytes, because the allocation
limit is 1GB and each SortTuple is 16 bytes. So if this is done as a
32-bit calculation, we'll potentially overflow if allowedMem is more
than 64 bytes i.e. almost for sure. If it is done as a 64-byte
calculation we'll potentially overflow if allowedMem is more than 2^38
bytes = 256GB. The actual declared type is "long" which I assume is
probably 64 bits at least for most people these days, but maybe not
for everyone, and even 256GB is not necessarily safe. The highest
value for work_mem I can set here is 2047GB. So I think there is an
issue here, unless there's something wrong with my analysis.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
