From 25bf7f8b9b3ce0f04b498988bb98d9d5cf9bad67 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 27 Mar 2009 18:30:21 +0000 Subject: [PATCH] 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. --- src/backend/executor/execQual.c | 4 ++-- src/backend/executor/functions.c | 6 +++--- src/backend/executor/nodeCtescan.c | 8 ++++++-- src/backend/executor/nodeFunctionscan.c | 3 ++- src/backend/executor/nodeMaterial.c | 4 ++-- src/backend/executor/nodeWindowAgg.c | 15 +++++++++------ src/backend/executor/nodeWorktablescan.c | 8 ++++++-- src/backend/tcop/pquery.c | 5 +++-- src/backend/utils/sort/tuplestore.c | 21 ++++++++++++++++++--- src/include/utils/tuplestore.h | 4 ++-- 10 files changed, 53 insertions(+), 25 deletions(-) diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index a1e2589162..d66bf4da82 100644 --- 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 a8673b1f60..f71807835f 100644 --- 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 56fc15dccc..67589908ce 100644 --- 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 4bb12e3a77..3f34e7c835 100644 --- 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 111bb81ddc..88511d471d 100644 --- 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 7c3733a055..263cb0a78c 100644 --- 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 aaac9d19a8..24fd2c5f73 100644 --- 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 062881858c..2521ee010a 100644 --- 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 6bfeed1089..10afc4851b 100644 --- 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 adf6104710..2883ae75a4 100644 --- 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); -- GitLab