Read-only access to temp tables for 2PC transactions

commit 5d0c2d5b847bd48aa4c187077717ca99a90086bd
Author: Stas Kelvich <stanconn@gmail.com>
Date:   Tue May 14 12:47:59 2019 +0300

    Read-only access to temp tables for 2PC transactions

diff --git a/src/backend/access/common/relation.c b/src/backend/access/common/relation.c
index 41a0051f88..c70449bf4c 100644
--- a/src/backend/access/common/relation.c
+++ b/src/backend/access/common/relation.c
@@ -23,6 +23,7 @@
 #include "access/relation.h"
 #include "access/xact.h"
 #include "catalog/namespace.h"
+#include "commands/tablecmds.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/lmgr.h"
@@ -69,9 +70,19 @@ relation_open(Oid relationId, LOCKMODE lockmode)
 		   IsBootstrapProcessingMode() ||
 		   CheckRelationLockedByMe(r, AccessShareLock, true));
 
-	/* Make note that we've accessed a temporary relation */
-	if (RelationUsesLocalBuffers(r))
+	/*
+	 * Make note that we've accessed a temporary relation with at least
+	 * RowShareLock lockmode. See also comments above MyXactFlags check
+	 * in PrepareTransaction().
+	 */
+	if (RelationUsesLocalBuffers(r) &&
+		(lockmode > AccessShareLock ||
+		 (lockmode == NoLock &&
+		  CheckRelationLockedByMe(r, RowShareLock, true))))
+	{
+		register_temprel_modify(relationId);
 		MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE;
+	}
 
 	pgstat_initstats(r);
 
@@ -119,9 +130,19 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
 	Assert(lockmode != NoLock ||
 		   CheckRelationLockedByMe(r, AccessShareLock, true));
 
-	/* Make note that we've accessed a temporary relation */
-	if (RelationUsesLocalBuffers(r))
+	/*
+	 * Make note that we've accessed a temporary relation with at least
+	 * RowShareLock lockmode. See also comments above MyXactFlags check
+	 * in PrepareTransaction().
+	 */
+	if (RelationUsesLocalBuffers(r) &&
+		(lockmode > AccessShareLock ||
+		 (lockmode == NoLock &&
+		  CheckRelationLockedByMe(r, AccessShareLock, true))))
+	{
+		register_temprel_modify(relationId);
 		MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE;
+	}
 
 	pgstat_initstats(r);
 
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 20feeec327..39a4c1106d 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2352,12 +2352,14 @@ PrepareTransaction(void)
 	/* NOTIFY will be handled below */
 
 	/*
-	 * Don't allow PREPARE TRANSACTION if we've accessed a temporary table in
+	 * Don't allow PREPARE TRANSACTION if we've modified a temporary table in
 	 * this transaction.  Having the prepared xact hold locks on another
 	 * backend's temp table seems a bad idea --- for instance it would prevent
 	 * the backend from exiting.  There are other problems too, such as how to
 	 * clean up the source backend's local buffers and ON COMMIT state if the
-	 * prepared xact includes a DROP of a temp table.
+	 * prepared xact includes a DROP of a temp table.  Special case of reading
+	 * from a temporary table is allowed by dropping AccessShare locks during
+	 * PREPARE.
 	 *
 	 * Other objects types, like functions, operators or extensions, share the
 	 * same restriction as they should not be created, locked or dropped as
@@ -2368,8 +2370,9 @@ PrepareTransaction(void)
 	 * might still access a temp relation.
 	 *
 	 * XXX In principle this could be relaxed to allow some useful special
-	 * cases, such as a temp table created and dropped all within the
-	 * transaction.  That seems to require much more bookkeeping though.
+	 * cases besides reading from a temp table, such as a temp table created
+	 * and dropped all within the transaction.  That seems to require much more
+	 * bookkeeping though.
 	 */
 	if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
 		ereport(ERROR,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index baeb13e6a0..1e3ae90d95 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -122,6 +122,12 @@ typedef struct OnCommitItem
 
 static List *on_commits = NIL;
 
+/*
+ * Set of temp relation oid's for which we aquired locks stronger then
+ * AccessShare.  During commit this set will be intersected with on_commits
+ * to determine which relations needs truncation.
+ */
+static HTAB *modified_temprels = NULL;
 
 /*
  * State information for ALTER TABLE
@@ -14398,6 +14404,34 @@ register_on_commit_action(Oid relid, OnCommitAction action)
 	MemoryContextSwitchTo(oldcxt);
 }
 
+/*
+ * Track modifying access to temporary relations.
+ * Later we use that set to determine which relations should be
+ * truncated.
+ */
+void
+register_temprel_modify(Oid relid)
+{
+	/*
+	 * Init modified_temprels.
+	 * max_locks_per_xact is used as a guess to the amount of relations
+	 * that normally is accessed in a single transaction.
+	 */
+	if (modified_temprels == NULL)
+	{
+		HASHCTL		ctl;
+		MemSet(&ctl, 0, sizeof(ctl));
+		ctl.keysize = sizeof(Oid);
+		ctl.entrysize = sizeof(Oid);
+		ctl.hcxt = TopTransactionContext;
+		modified_temprels = hash_create("Temp rels modified in this tx",
+									max_locks_per_xact,
+									&ctl, HASH_ELEM | HASH_BLOBS);
+	}
+
+	(void) hash_search(modified_temprels, &relid, HASH_ENTER, NULL);
+}
+
 /*
  * Unregister any ON COMMIT action when a relation is deleted.
  *
@@ -14448,14 +14482,19 @@ PreCommit_on_commit_actions(void)
 				/* Do nothing (there shouldn't be such entries, actually) */
 				break;
 			case ONCOMMIT_DELETE_ROWS:
-
 				/*
-				 * If this transaction hasn't accessed any temporary
-				 * relations, we can skip truncating ON COMMIT DELETE ROWS
-				 * tables, as they must still be empty.
+				 * We will truncate only relations that we opened with lock
+				 * stronger then AccessShare (so possibly were modified).
 				 */
-				if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
-					oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
+				if (modified_temprels != NULL)
+				{
+					bool modified;
+					(void) hash_search(modified_temprels, &oc->relid,
+									HASH_FIND, &modified);
+
+					if (modified)
+						oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
+				}
 				break;
 			case ONCOMMIT_DROP:
 				oids_to_drop = lappend_oid(oids_to_drop, oc->relid);
@@ -14559,6 +14598,12 @@ AtEOXact_on_commit_actions(bool isCommit)
 			cur_item = lnext(prev_item);
 		}
 	}
