Andres Freund <and...@anarazel.de> writes: > On 2021-07-23 17:15:24 -0400, Tom Lane wrote: >> That's because they spill to disk where they did not before. The easy >> answer of "raise hash_mem_multiplier" doesn't help, because on Windows >> the product of work_mem and hash_mem_multiplier is clamped to 2GB, >> thanks to the ancient decision to do a lot of memory-space-related >> calculations in "long int", which is only 32 bits on Win64.
> We really ought to just remove every single use of long. I have no objection to that as a long-term goal. But I'm not volunteering to do all the work, and in any case it wouldn't be a back-patchable fix. I feel that we do need to do something about this performance regression in v13. > Hm. I wonder if we would avoid some overflow dangers on 32bit systems if > we made get_hash_memory_limit() and the relevant variables 64 bit, > rather than 32bit / size_t. E.g. No, I don't like that. Using size_t for memory-size variables is good discipline. Moreover, I'm not convinced that even with 64-bit ints, overflow would be impossible in all the places I fixed here. They're multiplying several potentially very large values (one of which is a float). I think this is just plain sloppy coding, independently of which bit-width you choose to be sloppy in. >> + skew_mcvs = skew_mcvs * SKEW_HASH_MEM_PERCENT / 100; > I always have to think about the evaluation order of things like this > (it's left to right for these), so I'd prefer parens around the > multiplication. I did wonder briefly whether the SKEW_HASH_MEM_PERCENT / > 100 just evaluates to 0... OK, will do. I see your point, because I'd sort of instinctively wanted to write that as skew_mcvs *= SKEW_HASH_MEM_PERCENT / 100; which of course would not work. Thanks for looking at the code. regards, tom lane