Fix possible failures when a tuplestore switches from in-memory to on-disk

mode while callers hold pointers to in-memory tuples.  I reported this for
the case of nodeWindowAgg's primary scan tuple, but inspection of the code
shows that all of the calls in nodeWindowAgg and nodeCtescan are at risk.
For the moment, fix it with a rather brute-force approach of copying
whenever one of the at-risk callers requests a tuple.  Later we might
think of some sort of reference-count approach to reduce tuple copying.

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index a1e2589162cb55f3302347b4f79124711b1e4380..d66bf4da8223f9b08d9a3af53a2a98f582827dcf 100644 (file)
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.242 2009/03/26 22:26:06 petere Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.243 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1385,7 +1385,7 @@ restart:
    if (fcache->funcResultStore)
    {
        Assert(isDone);             /* it was provided before ... */
-       if (tuplestore_gettupleslot(fcache->funcResultStore, true,
+       if (tuplestore_gettupleslot(fcache->funcResultStore, true, false,
                                    fcache->funcResultSlot))
        {
            *isDone = ExprMultipleResult;

diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index a8673b1f6074989c7b40336ae2f6b9fd3243f027..f71807835f26ee621769db2e86f57aac13bf1f59 100644 (file)
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/functions.c,v 1.132 2009/01/02 20:42:00 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/functions.c,v 1.133 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -736,7 +736,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
            /* Re-use the junkfilter's output slot to fetch back the tuple */
            Assert(fcache->junkFilter);
            slot = fcache->junkFilter->jf_resultSlot;
-           if (!tuplestore_gettupleslot(fcache->tstore, true, slot))
+           if (!tuplestore_gettupleslot(fcache->tstore, true, false, slot))
                elog(ERROR, "failed to fetch lazy-eval tuple");
            /* Extract the result as a datum, and copy out from the slot */
            result = postquel_get_single_result(slot, fcinfo,
@@ -822,7 +822,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
        {
            /* Re-use the junkfilter's output slot to fetch back the tuple */
            slot = fcache->junkFilter->jf_resultSlot;
-           if (tuplestore_gettupleslot(fcache->tstore, true, slot))
+           if (tuplestore_gettupleslot(fcache->tstore, true, false, slot))
                result = postquel_get_single_result(slot, fcinfo,
                                                    fcache, oldcontext);
            else

diff --git a/src/backend/executor/nodeCtescan.c b/src/backend/executor/nodeCtescan.c
index 56fc15dccc73e697ab918a4c432583c8d8ad6e0f..67589908ce16efb088d3764c00f879043a208757 100644 (file)
--- a/src/backend/executor/nodeCtescan.c
+++ b/src/backend/executor/nodeCtescan.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/nodeCtescan.c,v 1.3 2009/01/01 17:23:41 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/nodeCtescan.c,v 1.4 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -71,10 +71,14 @@ CteScanNext(CteScanState *node)
 
    /*
     * If we can fetch another tuple from the tuplestore, return it.
+    *
+    * Note: we have to use copy=true in the tuplestore_gettupleslot call,
+    * because we are sharing the tuplestore with other nodes that might
+    * write into the tuplestore before we get called again.
     */
    if (!eof_tuplestore)
    {
-       if (tuplestore_gettupleslot(tuplestorestate, forward, slot))
+       if (tuplestore_gettupleslot(tuplestorestate, forward, true, slot))
            return slot;
        if (forward)
            eof_tuplestore = true;

diff --git a/src/backend/executor/nodeFunctionscan.c b/src/backend/executor/nodeFunctionscan.c
index 4bb12e3a77f4ce16147d477b75ef981160f725fc..3f34e7c835c4e580a1a9116f880bdb4ddd79fa81 100644 (file)
--- a/src/backend/executor/nodeFunctionscan.c
+++ b/src/backend/executor/nodeFunctionscan.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/nodeFunctionscan.c,v 1.50 2009/01/01 17:23:41 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/nodeFunctionscan.c,v 1.51 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -74,6 +74,7 @@ FunctionNext(FunctionScanState *node)
    slot = node->ss.ss_ScanTupleSlot;
    (void) tuplestore_gettupleslot(tuplestorestate,
                                   ScanDirectionIsForward(direction),
+                                  false,
                                   slot);
    return slot;
 }

diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c
index 111bb81ddc7fc1a5433865360622a5c734471677..88511d471d35428eee2927d09dacefc699e27d41 100644 (file)
--- a/src/backend/executor/nodeMaterial.c
+++ b/src/backend/executor/nodeMaterial.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/nodeMaterial.c,v 1.65 2009/01/01 17:23:42 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/nodeMaterial.c,v 1.66 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -104,7 +104,7 @@ ExecMaterial(MaterialState *node)
    slot = node->ss.ps.ps_ResultTupleSlot;
    if (!eof_tuplestore)
    {
-       if (tuplestore_gettupleslot(tuplestorestate, forward, slot))
+       if (tuplestore_gettupleslot(tuplestorestate, forward, false, slot))
            return slot;
        if (forward)
            eof_tuplestore = true;

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 7c3733a05530e394944122c99eaa97564e44c1e3..263cb0a78c10ede6b210cf627569ec9f8e973c8c 100644 (file)
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -27,7 +27,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/nodeWindowAgg.c,v 1.3 2009/01/01 17:23:42 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/nodeWindowAgg.c,v 1.4 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -480,7 +480,8 @@ eval_windowaggregates(WindowAggState *winstate)
            spool_tuples(winstate, winstate->aggregatedupto);
            tuplestore_select_read_pointer(winstate->buffer,
                                           winstate->agg_ptr);
-           if (!tuplestore_gettupleslot(winstate->buffer, true, agg_row_slot))
+           if (!tuplestore_gettupleslot(winstate->buffer, true, true,
+                                        agg_row_slot))
                break;          /* must be end of partition */
        }
 
@@ -1001,12 +1002,14 @@ restart:
    /*
     * Read the current row from the tuplestore, and save in ScanTupleSlot.
     * (We can't rely on the outerplan's output slot because we may have to
-    * read beyond the current row.)
+    * read beyond the current row.  Also, we have to actually copy the row
+    * out of the tuplestore, since window function evaluation might cause
+    * the tuplestore to dump its state to disk.)
     *
     * Current row must be in the tuplestore, since we spooled it above.
     */
    tuplestore_select_read_pointer(winstate->buffer, winstate->current_ptr);
-   if (!tuplestore_gettupleslot(winstate->buffer, true,
+   if (!tuplestore_gettupleslot(winstate->buffer, true, true,
                                 winstate->ss.ss_ScanTupleSlot))
        elog(ERROR, "unexpected end of tuplestore");
 
@@ -1589,14 +1592,14 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
 
    while (winobj->seekpos > pos)
    {
-       if (!tuplestore_gettupleslot(winstate->buffer, false, slot))
+       if (!tuplestore_gettupleslot(winstate->buffer, false, true, slot))
            elog(ERROR, "unexpected end of tuplestore");
        winobj->seekpos--;
    }
 
    while (winobj->seekpos < pos)
    {
-       if (!tuplestore_gettupleslot(winstate->buffer, true, slot))
+       if (!tuplestore_gettupleslot(winstate->buffer, true, true, slot))
            elog(ERROR, "unexpected end of tuplestore");
        winobj->seekpos++;
    }

diff --git a/src/backend/executor/nodeWorktablescan.c b/src/backend/executor/nodeWorktablescan.c
index aaac9d19a8cdb7084a773c78c9b1987e5b29ae6f..24fd2c5f736bface7396697c33d8b327d5d0f76a 100644 (file)
--- a/src/backend/executor/nodeWorktablescan.c
+++ b/src/backend/executor/nodeWorktablescan.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/nodeWorktablescan.c,v 1.5 2009/01/01 17:23:42 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/nodeWorktablescan.c,v 1.6 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -43,6 +43,10 @@ WorkTableScanNext(WorkTableScanState *node)
     * worktable plan node, since it cannot appear high enough in the plan
     * tree of a scrollable cursor to be exposed to a backward-scan
     * requirement.  So it's not worth expending effort to support it.
+    *
+    * Note: we are also assuming that this node is the only reader of the
+    * worktable.  Therefore, we don't need a private read pointer for the
+    * tuplestore, nor do we need to tell tuplestore_gettupleslot to copy.
     */
    estate = node->ss.ps.state;
    Assert(ScanDirectionIsForward(estate->es_direction));
@@ -53,7 +57,7 @@ WorkTableScanNext(WorkTableScanState *node)
     * Get the next tuple from tuplestore. Return NULL if no more tuples.
     */
    slot = node->ss.ss_ScanTupleSlot;
-   (void) tuplestore_gettupleslot(tuplestorestate, true, slot);
+   (void) tuplestore_gettupleslot(tuplestorestate, true, false, slot);
    return slot;
 }
 

diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 062881858c194370730d12406fddf0500cd7c1e2..2521ee010af412af2fbf23c0f6939a52ce628595 100644 (file)
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/tcop/pquery.c,v 1.129 2009/01/02 20:42:00 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/tcop/pquery.c,v 1.130 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1118,7 +1118,8 @@ RunFromStore(Portal portal, ScanDirection direction, long count,
 
            oldcontext = MemoryContextSwitchTo(portal->holdContext);
 
-           ok = tuplestore_gettupleslot(portal->holdStore, forward, slot);
+           ok = tuplestore_gettupleslot(portal->holdStore, forward, false,
+                                        slot);
 
            MemoryContextSwitchTo(oldcontext);
 

diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
index 6bfeed108960372c93b50a7e15746aff4b73b31f..10afc4851b400886bb5239eab08b2209a9938fef 100644 (file)
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -47,7 +47,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/utils/sort/tuplestore.c,v 1.46 2009/01/01 17:23:53 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/utils/sort/tuplestore.c,v 1.47 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -871,10 +871,17 @@ tuplestore_gettuple(Tuplestorestate *state, bool forward,
  *
  * If successful, put tuple in slot and return TRUE; else, clear the slot
  * and return FALSE.
+ *
+ * If copy is TRUE, the slot receives a copied tuple (allocated in current
+ * memory context) that will stay valid regardless of future manipulations of
+ * the tuplestore's state.  If copy is FALSE, the slot may just receive a
+ * pointer to a tuple held within the tuplestore.  The latter is more
+ * efficient but the slot contents may be corrupted if additional writes to
+ * the tuplestore occur.  (If using tuplestore_trim, see comments therein.)
  */
 bool
 tuplestore_gettupleslot(Tuplestorestate *state, bool forward,
-                       TupleTableSlot *slot)
+                       bool copy, TupleTableSlot *slot)
 {
    MinimalTuple tuple;
    bool        should_free;
@@ -883,6 +890,11 @@ tuplestore_gettupleslot(Tuplestorestate *state, bool forward,
 
    if (tuple)
    {
+       if (copy && !should_free)
+       {
+           tuple = heap_copy_minimal_tuple(tuple);
+           should_free = true;
+       }
        ExecStoreMinimalTuple(tuple, slot, should_free);
        return true;
    }
@@ -1107,7 +1119,10 @@ tuplestore_trim(Tuplestorestate *state)
     * since tuplestore_gettuple returns a direct pointer to our
     * internal copy of the tuple, it's likely that the caller has
     * still got the tuple just before "current" referenced in a slot.
-    * So we keep one extra tuple before the oldest "current".
+    * So we keep one extra tuple before the oldest "current".  (Strictly
+    * speaking, we could require such callers to use the "copy" flag to
+    * tuplestore_gettupleslot, but for efficiency we allow this one case
+    * to not use "copy".)
     */
    nremove = oldest - 1;
    if (nremove <= 0)

diff --git a/src/include/utils/tuplestore.h b/src/include/utils/tuplestore.h
index adf6104710c17145eefc7fed7c4bc2f2c9a52370..2883ae75a43efdf70d20117cc3bc79deb430fc71 100644 (file)
--- a/src/include/utils/tuplestore.h
+++ b/src/include/utils/tuplestore.h
@@ -24,7 +24,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/utils/tuplestore.h,v 1.28 2009/01/01 17:24:02 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/utils/tuplestore.h,v 1.29 2009/03/27 18:30:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -71,7 +71,7 @@ extern void tuplestore_trim(Tuplestorestate *state);
 extern bool tuplestore_in_memory(Tuplestorestate *state);
 
 extern bool tuplestore_gettupleslot(Tuplestorestate *state, bool forward,
-                       TupleTableSlot *slot);
+                       bool copy, TupleTableSlot *slot);
 extern bool tuplestore_advance(Tuplestorestate *state, bool forward);
 
 extern bool tuplestore_ateof(Tuplestorestate *state);