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);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
