Be more careful to preserve consistency of a tuplestore.

Several places in tuplestore.c would leave the tuplestore data
structure effectively corrupt if some subroutine were to throw
an error.  Notably, if WRITETUP() failed after some number of
successful calls within dumptuples(), the tuplestore would
contain some memtuples pointers that were apparently live
entries but in fact pointed to pfree'd chunks.

In most cases this sort of thing is fine because transaction
abort cleanup is not too picky about the contents of memory that
it's going to throw away anyway.  There's at least one exception
though: if a Portal has a holdStore, we're going to call
tuplestore_end() on that, even during transaction abort.
So it's not cool if that tuplestore is corrupt, and that means
tuplestore.c has to be more careful.

This oversight demonstrably leads to crashes in v15 and before,
if a holdable cursor fails to persist its data due to an undersized
temp_file_limit setting.  Very possibly the same thing can happen in
v16 and v17 as well, though the specific test case submitted failed
to fail there (cf. 095555daf).  The failure is accidentally dodged
as of v18 because 590b045c3 got rid of tuplestore_end's retail tuple
deletion loop.  Still, it seems unwise to permit tuplestores to become
internally inconsistent in any branch, so I've applied the same fix
across the board.

Since the known test case for this is rather expensive and doesn't
fail in recent branches, I've omitted it.

Bug: #19438
Reported-by: Dmitriy Kuzmin <kuzmin.db4@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/19438-9d37b179c56d43aa@postgresql.org
Backpatch-through: 14

diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
index f60633df241b00cb52048c7255f47b042e5d1fbc..ce4057750586e98b4d1e5330b75ce0feb6ab6020 100644 (file)
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -673,10 +673,10 @@ grow_memtuples(Tuplestorestate *state)
 
        /* OK, do it */
        FREEMEM(state, GetMemoryChunkSpace(state->memtuples));
-       state->memtupsize = newmemtupsize;
        state->memtuples = (void **)
                repalloc_huge(state->memtuples,
-                                         state->memtupsize * sizeof(void *));
+                                         newmemtupsize * sizeof(void *));
+       state->memtupsize = newmemtupsize;
        USEMEM(state, GetMemoryChunkSpace(state->memtuples));
        if (LACKMEM(state))
                elog(ERROR, "unexpected out-of-memory situation in tuplestore");
@@ -1221,7 +1221,19 @@ dumptuples(Tuplestorestate *state)
                if (i >= state->memtupcount)
                        break;
                WRITETUP(state, state->memtuples[i]);
+
+               /*
+                * Increase memtupdeleted to track the fact that we just deleted that
+                * tuple.  Think not to remove this on the grounds that we'll reset
+                * memtupdeleted to zero below.  We might not reach that if some later
+                * WRITETUP fails (e.g. due to overrunning temp_file_limit).  If so,
+                * we'd error out leaving an effectively-corrupt tuplestore, which
+                * would be quite bad if it's a persistent data structure such as a
+                * Portal's holdStore.
+                */
+               state->memtupdeleted++;
        }
+       /* Now we can reset memtupdeleted along with memtupcount */
        state->memtupdeleted = 0;
        state->memtupcount = 0;
 }
@@ -1408,8 +1420,10 @@ tuplestore_trim(Tuplestorestate *state)
                FREEMEM(state, GetMemoryChunkSpace(state->memtuples[i]));
                pfree(state->memtuples[i]);
                state->memtuples[i] = NULL;
+               /* As in dumptuples(), increment memtupdeleted synchronously */
+               state->memtupdeleted++;
        }
-       state->memtupdeleted = nremove;
+       Assert(state->memtupdeleted == nremove);
 
        /* mark tuplestore as truncated (used for Assert crosschecks only) */
        state->truncated = true;