Re: BUG #17122: panic on prepare with subsequent pg_advisory_lock() and pg_advisory_xact_lock_shared()

I wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
>> The following statements lead to panic:
>> begin;
>> select pg_advisory_lock(1);
>> select pg_advisory_xact_lock_shared(1);
>> prepare transaction 'test';

> Thanks for the report!  Looks like the shared lock is sufficient
> to cause the problem:

Ah, scratch that.  I can't reproduce it now, and I think I messed
up yesterday by not remembering that pg_advisory_lock() takes a
session-level lock that would continue to be held after PREPARE.

> I also tried this:
> ...
> regression=*# prepare transaction 'test';
> ERROR:  cannot PREPARE while holding both session-level and transaction-level locks on the same object
> which makes me wonder why we didn't issue that error in your
> example case.

The reason why not is that the code that's meant to detect that is
just fundamentally inadequate.  It's examining individual LOCALLOCK
entries to detect conflicts, but we keep separate LOCALLOCK entries
for different lockmodes on the same object.  So pg_advisory_lock()
creates an entry with lockmode ExclusiveLock, while
pg_advisory_xact_lock_shared() creates a different entry with lockmode
AccessShareLock.  But they are pointing at the same PROCLOCK, and the
restriction we're dealing with here applies at the PROCLOCK level.

So I think we need something like the attached to fix this.

In the longer term, maybe it'd be better to rethink how we represent
LOCALLOCK entries so that they are one-to-one with PROCLOCKs.  But
rearranging that data structure for the convenience of PREPARE
TRANSACTION is probably going to be a net loss performance-wise.
In any case, we certainly wouldn't risk back-patching such a change.

            regards, tom lane

diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 108b4d9023..8c2138f107 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3204,6 +3204,102 @@ LockRefindAndRelease(LockMethod lockMethodTable, PGPROC *proc,
     }
 }

