Re: Transactions and temp tables

Emmanuel Cecchet wrote:
>> Emmanuel Cecchet wrote:
>>> As I have not found yet an elegant solution to deal with the DROP 
>>> CASCADE issue, here is a simpler patch that handles temp tables that 
>>> are dropped at commit time. This is a good first step and we will try 
>>> to elaborate further to support ON COMMIT DELETE ROWS.
>>
>> The problem with stopping the postmaster seems to still be there..
>>
>> All the problems are centered around locking. We need to address that 
>> and decide what is sane locking behavior wrt. temp tables and 2PC.
>>
>> First, there's the case where a temp table is created and dropped in 
>> the same transaction. It seems perfectly sane to me to simply drop all 
>> locks on the dropped table at PREPARE TRANSACTION. Does anyone see a 
>> problem with that? If not, we might as well do that for all tables, 
>> not just temporary ones. It seems just as safe for non-temporary tables.
> This seems good to me. Any access to the table after PREPARE TRANSACTION 
> but before COMMIT on that backend would return a relation not found 
> which is what we expect. For  a regular table, I don't know if that 
> makes a difference if the prepared transaction rollbacks?

I don't think there's any difference with temp tables and regular ones 
from locking point of view.

>> Secondly, there's the case of accessing a ON COMMIT DELETE ROWS table. 
>> There too, could we simply drop the locks at PREPARE TRANSACTION? I 
>> can't immediately see anything wrong with that.
> As there is no data anyway, I don't think the locks are going to change 
> anything. But in the most recent stripped-down version of the patch, on 
> delete rows is no more supported, I only allow on commit drop. I did not 
> find the flag to see if a temp table was created with the on delete rows 
> option.

Hmm. I think we can use the on_commits list in tablecmds.c for that.

> Do you want me to look at the locking code or will you have time to do 
> it? Hints will be welcome if you want me to do it.

I can give it a shot for change. Attached is a patch that allows the ON 
COMMIT DELETE ROWS case. The beef of the patch is:

- An entry is made into the on_commits list in tablecmds.c for all temp 
tables, even if there's no ON COMMIT action
- There's a new function, check_prepare_safe_temp_table(Oid relid) in 
tablecmds.c, that uses the on_commits list to determine if the access to 
the given relation is "PREPARE-safe". That is, it's not a temp table, or 
it's an access to an ON COMMIT DELETE ROWS temp table and the temp table 
wasn't created or dropped in the same transaction.
- MyXactMadeTempRelUpdate variable is gone. The check is driven from the 
lock manager again, like it was in 8.1, by calling the new 
check_prepare_sage_temp_table function for all relation locks in 
AtPrepare_Locks().
- changed the on_commits linked list in tablecmds.c into a hash table 
for performance

Somehow this feels pretty baroque, though. Perhaps a better approach 
would be to add a new AtPrepare_OnCommitActions function to tablecmds.c, 
that gets called before AtPrepare_Locks. It would scan through the 
on_commits list, and release all locks for the "PREPARE-safe" temp 
tables, and throw the error if necessary. I'll try that next.

BTW, there's a very relevant thread here:

http://archives.postgresql.org/pgsql-hackers/2008-03/msg00063.php

if you haven't read it already.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 33d5fab..b94e2bd 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -876,10 +876,6 @@ relation_open(Oid relationId, LOCKMODE lockmode)
 	if (!RelationIsValid(r))
 		elog(ERROR, "could not open relation with OID %u", relationId);
 
-	/* Make note that we've accessed a temporary relation */
-	if (r->rd_istemp)
-		MyXactAccessedTempRel = true;
-
 	pgstat_initstats(r);
 
 	return r;
@@ -924,10 +920,6 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
 	if (!RelationIsValid(r))
 		elog(ERROR, "could not open relation with OID %u", relationId);
 
-	/* Make note that we've accessed a temporary relation */
-	if (r->rd_istemp)
-		MyXactAccessedTempRel = true;
-
 	pgstat_initstats(r);
 
 	return r;
@@ -974,10 +966,6 @@ relation_open_nowait(Oid relationId, LOCKMODE lockmode)
 	if (!RelationIsValid(r))
 		elog(ERROR, "could not open relation with OID %u", relationId);
 
-	/* Make note that we've accessed a temporary relation */
-	if (r->rd_istemp)
-		MyXactAccessedTempRel = true;
-
 	pgstat_initstats(r);
 
 	return r;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b7d1285..c859874 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -67,14 +67,6 @@ int			CommitDelay = 0;	/* precommit delay in microseconds */
 int			CommitSiblings = 5; /* # concurrent xacts needed to sleep */
 
 /*
- * MyXactAccessedTempRel is set when a temporary relation is accessed.
- * We don't allow PREPARE TRANSACTION in that case.  (This is global
- * so that it can be set from heapam.c.)
- */
-bool		MyXactAccessedTempRel = false;
-
-
-/*
  *	transaction states - transaction state from server perspective
  */
 typedef enum TransState
@@ -1467,7 +1459,6 @@ StartTransaction(void)
 	XactIsoLevel = DefaultXactIsoLevel;
 	XactReadOnly = DefaultXactReadOnly;
 	forceSyncCommit = false;
-	MyXactAccessedTempRel = false;
 
 	/*
 	 * reinitialize within-transaction counters
@@ -1798,26 +1789,6 @@ PrepareTransaction(void)
 
 	/* NOTIFY and flatfiles will be handled below */
 
-	/*
-	 * Don't allow PREPARE TRANSACTION if we've accessed 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.
-	 *
-	 * We must check this after executing any ON COMMIT actions, because
-	 * they 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.
-	 */
-	if (MyXactAccessedTempRel)
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("cannot PREPARE a transaction that has operated on temporary tables")));
-
 	/* Prevent cancel/die interrupt while cleaning up */
 	HOLD_INTERRUPTS();
 
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index afc6977..99e941b 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -39,6 +39,7 @@
 #include "catalog/heap.h"
 #include "catalog/index.h"
 #include "catalog/indexing.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_attrdef.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_inherits.h"
@@ -1064,7 +1065,7 @@ heap_create_with_catalog(const char *relname,
 	/*
 	 * If there's a special on-commit action, remember it
 	 */
-	if (oncommit != ONCOMMIT_NOOP)
+	if (isTempOrToastNamespace(relnamespace))
 		register_on_commit_action(relid, oncommit);
 
 	/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6559470..f201e0d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -96,7 +96,7 @@ typedef struct OnCommitItem
 	SubTransactionId deleting_subid;
 } OnCommitItem;
 
-static List *on_commits = NIL;
+static HTAB *on_commits = NULL;
 
 
 /*
@@ -7575,26 +7575,38 @@ void
 register_on_commit_action(Oid relid, OnCommitAction action)
 {
 	OnCommitItem *oc;
-	MemoryContext oldcxt;
+	bool		  found;
 
 	/*
-	 * We needn't bother registering the relation unless there is an ON COMMIT
-	 * action we need to take.
+	 * We need to have an entry for all temporary tables, even for
+	 * no-op actions, see check_prepare_safe_temp_table().
 	 */
-	if (action == ONCOMMIT_NOOP || action == ONCOMMIT_PRESERVE_ROWS)
-		return;
 
-	oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
+	if (on_commits == NULL)
+	{
+		HASHCTL		hash_ctl;
+
+		/* Initialize hash tables used to track update chains */
+		memset(&hash_ctl, 0, sizeof(hash_ctl));
+		hash_ctl.keysize = sizeof(Oid);
+		hash_ctl.entrysize = sizeof(OnCommitItem);
+		hash_ctl.hcxt = CacheMemoryContext;
+		hash_ctl.hash = oid_hash;
+
+		on_commits = hash_create("On commit actions",
+								 4, /* small initial size to make scans fast */
+								 &hash_ctl,
+								 HASH_ELEM | HASH_FUNCTION | HASH_CONTEXT);
+	}
+
+	oc = (OnCommitItem *) hash_search(on_commits, &relid, HASH_ENTER, &found);
+	if (found)
+		elog(ERROR, "relid already in on commit action table");
 
-	oc = (OnCommitItem *) palloc(sizeof(OnCommitItem));
 	oc->relid = relid;
 	oc->oncommit = action;
 	oc->creating_subid = GetCurrentSubTransactionId();
 	oc->deleting_subid = InvalidSubTransactionId;
