Subject: FOR SHARE vs FOR UPDATE locks

From: Tom Lane <[email-masked]>
Date: Thu, 30 Nov 2006 17:06:13 -0500

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

---

From: Michael Paesold <[email-masked]>
Date: Fri, 01 Dec 2006 08:16:41 +0100

> I'm tempted to just error out in this scenario rather than allow the

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 02:46:59 -0500

> does not sound like the PostgreSQL way to solve problems (rather like 

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

> 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

---

From: "Simon Riggs" <[email-masked]>
Date: Fri, 01 Dec 2006 08:42:23 +0000

> 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

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

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 1 Dec 2006 08:37:11 -0300

> 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

---

From: "Heikki Linnakangas" <[email-masked]>
Date: Fri, 01 Dec 2006 12:02:12 +0000

> to do.  It's a single bit per Xid, but I don't see where to store such a

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?

> because for users that would be a regression.

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

-- 
   Heikki Linnakangas

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 10:12:06 -0500

> 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

---

From: "Zeugswetter Andreas ADI SD" <[email-masked]>
Date: Fri, 1 Dec 2006 16:45:00 +0100

> it as still live so the appearance of its XID in an XMAX field
shouldn't

fwiw, I think that is good.

Andreas

---

From: "Heikki Linnakangas" <[email-masked]>
Date: Fri, 01 Dec 2006 16:55:56 +0000

> it as still live so the appearance of its XID in an XMAX field shouldn't

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.

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 12:18:58 -0500

> "rollback to savepoint", right? Though it's still better than throwing 

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

---

From: "Joshua D. Drake" <[email-masked]>
Date: Fri, 01 Dec 2006 09:34:37 -0800

> > "rollback to savepoint", right? Though it's still better than throwing 

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

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 12:35:16 -0500

> savepoint? On COMMIT of the entire transaction?

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

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 12:40:46 -0500

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

---

From: "Heikki Linnakangas" <[email-masked]>
Date: Fri, 01 Dec 2006 17:46:31 +0000

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 12:58:31 -0500

> 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

---

From: Jeff Davis <[email-masked]>
Date: Fri, 01 Dec 2006 10:43:01 -0800

> 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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 13:46:57 -0500

> 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

---

From: "Joshua D. Drake" <[email-masked]>
Date: Fri, 01 Dec 2006 10:54:38 -0800

> None of these are very attractive, but I'm starting to think the last

/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

> TIP 5: don't forget to increase your free space map settings
> 
--

---

From: Josh Berkus <[email-masked]>
Date: Fri, 1 Dec 2006 10:55:05 -0800

Tom,

> None of these are very attractive, but I'm starting to think the last

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 14:02:13 -0500

> 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

---

From: Richard Troy <[email-masked]>
Date: Fri, 1 Dec 2006 11:09:08 -0800 (PST)

> None of these are very attractive, but I'm starting to think the last

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

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 1 Dec 2006 14:16:27 -0500 (EST)

Josh Berkus wrote:
> > ????????- 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.

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

-- 
  Bruce Momjian   [email-masked]
  EnterpriseDB    http://www.enterprisedb.com

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 14:27:13 -0500

> We cannot add something this major in a minor release --- it would have

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

---

From: "Merlin Moncure" <[email-masked]>
Date: Fri, 1 Dec 2006 15:11:59 -0500

> 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

---

From: "Simon Riggs" <[email-masked]>
Date: Fri, 01 Dec 2006 20:46:56 +0000

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

> None of these are very attractive, but I'm starting to think the last

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.

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 15:52:20 -0500

"Simon Riggs" <[email-masked]> writes:
> locking in the partitioned table case to consider.

Hm?  What does partitioning have to do with it?

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 15:55:30 -0500

> happens in practice.  maybe add a little note of caution pg/sql error

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

---

From: "Simon Riggs" <[email-masked]>
Date: Fri, 01 Dec 2006 22:14:49 +0000

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

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Dec 2006 19:53:06 -0500

> 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

---

From: Jim Nasby <[email-masked]>
Date: Sun, 3 Dec 2006 22:44:01 -0800

On Dec 1, 2006, at 10:46 AM, Tom Lane wrote:
> If we

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                                            [email-masked]
EnterpriseDB      http://enterprisedb.com      512.569.9461 (cell)

---

From: Tom Lane <[email-masked]>
Date: Tue, 05 Dec 2006 11:03:33 -0500

> 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

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 31 Jan 2007 23:36:15 -0500 (EST)

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:
> TIP 7: You can help support the PostgreSQL project by donating at
> 
>                 http://www.postgresql.org/about/donate