Thread

FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-11-30T22:06:13Z

I just realized that we have a bit of a problem with upgrading row
locks.  Consider the following sequence:

regression=# begin;
BEGIN
regression=# select * from int4_tbl where f1 = 0 for share;
 f1
----
  0
(1 row)

regression=# savepoint x;
SAVEPOINT
regression=# select * from int4_tbl where f1 = 0 for update;
 f1
----
  0
(1 row)

regression=# rollback to x;
ROLLBACK

The FOR UPDATE replaces the former shared row lock with an exclusive
lock in the name of the subtransaction.  After the ROLLBACK, the row
appears not to be locked at all (it is ex-locked with XMAX = a failed
transaction), so another backend could come along and modify it.
That shouldn't happen --- we should act as though the outer
transaction's FOR SHARE lock is still held.

Unfortunately, I don't think there is any good way to implement that,
since we surely don't have room in the tuple header to track multiple
locks.  One possibility is to try to assign the ex-lock in the name
of the highest subtransaction holding a row lock, but that seems messy,
and it wouldn't really have the correct semantics anyway --- in the
above example, the outer transaction would be left holding ex-lock
which would be surprising.

I'm tempted to just error out in this scenario rather than allow the
lock upgrade.  Thoughts?

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Michael Paesold <mpaesold@gmx.at> — 2006-12-01T07:16:41Z

Tom Lane wrote:
 > I'm tempted to just error out in this scenario rather than allow the
 > lock upgrade.  Thoughts?

Although this seems to be a technically hard problem, the above sentence 
does not sound like the PostgreSQL way to solve problems (rather like 
MySQL). ;-)

Now seriously, isn't this a perfectly feasible scenario? E.g. the outer 
transaction acquires a shared lock because of foreign key constraints, and 
the sub transaction later wants to update that row?

Best Regards
Michael Paesold

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T07:46:59Z

Michael Paesold <mpaesold@gmx.at> writes:
> Tom Lane wrote:
>>> I'm tempted to just error out in this scenario rather than allow the
>>> lock upgrade.  Thoughts?

> Although this seems to be a technically hard problem, the above sentence 
> does not sound like the PostgreSQL way to solve problems (rather like 
> MySQL). ;-)

No, the MySQL way is to let it do something that's easy, fast, and sort
of works most of the time ;-)

> Now seriously, isn't this a perfectly feasible scenario? E.g. the outer 
> transaction acquires a shared lock because of foreign key constraints, and 
> the sub transaction later wants to update that row?

Yeah, it's not implausible.  But the only way I can see to implement
that is to upgrade the outer xact's shared lock to exclusive, and that
doesn't seem real cool either.

More to the point, we are up against a release deadline, and so the only
options we have today are for things that are simple, bulletproof, and
don't lock us out of doing something smarter later.  Hence my suggestion
to throw an error.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Simon Riggs <simon@2ndquadrant.com> — 2006-12-01T08:42:23Z

On Thu, 2006-11-30 at 17:06 -0500, Tom Lane wrote:

> I just realized that we have a bit of a problem with upgrading row
> locks.  Consider the following sequence:
> 
> regression=# begin;
> BEGIN
> regression=# select * from int4_tbl where f1 = 0 for share;
>  f1
> ----
>   0
> (1 row)
> 
> regression=# savepoint x;
> SAVEPOINT
> regression=# select * from int4_tbl where f1 = 0 for update;
>  f1
> ----
>   0
> (1 row)
> 
> regression=# rollback to x;
> ROLLBACK
> 
> The FOR UPDATE replaces the former shared row lock with an exclusive
> lock in the name of the subtransaction.  After the ROLLBACK, the row
> appears not to be locked at all (it is ex-locked with XMAX = a failed
> transaction), so another backend could come along and modify it.
> That shouldn't happen --- we should act as though the outer
> transaction's FOR SHARE lock is still held.
> 
> Unfortunately, I don't think there is any good way to implement that,
> since we surely don't have room in the tuple header to track multiple
> locks.  One possibility is to try to assign the ex-lock in the name
> of the highest subtransaction holding a row lock, but that seems messy,
> and it wouldn't really have the correct semantics anyway --- in the
> above example, the outer transaction would be left holding ex-lock
> which would be surprising.

ISTM that multitrans could be used here. Two xids, one xmax.

Maybe the semantics of that use are slightly different from the normal
queueing mechanism, but it seems straightforward enough.

