Quantcast

FOR SHARE vs FOR UPDATE locks

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

FOR SHARE vs FOR UPDATE locks

Tom Lane-2
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

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

                http://www.postgresql.org/about/donate
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Michael Paesold
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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
Michael Paesold <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

               http://archives.postgresql.org
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Simon Riggs
In reply to this post by Tom Lane-2
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



---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Alvaro Herrera-7
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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
       subscribe-nomail command to [hidden email] so that your
       message can get through to the mailing list cleanly
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Heikki Linnakangas-2
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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
In reply to this post by Alvaro Herrera-7
Alvaro Herrera <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Zeugswetter Andreas ADI SD

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Heikki Linnakangas-2
In reply to this post by Tom Lane-2
Tom Lane wrote:

> Alvaro Herrera <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
       subscribe-nomail command to [hidden email] so that your
       message can get through to the mailing list cleanly
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
"Heikki Linnakangas" <[hidden email]> 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);

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Joshua D. Drake
On Fri, 2006-12-01 at 12:18 -0500, Tom Lane wrote:
> "Heikki Linnakangas" <[hidden email]> 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




---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
"Joshua D. Drake" <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
In reply to this post by Heikki Linnakangas-2
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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Heikki Linnakangas-2
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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
"Heikki Linnakangas" <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Jeff Davis-8
In reply to this post by Tom Lane-2
On Fri, 2006-12-01 at 02:46 -0500, Tom Lane wrote:
> Michael Paesold <[hidden email]> 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


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Tom Lane-2
In reply to this post by Heikki Linnakangas-2
"Heikki Linnakangas" <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Joshua D. Drake
On Fri, 2006-12-01 at 13:46 -0500, Tom Lane wrote:

> "Heikki Linnakangas" <[hidden email]> 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




---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [CORE] FOR SHARE vs FOR UPDATE locks

Josh Berkus
In reply to this post by Tom Lane-2
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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: FOR SHARE vs FOR UPDATE locks

Richard Troy
In reply to this post by Tom Lane-2


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
[hidden email], http://ScienceTools.com/


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq

12
Loading...