Use an MVCC snapshot, rather than SnapshotNow, for catalog scans.

SnapshotNow scans have the undesirable property that, in the face of
concurrent updates, the scan can fail to see either the old or the new
versions of the row.  In many cases, we work around this by requiring
DDL operations to hold AccessExclusiveLock on the object being
modified; in some cases, the existing locking is inadequate and random
failures occur as a result.  This commit doesn't change anything
related to locking, but will hopefully pave the way to allowing lock
strength reductions in the future.

The major issue has held us back from making this change in the past
is that taking an MVCC snapshot is significantly more expensive than
using a static special snapshot such as SnapshotNow.  However, testing
of various worst-case scenarios reveals that this problem is not
severe except under fairly extreme workloads.  To mitigate those
problems, we avoid retaking the MVCC snapshot for each new scan;
instead, we take a new snapshot only when invalidation messages have
been processed.  The catcache machinery already requires that
invalidation messages be sent before releasing the related heavyweight
lock; else other backends might rely on locally-cached data rather
than scanning the catalog at all.  Thus, making snapshot reuse
dependent on the same guarantees shouldn't break anything that wasn't
already subtly broken.

Patch by me.  Review by Michael Paquier and Andres Freund.

Author: robertmhaas

contrib/dblink/dblink.c

@@ -2046,7 +2046,7 @@ get_pkey_attnames(Relation rel, int16 *numatts)
 				ObjectIdGetDatum(RelationGetRelid(rel)));
 
 	scan = systable_beginscan(indexRelation, IndexIndrelidIndexId, true,
-							  SnapshotNow, 1, &skey);
+							  NULL, 1, &skey);
 
 	while (HeapTupleIsValid(indexTuple = systable_getnext(scan)))
 	{

contrib/sepgsql/label.c

@@ -727,7 +727,7 @@ exec_object_restorecon(struct selabel_handle * sehnd, Oid catalogId)
 	rel = heap_open(catalogId, AccessShareLock);
 
 	sscan = systable_beginscan(rel, InvalidOid, false,
-							   SnapshotNow, 0, NULL);
+							   NULL, 0, NULL);
 	while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
 	{
 		Form_pg_database datForm;

doc/src/sgml/indexam.sgml

@@ -713,7 +713,7 @@ amrestrpos (IndexScanDesc scan);
    When using an MVCC-compliant snapshot, there is no problem because
    the new occupant of the slot is certain to be too new to pass the
    snapshot test.  However, with a non-MVCC-compliant snapshot (such as
-   <literal>SnapshotNow</>), it would be possible to accept and return
+   <literal>SnapshotAny</>), it would be possible to accept and return
    a row that does not in fact match the scan keys.  We could defend
    against this scenario by requiring the scan keys to be rechecked
    against the heap row in all cases, but that is too expensive.  Instead,

src/backend/access/heap/heapam.c

@@ -80,7 +80,7 @@ static HeapScanDesc heap_beginscan_internal(Relation relation,
 						Snapshot snapshot,
 						int nkeys, ScanKey key,
 						bool allow_strat, bool allow_sync,
-						bool is_bitmapscan);
+						bool is_bitmapscan, bool temp_snap);
 static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup,
 					TransactionId xid, CommandId cid, int options);
 static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
@@ -1286,7 +1286,17 @@ heap_beginscan(Relation relation, Snapshot snapshot,
 			   int nkeys, ScanKey key)
 {
 	return heap_beginscan_internal(relation, snapshot, nkeys, key,
-								   true, true, false);
+								   true, true, false, false);
+}
+
+HeapScanDesc
+heap_beginscan_catalog(Relation relation, int nkeys, ScanKey key)
+{
+	Oid			relid = RelationGetRelid(relation);
+	Snapshot	snapshot = RegisterSnapshot(GetCatalogSnapshot(relid));
+
+	return heap_beginscan_internal(relation, snapshot, nkeys, key,
+								   true, true, false, true);
 }
 
 HeapScanDesc
@@ -1295,22 +1305,22 @@ heap_beginscan_strat(Relation relation, Snapshot snapshot,
 					 bool allow_strat, bool allow_sync)
 {
 	return heap_beginscan_internal(relation, snapshot, nkeys, key,
-								   allow_strat, allow_sync, false);
+								   allow_strat, allow_sync, false, false);
 }
 
 HeapScanDesc
 heap_beginscan_bm(Relation relation, Snapshot snapshot,
 				  int nkeys, ScanKey key)
 {
 	return heap_beginscan_internal(relation, snapshot, nkeys, key,
-								   false, false, true);
+								   false, false, true, false);
 }
 
 static HeapScanDesc
 heap_beginscan_internal(Relation relation, Snapshot snapshot,
 						int nkeys, ScanKey key,
 						bool allow_strat, bool allow_sync,
-						bool is_bitmapscan)
+						bool is_bitmapscan, bool temp_snap)
 {
 	HeapScanDesc scan;
 
@@ -1335,6 +1345,7 @@ heap_beginscan_internal(Relation relation, Snapshot snapshot,
 	scan->rs_strategy = NULL;	/* set in initscan */
 	scan->rs_allow_strat = allow_strat;
 	scan->rs_allow_sync = allow_sync;
+	scan->rs_temp_snap = temp_snap;
 
 	/*
 	 * we can use page-at-a-time mode if it's an MVCC-safe snapshot
@@ -1421,6 +1432,9 @@ heap_endscan(HeapScanDesc scan)
 	if (scan->rs_strategy != NULL)
 		FreeAccessStrategy(scan->rs_strategy);
 
+	if (scan->rs_temp_snap)
+		UnregisterSnapshot(scan->rs_snapshot);
+
 	pfree(scan);
 }
 

src/backend/access/index/genam.c

@@ -28,6 +28,7 @@
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
+#include "utils/snapmgr.h"
 #include "utils/tqual.h"
 
 
@@ -231,7 +232,7 @@ BuildIndexValueDescription(Relation indexRelation,
  *	rel: catalog to scan, already opened and suitably locked
  *	indexId: OID of index to conditionally use
  *	indexOK: if false, forces a heap scan (see notes below)
- *	snapshot: time qual to use (usually should be SnapshotNow)
+ *	snapshot: time qual to use (NULL for a recent catalog snapshot)
  *	nkeys, key: scan keys
  *
  * The attribute numbers in the scan key should be set for the heap case.
@@ -266,6 +267,19 @@ systable_beginscan(Relation heapRelation,
 	sysscan->heap_rel = heapRelation;
 	sysscan->irel = irel;
 
+	if (snapshot == NULL)
+	{
+		Oid		relid = RelationGetRelid(heapRelation);
+
+		snapshot = RegisterSnapshot(GetCatalogSnapshot(relid));
+		sysscan->snapshot = snapshot;
+	}
+	else
+	{
+		/* Caller is responsible for any snapshot. */
+		sysscan->snapshot = NULL;
+	}
+
 	if (irel)
 	{
 		int			i;
@@ -401,6 +415,9 @@ systable_endscan(SysScanDesc sysscan)
 	else
 		heap_endscan(sysscan->scan);
 
+	if (sysscan->snapshot)
+		UnregisterSnapshot(sysscan->snapshot);
+
 	pfree(sysscan);
 }
 
@@ -444,6 +461,19 @@ systable_beginscan_ordered(Relation heapRelation,
 	sysscan->heap_rel = heapRelation;
 	sysscan->irel = indexRelation;
 
+	if (snapshot == NULL)
+	{
+		Oid		relid = RelationGetRelid(heapRelation);
+
+		snapshot = RegisterSnapshot(GetCatalogSnapshot(relid));
+		sysscan->snapshot = snapshot;
+	}
+	else
+	{
+		/* Caller is responsible for any snapshot. */
+		sysscan->snapshot = NULL;
+	}
+
 	/* Change attribute numbers to be index column numbers. */
 	for (i = 0; i < nkeys; i++)
 	{
@@ -494,5 +524,7 @@ systable_endscan_ordered(SysScanDesc sysscan)
 {
 	Assert(sysscan->irel);
 	index_endscan(sysscan->iscan);
+	if (sysscan->snapshot)
+		UnregisterSnapshot(sysscan->snapshot);
 	pfree(sysscan);
 }

src/backend/access/nbtree/README

@@ -141,9 +141,10 @@ deletes index entries before deleting tuples, the super-exclusive lock
 guarantees that VACUUM can't delete any heap tuple that an indexscanning
 process might be about to visit.  (This guarantee works only for simple
 indexscans that visit the heap in sync with the index scan, not for bitmap
-scans.  We only need the guarantee when using non-MVCC snapshot rules such
-as SnapshotNow, so in practice this is only important for system catalog
-accesses.)
+scans.  We only need the guarantee when using non-MVCC snapshot rules; in
+an MVCC snapshot, it wouldn't matter if the heap tuple were replaced with
+an unrelated tuple at the same TID, because the new tuple wouldn't be
+visible to our scan anyway.)
 
 Because a page can be split even while someone holds a pin on it, it is
 possible that an indexscan will return items that are no longer stored on

src/backend/access/rmgrdesc/xactdesc.c

@@ -69,11 +69,14 @@ xact_desc_commit(StringInfo buf, xl_xact_commit *xlrec)
 				appendStringInfo(buf, " catalog %u", msg->cat.catId);
 			else if (msg->id == SHAREDINVALRELCACHE_ID)
 				appendStringInfo(buf, " relcache %u", msg->rc.relId);
-			/* remaining cases not expected, but print something anyway */
+			/* not expected, but print something anyway */
 			else if (msg->id == SHAREDINVALSMGR_ID)
 				appendStringInfo(buf, " smgr");
+			/* not expected, but print something anyway */
 			else if (msg->id == SHAREDINVALRELMAP_ID)
 				appendStringInfo(buf, " relmap");
+			else if (msg->id == SHAREDINVALSNAPSHOT_ID)
+				appendStringInfo(buf, " snapshot %u", msg->sn.relId);
 			else
 				appendStringInfo(buf, " unknown id %d", msg->id);
 		}

src/backend/bootstrap/bootstrap.c

@@ -611,7 +611,7 @@ boot_openrel(char *relname)
 	{
 		/* We can now load the pg_type data */
 		rel = heap_open(TypeRelationId, NoLock);
-		scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+		scan = heap_beginscan_catalog(rel, 0, NULL);
 		i = 0;
 		while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
 			++i;
@@ -620,7 +620,7 @@ boot_openrel(char *relname)
 		while (i-- > 0)
 			*app++ = ALLOC(struct typmap, 1);
 		*app = NULL;
-		scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+		scan = heap_beginscan_catalog(rel, 0, NULL);
 		app = Typ;
 		while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
 		{
@@ -918,7 +918,7 @@ gettype(char *type)
 		}
 		elog(DEBUG4, "external type: %s", type);
 		rel = heap_open(TypeRelationId, NoLock);
-		scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+		scan = heap_beginscan_catalog(rel, 0, NULL);
 		i = 0;
 		while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
 			++i;
@@ -927,7 +927,7 @@ gettype(char *type)
 		while (i-- > 0)
 			*app++ = ALLOC(struct typmap, 1);
 		*app = NULL;
-		scan = heap_beginscan(rel, SnapshotNow, 0, NULL);
+		scan = heap_beginscan_catalog(rel, 0, NULL);
 		app = Typ;
 		while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
 		{

src/backend/catalog/aclchk.c

@@ -788,7 +788,7 @@ objectsInSchemaToOids(GrantObjectType objtype, List *nspnames)
 								ObjectIdGetDatum(namespaceId));
 
 					rel = heap_open(ProcedureRelationId, AccessShareLock);
-					scan = heap_beginscan(rel, SnapshotNow, 1, key);
+					scan = heap_beginscan_catalog(rel, 1, key);
 
 					while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
 					{
@@ -833,7 +833,7 @@ getRelationsInNamespace(Oid namespaceId, char relkind)
 				CharGetDatum(relkind));
 
 	rel = heap_open(RelationRelationId, AccessShareLock);
-	scan = heap_beginscan(rel, SnapshotNow, 2, key);
+	scan = heap_beginscan_catalog(rel, 2, key);
 
 	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
 	{
@@ -1332,7 +1332,7 @@ RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
 					ObjectIdGetDatum(objid));
 
 		scan = systable_beginscan(rel, DefaultAclOidIndexId, true,
-								  SnapshotNow, 1, skey);
+								  NULL, 1, skey);
 
 		tuple = systable_getnext(scan);
 
@@ -1452,7 +1452,7 @@ RemoveDefaultACLById(Oid defaclOid)
 				ObjectIdGetDatum(defaclOid));
 
 	scan = systable_beginscan(rel, DefaultAclOidIndexId, true,
-							  SnapshotNow, 1, skey);
+							  NULL, 1, skey);
 
 	tuple = systable_getnext(scan);
 
@@ -2705,7 +2705,7 @@ ExecGrant_Largeobject(InternalGrant *istmt)
 
 		scan = systable_beginscan(relation,
 								  LargeObjectMetadataOidIndexId, true,
-								  SnapshotNow, 1, entry);
+								  NULL, 1, entry);
 
 		tuple = systable_getnext(scan);
 		if (!HeapTupleIsValid(tuple))
@@ -3468,7 +3468,7 @@ pg_aclmask(AclObjectKind objkind, Oid table_oid, AttrNumber attnum, Oid roleid,
 			return pg_language_aclmask(table_oid, roleid, mask, how);
 		case ACL_KIND_LARGEOBJECT:
 			return pg_largeobject_aclmask_snapshot(table_oid, roleid,
-												   mask, how, SnapshotNow);
+												   mask, how, NULL);
 		case ACL_KIND_NAMESPACE:
 			return pg_namespace_aclmask(table_oid, roleid, mask, how);
 		case ACL_KIND_TABLESPACE:
@@ -3856,10 +3856,13 @@ pg_language_aclmask(Oid lang_oid, Oid roleid,
  * Exported routine for examining a user's privileges for a largeobject
  *
  * When a large object is opened for reading, it is opened relative to the
- * caller's snapshot, but when it is opened for writing, it is always relative
- * to SnapshotNow, as documented in doc/src/sgml/lobj.sgml.  This function
- * takes a snapshot argument so that the permissions check can be made relative
- * to the same snapshot that will be used to read the underlying data.
+ * caller's snapshot, but when it is opened for writing, a current
+ * MVCC snapshot will be used.  See doc/src/sgml/lobj.sgml.  This function
+ * takes a snapshot argument so that the permissions check can be made
+ * relative to the same snapshot that will be used to read the underlying
+ * data.  The caller will actually pass NULL for an instantaneous MVCC
+ * snapshot, since all we do with the snapshot argument is pass it through
+ * to systable_beginscan().  
  */
 AclMode
 pg_largeobject_aclmask_snapshot(Oid lobj_oid, Oid roleid,
@@ -4644,7 +4647,7 @@ pg_language_ownercheck(Oid lan_oid, Oid roleid)
  * Ownership check for a largeobject (specified by OID)
  *
  * This is only used for operations like ALTER LARGE OBJECT that are always
- * relative to SnapshotNow.
+ * relative to an up-to-date snapshot.
  */
 bool
 pg_largeobject_ownercheck(Oid lobj_oid, Oid roleid)
@@ -4670,7 +4673,7 @@ pg_largeobject_ownercheck(Oid lobj_oid, Oid roleid)
 
 	scan = systable_beginscan(pg_lo_meta,
 							  LargeObjectMetadataOidIndexId, true,
-							  SnapshotNow, 1, entry);
+							  NULL, 1, entry);
 
 	tuple = systable_getnext(scan);
 	if (!HeapTupleIsValid(tuple))
@@ -5032,7 +5035,7 @@ pg_extension_ownercheck(Oid ext_oid, Oid roleid)
 
 	scan = systable_beginscan(pg_extension,
 							  ExtensionOidIndexId, true,
-							  SnapshotNow, 1, entry);
+							  NULL, 1, entry);
 
 	tuple = systable_getnext(scan);
 	if (!HeapTupleIsValid(tuple))

src/backend/catalog/catalog.c

@@ -218,20 +218,16 @@ IsReservedName(const char *name)
  *		Given the OID of a relation, determine whether it's supposed to be
  *		shared across an entire database cluster.
  *
- * Hard-wiring this list is pretty grotty, but we really need it so that
- * we can compute the locktag for a relation (and then lock it) without
- * having already read its pg_class entry.	If we try to retrieve relisshared
- * from pg_class with no pre-existing lock, there is a race condition against
- * anyone who is concurrently committing a change to the pg_class entry:
- * since we read system catalog entries under SnapshotNow, it's possible
- * that both the old and new versions of the row are invalid at the instants
- * we scan them.  We fix this by insisting that updaters of a pg_class
- * row must hold exclusive lock on the corresponding rel, and that users
- * of a relation must hold at least AccessShareLock on the rel *before*
- * trying to open its relcache entry.  But to lock a rel, you have to
- * know if it's shared.  Fortunately, the set of shared relations is
- * fairly static, so a hand-maintained list of their OIDs isn't completely
- * impractical.
+ * In older releases, this had to be hard-wired so that we could compute the
+ * locktag for a relation and lock it before examining its catalog entry.
+ * Since we now have MVCC catalog access, the race conditions that made that
+ * a hard requirement are gone, so we could look at relaxing this restriction.
+ * However, if we scanned the pg_class entry to find relisshared, and only
+ * then locked the relation, pg_class could get updated in the meantime,
+ * forcing us to scan the relation again, which would definitely be complex
+ * and might have undesirable performance consequences.  Fortunately, the set
+ * of shared relations is fairly static, so a hand-maintained list of their
+ * OIDs isn't completely impractical.
  */
 bool
 IsSharedRelation(Oid relationId)

src/backend/catalog/dependency.c

@@ -558,7 +558,7 @@ findDependentObjects(const ObjectAddress *object,
 		nkeys = 2;
 
 	scan = systable_beginscan(*depRel, DependDependerIndexId, true,
-							  SnapshotNow, nkeys, key);
+							  NULL, nkeys, key);
 
 	while (HeapTupleIsValid(tup = systable_getnext(scan)))
 	{
@@ -733,7 +733,7 @@ findDependentObjects(const ObjectAddress *object,
 		nkeys = 2;
 
 	scan = systable_beginscan(*depRel, DependReferenceIndexId, true,
-							  SnapshotNow, nkeys, key);
+							  NULL, nkeys, key);
 
 	while (HeapTupleIsValid(tup = systable_getnext(scan)))
 	{
@@ -1069,7 +1069,7 @@ deleteOneObject(const ObjectAddress *object, Relation *depRel, int flags)
 		nkeys = 2;
 
 	scan = systable_beginscan(*depRel, DependDependerIndexId, true,
-							  SnapshotNow, nkeys, key);
+							  NULL, nkeys, key);
 
 	while (HeapTupleIsValid(tup = systable_getnext(scan)))
 	{