> I'm tempted to just error out in this scenario rather than allow the
> lock upgrade.  Thoughts?

This close to release, I'll support you in choosing to just throw an
error. This should be fairly rare. Lock upgrades are deadlock prone
anyhow, so not a recommended coding practice and we would have a valid
practical reason for not allowing them (at this time).

It is something to fix later though: If I did need to do a lock upgrade,
I would code it with a savepoint so that deadlocks can be trapped and
retried.

IMHO the savepoint-related locking semantics aren't documented at all,
which is probably why such things have gone so long undetected.

-- 
  Simon Riggs             
  EnterpriseDB   http://www.enterprisedb.com

Re: FOR SHARE vs FOR UPDATE locks

Alvaro Herrera <alvherre@commandprompt.com> — 2006-12-01T11:37:11Z

Simon Riggs wrote:

> ISTM that multitrans could be used here. Two xids, one xmax.

Hmm, yeah, this seems a reasonable suggestion.  The problem is that we
don't have a mechanism today for saying "this Xid holds a shared lock,
this one holds an exclusive lock".  So code-wise it wouldn't be simple
to do.  It's a single bit per Xid, but I don't see where to store such a
thing.

I'm not sure we can use the simple "raise an ERROR" answer though,
because for users that would be a regression.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: FOR SHARE vs FOR UPDATE locks

Heikki Linnakangas <heikki@enterprisedb.com> — 2006-12-01T12:02:12Z

Alvaro Herrera wrote:
> Simon Riggs wrote:
> 
>> ISTM that multitrans could be used here. Two xids, one xmax.
> 
> Hmm, yeah, this seems a reasonable suggestion.  The problem is that we
> don't have a mechanism today for saying "this Xid holds a shared lock,
> this one holds an exclusive lock".  So code-wise it wouldn't be simple
> to do.  It's a single bit per Xid, but I don't see where to store such a
> thing.

We could store an extra byte in front of each xid in the multixact 
member file. That would screw up alignment, though, unless we pad it up 
to int32, but that would double the space taken by the members file.

Could we make the combination HEAP_XMAX_EXCL_LOCK | HEAP_XMAX_IS_MULTI 
legal, with the meaning that the last member in the multixact is an 
exclusive locker, if it's still in-progress?

> I'm not sure we can use the simple "raise an ERROR" answer though,
> because for users that would be a regression.

Yeah, that's ugly. Though it doesn't behave correctly as it is either...

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T15:12:06Z

Alvaro Herrera <alvherre@commandprompt.com> writes:
> I'm not sure we can use the simple "raise an ERROR" answer though,
> because for users that would be a regression.

I've reconsidered the idea of upgrading the outer xact's shared lock to
exclusive: at first I thought that would be hard to implement correctly,
but now I realize it's easy.  Just re-use the XID that's in the multixact
as the one to store as the exclusive locker, instead of storing our
current subxact XID.  In some cases this will be a subcommitted XID of
the current subxact or a parent, but the locking semantics are the same,
and even though we think such an XID is finished everyone else will see
it as still live so the appearance of its XID in an XMAX field shouldn't
be an issue.

So that's what I propose doing.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Zeugswetter Andreas DCP SD <zeugswettera@spardat.at> — 2006-12-01T15:45:00Z

> > I'm not sure we can use the simple "raise an ERROR" answer though,
> > because for users that would be a regression.
> 
> I've reconsidered the idea of upgrading the outer xact's shared lock
to
> exclusive: at first I thought that would be hard to implement
correctly,
> but now I realize it's easy.  Just re-use the XID that's in the
multixact
> as the one to store as the exclusive locker, instead of storing our
> current subxact XID.  In some cases this will be a subcommitted XID of
> the current subxact or a parent, but the locking semantics are the
same,
> and even though we think such an XID is finished everyone else will
see
> it as still live so the appearance of its XID in an XMAX field
shouldn't
> be an issue.

fwiw, I think that is good.

Andreas

Re: FOR SHARE vs FOR UPDATE locks

Heikki Linnakangas <heikki@enterprisedb.com> — 2006-12-01T16:55:56Z

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> I'm not sure we can use the simple "raise an ERROR" answer though,
>> because for users that would be a regression.
> 
> I've reconsidered the idea of upgrading the outer xact's shared lock to
> exclusive: at first I thought that would be hard to implement correctly,
> but now I realize it's easy.  Just re-use the XID that's in the multixact
> as the one to store as the exclusive locker, instead of storing our
> current subxact XID.  In some cases this will be a subcommitted XID of
> the current subxact or a parent, but the locking semantics are the same,
> and even though we think such an XID is finished everyone else will see
> it as still live so the appearance of its XID in an XMAX field shouldn't
> be an issue.

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.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T17:18:58Z

