Re: Transactions and temp tables - Mailing list pgsql-hackers

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
COMMITDELETE 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);