-
-	on_commits = lcons(oc, on_commits);
-
-	MemoryContextSwitchTo(oldcxt);
 }
 
 /*
@@ -7605,17 +7617,90 @@ register_on_commit_action(Oid relid, OnCommitAction action)
 void
 remove_on_commit_action(Oid relid)
 {
-	ListCell   *l;
+	OnCommitItem *oc;
+
+	if (on_commits == NULL)
+		return;
 
-	foreach(l, on_commits)
+	oc = (OnCommitItem *) hash_search(on_commits, &relid, HASH_FIND, NULL);
+
+	if (oc != NULL)
+		oc->deleting_subid = GetCurrentSubTransactionId();
+}
+
+/*
+ * Don't allow PREPARE TRANSACTION if we've accessed 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.
+ *
+ * We must check this after executing any ON COMMIT actions, because
+ * they might still access a temp relation.
+ *
+ * However, since PostgreSQL 8.4, we do allow PREPARE TRANSATION if the
+ * transaction has accessed a temporary table with ON COMMIT DELETE ROWS
+ * action, as long as the transaction hasn't created or dropped one.  We
+ * work around the lock problem by releasing the locks early, in the PREPARE
+ * phase. That doesn't violate the two-phase locking protocol (not to be
+ * confused with two-phase commit!), as the lock would be released at the
+ * 2nd phase commit or rollback anyway, and the transaction won't acquire
+ * any more locks after PREPARE.
+ *
+ * This function is called by the lock manager in the PREPARE phase, to
+ * determine if the given relation is an ON COMMIT DELETE ROWS temporary
+ * table, and the lock manager should drop locks early.  Returns 'true', if
+ * it is, and 'false' if the table is not a temporary table. If the table
+ * is a temporary table, but it's not safe for PREPARE TRANSACTION, an error
+ * is thrown.
+ *
+ * The reason why we only do that for ON COMMIT DELETE ROWS tables, is that
+ * VACUUM expects that all xids in the table are finished. XXX That's not
+ * strictly true, it can deal with them but prints messages to log. Is there
+ * any other code that could be confused by that?
+ *
+ * XXX In principle this could be relaxed to allow some other useful special
+ * cases, such as a temp table created and dropped all within the
+ * transaction.  We would need to at least drop the local buffers, however.
+ */
+bool
+check_prepare_safe_temp_table(Oid relid)
+{
+	OnCommitItem *oc;
+
+	if (on_commits == NULL)
+		return false; /* No temp tables */
+
+	oc = (OnCommitItem *) hash_search(on_commits, &relid, HASH_FIND, NULL);
+
+	if (oc == NULL)
+		return false; /* Not a temp table */
+
+	/*
+	 * Allow access to ON COMMIT DELETE ROWS temporary tables,
+	 * that have not been created or dropped in this transaction.
+	 */
+	if (oc->oncommit == ONCOMMIT_DELETE_ROWS &&
+		oc->creating_subid == InvalidSubTransactionId &&
+		oc->deleting_subid == InvalidSubTransactionId)
 	{
-		OnCommitItem *oc = (OnCommitItem *) lfirst(l);
+		return true;
+	}
+	else
+	{
+		/* Give appropriate error message */
+		if (oc->creating_subid != InvalidSubTransactionId ||
+			oc->deleting_subid != InvalidSubTransactionId)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot PREPARE a transaction that has created or dropped a temporary table")));
+		else
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot PREPARE a transaction that has accessed a temporary table not defined with ON COMMIT DELETE ROWS")));
 
-		if (oc->relid == relid)
-		{
-			oc->deleting_subid = GetCurrentSubTransactionId();
-			break;
-		}
+		return false; /* keep compiler quite */
 	}
 }
 