"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.

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

Re: FOR SHARE vs FOR UPDATE locks

Joshua D. Drake <jd@commandprompt.com> — 2006-12-01T17:34:37Z

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

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T17:35:16Z

"Joshua D. Drake" <jd@commandprompt.com> writes:
> At what point is the exclusive lock released? When I create a new
> savepoint? On COMMIT of the entire transaction?

COMMIT, or rollback to a savepoint before the original shared lock
was taken.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T17:40:46Z

Actually ... wait a minute.  The proposed hack covers the case of
SELECT FOR SHARE followed by SELECT FOR UPDATE within a subtransaction.
But what about SELECT FOR SHARE followed by an actual UPDATE (or DELETE)?

We certainly don't want to mark the UPDATE/DELETE as having been carried
out by the upper transaction, but there's no way we can record the
UPDATE while still remembering the previous share-lock.

So I think I'm back to the position that we should throw an error here.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Heikki Linnakangas <heikki@enterprisedb.com> — 2006-12-01T17:46:31Z

Tom Lane wrote:
> Actually ... wait a minute.  The proposed hack covers the case of
> SELECT FOR SHARE followed by SELECT FOR UPDATE within a subtransaction.
> But what about SELECT FOR SHARE followed by an actual UPDATE (or DELETE)?
> 
> We certainly don't want to mark the UPDATE/DELETE as having been carried
> out by the upper transaction, but there's no way we can record the
> UPDATE while still remembering the previous share-lock.
> 
> So I think I'm back to the position that we should throw an error here.

Yeah. Even without a real update, I just figured you can't upgrade a 
shared lock held by an arbitrarily chosen parent to an exclusive lock. 
If that subxid aborts, and if any of the parents of that subtransaction 
held the shared lock, that lock would be released incorrectly. Which is 
essentially the same problem we began with.

Let's throw an error for now. We have to come back to this in 8.3, I think.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T17:58:31Z

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Yeah. Even without a real update, I just figured you can't upgrade a 
> shared lock held by an arbitrarily chosen parent to an exclusive lock. 
> If that subxid aborts, and if any of the parents of that subtransaction 
> held the shared lock, that lock would be released incorrectly. Which is 
> essentially the same problem we began with.

Well, there's nothing "arbitrarily chosen" about it, because the lock
shown in the tuple would always be the most senior live subtransaction.
So I don't think your concern above is a real problem.  Nonetheless, the
proposed hack is definitely playing some games with the semantics, and
while we might be able to get away with that for the question of "is this
lock shared or exclusive", it certainly won't do for an actual update.

> Let's throw an error for now. We have to come back to this in 8.3, I think.

I think it's OK to fix it so that we allow the pre-existing lock to be
held by a subtransaction of the current xact, but not a parent.

			regards, tom lane

Re: [HACKERS] FOR SHARE vs FOR UPDATE locks

Jeff Davis <pgsql@j-davis.com> — 2006-12-01T18:43:01Z

On Fri, 2006-12-01 at 02:46 -0500, Tom Lane wrote:
> Michael Paesold <mpaesold@gmx.at> writes:
> > Now seriously, isn't this a perfectly feasible scenario? E.g. the outer 
> > transaction acquires a shared lock because of foreign key constraints, and 
> > the sub transaction later wants to update that row?
> 
> Yeah, it's not implausible.  But the only way I can see to implement
> that is to upgrade the outer xact's shared lock to exclusive, and that
> doesn't seem real cool either.
> 

If it's a plausible enough sequence of events, is it worth adding a note
to the "migration" section of the release notes?

Regards,
	Jeff Davis

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T18:46:57Z

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> Let's throw an error for now. We have to come back to this in 8.3, I think.

After further thought I think we should also seriously consider plan C:
do nothing for now.  We now realize that there have been related bugs
since 8.0, namely that

	begin;
	select some rows for update;
	savepoint x;
	update the same rows;
	rollback to x;

leaves the tuple(s) not locked.  The lack of complaints about this from
the field suggests that this isn't a huge problem in practice.  If we
do make it throw an error I'm afraid that we will break applications
that aren't having a problem at the moment.

I'm also realizing that a fix along the throw-an-error line is
nontrivial, eg, HeapTupleSatisfiesUpdate would need another return code.

