Remove SnapshotNow and HeapTupleSatisfiesNow.

We now use MVCC catalog scans, and, per discussion, have eliminated
all other remaining uses of SnapshotNow, so that we can now get rid of
it.  This will break third-party code which is still using it, which
is intentional, as we want such code to be updated to do things the
new way.

Author: robertmhaas

src/backend/catalog/index.c

@@ -1258,10 +1258,8 @@ index_constraint_create(Relation heapRelation,
 	/*
 	 * If needed, mark the index as primary and/or deferred in pg_index.
 	 *
-	 * Note: since this is a transactional update, it's unsafe against
-	 * concurrent SnapshotNow scans of pg_index.  When making an existing
-	 * index into a constraint, caller must have a table lock that prevents
-	 * concurrent table updates; if it's less than a full exclusive lock,
+	 * Note: When making an existing index into a constraint, caller must
+	 * have a table lock that prevents concurrent table updates; otherwise,
 	 * there is a risk that concurrent readers of the table will miss seeing
 	 * this index at all.
 	 */
@@ -2989,17 +2987,18 @@ validate_index_heapscan(Relation heapRelation,
  * index_set_state_flags - adjust pg_index state flags
  *
  * This is used during CREATE/DROP INDEX CONCURRENTLY to adjust the pg_index
- * flags that denote the index's state.  We must use an in-place update of
- * the pg_index tuple, because we do not have exclusive lock on the parent
- * table and so other sessions might concurrently be doing SnapshotNow scans
- * of pg_index to identify the table's indexes.  A transactional update would
- * risk somebody not seeing the index at all.  Because the update is not
+ * flags that denote the index's state.  Because the update is not
  * transactional and will not roll back on error, this must only be used as
  * the last step in a transaction that has not made any transactional catalog
  * updates!
  *
  * Note that heap_inplace_update does send a cache inval message for the
  * tuple, so other sessions will hear about the update as soon as we commit.
+ *
+ * NB: In releases prior to PostgreSQL 9.4, the use of a non-transactional
+ * update here would have been unsafe; now that MVCC rules apply even for
+ * system catalog scans, we could potentially use a transactional update here
+ * instead.
  */
 void
 index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
@@ -3207,14 +3206,6 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
 	 * be conservative.)  In this case advancing the usability horizon is
 	 * appropriate.
 	 *
-	 * Note that if we have to update the tuple, there is a risk of concurrent
-	 * transactions not seeing it during their SnapshotNow scans of pg_index.
-	 * While not especially desirable, this is safe because no such
-	 * transaction could be trying to update the table (since we have
-	 * ShareLock on it).  The worst case is that someone might transiently
-	 * fail to use the index for a query --- but it was probably unusable
-	 * before anyway, if we are updating the tuple.
-	 *
 	 * Another reason for avoiding unnecessary updates here is that while
 	 * reindexing pg_index itself, we must not try to update tuples in it.
 	 * pg_index's indexes should always have these flags in their clean state,

src/backend/catalog/pg_enum.c

@@ -462,30 +462,9 @@ AddEnumLabel(Oid enumTypeOid,
  *		Renumber existing enum elements to have sort positions 1..n.
  *
  * We avoid doing this unless absolutely necessary; in most installations
- * it will never happen.  The reason is that updating existing pg_enum
- * entries creates hazards for other backends that are concurrently reading
- * pg_enum with SnapshotNow semantics.	A concurrent SnapshotNow scan could
- * see both old and new versions of an updated row as valid, or neither of
- * them, if the commit happens between scanning the two versions.  It's
- * also quite likely for a concurrent scan to see an inconsistent set of
- * rows (some members updated, some not).
- *
- * We can avoid these risks by reading pg_enum with an MVCC snapshot
- * instead of SnapshotNow, but that forecloses use of the syscaches.
- * We therefore make the following choices:
- *
- * 1. Any code that is interested in the enumsortorder values MUST read
- * pg_enum with an MVCC snapshot, or else acquire lock on the enum type
- * to prevent concurrent execution of AddEnumLabel().  The risk of
- * seeing inconsistent values of enumsortorder is too high otherwise.
- *
- * 2. Code that is not examining enumsortorder can use a syscache
- * (for example, enum_in and enum_out do so).  The worst that can happen
- * is a transient failure to find any valid value of the row.  This is
- * judged acceptable in view of the infrequency of use of RenumberEnumType.
- *
- * XXX. Now that we have MVCC catalog scans, the above reasoning is no longer
- * correct.  Should we revisit any decisions here?
+ * it will never happen.  Before we had MVCC catalog scans, this function
+ * posed various concurrency hazards.  It no longer does, but it's still
+ * extra work, so we don't do it unless it's needed.
  */
 static void
 RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems)

src/backend/commands/cluster.c

@@ -476,16 +476,6 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
  * mark_index_clustered: mark the specified index as the one clustered on
  *
  * With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
- *
- * Note: we do transactional updates of the pg_index rows, which are unsafe
- * against concurrent SnapshotNow scans of pg_index.  Therefore this is unsafe
- * to execute with less than full exclusive lock on the parent table;
- * otherwise concurrent executions of RelationGetIndexList could miss indexes.
- *
- * XXX: Now that we have MVCC catalog access, SnapshotNow scans of pg_index
- * shouldn't be common enough to worry about.  The above comment needs
- * to be updated, and it may be possible to simplify the logic here in other
- * ways also.
  */
 void
 mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)

