From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
To: Emmanuel Cecchet <manu@frogthinker.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Transactions and temp tables
Date: Tue, 18 Nov 2008 13:31:36 +0200
Message-ID: <4922A798.7050709@enterprisedb.com> (raw)
In-Reply-To: <492221A3.1060608@frogthinker.org>
References: <48EA7B12.6050906@frogthinker.org>
<48EB0551.50205@enterprisedb.com>
<48EB95B7.10604@frogthinker.org>
<48EC7EB5.5020001@enterprisedb.com>
<48ECAA8F.3070306@frogthinker.org>
<28137.1223472191@sss.pgh.pa.us>
<48ECBBC2.6060003@frogthinker.org>
<29132.1223475486@sss.pgh.pa.us>
<48EFA13C.2060407@frogthinker.org>
<490F804A.60108@enterprisedb.com>
<490FB4B3.908@frogthinker.org>
<49106B10.2090407@enterprisedb.com>
<4910DFC0.3080301@frogthinker.org>
<4914CD14.2060608@frogthinker.org>
<4921C42A.3030600@enterprisedb.com>
<492221A3.1060608@frogthinker.org>
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.comAttachments:2pc-temp-tables-heikki-1.patch (text/x-diff, 16.4K) downloaddiff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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.hindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: heikki.linnakangas@enterprisedb.com, manu@frogthinker.org, tgl@sss.pgh.pa.us
Subject: Re: Transactions and temp tables
In-Reply-To: <4922A798.7050709@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox