Re: FOR SHARE vs FOR UPDATE locks

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

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

Michael Paesold <mpaesold(at)gmx(dot)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

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

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

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

Alvaro Herrera <alvherre(at)commandprompt(dot)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

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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)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);

On Fri, 2006-12-01 at 12:18 -0500, Tom Lane wrote:
> "Heikki Linnakangas" <heikki(at)enterprisedb(dot)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

"Joshua D. Drake" <jd(at)commandprompt(dot)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

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

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

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)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

On Fri, 2006-12-01 at 02:46 -0500, Tom Lane wrote:
> Michael Paesold <mpaesold(at)gmx(dot)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

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)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

On Fri, 2006-12-01 at 13:46 -0500, Tom Lane wrote:
> "Heikki Linnakangas" <heikki(at)enterprisedb(dot)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

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

Josh Berkus <josh(at)agliodbs(dot)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

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(at)ScienceTools(dot)com, http://ScienceTools.com/

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(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)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

On 12/1/06, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Heikki Linnakangas" <heikki(at)enterprisedb(dot)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

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

"Simon Riggs" <simon(at)2ndquadrant(dot)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

"Merlin Moncure" <mmoncure(at)gmail(dot)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

On Fri, 2006-12-01 at 15:52 -0500, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)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

"Simon Riggs" <simon(at)2ndquadrant(dot)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

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(at)nasby(dot)net
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)

Jim Nasby <decibel(at)decibel(dot)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

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(at)decibel(dot)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(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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