Thread: Bug in 9.6 tuplesort batch memory growth logic

While working on my parallel CREATE INDEX patch, I came across a
problem. I initially assumed that what I saw was just a bug in my
development branch. During a final merge in a parallel worker, with
very little maintenance_work_mem, workers attempted to allocate an
amount of memory slightly less than 2 ^ 64, and fail, since that
exceeds MaxAllocHugeSize. I noticed that this happened immediately
following batchmemtuples() leaving us in a LACKMEM() state due to a
fault in its calculation (or, if you prefer, the lack of any defense
against that fault).

Further analysis led me to believe that this is a 9.6 bug. We should
have a check for sane memtuples growth, in line with the
grow_memtuples() check, where "We need to be sure that we do not cause
LACKMEM to become true, else the space management algorithm will go
nuts".  Because of the way grow_memtuples() is called by
batchmemtuples() in practice (in particular, because
availMemLessRefund may be < 0 in these corner cases),
grow_memtuples()'s own protections may not save us as previously
assumed. FREEMEM() *takes away* memory from the availMem budget when
"availMemLessRefund < 0", just after the conventional grow_memtuples()
checks run.

Attached patch adds a defense. FWIW, there is no reason to think that
more careful accounting could fix this problem, since in general a
LACKMEM() condition may not immediately lead to tuples being dumped
out.

I haven't been able to reproduce this on 9.6, but that seems
unnecessary. I can reproduce it with my parallel CREATE INDEX patch
applied, with just the right test case and right number of workers
(it's rather delicate). After careful consideration, I can think of no
reason why 9.6 would be unaffected.

--
Peter Geoghegan

Peter Geoghegan <pg@heroku.com> 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

On Tue, Sep 6, 2016 at 6:17 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

Peter Geoghegan <pg@heroku.com> writes:
> On Tue, Sep 6, 2016 at 6:17 AM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

I rewrote the comment and pushed it.
        regards, tom lane

On Tue, Sep 6, 2016 at 12:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I rewrote the comment and pushed it.

Thank you.

-- 
Peter Geoghegan