Re: [HACKERS] Bug in 9.6 tuplesort batch memory growth logic

[Peter Geoghegan]

On Tue, Sep 6, 2016 at 12:51 PM, Tom Lane  wrote:
> I rewrote the comment and pushed it.

Thank you.

-- 
Peter Geoghegan


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

Re: [HACKERS] Bug in 9.6 tuplesort batch memory growth logic

[Tom Lane]

I wrote:
> I see.  The comment could do with a bit of rewriting, perhaps.

I rewrote the comment and pushed it.

regards, tom lane


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

Re: [HACKERS] Bug in 9.6 tuplesort batch memory growth logic

[Tom Lane]

Peter Geoghegan  writes:
> On Tue, Sep 6, 2016 at 6:17 AM, Tom Lane  wrote:
>> It doesn't seem to me that this limit has anything to do with anything,
>> and the comment claiming only that it's "noncritical" isn't helping.

> You've not understood the problem at all. The only thing that's
> critical is that the calculation not fail at all, through a later
> availMem that is < 0 (i.e. a LACKMEM() condition).

I see.  The comment could do with a bit of rewriting, perhaps.

regards, tom lane


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

Re: [HACKERS] Bug in 9.6 tuplesort batch memory growth logic

[Peter Geoghegan]

On Tue, Sep 6, 2016 at 6:17 AM, Tom Lane  wrote:
> It doesn't seem to me that this limit has anything to do with anything,
> and the comment claiming only that it's "noncritical" isn't helping.
> If the point is to prevent the later LACKMEM check from failing, then
> certainly there is something critical somewhere.  I'd rather see this
> explained as "we need at least X, but we choose to require at least Y
> to avoid repalloc thrashing".

You've not understood the problem at all. The only thing that's
critical is that the calculation not fail at all, through a later
availMem that is < 0 (i.e. a LACKMEM() condition).

> And maybe the code should use Max(X,Y)
> rather than blindly assuming that ALLOCSET_DEFAULT_INITSIZE exceeds
> whatever the true minimum is.

The true minimum is 0, so that seems like a safe bet. Comments point
this out directly, and that we are not reliant on having any
particular amount of memory available, even enough for one tuple (the
overflow mechanism will later save us).

I knew that the patch would be criticized for still allowing a useless
palloc, and some threshold was needed. I also knew any choice of
constant would be criticized (e.g., "that's voodoo"), so pointed out
specifically that it was non-critical.

What threshold would you use?
-- 
Peter Geoghegan


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

Re: [HACKERS] Bug in 9.6 tuplesort batch memory growth logic

[Tom Lane]

Peter Geoghegan  writes:
> While working on my parallel CREATE INDEX patch, I came across a
> problem.

I took a quick look at this.  I do not follow the logic in this new bit:

+   if (availMemLessRefund <=
+   (int64) state->activeTapes * ALLOCSET_DEFAULT_INITSIZE)
+  return;

It doesn't seem to me that this limit has anything to do with anything,
and the comment claiming only that it's "noncritical" isn't helping.
If the point is to prevent the later LACKMEM check from failing, then
certainly there is something critical somewhere.  I'd rather see this
explained as "we need at least X, but we choose to require at least Y
to avoid repalloc thrashing".  And maybe the code should use Max(X,Y)
rather than blindly assuming that ALLOCSET_DEFAULT_INITSIZE exceeds
whatever the true minimum is.

regards, tom lane


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