Subject: Re: FOR SHARE vs FOR UPDATE locks
From: "Joshua D. Drake" <jd@commandprompt.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <heikki@enterprisedb.com>,
	Alvaro Herrera <alvherre@commandprompt.com>,
	Simon Riggs <simon@2ndquadrant.com>, pgsql-hackers@postgresql.org
In-Reply-To: <24606.1164993538@sss.pgh.pa.us>
References: <1144.1164924373@sss.pgh.pa.us>
	<1164962544.3778.847.camel@silverbirch.site>
	<20061201113711.GC30441@alvh.no-ip.org>
	<15834.1164985926@sss.pgh.pa.us>
	<45705E9C.2060003@enterprisedb.com>  <24606.1164993538@sss.pgh.pa.us>
Content-Type: text/plain
Organization: Command Prompt, Inc.
Date: Fri, 01 Dec 2006 09:34:37 -0800
Message-Id: <1164994477.29643.33.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

On Fri, 2006-12-01 at 12:18 -0500, Tom Lane wrote:
> "Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> > That way, the lock won't be downgraded back to a shared lock on 
> > "rollback to savepoint", right? Though it's still better than throwing 
> > an error, I think.

So for us non-c programmers out there may I clarify?

If I have a long running transaction that uses multiple savepoints. If I
rollback to a save point the tuple that was being modified before the
rollback will have an exclusive lock?

At what point is the exclusive lock released? When I create a new
savepoint? On COMMIT of the entire transaction?

Joshua D. Drake