@@ -7628,19 +7713,24 @@ remove_on_commit_action(Oid relid)
 void
 PreCommit_on_commit_actions(void)
 {
-	ListCell   *l;
+	HASH_SEQ_STATUS seq_status;
 	List	   *oids_to_truncate = NIL;
+	OnCommitItem *oc;
 
-	foreach(l, on_commits)
-	{
-		OnCommitItem *oc = (OnCommitItem *) lfirst(l);
-
-		/* Ignore entry if already dropped in this xact */
-		if (oc->deleting_subid != InvalidSubTransactionId)
-			continue;
+	if (on_commits == NULL)
+		return;
 
-		switch (oc->oncommit)
+	hash_seq_init(&seq_status, on_commits);
+	PG_TRY();
+	{
+		while ((oc = hash_seq_search(&seq_status)) != NULL)
 		{
+			/* Ignore entry if already dropped in this xact */
+			if (oc->deleting_subid != InvalidSubTransactionId)
+				continue;
+
+			switch (oc->oncommit)
+			{
 			case ONCOMMIT_NOOP:
 			case ONCOMMIT_PRESERVE_ROWS:
 				/* Do nothing (there shouldn't be such entries, actually) */
@@ -7665,8 +7755,15 @@ PreCommit_on_commit_actions(void)
 					Assert(oc->deleting_subid != InvalidSubTransactionId);
 					break;
 				}
+			}
 		}
 	}
+	PG_CATCH();
+	{
+		hash_seq_term(&seq_status);
+	}
+	PG_END_TRY();
+
 	if (oids_to_truncate != NIL)
 	{
 		heap_truncate(oids_to_truncate);
@@ -7685,36 +7782,36 @@ PreCommit_on_commit_actions(void)
 void
 AtEOXact_on_commit_actions(bool isCommit)
 {
-	ListCell   *cur_item;
-	ListCell   *prev_item;
+	HASH_SEQ_STATUS seq_status;
+	OnCommitItem *oc;
 
-	prev_item = NULL;
-	cur_item = list_head(on_commits);
+	if (on_commits == NULL)
+		return;
 
-	while (cur_item != NULL)
+	hash_seq_init(&seq_status, on_commits);
+	PG_TRY();
 	{
-		OnCommitItem *oc = (OnCommitItem *) lfirst(cur_item);
-
-		if (isCommit ? oc->deleting_subid != InvalidSubTransactionId :
-			oc->creating_subid != InvalidSubTransactionId)
+		while ((oc = hash_seq_search(&seq_status)) != NULL)
 		{
-			/* cur_item must be removed */
-			on_commits = list_delete_cell(on_commits, cur_item, prev_item);
-			pfree(oc);
-			if (prev_item)
-				cur_item = lnext(prev_item);
+			if (isCommit ? oc->deleting_subid != InvalidSubTransactionId :
+				oc->creating_subid != InvalidSubTransactionId)
+			{
+				/* current item must be removed */
+				hash_search(on_commits, &oc->relid, HASH_REMOVE, NULL);
+			}
 			else
-				cur_item = list_head(on_commits);
-		}
-		else
-		{
-			/* cur_item must be preserved */
-			oc->creating_subid = InvalidSubTransactionId;
-			oc->deleting_subid = InvalidSubTransactionId;
-			prev_item = cur_item;
-			cur_item = lnext(prev_item);
+			{
+				/* current item must be preserved */
+				oc->creating_subid = InvalidSubTransactionId;
+				oc->deleting_subid = InvalidSubTransactionId;
+			}
 		}
 	}
+	PG_CATCH();
+	{
+		hash_seq_term(&seq_status);
+	}
+	PG_END_TRY();
 }
 
 /*
@@ -7728,35 +7825,35 @@ void
 AtEOSubXact_on_commit_actions(bool isCommit, SubTransactionId mySubid,
 							  SubTransactionId parentSubid)
 {
-	ListCell   *cur_item;
-	ListCell   *prev_item;
+	HASH_SEQ_STATUS seq_status;
+	OnCommitItem *oc;
 
-	prev_item = NULL;
-	cur_item = list_head(on_commits);
+	if (on_commits == NULL)
+		return;
 
-	while (cur_item != NULL)
+	hash_seq_init(&seq_status, on_commits);
+	PG_TRY();
 	{
-		OnCommitItem *oc = (OnCommitItem *) lfirst(cur_item);
-
-		if (!isCommit && oc->creating_subid == mySubid)
+		while ((oc = hash_seq_search(&seq_status)) != NULL)
 		{
-			/* cur_item must be removed */
-			on_commits = list_delete_cell(on_commits, cur_item, prev_item);
-			pfree(oc);
-			if (prev_item)
-				cur_item = lnext(prev_item);
+			if (!isCommit && oc->creating_subid == mySubid)
+			{
+				/* current item must be removed */
+				hash_search(on_commits, &oc->relid, HASH_REMOVE, NULL);
+			}
 			else
-				cur_item = list_head(on_commits);
-		}
-		else
-		{
-			/* cur_item must be preserved */
-			if (oc->creating_subid == mySubid)
-				oc->creating_subid = parentSubid;
-			if (oc->deleting_subid == mySubid)
-				oc->deleting_subid = isCommit ? parentSubid : InvalidSubTransactionId;
-			prev_item = cur_item;
-			cur_item = lnext(prev_item);
+			{
+				/* current item must be preserved */
+				if (oc->creating_subid == mySubid)
+					oc->creating_subid = parentSubid;
+				if (oc->deleting_subid == mySubid)
+					oc->deleting_subid = isCommit ? parentSubid : InvalidSubTransactionId;
+			}
 		}
 	}
