Revert patch for taking fewer snapshots.

tglsfdc · tglsfdc · commit 786afc1ce531 · 2012-11-26T15:55:51.000-05:00
This reverts commit d573e23, "Take fewer snapshots". While that seemed like a good idea at the time, it caused execution to use a snapshot that had been acquired before locking any of the tables mentioned in the query. This created user-visible anomalies that were not present in any prior release of Postgres, as reported by Tomas Vondra. While this whole area could do with a redesign (since there are related cases that have anomalies anyway), it doesn't seem likely that any future patch would be reasonably back-patchable; and we don't want 9.2 to exhibit a behavior that's subtly unlike either past or future releases. Hence, revert to prior code while we rethink the problem.
diff --git a/doc/src/sgml/release-9.2.sgml b/doc/src/sgml/release-9.2.sgml
@@ -809,13 +809,6 @@
        </para>
       </listitem>
 
-      <listitem>
-       <para>
-        Take fewer <acronym>MVCC</acronym> snapshots
-        (Robert Haas)
-       </para>
-      </listitem>
-
       <listitem>
        <para>
         Make the number of CLOG buffers scale based on <link
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
@@ -121,7 +121,7 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
 	/*
 	 * Start execution, inserting parameters if any.
 	 */
-	PortalStart(portal, params, 0, true);
+	PortalStart(portal, params, 0, GetActiveSnapshot());
 
 	Assert(portal->strategy == PORTAL_ONE_SELECT);
 
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
@@ -289,7 +289,7 @@ ExecuteQuery(ExecuteStmt *stmt, IntoClause *intoClause,
 	/*
 	 * Run the portal as appropriate.
 	 */
-	PortalStart(portal, paramLI, eflags, true);
+	PortalStart(portal, paramLI, eflags, GetActiveSnapshot());
 
 	(void) PortalRun(portal, count, false, dest, dest, completionTag);
 
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
@@ -1126,6 +1126,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
 	CachedPlan *cplan;
 	List	   *stmt_list;
 	char	   *query_string;
+	Snapshot	snapshot;
 	MemoryContext oldcontext;
 	Portal		portal;
 
@@ -1268,6 +1269,15 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
 		}
 	}
 
+	/* Set up the snapshot to use. */
+	if (read_only)
+		snapshot = GetActiveSnapshot();
+	else
+	{
+		CommandCounterIncrement();
+		snapshot = GetTransactionSnapshot();
+	}
+
 	/*
 	 * If the plan has parameters, copy them into the portal.  Note that this
 	 * must be done after revalidating the plan, because in dynamic parameter
@@ -1283,13 +1293,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
 	/*
 	 * Start portal execution.
 	 */
-	if (read_only)
-		PortalStart(portal, paramLI, 0, true);
-	else
-	{
-		CommandCounterIncrement();
-		PortalStart(portal, paramLI, 0, false);
-	}
+	PortalStart(portal, paramLI, 0, snapshot);
 
 	Assert(portal->strategy != PORTAL_MULTI_QUERY);
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
@@ -974,6 +974,10 @@ exec_simple_query(const char *query_string)
 
 		plantree_list = pg_plan_queries(querytree_list, 0, NULL);
 
+		/* Done with the snapshot used for parsing/planning */
+		if (snapshot_set)
+			PopActiveSnapshot();
+
 		/* If we got a cancel signal in analysis or planning, quit */
 		CHECK_FOR_INTERRUPTS();
 
@@ -998,19 +1002,9 @@ exec_simple_query(const char *query_string)
 						  NULL);
 
 		/*
-		 * Start the portal.
-		 *
-		 * If we took a snapshot for parsing/planning, the portal may be able
-		 * to reuse it for the execution phase.  Currently, this will only
-		 * happen in PORTAL_ONE_SELECT mode.  But even if PortalStart doesn't
-		 * end up being able to do this, keeping the parse/plan snapshot
-		 * around until after we start the portal doesn't cost much.
+		 * Start the portal.  No parameters here.
 		 */
-		PortalStart(portal, NULL, 0, snapshot_set);
-
-		/* Done with the snapshot used for parsing/planning */
-		if (snapshot_set)
-			PopActiveSnapshot();
+		PortalStart(portal, NULL, 0, InvalidSnapshot);
 
 		/*
 		 * Select the appropriate output format: text unless we are doing a
@@ -1733,19 +1727,15 @@ exec_bind_message(StringInfo input_message)
 					  cplan->stmt_list,
 					  cplan);
 
-	/*
-	 * And we're ready to start portal execution.
-	 *
-	 * If we took a snapshot for parsing/planning, we'll try to reuse it for
-	 * query execution (currently, reuse will only occur if PORTAL_ONE_SELECT
-	 * mode is chosen).
-	 */
-	PortalStart(portal, params, 0, snapshot_set);
-
 	/* Done with the snapshot used for parameter I/O and parsing/planning */
 	if (snapshot_set)
 		PopActiveSnapshot();
 
+	/*
+	 * And we're ready to start portal execution.
+	 */
+	PortalStart(portal, params, 0, InvalidSnapshot);
+
 	/*
 	 * Apply the result format requests to the portal.
 	 */
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
@@ -447,18 +447,17 @@ FetchStatementTargetList(Node *stmt)
  * currently only honored for PORTAL_ONE_SELECT portals).  Most callers
  * should simply pass zero.
  *
- * The use_active_snapshot parameter is currently used only for
- * PORTAL_ONE_SELECT portals.  If it is true, the active snapshot will
- * be used when starting up the executor; if false, a new snapshot will
- * be taken.  This is used both for cursors and to avoid taking an entirely
- * new snapshot when it isn't necessary.
+ * The caller can optionally pass a snapshot to be used; pass InvalidSnapshot
+ * for the normal behavior of setting a new snapshot.  This parameter is
+ * presently ignored for non-PORTAL_ONE_SELECT portals (it's only intended
+ * to be used for cursors).
  *
  * On return, portal is ready to accept PortalRun() calls, and the result
  * tupdesc (if any) is known.
  */
 void
 PortalStart(Portal portal, ParamListInfo params,
-			int eflags, bool use_active_snapshot)
+			int eflags, Snapshot snapshot)
 {
 	Portal		saveActivePortal;
 	ResourceOwner saveResourceOwner;
@@ -500,8 +499,8 @@ PortalStart(Portal portal, ParamListInfo params,
 			case PORTAL_ONE_SELECT:
 
 				/* Must set snapshot before starting executor. */
-				if (use_active_snapshot)
-					PushActiveSnapshot(GetActiveSnapshot());
+				if (snapshot)
+					PushActiveSnapshot(snapshot);
 				else
 					PushActiveSnapshot(GetTransactionSnapshot());
 
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
@@ -28,7 +28,7 @@ extern List *FetchPortalTargetList(Portal portal);
 extern List *FetchStatementTargetList(Node *stmt);
 
 extern void PortalStart(Portal portal, ParamListInfo params,
-			int eflags, bool use_active_snapshot);
+			int eflags, Snapshot snapshot);
 
 extern void PortalSetResultFormat(Portal portal, int nFormats,
 					  int16 *formats);