> 
> Correct, a rollback would leave the tuple still exclusive-locked.
> So not perfect, but it's hard to see how to do better without a whole
> lot more infrastructure, which the case is probably not worth.
> 
> I've just finished coding up the patch --- untested as yet, but anyone
> see any problems?
> 
> 			regards, tom lane
> 
> *** src/backend/access/heap/heapam.c.orig	Fri Nov 17 13:00:14 2006
> --- src/backend/access/heap/heapam.c	Fri Dec  1 12:18:04 2006
> ***************
> *** 2360,2365 ****
> --- 2360,2366 ----
>   	PageHeader	dp;
>   	TransactionId xid;
>   	TransactionId xmax;
> + 	TransactionId existing_subxact = InvalidTransactionId;
>   	uint16		old_infomask;
>   	uint16		new_infomask;
>   	LOCKMODE	tuple_lock_type;
> ***************
> *** 2398,2419 ****
>   		LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
>   
>   		/*
> ! 		 * If we wish to acquire share lock, and the tuple is already
> ! 		 * share-locked by a multixact that includes any subtransaction of the
> ! 		 * current top transaction, then we effectively hold the desired lock
> ! 		 * already.  We *must* succeed without trying to take the tuple lock,
> ! 		 * else we will deadlock against anyone waiting to acquire exclusive
> ! 		 * lock.  We don't need to make any state changes in this case.
>   		 */
> ! 		if (mode == LockTupleShared &&
> ! 			(infomask & HEAP_XMAX_IS_MULTI) &&
> ! 			MultiXactIdIsCurrent((MultiXactId) xwait))
>   		{
>   			Assert(infomask & HEAP_XMAX_SHARED_LOCK);
> ! 			/* Probably can't hold tuple lock here, but may as well check */
> ! 			if (have_tuple_lock)
> ! 				UnlockTuple(relation, tid, tuple_lock_type);
> ! 			return HeapTupleMayBeUpdated;
>   		}
>   
>   		/*
> --- 2399,2430 ----
>   		LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
>   
>   		/*
> ! 		 * If the tuple is currently share-locked by a multixact, we have to
> ! 		 * check whether the multixact includes any live subtransaction of the
> ! 		 * current top transaction.  If so, then we effectively already hold
> ! 		 * share-lock, even if that XID isn't the current subxact.  That's
> ! 		 * because no such subtransaction could be aborted without also
> ! 		 * aborting the current subtransaction, and so its locks are as good
> ! 		 * as ours.
>   		 */
> ! 		if (infomask & HEAP_XMAX_IS_MULTI)
>   		{
>   			Assert(infomask & HEAP_XMAX_SHARED_LOCK);
> ! 			existing_subxact = MultiXactIdGetCurrent((MultiXactId) xwait);
> ! 			/*
> ! 			 * Done if we have share lock and that's what the caller wants.
> ! 			 * We *must* check this before trying to take the tuple lock, else
> ! 			 * we will deadlock against anyone waiting to acquire exclusive
> ! 			 * lock.  We don't need to make any state changes in this case.
> ! 			 */
> ! 			if (mode == LockTupleShared &&
> ! 				TransactionIdIsValid(existing_subxact))
> ! 			{
> ! 				/* Probably can't hold tuple lock here, but check anyway */
> ! 				if (have_tuple_lock)
> ! 					UnlockTuple(relation, tid, tuple_lock_type);
> ! 				return HeapTupleMayBeUpdated;
> ! 			}
>   		}
>   
>   		/*
> ***************
> *** 2570,2593 ****
>   	if (!(old_infomask & (HEAP_XMAX_INVALID |
>   						  HEAP_XMAX_COMMITTED |
>   						  HEAP_XMAX_IS_MULTI)) &&
> - 		(mode == LockTupleShared ?
> - 		 (old_infomask & HEAP_IS_LOCKED) :
> - 		 (old_infomask & HEAP_XMAX_EXCL_LOCK)) &&
>   		TransactionIdIsCurrentTransactionId(xmax))
>   	{
> ! 		LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
> ! 		/* Probably can't hold tuple lock here, but may as well check */
> ! 		if (have_tuple_lock)
> ! 			UnlockTuple(relation, tid, tuple_lock_type);
> ! 		return HeapTupleMayBeUpdated;
>   	}
>   
>   	/*
>   	 * Compute the new xmax and infomask to store into the tuple.  Note we do
>   	 * not modify the tuple just yet, because that would leave it in the wrong
>   	 * state if multixact.c elogs.
>   	 */
> ! 	xid = GetCurrentTransactionId();
>   
>   	new_infomask = old_infomask & ~(HEAP_XMAX_COMMITTED |
>   									HEAP_XMAX_INVALID |
> --- 2581,2621 ----
>   	if (!(old_infomask & (HEAP_XMAX_INVALID |
>   						  HEAP_XMAX_COMMITTED |
>   						  HEAP_XMAX_IS_MULTI)) &&
>   		TransactionIdIsCurrentTransactionId(xmax))
>   	{
> ! 		/* The tuple is locked by some existing subxact ... */
> ! 		Assert(old_infomask & HEAP_IS_LOCKED);
> ! 		existing_subxact = xmax;
> ! 		/* ... but is it the desired lock type or stronger? */
> ! 		if (mode == LockTupleShared ||
> ! 			(old_infomask & HEAP_XMAX_EXCL_LOCK))
> ! 		{
> ! 			LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
> ! 			/* Probably can't hold tuple lock here, but check anyway */
> ! 			if (have_tuple_lock)
> ! 				UnlockTuple(relation, tid, tuple_lock_type);
> ! 			return HeapTupleMayBeUpdated;
> ! 		}
>   	}
>   
>   	/*
>   	 * Compute the new xmax and infomask to store into the tuple.  Note we do
>   	 * not modify the tuple just yet, because that would leave it in the wrong
>   	 * state if multixact.c elogs.
> + 	 *
> + 	 * If we are upgrading a shared lock held by another subxact to exclusive,
> + 	 * we need to mark the tuple as exclusively locked by the other subxact
> + 	 * not this one.  Otherwise, a rollback of this subxact would leave the
> + 	 * tuple apparently not locked at all.  We don't have enough
> + 	 * infrastructure to keep track of both types of tuple lock, so we
> + 	 * compromise by making the tuple appear to be exclusive-locked by the
> + 	 * other, possibly longer-lived subxact.  (Again, there are no cases where
> + 	 * a live subxact could be shorter-lived than the current one.)
>   	 */
> ! 	if (TransactionIdIsValid(existing_subxact))
> ! 		xid = existing_subxact;
> ! 	else
> ! 		xid = GetCurrentTransactionId();
>   
>   	new_infomask = old_infomask & ~(HEAP_XMAX_COMMITTED |
>   									HEAP_XMAX_INVALID |
> *** src/backend/access/transam/multixact.c.orig	Fri Nov 17 13:00:15 2006
> --- src/backend/access/transam/multixact.c	Fri Dec  1 12:17:57 2006
> ***************
> *** 381,388 ****
>   
>   	/*
>   	 * Checking for myself is cheap compared to looking in shared memory,
> ! 	 * so first do the equivalent of MultiXactIdIsCurrent().  This is not
> ! 	 * needed for correctness, it's just a fast path.
>   	 */
>   	for (i = 0; i < nmembers; i++)
>   	{
> --- 381,388 ----
>   
>   	/*
>   	 * Checking for myself is cheap compared to looking in shared memory,
> ! 	 * so try that first.  This is not needed for correctness, it's just a
> ! 	 * fast path.
>   	 */
>   	for (i = 0; i < nmembers; i++)
>   	{
> ***************
> *** 418,437 ****
>   }
>   
>   /*
> !  * MultiXactIdIsCurrent
> !  *		Returns true if the current transaction is a member of the MultiXactId.
>    *
> !  * We return true if any live subtransaction of the current top-level
> !  * transaction is a member.  This is appropriate for the same reason that a
> !  * lock held by any such subtransaction is globally equivalent to a lock
> !  * held by the current subtransaction: no such lock could be released without
> !  * aborting this subtransaction, and hence releasing its locks.  So it's not
> !  * necessary to add the current subxact to the MultiXact separately.
>    */
> ! bool
> ! MultiXactIdIsCurrent(MultiXactId multi)
>   {
> ! 	bool		result = false;
>   	TransactionId *members;
>   	int			nmembers;
>   	int			i;
> --- 418,437 ----
>   }
>   
>   /*
> !  * MultiXactIdGetCurrent
> !  *		If any live subtransaction of the current backend is a member of
> !  *		the MultiXactId, return its XID; else return InvalidTransactionId.
>    *
> !  * If the MXACT contains multiple such subtransactions, it is unspecified
> !  * which one is returned.  This doesn't matter in current usage because
> !  * heap_lock_tuple takes care not to insert multiple subtransactions of the
> !  * same backend into any MXACT.  If need be, we could modify this code to
> !  * return the oldest such subxact, or some such rule.
>    */
> ! TransactionId
> ! MultiXactIdGetCurrent(MultiXactId multi)
>   {
> ! 	TransactionId result = InvalidTransactionId;
>   	TransactionId *members;
>   	int			nmembers;
>   	int			i;
> ***************
> *** 439,451 ****
>   	nmembers = GetMultiXactIdMembers(multi, &members);
>   
>   	if (nmembers < 0)
> ! 		return false;
>   
>   	for (i = 0; i < nmembers; i++)
>   	{
>   		if (TransactionIdIsCurrentTransactionId(members[i]))
>   		{
> ! 			result = true;
>   			break;
>   		}
>   	}
> --- 439,451 ----
>   	nmembers = GetMultiXactIdMembers(multi, &members);
>   
>   	if (nmembers < 0)
> ! 		return result;
>   
>   	for (i = 0; i < nmembers; i++)
>   	{
>   		if (TransactionIdIsCurrentTransactionId(members[i]))
>   		{
> ! 			result = members[i];
>   			break;
>   		}
>   	}
> *** src/include/access/multixact.h.orig	Fri Nov 17 13:00:15 2006
> --- src/include/access/multixact.h	Fri Dec  1 12:17:49 2006
> ***************
> *** 45,51 ****
>   extern MultiXactId MultiXactIdCreate(TransactionId xid1, TransactionId xid2);
>   extern MultiXactId MultiXactIdExpand(MultiXactId multi, TransactionId xid);
>   extern bool MultiXactIdIsRunning(MultiXactId multi);
> ! extern bool MultiXactIdIsCurrent(MultiXactId multi);
>   extern void MultiXactIdWait(MultiXactId multi);
>   extern bool ConditionalMultiXactIdWait(MultiXactId multi);
>   extern void MultiXactIdSetOldestMember(void);
> --- 45,51 ----
>   extern MultiXactId MultiXactIdCreate(TransactionId xid1, TransactionId xid2);
>   extern MultiXactId MultiXactIdExpand(MultiXactId multi, TransactionId xid);
>   extern bool MultiXactIdIsRunning(MultiXactId multi);
> ! extern TransactionId MultiXactIdGetCurrent(MultiXactId multi);
>   extern void MultiXactIdWait(MultiXactId multi);
>   extern bool ConditionalMultiXactIdWait(MultiXactId multi);
>   extern void MultiXactIdSetOldestMember(void);
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
> 
-- 

      === The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240
Providing the most comprehensive  PostgreSQL solutions since 1997
             http://www.commandprompt.com/

Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate