intra-grant-inplace-via-tuplock-v0.patch

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 14de815..8e51557 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3730,6 +3730,15 @@ l2:
 										   id_has_external,
 										   &old_key_copied);
 
+	/*
+	 * FIXME fail if updating pg_class without holding either (a)
+	 * LOCKTAG_RELATION on this row's rel, in a mode conflicting w/ ShareLock,
+	 * or (b) LOCKTAG_TUPLE.  Fail if updating pg_database without holding
+	 * either (a) LOCKTAG_OBJECT on this row's database OID, in FIXME mode, or
+	 * (b) LOCKTAG_TUPLE.  This may imply the executor taking the tuple locks
+	 * during SQL UPDATE of those tables.
+	 */
+
 	/* NO EREPORT(ERROR) from here till changes are logged */
 	START_CRIT_SECTION();
 
@@ -5892,6 +5901,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
 	HeapTupleHeader htup;
 	uint32		oldlen;
 	uint32		newlen;
+	HeapTupleData oldtup;
 
 	/*
 	 * For now, we don't allow parallel updates.  Unlike a regular update,
@@ -5904,6 +5914,8 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
 				(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
 				 errmsg("cannot update tuples during a parallel operation")));
 
+	/* FIXME fail unless holding LockTuple(relation, &tuple->t-self, AccessExclusiveLock */
+
 	buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(&(tuple->t_self)));
 	LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 	page = (Page) BufferGetPage(buffer);
@@ -5922,6 +5934,16 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
 	if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff)
 		elog(ERROR, "wrong tuple length");
 
+	/* evaluate whether updated */
+	/* FIXME wait for a concurrent updater, in case it aborts */
+	oldtup.t_tableOid = RelationGetRelid(relation);
+	oldtup.t_data = htup;
+	oldtup.t_len = ItemIdGetLength(lp);
+	oldtup.t_self = tuple->t_self;
+	if (TM_Ok !=
+		HeapTupleSatisfiesUpdate(&oldtup, GetCurrentCommandId(false), buffer))
+		elog(ERROR, "tuple concurrently updated");
+
 	/* NO EREPORT(ERROR) from here till changes are logged */
 	START_CRIT_SECTION();
 
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 3ce6c09..dba3137 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1853,7 +1853,7 @@ ExecGrant_Relation(InternalGrant *istmt)
 		HeapTuple	tuple;
 		ListCell   *cell_colprivs;
 
-		tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
+		tuple = SearchSysCacheLocked1(relation, RELOID, ObjectIdGetDatum(relOid));
 		if (!HeapTupleIsValid(tuple))
 			elog(ERROR, "cache lookup failed for relation %u", relOid);
 		pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple);
@@ -2190,7 +2190,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
 		Oid		   *oldmembers;
 		Oid		   *newmembers;
 
-		tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid));
+		tuple = SearchSysCacheLocked1(relation, cacheid, ObjectIdGetDatum(objectid));
 		if (!HeapTupleIsValid(tuple))
 			elog(ERROR, "cache lookup failed for %s %u", get_object_class_descr(classid), objectid);
 
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 143fae0..7c02380 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2860,11 +2860,19 @@ index_update_stats(Relation rel,
 		tuple = heap_getnext(pg_class_scan, ForwardScanDirection);
 		tuple = heap_copytuple(tuple);
 		table_endscan(pg_class_scan);
+		LockTuple(pg_class, &tuple->t_self, AccessExclusiveLock);
+		/* FIXME loop in case the pg_class tuple for pg_class moved */
 	}
 	else
 	{
+		HeapTuple cachetup;
+
 		/* normal case, use syscache */
-		tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
+		cachetup = SearchSysCacheLocked1(pg_class,
+										 RELOID, ObjectIdGetDatum(relid));
+		tuple = heap_copytuple(cachetup);
+		if (HeapTupleIsValid(cachetup))
+			ReleaseSysCache(cachetup);
 	}
 
 	if (!HeapTupleIsValid(tuple))
@@ -2933,6 +2941,7 @@ index_update_stats(Relation rel,
 		CacheInvalidateRelcacheByTuple(tuple);
 	}
 
+	UnlockTuple(pg_class, &tuple->t_self, AccessExclusiveLock);
 	heap_freetuple(tuple);
 
 	table_close(pg_class, RowExclusiveLock);
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 8dbda00..76be8b5 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -76,6 +76,7 @@
 #include "catalog/pg_type.h"
 #include "catalog/pg_user_mapping.h"
 #include "lib/qunique.h"
+#include "storage/lmgr.h"
 #include "utils/catcache.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
@@ -827,6 +828,70 @@ SearchSysCache1(int cacheId,
 	return SearchCatCache1(SysCache[cacheId], key1);
 }
 
