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
From 850af5f9f7b1b85f98a54f5b4a757bbd00df77ec Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <p...@bowt.ie> Date: Sun, 4 Sep 2016 16:05:49 -0700 Subject: [PATCH] Defend against faulty batch memtuples calculation Add a new test to batchmemtuples() that must be passed in order to proceed with actually growing memtuples. This avoids problems with spurious target allocation sizes in corner cases with low memory. Backpatch to 9.6, where the tuplesort batch memory optimization was added. --- src/backend/utils/sort/tuplesort.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c index c8fbcf8..3478277 100644 --- a/src/backend/utils/sort/tuplesort.c +++ b/src/backend/utils/sort/tuplesort.c @@ -2868,6 +2868,7 @@ batchmemtuples(Tuplesortstate *state) /* For simplicity, assume no memtuples are actually currently counted */ Assert(state->memtupcount == 0); + Assert(state->activeTapes > 0); /* * Refund STANDARDCHUNKHEADERSIZE per tuple. @@ -2880,6 +2881,20 @@ batchmemtuples(Tuplesortstate *state) availMemLessRefund = state->availMem - refund; /* + * We need to be sure that we do not cause LACKMEM to become true, else a + * negative number of bytes could be calculated as batch allocation size, + * causing havoc (a negative availMemLessRefund cannot be accepted). Note + * that we are never reliant on a tape's batch allocation being large + * enough to fit even a single tuple; the use of the constant + * ALLOCSET_DEFAULT_INITSIZE here is not critical (a threshold like this + * avoids a useless repalloc that only grows memtuples by a tiny amount, + * which seems worth avoiding here in passing). + */ + if (availMemLessRefund <= + (int64) state->activeTapes * ALLOCSET_DEFAULT_INITSIZE) + return; + + /* * To establish balanced memory use after refunding palloc overhead, * temporarily have our accounting indicate that we've allocated all * memory we're allowed to less that refund, and call grow_memtuples() to @@ -2889,8 +2904,11 @@ batchmemtuples(Tuplesortstate *state) USEMEM(state, availMemLessRefund); (void) grow_memtuples(state); /* Should not matter, but be tidy */ + state->growmemtuples = false; + /* availMem must stay accurate for spacePerTape calculation */ FREEMEM(state, availMemLessRefund); - state->growmemtuples = false; + if (LACKMEM(state)) + elog(ERROR, "unexpected out-of-memory situation in tuplesort"); #ifdef TRACE_SORT if (trace_sort) -- 2.7.4
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