So at this point we are facing three options:
	- throw in a large and poorly tested "fix" at the last moment;
	- postpone 8.2 until we can think of a real fix, which might
	  be a major undertaking;
	- ship 8.2 with the same behavior 8.0 and 8.1 had.
None of these are very attractive, but I'm starting to think the last
is the least bad.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Joshua D. Drake <jd@commandprompt.com> — 2006-12-01T18:54:38Z

On Fri, 2006-12-01 at 13:46 -0500, Tom Lane wrote:
> "Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> > Let's throw an error for now. We have to come back to this in 8.3, I think.
> 
> After further thought I think we should also seriously consider plan C:
> do nothing for now.  We now realize that there have been related bugs
> since 8.0, namely that
> 
> 	begin;
> 	select some rows for update;
> 	savepoint x;
> 	update the same rows;
> 	rollback to x;
> 
> leaves the tuple(s) not locked.  The lack of complaints about this from
> the field suggests that this isn't a huge problem in practice.  If we
> do make it throw an error I'm afraid that we will break applications
> that aren't having a problem at the moment.
> 
> I'm also realizing that a fix along the throw-an-error line is
> nontrivial, eg, HeapTupleSatisfiesUpdate would need another return code.
> 
> So at this point we are facing three options:
> 	- throw in a large and poorly tested "fix" at the last moment;
> 	- postpone 8.2 until we can think of a real fix, which might
> 	  be a major undertaking;
> 	- ship 8.2 with the same behavior 8.0 and 8.1 had.
> None of these are very attractive, but I'm starting to think the last
> is the least bad.

/me struggles...

IMHO:

option 2 is the correct option, but the least favorable.
option 1 is probably bad
option 3 is the lesser of the evils if we document it loudly.

Joshua D. Drake




> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
> 
-- 

      === 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

Re: [CORE] FOR SHARE vs FOR UPDATE locks

Josh Berkus <josh@agliodbs.com> — 2006-12-01T18:55:05Z

Tom,

> So at this point we are facing three options:
>         - throw in a large and poorly tested "fix" at the last moment;
>         - postpone 8.2 until we can think of a real fix, which might
>           be a major undertaking;
>         - ship 8.2 with the same behavior 8.0 and 8.1 had.
> None of these are very attractive, but I'm starting to think the last
> is the least bad.

Yes.  If it was earlier in the beta cycle I'd say no, but frankly this 
behavior has existed for two years without examples of real-life data 
loss.  Further, the TPC tests, which are supposed to give ACID properties 
a workout, would not break this, so the industry doesn't consider it very 
important either.

So, I think it needs to go on the list for 8.2.1 or 8.3 (depending on what 
changes the fix requires) but I don't think we should hold up the release.

As PR maven, though, you know I'm biased about the release date.

I would suggest a last-minute doc patch documenting the behavior and 
suggesting that locks should always be declared in the outer transaction 
prior to any savepoints.

-- 
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

Re: [CORE] FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T19:02:13Z

Josh Berkus <josh@agliodbs.com> writes:
> So, I think it needs to go on the list for 8.2.1 or 8.3 (depending on what 
> changes the fix requires) but I don't think we should hold up the release.

That's pretty much my feeling as well.  The thing is that postponing 8.2
any further will deprive users of a lot of good stuff, in order to fix a
problem that apparently isn't biting anyone in the field.  And it's not
clear that we can fix this on a shorter-than-8.3-ish timescale anyway.
The only obvious solution involves adding another header field, which
I'm sure is not very acceptable.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Richard Troy <rtroy@sciencetools.com> — 2006-12-01T19:09:08Z

On Fri, 1 Dec 2006, Tom Lane wrote:
>
> So at this point we are facing three options:
> 	- throw in a large and poorly tested "fix" at the last moment;
> 	- postpone 8.2 until we can think of a real fix, which might
> 	  be a major undertaking;
> 	- ship 8.2 with the same behavior 8.0 and 8.1 had.
> None of these are very attractive, but I'm starting to think the last
> is the least bad.
>
> 			regards, tom lane

I'd go with that last option; it's important to get this release out now,
I think, as it has a lot of value add, and people get it that things
aren't always perfect. I do, however, feel that the "real fix" is vital,
whenever it can occur. It's attention to detail like this that elevates
this group from good to great.

Richard

-- 
Richard Troy, Chief Scientist
Science Tools Corporation
510-924-1363 or 202-747-1263
rtroy@ScienceTools.com, http://ScienceTools.com/

Re: [CORE] FOR SHARE vs FOR UPDATE locks

Bruce Momjian <bruce@momjian.us> — 2006-12-01T19:16:27Z

Josh Berkus wrote:
> Tom,
> 
> > So at this point we are facing three options:
> > ????????- throw in a large and poorly tested "fix" at the last moment;
> > ????????- postpone 8.2 until we can think of a real fix, which might
> > ???????? ?be a major undertaking;
> > ????????- ship 8.2 with the same behavior 8.0 and 8.1 had.
> > None of these are very attractive, but I'm starting to think the last
> > is the least bad.
> 
> Yes.  If it was earlier in the beta cycle I'd say no, but frankly this 
> behavior has existed for two years without examples of real-life data 
> loss.  Further, the TPC tests, which are supposed to give ACID properties 
> a workout, would not break this, so the industry doesn't consider it very 
> important either.
> 
> So, I think it needs to go on the list for 8.2.1 or 8.3 (depending on what 
> changes the fix requires) but I don't think we should hold up the release.

We cannot add something this major in a minor release --- it would have
to be 8.3.

-- 
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

Re: [CORE] FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T19:27:13Z

Bruce Momjian <bruce@momjian.us> writes:
> Josh Berkus wrote:
>> So, I think it needs to go on the list for 8.2.1 or 8.3 (depending on what 
>> changes the fix requires) but I don't think we should hold up the release.

> We cannot add something this major in a minor release --- it would have
> to be 8.3.

If someone thinks of a brilliant solution that doesn't change on-disk
layout, maybe we could implement it in 8.2.x, but right now I'm not
feeling hopeful about that.

The best idea I have at the moment is that we might be able to do
something as part of the proposed plan to fold cmin/cmax into a single
field.  The thought there was that there could be some in-memory state
for tuples that had been modified multiple times by a single xact ---
perhaps that could be extended to cover this problem.  This is just
handwaving at the moment though.  In particular, is-the-tuple-locked-or-not
seems like it has to be externally visible state, so maybe it can't work.

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Merlin Moncure <mmoncure@gmail.com> — 2006-12-01T20:11:59Z

On 12/1/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Heikki Linnakangas" <heikki@enterprisedb.com> writes:
> > Let's throw an error for now. We have to come back to this in 8.3, I think.
>
> After further thought I think we should also seriously consider plan C:
> do nothing for now.  We now realize that there have been related bugs
> since 8.0, namely that
>
>         begin;
>         select some rows for update;
>         savepoint x;
>         update the same rows;
>         rollback to x;
>
> leaves the tuple(s) not locked.  The lack of complaints about this from
> the field suggests that this isn't a huge problem in practice.  If we
> do make it throw an error I'm afraid that we will break applications
> that aren't having a problem at the moment.

imo, the most likely scenario would be a begin/exception/end block in
pg/sql. i would venture to guess that very little true savepointing
happens in practice.  maybe add a little note of caution pg/sql error
handling documentation?

merlin

Re: FOR SHARE vs FOR UPDATE locks

Simon Riggs <simon@2ndquadrant.com> — 2006-12-01T20:46:56Z

On Fri, 2006-12-01 at 13:46 -0500, Tom Lane wrote:

> I'm also realizing that a fix along the throw-an-error line is
> nontrivial, eg, HeapTupleSatisfiesUpdate would need another return code.

Yes, thats starting to get hairy. The fix could easily break something
else in another corner of MVCC.

> So at this point we are facing three options:
> 	- throw in a large and poorly tested "fix" at the last moment;
> 	- postpone 8.2 until we can think of a real fix, which might
> 	  be a major undertaking;
> 	- ship 8.2 with the same behavior 8.0 and 8.1 had.
> None of these are very attractive, but I'm starting to think the last
> is the least bad.

The functionality in this area isn't yet complete anyway; we still have
locking in the partitioned table case to consider. It's not that bad
just to leave it as is. So last option gets my vote.

-- 
  Simon Riggs             
  EnterpriseDB   http://www.enterprisedb.com

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T20:52:20Z

"Simon Riggs" <simon@2ndquadrant.com> writes:
> The functionality in this area isn't yet complete anyway; we still have
> locking in the partitioned table case to consider.

Hm?  What does partitioning have to do with it?

			regards, tom lane

Re: [CORE] FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-01T20:55:30Z

"Merlin Moncure" <mmoncure@gmail.com> writes:
> imo, the most likely scenario would be a begin/exception/end block in
> pg/sql. i would venture to guess that very little true savepointing
> happens in practice.  maybe add a little note of caution pg/sql error
> handling documentation?

I mentioned exception blocks as a risk factor, but I think the right
place to document it is under FOR UPDATE/SHARE:
http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/ref/select.sgml.diff?r1=1.93;r2=1.94

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Simon Riggs <simon@2ndquadrant.com> — 2006-12-01T22:14:49Z

On Fri, 2006-12-01 at 15:52 -0500, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > The functionality in this area isn't yet complete anyway; we still have
> > locking in the partitioned table case to consider.
> 
> Hm?  What does partitioning have to do with it?

SELECT FOR UPDATE/SHARE is not supported for inheritance queries.

My point was that the implementation of row locking is not yet complete,
so the slight wrinkle around lock upgrading is not a solitary eyesore.

-- 
  Simon Riggs             
  EnterpriseDB   http://www.enterprisedb.com

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-02T00:53:06Z

"Simon Riggs" <simon@2ndquadrant.com> writes:
> On Fri, 2006-12-01 at 15:52 -0500, Tom Lane wrote:
>> Hm?  What does partitioning have to do with it?

> SELECT FOR UPDATE/SHARE is not supported for inheritance queries.

True, but that's a planner/executor issue not a question of the
fundamental representation of the state on-disk.  (I have some ideas
about how to fix that one.)

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Decibel! <decibel@decibel.org> — 2006-12-04T06:44:01Z

On Dec 1, 2006, at 10:46 AM, Tom Lane wrote:
> If we
> do make it throw an error I'm afraid that we will break applications
> that aren't having a problem at the moment.

What about throwing a warning? Shouldn't break anything, but at least  
then anyone who's experiencing this and has just gotten lucky so far  
will have a better idea that it's happening.

As for possibly using the in-memory store of multiple CIDs affecting  
a tuple, could that not work if that store contained enough  
information to 'rollback' the lock to it's original state when  
restoring to a savepoint? AFAIK other backends would only need to  
know what the current lock being held was, they wouldn't need to know  
the history of it themselves...
--
Jim Nasby                                            jim@nasby.net
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane <tgl@sss.pgh.pa.us> — 2006-12-05T16:03:33Z

Jim Nasby <decibel@decibel.org> writes:
> As for possibly using the in-memory store of multiple CIDs affecting  
> a tuple, could that not work if that store contained enough  
> information to 'rollback' the lock to it's original state when  
> restoring to a savepoint? AFAIK other backends would only need to  
> know what the current lock being held was, they wouldn't need to know  
> the history of it themselves...

One of the interesting problems is that if you upgrade shared lock to
exclusive and then want to roll that back, you might need to un-block
other processes that tried to acquire share lock after you acquired
exclusive.  We have no way to do that in the current implementation.
(Any such processes will be blocked on your transaction ID lock, which
you can't release without possibly unblocking the wrong processes.)

			regards, tom lane

Re: FOR SHARE vs FOR UPDATE locks

Bruce Momjian <bruce@momjian.us> — 2007-02-01T04:36:15Z

Added to TODO:

* Fix problem when multiple subtransactions of the same outer transaction
  hold different types of locks, and one subtransaction aborts

  http://archives.postgresql.org/pgsql-hackers/2006-11/msg01011.php
  http://archives.postgresql.org/pgsql-hackers/2006-12/msg00001.php


---------------------------------------------------------------------------

Tom Lane wrote:
> Jim Nasby <decibel@decibel.org> writes:
> > As for possibly using the in-memory store of multiple CIDs affecting  
> > a tuple, could that not work if that store contained enough  
> > information to 'rollback' the lock to it's original state when  
> > restoring to a savepoint? AFAIK other backends would only need to  
> > know what the current lock being held was, they wouldn't need to know  
> > the history of it themselves...
> 
> One of the interesting problems is that if you upgrade shared lock to
> exclusive and then want to roll that back, you might need to un-block
> other processes that tried to acquire share lock after you acquired
> exclusive.  We have no way to do that in the current implementation.
> (Any such processes will be blocked on your transaction ID lock, which
> you can't release without possibly unblocking the wrong processes.)
> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 7: You can help support the PostgreSQL project by donating at
> 
>                 http://www.postgresql.org/about/donate

-- 
  Bruce Momjian   bruce@momjian.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +