Re: Transactions and temp tables

Hi,

I am attaching a new patch that deals with the issue of the locks on
temporary tables that have been accessed in transactions that have been
prepared but not committed.
I have added another list that keeps track of temp tables accessed by
transactions that are prepared but not committed. The RemoveTempTable
callback does not try to acquire locks on these tables. Now postmaster
can terminate even with transactions in the prepared state that accessed
temp tables.

Let me know what you think.
manu

Tom Lane wrote:
> Emmanuel Cecchet <manu@frogthinker.org> writes:
>
>> Ok, so actually I don't see any different behavior between a temp table
>> or a regular table. The locking happens the same way and as long as the
>> commit prepared happens (potentially in another session), the lock is
>> released at commit time which seems to make sense.
>>
>
> Right, the problem is that you can't shut down the original backend
> because it'll try to drop the temp table at exit, and then block on
> the lock that the prepared xact is holding.  From a database management
> standpoint that is unacceptable --- it means for instance that you can't
> shut down the database cleanly while such a prepared transaction is
> pending.  The difference from a regular table is that no such automatic
> action is taken at backend exit for regular tables.
>
> The whole business of having restrictions on temp table access is
> annoying; I wish we could get rid of them not add complexity to
> enforcing them.  The local-buffer-management end of the issue seems
> readily solvable: we need only decree that PREPARE has to flush out any
> dirty local buffers (and maybe discard local buffers altogether, not
> sure).  But I've not been able to see a decent solution to the lock
> behavior.
>
>             regards, tom lane
>
>

### Eclipse Workspace Patch 1.0
#P Postgres-HEAD
Index: src/backend/access/heap/heapam.c
===================================================================
RCS file: /root/cvsrepo/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.264
diff -u -r1.264 heapam.c
--- src/backend/access/heap/heapam.c    30 Sep 2008 10:52:10 -0000    1.264
+++ src/backend/access/heap/heapam.c    9 Oct 2008 21:44:04 -0000
@@ -878,7 +878,7 @@

     /* Make note that we've accessed a temporary relation */
     if (r->rd_istemp)
-        MyXactAccessedTempRel = true;
+        enlistRelationIdFor2PCChecks(relationId);

     pgstat_initstats(r);

@@ -926,7 +926,7 @@

     /* Make note that we've accessed a temporary relation */
     if (r->rd_istemp)
-        MyXactAccessedTempRel = true;
+        enlistRelationIdFor2PCChecks(relationId);

     pgstat_initstats(r);

@@ -976,7 +976,7 @@

     /* Make note that we've accessed a temporary relation */
     if (r->rd_istemp)
-        MyXactAccessedTempRel = true;
+        enlistRelationIdFor2PCChecks(relationId);

     pgstat_initstats(r);

Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /root/cvsrepo/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.265
diff -u -r1.265 xact.c
--- src/backend/access/transam/xact.c    11 Aug 2008 11:05:10 -0000    1.265
+++ src/backend/access/transam/xact.c    9 Oct 2008 21:44:04 -0000
@@ -47,6 +47,7 @@
 #include "utils/memutils.h"
 #include "utils/relcache.h"
 #include "utils/snapmgr.h"
+#include "utils/tqual.h"
 #include "utils/xml.h"
 #include "pg_trace.h"

@@ -65,13 +66,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
@@ -209,6 +203,13 @@
  */
 static MemoryContext TransactionAbortContext = NULL;

+/* Hash table containing Oids of accessed temporary relations */
+HTAB    *accessedTempRel;
+/* Hash table containing Oids of accessed temporary relations that have been
+ * prepared commit but not committed yet
+ */
+HTAB    *preparedTempRel;
+
 /*
  * List of add-on start- and end-of-xact callbacks
  */
@@ -250,6 +251,7 @@
                      SubTransactionId mySubid,
                      SubTransactionId parentSubid);
 static void CleanupTransaction(void);
+static void CleanupAccessedTempRel(void);
 static void CommitTransaction(void);
 static TransactionId RecordTransactionAbort(bool isSubXact);
 static void StartTransaction(void);
@@ -624,7 +626,7 @@

         /* Propagate new command ID into static snapshots */
         SnapshotSetCommandId(currentCommandId);