+	PG_CATCH();
+	{
+		hash_seq_term(&seq_status);
+	}
+	PG_END_TRY();
 }
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index ccc6562..eb7d70f 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -35,6 +35,7 @@
 #include "access/transam.h"
 #include "access/twophase.h"
 #include "access/twophase_rmgr.h"
+#include "commands/tablecmds.h"
 #include "miscadmin.h"
 #include "pg_trace.h"
 #include "pgstat.h"
@@ -1863,6 +1864,17 @@ AtPrepare_Locks(void)
 		if (locallock->nLocks <= 0)
 			continue;
 
+		/*
+		 * Locks on ON COMMIT DELETE ROWS temp tables are released in
+		 * PREPARE TRANSACTION phase
+		 */
+		if (locallock->tag.lock.locktag_type == LOCKTAG_RELATION)
+		{
+			Oid relid = locallock->tag.lock.locktag_field2;
+			if (check_prepare_safe_temp_table(relid))
+				continue;
+		}
+
 		/* Scan to verify there are no session locks */
 		for (i = locallock->numLockOwners - 1; i >= 0; i--)
 		{
@@ -1944,6 +1956,17 @@ PostPrepare_Locks(TransactionId xid)
 		if (locallock->tag.lock.locktag_type == LOCKTAG_VIRTUALTRANSACTION)
 			continue;
 
+		/*
+		 * Locks on ON COMMIT DELETE ROWS temp tables are released in
+		 * PREPARE TRANSACTION phase
+		 */
+		if (locallock->tag.lock.locktag_type == LOCKTAG_RELATION)
+		{
+			Oid relid = locallock->tag.lock.locktag_field2;
+			if (check_prepare_safe_temp_table(relid))
+				continue;
+		}
+
 		/* We already checked there are no session locks */
 
 		/* Mark the proclock to show we need to release this lockmode */
@@ -1993,6 +2016,17 @@ PostPrepare_Locks(TransactionId xid)
 			if (lock->tag.locktag_type == LOCKTAG_VIRTUALTRANSACTION)
 				goto next_item;
 
+			/*
+			 * Locks on ON COMMIT DELETE ROWS temp tables are released in
+			 * PREPARE TRANSACTION phase
+			 */
+			if (lock->tag.locktag_type == LOCKTAG_RELATION)
+			{
+				Oid relid = lock->tag.locktag_field2;
+				if (check_prepare_safe_temp_table(relid))
+					goto next_item;
+			}
+
 			PROCLOCK_PRINT("PostPrepare_Locks", proclock);
 			LOCK_PRINT("PostPrepare_Locks", lock, 0);
 			Assert(lock->nRequested >= 0);
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index dad6ef8..5fca0d6 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -44,9 +44,6 @@ extern bool XactReadOnly;
 /* Asynchronous commits */
 extern bool XactSyncCommit;
 
-/* Kluge for 2PC support */
-extern bool MyXactAccessedTempRel;
-
 /*
  *	start- and end-of-transaction callbacks for dynamically loaded modules
  */
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 5a43d27..81dd51c 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -63,6 +63,7 @@ extern void change_varattnos_of_a_node(Node *node, const AttrNumber *newattno);
 
 extern void register_on_commit_action(Oid relid, OnCommitAction action);
 extern void remove_on_commit_action(Oid relid);
+extern bool check_prepare_safe_temp_table(Oid relid);
 
 extern void PreCommit_on_commit_actions(void);
 extern void AtEOXact_on_commit_actions(bool isCommit);