src/backend/utils/time/tqual.c

@@ -30,10 +30,8 @@
  *
  *	 HeapTupleSatisfiesMVCC()
  *		  visible to supplied snapshot, excludes current command
- *	 HeapTupleSatisfiesNow()
- *		  visible to instant snapshot, excludes current command
  *	 HeapTupleSatisfiesUpdate()
- *		  like HeapTupleSatisfiesNow(), but with user-supplied command
+ *		  visible to instant snapshot, with user-supplied command
  *		  counter and more complex result
  *	 HeapTupleSatisfiesSelf()
  *		  visible to instant snapshot and current command
@@ -68,7 +66,6 @@
 
 
 /* Static variables representing various special snapshot semantics */
-SnapshotData SnapshotNowData = {HeapTupleSatisfiesNow};
 SnapshotData SnapshotSelfData = {HeapTupleSatisfiesSelf};
 SnapshotData SnapshotAnyData = {HeapTupleSatisfiesAny};
 SnapshotData SnapshotToastData = {HeapTupleSatisfiesToast};
@@ -323,212 +320,6 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
 	return false;
 }
 
-/*
- * HeapTupleSatisfiesNow
- *		True iff heap tuple is valid "now".
- *
- *	Here, we consider the effects of:
- *		all committed transactions (as of the current instant)
- *		previous commands of this transaction
- *
- * Note we do _not_ include changes made by the current command.  This
- * solves the "Halloween problem" wherein an UPDATE might try to re-update
- * its own output tuples, http://en.wikipedia.org/wiki/Halloween_Problem.
- *
- * Note:
- *		Assumes heap tuple is valid.
- *
- * The satisfaction of "now" requires the following:
- *
- * ((Xmin == my-transaction &&				inserted by the current transaction
- *	 Cmin < my-command &&					before this command, and
- *	 (Xmax is null ||						the row has not been deleted, or
- *	  (Xmax == my-transaction &&			it was deleted by the current transaction
- *	   Cmax >= my-command)))				but not before this command,
- * ||										or
- *	(Xmin is committed &&					the row was inserted by a committed transaction, and
- *		(Xmax is null ||					the row has not been deleted, or
- *		 (Xmax == my-transaction &&			the row is being deleted by this transaction
- *		  Cmax >= my-command) ||			but it's not deleted "yet", or
- *		 (Xmax != my-transaction &&			the row was deleted by another transaction
- *		  Xmax is not committed))))			that has not been committed
- *
- */
-bool
-HeapTupleSatisfiesNow(HeapTuple htup, Snapshot snapshot, Buffer buffer)
-{
-	HeapTupleHeader tuple = htup->t_data;
-	Assert(ItemPointerIsValid(&htup->t_self));
-	Assert(htup->t_tableOid != InvalidOid);
-
-	if (!(tuple->t_infomask & HEAP_XMIN_COMMITTED))
-	{
-		if (tuple->t_infomask & HEAP_XMIN_INVALID)
-			return false;
-
-		/* Used by pre-9.0 binary upgrades */
-		if (tuple->t_infomask & HEAP_MOVED_OFF)
-		{
-			TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
-			if (TransactionIdIsCurrentTransactionId(xvac))
-				return false;
-			if (!TransactionIdIsInProgress(xvac))
-			{
-				if (TransactionIdDidCommit(xvac))
-				{
-					SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
-								InvalidTransactionId);
-					return false;
-				}
-				SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
-							InvalidTransactionId);
-			}
-		}
-		/* Used by pre-9.0 binary upgrades */
-		else if (tuple->t_infomask & HEAP_MOVED_IN)
-		{
-			TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
-
-			if (!TransactionIdIsCurrentTransactionId(xvac))
-			{
-				if (TransactionIdIsInProgress(xvac))
-					return false;
-				if (TransactionIdDidCommit(xvac))
-					SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
-								InvalidTransactionId);
-				else
-				{
-					SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
-								InvalidTransactionId);
-					return false;
-				}
-			}
-		}
-		else if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple)))
-		{
-			if (HeapTupleHeaderGetCmin(tuple) >= GetCurrentCommandId(false))
-				return false;	/* inserted after scan started */
-
-			if (tuple->t_infomask & HEAP_XMAX_INVALID)	/* xid invalid */
-				return true;
-
-			if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))	/* not deleter */
-				return true;
-
-			if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
-			{
-				TransactionId xmax;
-
-				xmax = HeapTupleGetUpdateXid(tuple);
-				if (!TransactionIdIsValid(xmax))
-					return true;
-
-				/* updating subtransaction must have aborted */
-				if (!TransactionIdIsCurrentTransactionId(xmax))
-					return true;
-				else
-					return false;
-			}
-
-			if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmax(tuple)))
-			{
-				/* deleting subtransaction must have aborted */
-				SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
-							InvalidTransactionId);
-				return true;
-			}
-
-			if (HeapTupleHeaderGetCmax(tuple) >= GetCurrentCommandId(false))
-				return true;	/* deleted after scan started */
-			else
-				return false;	/* deleted before scan started */
-		}
-		else if (TransactionIdIsInProgress(HeapTupleHeaderGetXmin(tuple)))
-			return false;
-		else if (TransactionIdDidCommit(HeapTupleHeaderGetXmin(tuple)))
-			SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
-						HeapTupleHeaderGetXmin(tuple));
-		else
-		{
-			/* it must have aborted or crashed */
-			SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
-						InvalidTransactionId);
-			return false;
-		}
-	}
-
-	/* by here, the inserting transaction has committed */
-
-	if (tuple->t_infomask & HEAP_XMAX_INVALID)	/* xid invalid or aborted */
-		return true;
-
-	if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
-	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
-		return false;
-	}
-
-	if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
-	{
-		TransactionId xmax;
-
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
-
-		xmax = HeapTupleGetUpdateXid(tuple);
-		if (!TransactionIdIsValid(xmax))
-			return true;
-		if (TransactionIdIsCurrentTransactionId(xmax))
-		{
-			if (HeapTupleHeaderGetCmax(tuple) >= GetCurrentCommandId(false))
-				return true;	/* deleted after scan started */
-			else
-				return false;	/* deleted before scan started */
-		}
-		if (TransactionIdIsInProgress(xmax))
-			return true;
-		if (TransactionIdDidCommit(xmax))
-			return false;
-		return true;
-	}
-
-	if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetRawXmax(tuple)))
-	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
-		if (HeapTupleHeaderGetCmax(tuple) >= GetCurrentCommandId(false))
-			return true;		/* deleted after scan started */
-		else
-			return false;		/* deleted before scan started */
-	}
-
-	if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
-		return true;
-
-	if (!TransactionIdDidCommit(HeapTupleHeaderGetRawXmax(tuple)))
-	{
-		/* it must have aborted or crashed */
-		SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
-					InvalidTransactionId);
-		return true;
-	}
-
-	/* xmax transaction committed */
-
-	if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-	{
-		SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
-					InvalidTransactionId);
-		return true;
-	}
-
-	SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED,
-				HeapTupleHeaderGetRawXmax(tuple));
-	return false;
-}
-
 /*
  * HeapTupleSatisfiesAny
  *		Dummy "satisfies" routine: any tuple satisfies SnapshotAny.
@@ -614,10 +405,10 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
 /*
  * HeapTupleSatisfiesUpdate
  *
- *	Same logic as HeapTupleSatisfiesNow, but returns a more detailed result
- *	code, since UPDATE needs to know more than "is it visible?".  Also,
- *	tuples of my own xact are tested against the passed CommandId not
- *	CurrentCommandId.
+ *	This function returns a more detailed result code than most of the
+ *	functions in this file, since UPDATE needs to know more than "is it
+ *	visible?".  It also allows for user-supplied CommandId rather than
+ *	relying on CurrentCommandId.
  *
  *	The possible return codes are:
  *
@@ -1051,10 +842,6 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
  *		transactions started after the snapshot was taken
  *		changes made by the current command
  *
- * This is the same as HeapTupleSatisfiesNow, except that transactions that
- * were in progress or as yet unstarted when the snapshot was taken will
- * be treated as uncommitted, even if they have committed by now.
- *
  * (Notice, however, that the tuple status hint bits will be updated on the
  * basis of the true state of the transaction, even if we then pretend we
  * can't see it.)

src/include/utils/tqual.h

@@ -19,12 +19,10 @@
 
 
 /* Static variables representing various special snapshot semantics */
-extern PGDLLIMPORT SnapshotData SnapshotNowData;
 extern PGDLLIMPORT SnapshotData SnapshotSelfData;
 extern PGDLLIMPORT SnapshotData SnapshotAnyData;
 extern PGDLLIMPORT SnapshotData SnapshotToastData;
 
-#define SnapshotNow			(&SnapshotNowData)
 #define SnapshotSelf		(&SnapshotSelfData)
 #define SnapshotAny			(&SnapshotAnyData)
 #define SnapshotToast		(&SnapshotToastData)
@@ -67,8 +65,6 @@ typedef enum
 /* These are the "satisfies" test routines for the various snapshot types */
 extern bool HeapTupleSatisfiesMVCC(HeapTuple htup,
 					   Snapshot snapshot, Buffer buffer);
-extern bool HeapTupleSatisfiesNow(HeapTuple htup,
-					  Snapshot snapshot, Buffer buffer);
 extern bool HeapTupleSatisfiesSelf(HeapTuple htup,
 					   Snapshot snapshot, Buffer buffer);
 extern bool HeapTupleSatisfiesAny(HeapTuple htup,