-
+
         /*
          * Make any catalog changes done by the just-completed command
          * visible in the local syscache.  We obviously don't need to do
@@ -1119,10 +1121,10 @@
          */
         if (s->parent->childXids == NULL)
             new_childXids =
-                MemoryContextAlloc(TopTransactionContext,
+                MemoryContextAlloc(TopTransactionContext,
                                    new_maxChildXids * sizeof(TransactionId));
         else
-            new_childXids = repalloc(s->parent->childXids,
+            new_childXids = repalloc(s->parent->childXids,
                                      new_maxChildXids * sizeof(TransactionId));

         s->parent->childXids  = new_childXids;
@@ -1511,7 +1513,6 @@
     XactIsoLevel = DefaultXactIsoLevel;
     XactReadOnly = DefaultXactReadOnly;
     forceSyncCommit = false;
-    MyXactAccessedTempRel = false;

     /*
      * reinitialize within-transaction counters
@@ -1760,6 +1761,8 @@

     AtCommit_Memory();

+    CleanupAccessedTempRel();
+
     s->transactionId = InvalidTransactionId;
     s->subTransactionId = InvalidSubTransactionId;
     s->nestingLevel = 0;
@@ -1777,6 +1780,74 @@
     RESUME_INTERRUPTS();
 }

+/* ----------------
+ *     enlistRelationIdFor2PCChecks - enlist a relation in the list of
+ *      resources to check at PREPARE COMMIT time if we are part of
+ *      a 2PC transaction. The resource will be removed from the list
+ *      if the table is dropped before commit.
+ * ----------------
+ */
+void
+enlistRelationIdFor2PCChecks(Oid relationId)
+{
+    Oid *tid;
+
+    /*
+     * Each time a temporary relation is accessed, it is added to the
+     * accessedTempRel list. PREPARE TRANSACTION will fail if any
+     * of the accessed relation is still valid (not dropped).  (This is
+     * called from from heapam.c.)
+     */
+    if (accessedTempRel == NULL)
+    { // Allocate the list on-demand
+        HASHCTL ctl;
+
+        ctl.keysize = sizeof(Oid);
+        ctl.entrysize = sizeof(Oid);
+        accessedTempRel = hash_create("accessed temp tables", 4, &ctl,
+                HASH_ELEM);
+    }
+
+    // Add to the hash list if missing
+    tid = hash_search(accessedTempRel, &relationId, HASH_ENTER, NULL);
+    *tid = relationId;
+}
+
+/*
+ * Cleanup the list of prepared temp tables that were accessed during this transaction.
+ */
+static void CleanupAccessedTempRel(void)
+{
+    if (accessedTempRel != NULL)
+    {
+        if (preparedTempRel != NULL)
+        { // We have prepared transactions with temp tables
+            HASH_SEQ_STATUS    status;
+            Oid*            tempTableOid;
+
+            hash_seq_init(&status, accessedTempRel);
+            while ((tempTableOid = (Oid *) hash_seq_search(&status)) != NULL)
+            { // Add all relations to the hash list
+                hash_search(preparedTempRel, tempTableOid, HASH_REMOVE, NULL);
+            }
+        }
+        hash_destroy(accessedTempRel);
+        accessedTempRel = NULL;
+    }
+}
+
+/*
+ * Returns true if the provided relationId is a temp table that has been
+ * prepared commit but not committed yet.
+ */
+bool
+isPreparedTempRelation(Oid relationId)
+{
+    if (preparedTempRel == NULL)
+        return false;
+
+    return hash_search(preparedTempRel, &relationId, HASH_FIND, NULL) != NULL;
+}

 /*
  *    PrepareTransaction
@@ -1853,14 +1924,61 @@
      * 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.
+     * We only allow to proceed further if the accessed temp tables have
+     * been dropped before PREPARE COMMIT.
      */
-    if (MyXactAccessedTempRel)
-        ereport(ERROR,
-                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                 errmsg("cannot PREPARE a transaction that has operated on temporary tables")));
+    if (accessedTempRel != NULL)
+    {
+        HASH_SEQ_STATUS    status;
+        Oid*            tempTableOid;
+
+        /* Prevent further updates to the list as recommended in hash_seq_init doc. */
+        hash_freeze(accessedTempRel);
+        hash_seq_init(&status, accessedTempRel);
+        while ((tempTableOid = (Oid *) hash_seq_search(&status)) != NULL)
+        { /* Check all accessed temp tables. If the table has been dropped,
+           * try_relation_open will fail and we can safely continue. */
+            Relation tempTable = try_relation_open(*tempTableOid, NoLock);
+
+            if (tempTable != NULL)
+            { /* We have an open temp table at PREPARE COMMIT time. We
+               * will only accept empty temp tables and throw an error
+               * in other cases. */
+                HeapScanDesc scan;
+                HeapTuple tuple;
+                scan = heap_beginscan(tempTable, SnapshotNow, 0, NULL);
+                if ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+                {
+                    hash_seq_term(&status);
+                    hash_destroy(accessedTempRel);
+                    accessedTempRel = NULL;
+                    ereport(ERROR,
+                            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                    errmsg("cannot PREPARE a transaction that has operated on temporary tables that
arenot empty at commit time"))); 
+                }
+                heap_endscan(scan);
+                relation_close(tempTable, NoLock);
+            }
+        }
+
+        // Success! All temp relations are empty, we need to add them to the list of prepared commit temp relations
+        if (preparedTempRel == NULL)
+        { // Create list if it does not exist yet
+            HASHCTL ctl;
+
+            ctl.keysize = sizeof(Oid);
+            ctl.entrysize = sizeof(Oid);
+            preparedTempRel = hash_create("prepared temp tables", 10, &ctl,
+                    HASH_ELEM);
+        }
+
+        hash_seq_init(&status, accessedTempRel);
+        while ((tempTableOid = (Oid *) hash_seq_search(&status)) != NULL)
+        { // Add all relations to the hash list
+            Oid *tid = hash_search(preparedTempRel, tempTableOid, HASH_ENTER, NULL);
+            *tid = *tempTableOid;
+        }
+    }

     /* Prevent cancel/die interrupt while cleaning up */
     HOLD_INTERRUPTS();
@@ -2151,6 +2269,14 @@
         elog(FATAL, "CleanupTransaction: unexpected state %s",
              TransStateAsString(s->state));

+    if (accessedTempRel != NULL)
+    {
+        CleanupAccessedTempRel();
+
+        hash_destroy(accessedTempRel);
+        accessedTempRel = NULL;
+    }
+
     /*
      * do abort cleanup processing
      */
Index: src/include/access/xact.h
===================================================================
RCS file: /root/cvsrepo/pgsql/src/include/access/xact.h,v
retrieving revision 1.95
diff -u -r1.95 xact.h
--- src/include/access/xact.h    11 Aug 2008 11:05:11 -0000    1.95
+++ src/include/access/xact.h    9 Oct 2008 21:44:04 -0000
@@ -18,6 +18,7 @@
 #include "nodes/pg_list.h"
 #include "storage/relfilenode.h"
 #include "utils/timestamp.h"
+#include "postgres_ext.h"


 /*
@@ -44,8 +45,10 @@
 /* Asynchronous commits */
 extern bool XactSyncCommit;

-/* Kluge for 2PC support */
-extern bool MyXactAccessedTempRel;
+/* List of temp tables accessed during a transaction for 2PC support */
+extern void enlistRelationIdFor2PCChecks(Oid relationId);
+/* Check if a temp table has been prepared for 2PC but not committed yet */
+extern bool isPreparedTempRelation(Oid relationId);

 /*
  *    start- and end-of-transaction callbacks for dynamically loaded modules
Index: src/backend/catalog/dependency.c
===================================================================
RCS file: /root/cvsrepo/pgsql/src/backend/catalog/dependency.c,v
retrieving revision 1.81
diff -u -r1.81 dependency.c
--- src/backend/catalog/dependency.c    4 Oct 2008 21:56:52 -0000    1.81
+++ src/backend/catalog/dependency.c    9 Oct 2008 21:44:04 -0000
@@ -677,6 +677,12 @@
         otherObject.objectId = foundDep->objid;
         otherObject.objectSubId = foundDep->objsubid;

+        if (isPreparedTempRelation(foundDep->objid))
+        {
+            elog(DEBUG3, "temp relation %d is already prepared, ignoring", foundDep->objid);
+            continue;
+        }
+
         /*
          * Must lock the dependent object before recursing to it.
          */