Overdue code review for transaction-level advisory locks patch.

Commit 62c7bd31c8878dd45c9b9b2429ab7a12103f3590 had assorted problems, most
visibly that it broke PREPARE TRANSACTION in the presence of session-level
advisory locks (which should be ignored by PREPARE), as per a recent
complaint from Stephen Rees.  More abstractly, the patch made the
LockMethodData.transactional flag not merely useless but outright
dangerous, because in point of fact that flag no longer tells you anything
at all about whether a lock is held transactionally.  This fix therefore
removes that flag altogether.  We now rely entirely on the convention
already in use in lock.c that transactional lock holds must be owned by
some ResourceOwner, while session holds are never so owned.  Setting the
locallock struct's owner link to NULL thus denotes a session hold, and
there is no redundant marker for that.

PREPARE TRANSACTION now works again when there are session-level advisory
locks, and it is also able to transfer transactional advisory locks to the
prepared transaction, but for implementation reasons it throws an error if
we hold both types of lock on a single lockable object.  Perhaps it will be
worth improving that someday.

Assorted other minor cleanup and documentation editing, as well.

Back-patch to 9.1, except that in the 9.1 branch I did not remove the
LockMethodData.transactional flag for fear of causing an ABI break for
any external code that might be examining those structs.

1 files changed, 5 insertions, 7 deletions

diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index faddef579c7..17b894285ba 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -96,15 +96,12 @@ typedef int LOCKMODE;
 /*
  * This data structure defines the locking semantics associated with a
  * "lock method".  The semantics specify the meaning of each lock mode
- * (by defining which lock modes it conflicts with), and also whether locks
- * of this method are transactional (ie, are released at transaction end).
+ * (by defining which lock modes it conflicts with).
  * All of this data is constant and is kept in const tables.
  *
  * numLockModes -- number of lock modes (READ,WRITE,etc) that
  *		are defined in this lock method.  Must be less than MAX_LOCKMODES.
  *
- * transactional -- TRUE if locks are released automatically at xact end.
- *
  * conflictTab -- this is an array of bitmasks showing lock
  *		mode conflicts.  conflictTab[i] is a mask with the j-th bit
  *		turned on if lock modes i and j conflict.  Lock modes are
@@ -112,12 +109,13 @@ typedef int LOCKMODE;
  *
  * lockModeNames -- ID strings for debug printouts.
  *
- * trace_flag -- pointer to GUC trace flag for this lock method.
+ * trace_flag -- pointer to GUC trace flag for this lock method.  (The
+ * GUC variable is not constant, but we use "const" here to denote that
+ * it can't be changed through this reference.)
  */
 typedef struct LockMethodData
 {
 	int			numLockModes;
-	bool		transactional;
 	const LOCKMASK *conflictTab;
 	const char *const * lockModeNames;
 	const bool *trace_flag;
@@ -492,8 +490,8 @@ extern LockAcquireResult LockAcquireExtended(const LOCKTAG *locktag,
 extern void AbortStrongLockAcquire(void);
 extern bool LockRelease(const LOCKTAG *locktag,
 			LOCKMODE lockmode, bool sessionLock);
-extern void LockReleaseSession(LOCKMETHODID lockmethodid);
 extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks);
+extern void LockReleaseSession(LOCKMETHODID lockmethodid);
 extern void LockReleaseCurrentOwner(void);
 extern void LockReassignCurrentOwner(void);
 extern VirtualTransactionId *GetLockConflicts(const LOCKTAG *locktag,