+
+	/*
+	 * Reset modified_temprels. Hash table itself will be released with
+	 * TopTransactionContext.
+	 */
+	modified_temprels = NULL;
 }
 
 /*
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index c8958766f1..9eee017c3d 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3212,6 +3212,13 @@ AtPrepare_Locks(void)
 			locallock->lock = locallock->proclock->tag.myLock;
 		}
 
+		/*
+		 * Do not save any AccessShare locks as they will not be transfered to
+		 * a dummy PGPRPROC by PostPrepare_Locks().
+		 */
+		if (locallock->tag.mode == AccessShareLock)
+			continue;
+
 		/*
 		 * Arrange to not release any strong lock count held by this lock
 		 * entry.  We must retain the count until the prepared transaction is
@@ -3382,6 +3389,33 @@ PostPrepare_Locks(TransactionId xid)
 			if (proclock->releaseMask != proclock->holdMask)
 				elog(PANIC, "we seem to have dropped a bit somewhere");
 
+			/*
+			 * To be able to safely read from temp tables prepared transaction
+			 * should not hold AccessShare locks. So here we are releasing any
+			 * AccessShare locks without transfering them further to a dummy
+			 * PGPROC.  See also comments above MyXactFlags check in
+			 * PrepareTransaction().
+			 */
+			if (proclock->holdMask & LOCKBIT_ON(AccessShareLock))
+			{
+				LockMethod	lockMethodTable;
+				bool wakeupNeeded;
+
+				lockMethodTable = LockMethods[lock->tag.locktag_lockmethodid];
+				wakeupNeeded = UnGrantLock(lock, AccessShareLock,
+										   proclock, lockMethodTable);
+
+				CleanUpLock(lock, proclock, lockMethodTable,
+							LockTagHashCode(&lock->tag), wakeupNeeded);
+
+				/*
+				 * If we were holding lock only in AccessShare mode than
+				 * there is nothing to pass to a new PGPROC.
+				 */
+				if (proclock->holdMask == 0)
+					continue;
+			}
+
 			/*
 			 * We cannot simply modify proclock->tag.myProc to reassign
 			 * ownership of the lock, because that's part of the hash key and
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 96927b900d..f3c75e70a6 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -80,6 +80,7 @@ extern void register_on_commit_action(Oid relid, OnCommitAction action);
 extern void remove_on_commit_action(Oid relid);
 
 extern void PreCommit_on_commit_actions(void);
+extern void register_temprel_modify(Oid relid);
 extern void AtEOXact_on_commit_actions(bool isCommit);
 extern void AtEOSubXact_on_commit_actions(bool isCommit,
 							  SubTransactionId mySubid,
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index eb77c18788..14ea813342 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -241,6 +241,36 @@ SELECT * FROM pxtest3;
 ERROR:  relation "pxtest3" does not exist
 LINE 1: SELECT * FROM pxtest3;
                       ^
+-- Read-only access to temporary table is allowed from a prepared
+-- transaction
+create temp table twophase_tab_preparable (a int);
+begin;
+select a from twophase_tab_preparable;
+ a 
+---
+(0 rows)
+
+prepare transaction 'twophase_tab_preparable';
+-- prepared transactions should not hold AccessShare locks
+create table twophase_tab_testdrop1 (a int);
+create table twophase_tab_testdrop2 (a int);
+begin;
+select a from twophase_tab_testdrop1;
+ a 
+---
+(0 rows)
+
+select a from twophase_tab_testdrop2;
+ a 
+---
+(0 rows)
+
+prepare transaction 'twophase_tab_testdrop';
+drop table twophase_tab_testdrop1;
+\c -
+drop table twophase_tab_testdrop2;
+commit prepared 'twophase_tab_preparable';
+commit prepared 'twophase_tab_testdrop';
 -- There should be no prepared transactions
 SELECT gid FROM pg_prepared_xacts;
  gid 
diff --git a/src/test/regress/expected/prepared_xacts_1.out b/src/test/regress/expected/prepared_xacts_1.out
index 0857d259e0..b7f0f56818 100644
--- a/src/test/regress/expected/prepared_xacts_1.out
+++ b/src/test/regress/expected/prepared_xacts_1.out
@@ -235,6 +235,42 @@ SELECT * FROM pxtest3;
 -----
 (0 rows)
 
+-- Read-only access to temporary table is allowed from a prepared
+-- transaction
+create temp table twophase_tab_preparable (a int);
+begin;
+select a from twophase_tab_preparable;
+ a 
+---
+(0 rows)
+
+prepare transaction 'twophase_tab_preparable';
+ERROR:  prepared transactions are disabled
+HINT:  Set max_prepared_transactions to a nonzero value.
+-- prepared transactions should not hold AccessShare locks
+create table twophase_tab_testdrop1 (a int);
+create table twophase_tab_testdrop2 (a int);
+begin;
+select a from twophase_tab_testdrop1;
+ a 
+---
+(0 rows)
+
+select a from twophase_tab_testdrop2;
+ a 
+---
+(0 rows)
+
+prepare transaction 'twophase_tab_testdrop';
+ERROR:  prepared transactions are disabled
+HINT:  Set max_prepared_transactions to a nonzero value.
+drop table twophase_tab_testdrop1;
+\c -
+drop table twophase_tab_testdrop2;
+commit prepared 'twophase_tab_preparable';
+ERROR:  prepared transaction with identifier "twophase_tab_preparable" does not exist
+commit prepared 'twophase_tab_testdrop';
+ERROR:  prepared transaction with identifier "twophase_tab_testdrop" does not exist
 -- There should be no prepared transactions
 SELECT gid FROM pg_prepared_xacts;
  gid 
diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out
index ad7d558191..1f4a342f75 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -336,17 +336,9 @@ begin;
 create sequence pg_temp.twophase_seq;
 prepare transaction 'twophase_sequence';
 ERROR:  cannot PREPARE a transaction that has operated on temporary objects
--- Temporary tables cannot be used with two-phase commit.
+-- Temporary tables cannot be modified by prepared transaction.
 create temp table twophase_tab (a int);
 begin;
-select a from twophase_tab;
- a 
----
-(0 rows)
-
-prepare transaction 'twophase_tab';
-ERROR:  cannot PREPARE a transaction that has operated on temporary objects
-begin;
 insert into twophase_tab values (1);
 prepare transaction 'twophase_tab';
 ERROR:  cannot PREPARE a transaction that has operated on temporary objects
diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql
index d8249a27dc..59c5435239 100644
--- a/src/test/regress/sql/prepared_xacts.sql
+++ b/src/test/regress/sql/prepared_xacts.sql
@@ -149,6 +149,26 @@ SELECT gid FROM pg_prepared_xacts;
 COMMIT PREPARED 'regress-two';
 SELECT * FROM pxtest3;
 
+-- Read-only access to temporary table is allowed from a prepared
+-- transaction
+create temp table twophase_tab_preparable (a int);
+begin;
+select a from twophase_tab_preparable;
+prepare transaction 'twophase_tab_preparable';
+
+-- prepared transactions should not hold AccessShare locks
+create table twophase_tab_testdrop1 (a int);
+create table twophase_tab_testdrop2 (a int);
+begin;
+select a from twophase_tab_testdrop1;
+select a from twophase_tab_testdrop2;
+prepare transaction 'twophase_tab_testdrop';
+drop table twophase_tab_testdrop1;
+\c -
+drop table twophase_tab_testdrop2;
+commit prepared 'twophase_tab_preparable';
+commit prepared 'twophase_tab_testdrop';
+
 -- There should be no prepared transactions
 SELECT gid FROM pg_prepared_xacts;
 
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index e634ddb9ca..4643278d84 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -257,12 +257,9 @@ begin;
 create sequence pg_temp.twophase_seq;
 prepare transaction 'twophase_sequence';
 
--- Temporary tables cannot be used with two-phase commit.
+-- Temporary tables cannot be modified by prepared transaction.
 create temp table twophase_tab (a int);
 begin;
-select a from twophase_tab;
-prepare transaction 'twophase_tab';
-begin;
 insert into twophase_tab values (1);
 prepare transaction 'twophase_tab';
 begin;