+/*
+ * Like SearchSysCache1, but a LOCKTAG_TUPLE heavyweight lock is held in
+ * AccessExclusiveMode on return.
+ *
+ * pg_class and pg_database are subject to both heap_inplace_update() and
+ * regular heap_update().  We must not lose the effects of any inplace update.
+ * That might arise by heap_inplace_update() mutating a tuple, the xmax of
+ * which then commits.  Alternatively, it could arise by heap_inplace_update()
+ * mutating a tuple before an ongoing transaction's heap_update(), after the
+ * ongoing transaction fetches the old tuple to modify.  We prevent both with
+ * locking as follows.  All heap_inplace_update(pg_class) callers acquire two
+ * locks.  First, they acquire LOCKTAG_RELATION in mode ShareLock,
+ * ShareUpdateExclusiveLock, or a mode with strictly more conflicts.  Second,
+ * they acquire LOCKTAG_TUPLE.  heap_update(pg_class) callers can either take
+ * any lock that conflicts with one of those two.  Acquire one's choice of
+ * lock before fetching the tuple to be modified.  FIXME write corresponding
+ * story for pg_database.
+ *
+ * This function bundles the tasks of retrieving a tuple and acquiring the
+ * LOCKTAG_TUPLE.  Since much DDL opts to acquire LOCKTAG_RELATION, the
+ * LOCKTAG_TUPLE doesn't block such updaters of the returned tuple.  Callers
+ * shall reject superseded tuples, such as by checking with
+ * HeapTupleSatisfiesUpdate() while holding an exclusive lock on the tuple's
+ * buffer.
+ *
+ * We opt to loop until the search finds the same tuple we've locked.  FIXME
+ * This might not be strictly necessary, but it likely avoids some spurious
+ * errors.  In the happy case, this takes two fetches, one to determine the
+ * tid to lock and another to confirm that the TID remains the latest tuple.
+ *
+ * FIXME consider dropping Relation arg and deducing applicable locktag fields
+ * from cacheId.
+ *
+ * FIXME this may belong in a different file, with available key counts other
+ * than 1, etc.
+ */
+HeapTuple
+SearchSysCacheLocked1(Relation rel,
+					  int cacheId,
+					  Datum key1)
+{
+	ItemPointerData tid;
+	bool retry = false;
+
+	for (;;)
+	{
+		HeapTuple	tuple;
+
+		tuple = SearchSysCache1(cacheId, key1);
+		if (!HeapTupleIsValid(tuple) ||
+			(retry && ItemPointerEquals(&tid, &tuple->t_self)))
+			return tuple;
+
+		if (retry)
+			UnlockTuple(rel, &tid, AccessExclusiveLock);
+
+		tid = tuple->t_self;
+		ReleaseSysCache(tuple);
+		LockTuple(rel, &tid, AccessExclusiveLock);
+		retry = true;
+	}
+}
+
+
 HeapTuple
 SearchSysCache2(int cacheId,
 				Datum key1, Datum key2)
diff --git a/src/include/utils/syscache.h b/src/include/utils/syscache.h
index 5d47a65..aba5385 100644
--- a/src/include/utils/syscache.h
+++ b/src/include/utils/syscache.h
@@ -18,6 +18,7 @@
 
 #include "access/attnum.h"
 #include "access/htup.h"
+#include "utils/rel.h"
 /* we intentionally do not include utils/catcache.h here */
 
 /*
@@ -174,6 +175,10 @@ extern bool RelationInvalidatesSnapshotsOnly(Oid relid);
 extern bool RelationHasSysCache(Oid relid);
 extern bool RelationSupportsSysCache(Oid relid);
 
+extern HeapTuple SearchSysCacheLocked1(Relation rel,
+									   int cacheId,
+									   Datum key1);
+
 /*
  * The use of the macros below rather than direct calls to the corresponding
  * functions is encouraged, as it insulates the caller from changes in the
diff --git a/src/test/isolation/expected/intra-grant-inplace.out b/src/test/isolation/expected/intra-grant-inplace.out
new file mode 100644
index 0000000..a1eed4e
--- /dev/null
+++ b/src/test/isolation/expected/intra-grant-inplace.out
@@ -0,0 +1,23 @@
+Parsed test spec with 2 sessions
+
+starting permutation: b1 g1 s2 a2 c1 s2
+step b1: BEGIN ISOLATION LEVEL READ COMMITTED;
+step g1: GRANT SELECT ON intra_grant_inplace TO PUBLIC;
+step s2: SELECT relhasindex FROM pg_class
+           WHERE oid = 'intra_grant_inplace'::regclass;
+relhasindex
+-----------
+f          
+(1 row)
+
+step a2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); <waiting ...>
+step c1: COMMIT;
+step a2: <... completed>
+ERROR:  tuple concurrently updated
+step s2: SELECT relhasindex FROM pg_class
+           WHERE oid = 'intra_grant_inplace'::regclass;
+relhasindex
+-----------
+f          
+(1 row)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88e..fcc4ad4 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -86,6 +86,7 @@ test: alter-table-3
 test: alter-table-4
 test: create-trigger
 test: sequence-ddl
+test: intra-grant-inplace
 test: async-notify
 test: vacuum-no-cleanup-lock
 test: timeouts
diff --git a/src/test/isolation/specs/intra-grant-inplace.spec b/src/test/isolation/specs/intra-grant-inplace.spec
new file mode 100644
index 0000000..85059ed
--- /dev/null
+++ b/src/test/isolation/specs/intra-grant-inplace.spec
@@ -0,0 +1,25 @@
+# GRANT's lock is the catalog tuple xmax.  GRANT doesn't acquire a heavyweight
+# lock on the object undergoing an ACL change.  In-place updates, such as
+# relhasindex=true, need special code to cope.
+
+setup
+{
+  CREATE TABLE intra_grant_inplace (c int);
+}
+
+teardown
+{
+  DROP TABLE intra_grant_inplace;
+}
+
+session s1
+step b1  { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step g1  { GRANT SELECT ON intra_grant_inplace TO PUBLIC; }
+step c1  { COMMIT; }
+
+session s2
+step s2  { SELECT relhasindex FROM pg_class
+           WHERE oid = 'intra_grant_inplace'::regclass; }
+step a2  { ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); }
+
+permutation b1 g1 s2 a2 c1 s2