+/*
+ * CheckForSessionAndXactLocks
+ *        Check to see if transaction holds both session-level and xact-level
+ *        locks on the same object; if so, throw an error.
+ *
+ * If we have both session- and transaction-level locks on the same object,
+ * PREPARE TRANSACTION must fail.  This should never happen with regular
+ * locks, since we only take those at session level in some special operations
+ * like VACUUM.  It's possible to hit this with advisory locks, though.
+ *
+ * It would be nice if we could keep the session hold and give away the
+ * transactional hold to the prepared xact.  However, that would require two
+ * PROCLOCK objects, and we cannot be sure that another PROCLOCK will be
+ * available when it comes time for PostPrepare_Locks to do the deed.
+ * So for now, we error out while we can still do so safely.
+ *
+ * Since the LOCALLOCK table stores a separate entry for each lockmode,
+ * we can't implement this check by examining LOCALLOCK entries in isolation.
+ * We must build a transient hashtable that is indexed by locktag only.
+ */
+static void
+CheckForSessionAndXactLocks(void)
+{
+    typedef struct
+    {
+        LOCKTAG        lock;        /* identifies the lockable object */
+        bool        sessLock;    /* is any lockmode held at session level? */
+        bool        xactLock;    /* is any lockmode held at xact level? */
+    } PerLockTagEntry;
+
+    HASHCTL        hash_ctl;
+    HTAB       *lockhtab;
+    HASH_SEQ_STATUS status;
+    LOCALLOCK  *locallock;
+
+    /* Create a local hash table keyed by LOCKTAG only */
+    hash_ctl.keysize = sizeof(LOCKTAG);
+    hash_ctl.entrysize = sizeof(PerLockTagEntry);
+    hash_ctl.hcxt = CurrentMemoryContext;
+
+    lockhtab = hash_create("CheckForSessionAndXactLocks table",
+                           256, /* arbitrary initial size */
+                           &hash_ctl,
+                           HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+    /* Scan local lock table to find entries for each LOCKTAG */
+    hash_seq_init(&status, LockMethodLocalHash);
+
+    while ((locallock = (LOCALLOCK *) hash_seq_search(&status)) != NULL)
+    {
+        LOCALLOCKOWNER *lockOwners = locallock->lockOwners;
+        PerLockTagEntry *hentry;
+        bool        found;
+        int            i;
+
+        /*
+         * Ignore VXID locks.  We don't want those to be held by prepared
+         * transactions, since they aren't meaningful after a restart.
+         */
+        if (locallock->tag.lock.locktag_type == LOCKTAG_VIRTUALTRANSACTION)
+            continue;
+
+        /* Ignore it if we don't actually hold the lock */
+        if (locallock->nLocks <= 0)
+            continue;
+
+        /* Otherwise, find or make an entry in lockhtab */
+        hentry = (PerLockTagEntry *) hash_search(lockhtab,
+                                                 (void *) &locallock->tag.lock,
+                                                 HASH_ENTER, &found);
+        if (!found)                /* initialize, if newly created */
+            hentry->sessLock = hentry->xactLock = false;
+
+        /* Scan to see if we hold lock at session or xact level or both */
+        for (i = locallock->numLockOwners - 1; i >= 0; i--)
+        {
+            if (lockOwners[i].owner == NULL)
+                hentry->sessLock = true;
+            else
+                hentry->xactLock = true;
+        }
+
+        /*
+         * We can throw error immediately when we see both types of locks; no
+         * need to wait around to see if there are more violations.
+         */
+        if (hentry->sessLock && hentry->xactLock)
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("cannot PREPARE while holding both session-level and transaction-level locks on the same
object")));
+    }
+
+    /* Success, so clean up */
+    hash_destroy(lockhtab);
+}
+
 /*
  * AtPrepare_Locks
  *        Do the preparatory work for a PREPARE: make 2PC state file records
@@ -3211,11 +3307,10 @@ LockRefindAndRelease(LockMethod lockMethodTable, PGPROC *proc,
  *
  * Session-level locks are ignored, as are VXID locks.
  *
- * There are some special cases that we error out on: we can't be holding any
- * locks at both session and transaction level (since we must either keep or
- * give away the PROCLOCK object), and we can't be holding any locks on
- * temporary objects (since that would mess up the current backend if it tries
- * to exit before the prepared xact is committed).
+ * For the most part, we don't need to touch shared memory for this ---
+ * all the necessary state information is in the locallock table.
+ * Fast-path locks are an exception, however: we move any such locks to
+ * the main table before allowing PREPARE TRANSACTION to succeed.
  */
 void
 AtPrepare_Locks(void)
@@ -3223,12 +3318,10 @@ AtPrepare_Locks(void)
     HASH_SEQ_STATUS status;
     LOCALLOCK  *locallock;

-    /*
-     * For the most part, we don't need to touch shared memory for this ---
-     * all the necessary state information is in the locallock table.
-     * Fast-path locks are an exception, however: we move any such locks to
-     * the main table before allowing PREPARE TRANSACTION to succeed.
-     */
+    /* First, verify there aren't locks of both xact and session level */
+    CheckForSessionAndXactLocks();
+
+    /* Now do the per-locallock cleanup work */
     hash_seq_init(&status, LockMethodLocalHash);

     while ((locallock = (LOCALLOCK *) hash_seq_search(&status)) != NULL)
@@ -3264,19 +3357,7 @@ AtPrepare_Locks(void)
         if (!haveXactLock)
             continue;

-        /*
-         * If we have both session- and transaction-level locks, fail.  This
-         * should never happen with regular locks, since we only take those at
-         * session level in some special operations like VACUUM.  It's
-         * possible to hit this with advisory locks, though.
-         *
-         * It would be nice if we could keep the session hold and give away
-         * the transactional hold to the prepared xact.  However, that would
-         * require two PROCLOCK objects, and we cannot be sure that another
-         * PROCLOCK will be available when it comes time for PostPrepare_Locks
-         * to do the deed.  So for now, we error out while we can still do so
-         * safely.
-         */
+        /* This can't happen, because we already checked it */
         if (haveSessionLock)
             ereport(ERROR,
                     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index eb77c18788..ba8e3ccc6c 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -151,6 +151,22 @@ SELECT gid FROM pg_prepared_xacts;

 -- Clean up
 DROP TABLE pxtest1;
+-- Test detection of session-level and xact-level locks on same object
+BEGIN;
+SELECT pg_advisory_lock(1);
+ pg_advisory_lock
+------------------
+
+(1 row)
+
+SELECT pg_advisory_xact_lock_shared(1);
+ pg_advisory_xact_lock_shared
+------------------------------
+
+(1 row)
+
+PREPARE TRANSACTION 'foo6';  -- fails
+ERROR:  cannot PREPARE while holding both session-level and transaction-level locks on the same object
 -- Test subtransactions
 BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
   CREATE TABLE pxtest2 (a int);
diff --git a/src/test/regress/expected/prepared_xacts_1.out b/src/test/regress/expected/prepared_xacts_1.out
index 0857d259e0..2cd50ad947 100644
--- a/src/test/regress/expected/prepared_xacts_1.out
+++ b/src/test/regress/expected/prepared_xacts_1.out
@@ -153,6 +153,23 @@ SELECT gid FROM pg_prepared_xacts;

 -- Clean up
 DROP TABLE pxtest1;
+-- Test detection of session-level and xact-level locks on same object
+BEGIN;
+SELECT pg_advisory_lock(1);
+ pg_advisory_lock
+------------------
+
+(1 row)
+
+SELECT pg_advisory_xact_lock_shared(1);
+ pg_advisory_xact_lock_shared
+------------------------------
+
+(1 row)
+
+PREPARE TRANSACTION 'foo6';  -- fails
+ERROR:  prepared transactions are disabled
+HINT:  Set max_prepared_transactions to a nonzero value.
 -- Test subtransactions
 BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
   CREATE TABLE pxtest2 (a int);
diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql
index d8249a27dc..2f0bb55bb4 100644
--- a/src/test/regress/sql/prepared_xacts.sql
+++ b/src/test/regress/sql/prepared_xacts.sql
@@ -88,6 +88,12 @@ SELECT gid FROM pg_prepared_xacts;
 -- Clean up
 DROP TABLE pxtest1;

+-- Test detection of session-level and xact-level locks on same object
+BEGIN;
+SELECT pg_advisory_lock(1);
+SELECT pg_advisory_xact_lock_shared(1);
+PREPARE TRANSACTION 'foo6';  -- fails
+
 -- Test subtransactions
 BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
   CREATE TABLE pxtest2 (a int);