Redesign query-snapshot timing so that volatile functions in READ COMMITTED

mode see a fresh snapshot for each command in the function, rather than
using the latest interactive command's snapshot.  Also, suppress fresh
snapshots as well as CommandCounterIncrement inside STABLE and IMMUTABLE
functions, instead using the snapshot taken for the most closely nested
regular query.  (This behavior is only sane for read-only functions, so
the patch also enforces that such functions contain only SELECT commands.)
As per my proposal of 6-Sep-2004; I note that I floated essentially the
same proposal on 19-Jun-2002, but that discussion tailed off without any
action.  Since 8.0 seems like the right place to be taking possibly
nontrivial backwards compatibility hits, let's get it done now.

4 files changed, 170 insertions, 34 deletions

diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c
index 165b46475cf..66edf545d9c 100644
--- a/src/backend/tcop/fastpath.c
+++ b/src/backend/tcop/fastpath.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/tcop/fastpath.c,v 1.75 2004/08/29 05:06:49 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/tcop/fastpath.c,v 1.76 2004/09/13 20:07:05 tgl Exp $
  *
  * NOTES
  *	  This cruft is the server side of PQfn.
@@ -334,11 +334,6 @@ HandleFunctionRequest(StringInfo msgBuf)
 					   get_func_name(fid));
 
 	/*
-	 * Set up a query snapshot in case function needs one.
-	 */
-	SetQuerySnapshot();
-
-	/*
 	 * Prepare function call info block and insert arguments.
 	 */
 	MemSet(&fcinfo, 0, sizeof(fcinfo));
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 50364bd79d3..85fcc49c839 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.431 2004/09/10 18:39:59 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.432 2004/09/13 20:07:05 tgl Exp $
  *
  * NOTES
  *	  this is the "main" module of the postgres backend and
@@ -700,7 +700,7 @@ pg_plan_queries(List *querytrees, ParamListInfo boundParams,
 		{
 			if (needSnapshot)
 			{
-				SetQuerySnapshot();
+				ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
 				needSnapshot = false;
 			}
 			plan = pg_plan_query(query, boundParams);
@@ -883,7 +883,7 @@ exec_simple_query(const char *query_string)
 		/*
 		 * Start the portal.  No parameters here.
 		 */
-		PortalStart(portal, NULL);
+		PortalStart(portal, NULL, InvalidSnapshot);
 
 		/*
 		 * Select the appropriate output format: text unless we are doing
@@ -1539,7 +1539,7 @@ exec_bind_message(StringInfo input_message)
 					  pstmt->plan_list,
 					  pstmt->context);
 
-	PortalStart(portal, params);
+	PortalStart(portal, params, InvalidSnapshot);
 
 	/*
 	 * Apply the result format requests to the portal.
@@ -3027,6 +3027,9 @@ PostgresMain(int argc, char *argv[], const char *username)
 				/* switch back to message context */
 				MemoryContextSwitchTo(MessageContext);
 
+				/* set snapshot in case function needs one */
+				ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
+
 				if (HandleFunctionRequest(&input_message) == EOF)
 				{
 					/* lost frontend connection during F message input */
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index fca98fc0fa5..5a1c8b4867b 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.86 2004/09/10 18:40:00 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/tcop/pquery.c,v 1.87 2004/09/13 20:07:05 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -32,6 +32,11 @@
 Portal		ActivePortal = NULL;
 
 
+static void ProcessQuery(Query *parsetree,
+			 Plan *plan,
+			 ParamListInfo params,
+			 DestReceiver *dest,
+			 char *completionTag);
 static uint32 RunFromStore(Portal portal, ScanDirection direction, long count,
 			 DestReceiver *dest);
 static long PortalRunSelect(Portal portal, bool forward, long count,
@@ -54,6 +59,8 @@ static void DoPortalRewind(Portal portal);
 QueryDesc *
 CreateQueryDesc(Query *parsetree,
 				Plan *plantree,
+				Snapshot snapshot,
+				Snapshot crosscheck_snapshot,
 				DestReceiver *dest,
 				ParamListInfo params,
 				bool doInstrument)
@@ -63,6 +70,8 @@ CreateQueryDesc(Query *parsetree,
 	qd->operation = parsetree->commandType;		/* operation */
 	qd->parsetree = parsetree;	/* parse tree */
 	qd->plantree = plantree;	/* plan */
+	qd->snapshot = snapshot;	/* snapshot */
+	qd->crosscheck_snapshot = crosscheck_snapshot; /* RI check snapshot */
 	qd->dest = dest;			/* output dest */
 	qd->params = params;		/* parameter values passed into query */
 	qd->doInstrument = doInstrument;	/* instrumentation wanted? */
@@ -90,7 +99,7 @@ FreeQueryDesc(QueryDesc *qdesc)
 
 /*
  * ProcessQuery
- *		Execute a single query
+ *		Execute a single plannable query within a PORTAL_MULTI_QUERY portal
  *
  *	parsetree: the query tree
  *	plan: the plan tree for the query
@@ -104,7 +113,7 @@ FreeQueryDesc(QueryDesc *qdesc)
  * Must be called in a memory context that will be reset or deleted on
  * error; otherwise the executor's memory usage will be leaked.
  */
-void
+static void
 ProcessQuery(Query *parsetree,
 			 Plan *plan,
 			 ParamListInfo params,
@@ -114,6 +123,9 @@ ProcessQuery(Query *parsetree,
 	int			operation = parsetree->commandType;
 	QueryDesc  *queryDesc;
 
+	ereport(DEBUG3,
+			(errmsg_internal("ProcessQuery")));
+
 	/*
 	 * Check for special-case destinations
 	 */
@@ -133,9 +145,17 @@ ProcessQuery(Query *parsetree,
 	}
 
 	/*
+	 * Must always set snapshot for plannable queries.  Note we assume
+	 * that caller will take care of restoring ActiveSnapshot on exit/error.
+	 */
+	ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
+
+	/*
 	 * Create the QueryDesc object
 	 */
-	queryDesc = CreateQueryDesc(parsetree, plan, dest, params, false);
+	queryDesc = CreateQueryDesc(parsetree, plan,
+								ActiveSnapshot, InvalidSnapshot,
+								dest, params, false);
 
 	/*
 	 * Set up to collect AFTER triggers
@@ -145,7 +165,7 @@ ProcessQuery(Query *parsetree,
 	/*
 	 * Call ExecStart to prepare the plan for execution
 	 */
-	ExecutorStart(queryDesc, false, false);
+	ExecutorStart(queryDesc, false);
 
 	/*
 	 * Run the plan to completion.
@@ -195,6 +215,9 @@ ProcessQuery(Query *parsetree,
 	AfterTriggerEndQuery();
 
 	FreeQueryDesc(queryDesc);
+
+	FreeSnapshot(ActiveSnapshot);
+	ActiveSnapshot = NULL;
 }
 
 /*
@@ -238,13 +261,19 @@ ChoosePortalStrategy(List *parseTrees)
  * the query, they must be passed in here (caller is responsible for
  * giving them appropriate lifetime).
  *
+ * 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)
+PortalStart(Portal portal, ParamListInfo params, Snapshot snapshot)
 {
 	Portal		saveActivePortal;
+	Snapshot	saveActiveSnapshot;
 	ResourceOwner saveResourceOwner;
 	MemoryContext savePortalContext;
 	MemoryContext oldContext;
@@ -259,11 +288,13 @@ PortalStart(Portal portal, ParamListInfo params)
 	 * QueryContext?)
 	 */
 	saveActivePortal = ActivePortal;
+	saveActiveSnapshot = ActiveSnapshot;
 	saveResourceOwner = CurrentResourceOwner;
 	savePortalContext = PortalContext;
 	PG_TRY();
 	{
 		ActivePortal = portal;
+		ActiveSnapshot = NULL;				/* will be set later */
 		CurrentResourceOwner = portal->resowner;
 		PortalContext = PortalGetHeapMemory(portal);
 
@@ -285,9 +316,13 @@ PortalStart(Portal portal, ParamListInfo params)
 			case PORTAL_ONE_SELECT:
 
 				/*
-				 * Must set query snapshot before starting executor.
+				 * Must set snapshot before starting executor.  Be sure to
+				 * copy it into the portal's context.
 				 */
-				SetQuerySnapshot();
+				if (snapshot)
+					ActiveSnapshot = CopySnapshot(snapshot);
+				else
+					ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
 
 				/*
 				 * Create QueryDesc in portal's context; for the moment,
@@ -295,6 +330,8 @@ PortalStart(Portal portal, ParamListInfo params)
 				 */
 				queryDesc = CreateQueryDesc((Query *) linitial(portal->parseTrees),
 									(Plan *) linitial(portal->planTrees),
+											ActiveSnapshot,
+											InvalidSnapshot,
 											None_Receiver,
 											params,
 											false);
@@ -309,7 +346,7 @@ PortalStart(Portal portal, ParamListInfo params)
 				/*
 				 * Call ExecStart to prepare the plan for execution
 				 */
-				ExecutorStart(queryDesc, false, false);
+				ExecutorStart(queryDesc, false);
 
 				/*
 				 * This tells PortalCleanup to shut down the executor
@@ -333,8 +370,8 @@ PortalStart(Portal portal, ParamListInfo params)
 			case PORTAL_UTIL_SELECT:
 
 				/*
-				 * We don't set query snapshot here, because
-				 * PortalRunUtility will take care of it.
+				 * We don't set snapshot here, because
+				 * PortalRunUtility will take care of it if needed.
 				 */
 				portal->tupDesc =
 					UtilityTupleDescriptor(((Query *) linitial(portal->parseTrees))->utilityStmt);
@@ -361,6 +398,7 @@ PortalStart(Portal portal, ParamListInfo params)
 
 		/* Restore global vars and propagate error */
 		ActivePortal = saveActivePortal;
+		ActiveSnapshot = saveActiveSnapshot;
 		CurrentResourceOwner = saveResourceOwner;
 		PortalContext = savePortalContext;
 
@@ -371,6 +409,7 @@ PortalStart(Portal portal, ParamListInfo params)
 	MemoryContextSwitchTo(oldContext);
 
 	ActivePortal = saveActivePortal;
+	ActiveSnapshot = saveActiveSnapshot;
 	CurrentResourceOwner = saveResourceOwner;
 	PortalContext = savePortalContext;
 
@@ -453,6 +492,7 @@ PortalRun(Portal portal, long count,
 {
 	bool		result;
 	Portal		saveActivePortal;
+	Snapshot	saveActiveSnapshot;
 	ResourceOwner saveResourceOwner;
 	MemoryContext savePortalContext;
 	MemoryContext saveQueryContext;
@@ -485,12 +525,14 @@ PortalRun(Portal portal, long count,
 	 * Set up global portal context pointers.
 	 */
 	saveActivePortal = ActivePortal;
+	saveActiveSnapshot = ActiveSnapshot;
 	saveResourceOwner = CurrentResourceOwner;
 	savePortalContext = PortalContext;
 	saveQueryContext = QueryContext;
 	PG_TRY();
 	{
 		ActivePortal = portal;
+		ActiveSnapshot = NULL;				/* will be set later */
 		CurrentResourceOwner = portal->resowner;
 		PortalContext = PortalGetHeapMemory(portal);
 		QueryContext = portal->queryContext;
@@ -579,6 +621,7 @@ PortalRun(Portal portal, long count,
 
 		/* Restore global vars and propagate error */
 		ActivePortal = saveActivePortal;
+		ActiveSnapshot = saveActiveSnapshot;
 		CurrentResourceOwner = saveResourceOwner;
 		PortalContext = savePortalContext;
 		QueryContext = saveQueryContext;
@@ -590,6 +633,7 @@ PortalRun(Portal portal, long count,
 	MemoryContextSwitchTo(oldContext);
 
 	ActivePortal = saveActivePortal;
+	ActiveSnapshot = saveActiveSnapshot;
 	CurrentResourceOwner = saveResourceOwner;
 	PortalContext = savePortalContext;
 	QueryContext = saveQueryContext;
@@ -670,6 +714,7 @@ PortalRunSelect(Portal portal,
 			nprocessed = RunFromStore(portal, direction, count, dest);
 		else
 		{
+			ActiveSnapshot = queryDesc->snapshot;
 			ExecutorRun(queryDesc, direction, count);
 			nprocessed = queryDesc->estate->es_processed;
 		}
@@ -711,6 +756,7 @@ PortalRunSelect(Portal portal,
 			nprocessed = RunFromStore(portal, direction, count, dest);
 		else
 		{
+			ActiveSnapshot = queryDesc->snapshot;
 			ExecutorRun(queryDesc, direction, count);
 			nprocessed = queryDesc->estate->es_processed;
 		}
@@ -834,6 +880,9 @@ PortalRunUtility(Portal portal, Query *query,
 	 * the database --- if, say, it has to update an index with
 	 * expressions that invoke user-defined functions, then it had better
 	 * have a snapshot.
+	 *
+	 * Note we assume that caller will take care of restoring ActiveSnapshot
+	 * on exit/error.
 	 */
 	if (!(IsA(utilityStmt, TransactionStmt) ||
 		  IsA(utilityStmt, LockStmt) ||
@@ -847,7 +896,9 @@ PortalRunUtility(Portal portal, Query *query,
 		  IsA(utilityStmt, NotifyStmt) ||
 		  IsA(utilityStmt, UnlistenStmt) ||
 		  IsA(utilityStmt, CheckPointStmt)))
-		SetQuerySnapshot();
+		ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
+	else
+		ActiveSnapshot = NULL;
 
 	if (query->canSetTag)
 	{
@@ -864,6 +915,10 @@ PortalRunUtility(Portal portal, Query *query,
 
 	/* Some utility statements may change context on us */
 	MemoryContextSwitchTo(PortalGetHeapMemory(portal));
+
+	if (ActiveSnapshot)
+		FreeSnapshot(ActiveSnapshot);
+	ActiveSnapshot = NULL;
 }
 
 /*
@@ -924,15 +979,6 @@ PortalRunMulti(Portal portal,
 			/*
 			 * process a plannable query.
 			 */
-			ereport(DEBUG3,
-					(errmsg_internal("ProcessQuery")));
-
-			/* Must always set snapshot for plannable queries */
-			SetQuerySnapshot();
-
-			/*
-			 * execute the plan
-			 */
 			if (log_executor_stats)
 				ResetUsage();
 
@@ -1005,6 +1051,7 @@ PortalRunFetch(Portal portal,
 {
 	long		result;
 	Portal		saveActivePortal;
+	Snapshot	saveActiveSnapshot;
 	ResourceOwner saveResourceOwner;
 	MemoryContext savePortalContext;
 	MemoryContext saveQueryContext;
@@ -1025,12 +1072,14 @@ PortalRunFetch(Portal portal,
 	 * Set up global portal context pointers.
 	 */
 	saveActivePortal = ActivePortal;
+	saveActiveSnapshot = ActiveSnapshot;
 	saveResourceOwner = CurrentResourceOwner;
 	savePortalContext = PortalContext;
 	saveQueryContext = QueryContext;
 	PG_TRY();
 	{
 		ActivePortal = portal;
+		ActiveSnapshot = NULL;				/* will be set later */
 		CurrentResourceOwner = portal->resowner;
 		PortalContext = PortalGetHeapMemory(portal);
 		QueryContext = portal->queryContext;
@@ -1056,6 +1105,7 @@ PortalRunFetch(Portal portal,
 
 		/* Restore global vars and propagate error */
 		ActivePortal = saveActivePortal;
+		ActiveSnapshot = saveActiveSnapshot;
 		CurrentResourceOwner = saveResourceOwner;
 		PortalContext = savePortalContext;
 		QueryContext = saveQueryContext;
@@ -1070,6 +1120,7 @@ PortalRunFetch(Portal portal,
 	portal->status = PORTAL_READY;
 
 	ActivePortal = saveActivePortal;
+	ActiveSnapshot = saveActiveSnapshot;
 	CurrentResourceOwner = saveResourceOwner;
 	PortalContext = savePortalContext;
 	QueryContext = saveQueryContext;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6aec17bf9a5..d44e986e3c1 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/tcop/utility.c,v 1.229 2004/09/10 18:40:00 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/tcop/utility.c,v 1.230 2004/09/13 20:07:06 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -222,6 +222,46 @@ CheckRelationOwnership(RangeVar *rel, bool noCatalogs)
 }
 
 
+/*
+ * QueryIsReadOnly: is an analyzed/rewritten query read-only?
+ *
+ * This is a much stricter test than we apply for XactReadOnly mode;
+ * the query must be *in truth* read-only, because the caller wishes
+ * not to do CommandCounterIncrement for it.
+ */
+bool
+QueryIsReadOnly(Query *parsetree)
+{
+	switch (parsetree->commandType)
+	{
+		case CMD_SELECT:
+			if (parsetree->into != NULL)
+				return false;					/* SELECT INTO */
+			else if (parsetree->rowMarks != NIL)
+				return false;					/* SELECT FOR UPDATE */
+			else
+				return true;
+		case CMD_UPDATE:
+		case CMD_INSERT:
+		case CMD_DELETE:
+			return false;
+		case CMD_UTILITY:
+			/* For now, treat all utility commands as read/write */
+			return false;
+		default:
+			elog(WARNING, "unrecognized commandType: %d",
+				 (int) parsetree->commandType);
+			break;
+	}
+	return false;
+}
+
+/*
+ * check_xact_readonly: is a utility command read-only?
+ *
+ * Here we use the loose rules of XactReadOnly mode: no permanent effects
+ * on the database are allowed.
+ */
 static void
 check_xact_readonly(Node *parsetree)
 {
@@ -299,8 +339,7 @@ check_xact_readonly(Node *parsetree)
  *	completionTag: points to a buffer of size COMPLETION_TAG_BUFSIZE
  *		in which to store a command completion status string.
  *
- * completionTag is only set nonempty if we want to return a nondefault
- * status (currently, only used for MOVE/FETCH).
+ * completionTag is only set nonempty if we want to return a nondefault status.
  *
  * completionTag may be NULL if caller doesn't want a status string.
  */
@@ -1586,3 +1625,51 @@ CreateCommandTag(Node *parsetree)
 
 	return tag;
 }
+
+/*
+ * CreateQueryTag
+ *		utility to get a string representation of a Query operation.
+ *
+ * This is exactly like CreateCommandTag, except it works on a Query
+ * that has already been through parse analysis (and possibly further).
+ */
+const char *
+CreateQueryTag(Query *parsetree)
+{
+	const char *tag;
+
+	switch (parsetree->commandType)
+	{
+		case CMD_SELECT:
+			/*
+			 * We take a little extra care here so that the result will
+			 * be useful for complaints about read-only statements
+			 */
+			if (parsetree->into != NULL)
+				tag = "SELECT INTO";
+			else if (parsetree->rowMarks != NIL)
+				tag = "SELECT FOR UPDATE";
+			else
+				tag = "SELECT";
+			break;
+		case CMD_UPDATE:
+			tag = "UPDATE";
+			break;
+		case CMD_INSERT:
+			tag = "INSERT";
+			break;
+		case CMD_DELETE:
+			tag = "DELETE";
+			break;
+		case CMD_UTILITY:
+			tag = CreateCommandTag(parsetree->utilityStmt);
+			break;
+		default:
+			elog(WARNING, "unrecognized commandType: %d",
+				 (int) parsetree->commandType);
+			tag = "???";
+			break;
+	}
+
+	return tag;
+}