On 10/7/25 23:10, Melanie Plageman wrote:
> On Wed, Sep 24, 2025 at 7:02 PM Tomas Vondra wrote:
>>
>> Here's a couple draft patches fixing the bug:
>>
>> - 0001 adds the missing size_t cast, to fix the overflow
>> - 0002 fixes the balancing, by adjusting the hash table size limit
>> - 0003 adds th
On Wed, Oct 8, 2025 at 1:37 PM Melanie Plageman
wrote:
>
> I have updated my patch to fix the mistakes above. I also noticed then
> that I wasn't doubling space_allowed in the loop but instead setting
> it to hash_table_bytes at the end. This doesn't produce a power of 2
> because we subtract skew
On 9/22/25 22:45, David Rowley wrote:
> On Tue, 23 Sept 2025 at 01:57, Vaibhav Jain wrote:
>> With a1b4f28, to compute current_space, nbatch is being multiplied
>> by BLCKSZ. nbatch is int and when multiplied with BLCKSZ, it can
>> easily overflow the int limit.To keep the calculation safe for
Hi,
Here's a couple draft patches fixing the bug:
- 0001 adds the missing size_t cast, to fix the overflow
- 0002 fixes the balancing, by adjusting the hash table size limit
- 0003 adds the missing overflow protection for nbuckets and the hash
table limit
- 0004 rewords the comment explaining
On Wed, Oct 8, 2025 at 4:46 PM Tomas Vondra wrote:
>
> On 10/8/25 19:37, Melanie Plageman wrote:
>
> > Yes, this was wrong. I forgot an extra / sizeof(HashJoinTuple). I meant:
> >
> > hash_table_bytes / sizeof(HashJoinTuple) > MaxAllocSize /
> > sizeof(HashJoinTuple) / 2
>
> But the hash table is
On 10/8/25 19:37, Melanie Plageman wrote:
> On Tue, Oct 7, 2025 at 7:51 PM Tomas Vondra wrote:
>>
>> On 10/7/25 23:10, Melanie Plageman wrote:
>>
>> Hmm, so if I understand correctly you suggest stop when nbuckets gets
>> too high, while my code allowed reducing nbatches further (and just
>> ca
On 10/8/25 21:16, Melanie Plageman wrote:
> On Wed, Oct 8, 2025 at 1:37 PM Melanie Plageman
> wrote:
>>
>> I have updated my patch to fix the mistakes above. I also noticed then
>> that I wasn't doubling space_allowed in the loop but instead setting
>> it to hash_table_bytes at the end. This do
On 23/09/2025 6:11 PM, Tomas Vondra wrote:
Hi,
I kept looking at this, and unfortunately the it seems a bit worse :-(
Fixing the overflow is easy enough - adding the cast does the trick.
But then there's the second issue I mentioned - the loop does not adjust
the hash_table_bytes. It updates
Tomas Vondra writes:
> The question what to do about this. If we got this a week ago, I'd just
> probably just revert a1b4f289, and then try again for PG19. After all,
> the issue it meant to address is somewhat rare.
> But with 18 already stamped ...
18.0 is what it is. If this is the most seri
On Wed, Oct 8, 2025 at 5:08 PM Tomas Vondra wrote:
>
> On 10/8/25 21:16, Melanie Plageman wrote:
> > On Wed, Oct 8, 2025 at 1:37 PM Melanie Plageman
> > wrote:
> >>
> >> I have updated my patch to fix the mistakes above. I also noticed then
> >> that I wasn't doubling space_allowed in the loop bu
On Tue, 23 Sept 2025 at 13:01, Tomas Vondra wrote:
>
> On 9/23/25 02:02, David Rowley wrote:
> > Ok cool. We're just in the freeze for 18.0 at the moment. Once that's
> > over, should I take care of this, or do you want to?
> >
>
> Feel free to fix, but I can take care of it once 18 is out the doo
Hi,
I kept looking at this, and unfortunately the it seems a bit worse :-(
Fixing the overflow is easy enough - adding the cast does the trick.
But then there's the second issue I mentioned - the loop does not adjust
the hash_table_bytes. It updates the *space_allowed, but that's not what
the cu
> On Sep 23, 2025, at 07:35, David Rowley wrote:
>
> On Tue, 23 Sept 2025 at 11:21, Chao Li wrote:
>> I guess that because earlier in the function, nbatch is always clamped with:
>>
>> nbatch = pg_nextpower2_32(Max(2, minbatch));
>
> I don't follow which part of that line could be constitute
On Tue, Oct 7, 2025 at 7:51 PM Tomas Vondra wrote:
>
> On 10/7/25 23:10, Melanie Plageman wrote:
>
> Hmm, so if I understand correctly you suggest stop when nbuckets gets
> too high, while my code allowed reducing nbatches further (and just
> capped nbatches). I'm fine with this change, if it make
On Mon, Oct 13, 2025 at 12:05 PM Melanie Plageman
wrote:
>
> On Thu, Oct 9, 2025 at 7:36 PM Tomas Vondra wrote:
> >
> > 1) A couple comments adjusted. It feels a bit too audacious to correct
> > comments written by native speaker, but it seems cleaner to me like this.
>
> I attached a patch with
On Wed, Sep 24, 2025 at 7:02 PM Tomas Vondra wrote:
>
> Here's a couple draft patches fixing the bug:
>
> - 0001 adds the missing size_t cast, to fix the overflow
> - 0002 fixes the balancing, by adjusting the hash table size limit
> - 0003 adds the missing overflow protection for nbuckets and the
On 10/14/25 23:13, Tomas Vondra wrote:
> ...
>
> I'll give this a bit more testing and review tomorrow, and then I'll
> push. I don't want to hold this back through pgconf.eu.
>
Pushed and backpatched, after some minor tweaks. Thanks for the reviews
and feedback. I consider this is fixed now.
On 10/13/25 18:05, Melanie Plageman wrote:
> On Thu, Oct 9, 2025 at 7:36 PM Tomas Vondra wrote:
>>
>> 1) A couple comments adjusted. It feels a bit too audacious to correct
>> comments written by native speaker, but it seems cleaner to me like this.
>
> I attached a patch with a few more suggeste
On 10/9/25 16:30, Tomas Vondra wrote:
> On 10/9/25 16:16, Melanie Plageman wrote:
>> On Wed, Oct 8, 2025 at 4:46 PM Tomas Vondra wrote:
>>>
>>> On 10/8/25 19:37, Melanie Plageman wrote:
>>>
Yes, this was wrong. I forgot an extra / sizeof(HashJoinTuple). I meant:
hash_table_bytes / s
On Thu, Oct 9, 2025 at 7:36 PM Tomas Vondra wrote:
>
> 1) A couple comments adjusted. It feels a bit too audacious to correct
> comments written by native speaker, but it seems cleaner to me like this.
I attached a patch with a few more suggested adjustments (0003). The
more substantive tweaks ar
On 10/9/25 16:16, Melanie Plageman wrote:
> On Wed, Oct 8, 2025 at 4:46 PM Tomas Vondra wrote:
>>
>> On 10/8/25 19:37, Melanie Plageman wrote:
>>
>>> Yes, this was wrong. I forgot an extra / sizeof(HashJoinTuple). I meant:
>>>
>>> hash_table_bytes / sizeof(HashJoinTuple) > MaxAllocSize /
>>> sizeo
On 9/23/25 02:02, David Rowley wrote:
> On Tue, 23 Sept 2025 at 09:31, Tomas Vondra wrote:
>> On 9/22/25 22:45, David Rowley wrote:
>>> I think a1b4f289b mistakenly thought that there'd be size_t arithmetic
>>> in the following two lines because the final result is a size_t:
>>>
>>> size_t current
On 9/23/25 21:13, Konstantin Knizhnik wrote:
> On 23/09/2025 6:11 PM, Tomas Vondra wrote:
>
>> Hi,
>>
>> I kept looking at this, and unfortunately the it seems a bit worse :-(
>>
>> Fixing the overflow is easy enough - adding the cast does the trick.
>>
>> But then there's the second issue I me
On Tue, Sep 23, 2025 at 11:34:56AM -0400, Tom Lane wrote:
> 18.0 is what it is. If this is the most serious bug in it, I'll be
> a bit surprised. Take your time, fix it properly.
+1
--
nathan
On 9/23/25 03:20, David Rowley wrote:
> On Tue, 23 Sept 2025 at 13:01, Tomas Vondra wrote:
>>
>> On 9/23/25 02:02, David Rowley wrote:
>>> Ok cool. We're just in the freeze for 18.0 at the moment. Once that's
>>> over, should I take care of this, or do you want to?
>>>
>>
>> Feel free to fix, b
Thank you everyone for the reviews, feedback and the test.
Please find attached a new version of the patch.
On Tue, Sep 23, 2025 at 7:11 AM Chao Li wrote:
>
>
> On Sep 23, 2025, at 07:35, David Rowley wrote:
>
> On Tue, 23 Sept 2025 at 11:21, Chao Li wrote:
>
> I guess that because earlier in
On Tue, 23 Sept 2025 at 09:31, Tomas Vondra wrote:
> On 9/22/25 22:45, David Rowley wrote:
> > I think a1b4f289b mistakenly thought that there'd be size_t arithmetic
> > in the following two lines because the final result is a size_t:
> >
> > size_t current_space = hash_table_bytes + (2 * nbatch *
> On Sep 22, 2025, at 21:20, Vaibhav Jain wrote:
>
> Hi Everyone,
>
> With a1b4f28, to compute current_space, nbatch is being multiplied
> by BLCKSZ. nbatch is int and when multiplied with BLCKSZ, it can
> easily overflow the int limit.To keep the calculation safe for
> current_space, convert
On Tue, 23 Sept 2025 at 11:21, Chao Li wrote:
> I guess that because earlier in the function, nbatch is always clamped with:
>
> nbatch = pg_nextpower2_32(Max(2, minbatch));
I don't follow which part of that line could be constituted as
clamping. Maybe you've confused Max with Min?
David
On Tue, 23 Sept 2025 at 01:57, Vaibhav Jain wrote:
> With a1b4f28, to compute current_space, nbatch is being multiplied
> by BLCKSZ. nbatch is int and when multiplied with BLCKSZ, it can
> easily overflow the int limit.To keep the calculation safe for
> current_space, convert nbatch to size_t.
Th
Hi Everyone,
With a1b4f28, to compute current_space, nbatch is being multiplied
by BLCKSZ. nbatch is int and when multiplied with BLCKSZ, it can
easily overflow the int limit.To keep the calculation safe for
current_space, convert nbatch to size_t.
Please find a patch for the same.
Thanks,
Vaibh
31 matches
Mail list logo
