thread-index: AccVJK5Klc9i0xnBRICDMMKDoh7l/A==
Subject: Re: FOR SHARE vs FOR UPDATE locks
Content-Class: urn:content-classes:message
Importance: normal
From: "Simon Riggs" <simon@2ndquadrant.com>
To: "Tom Lane" <tgl@sss.pgh.pa.us>
Cc: "Alvaro Herrera" <alvherre@alvh.no-ip.org>, <pgsql-hackers@postgreSQL.org>
In-Reply-To: <1144.1164924373@sss.pgh.pa.us>
References: <1144.1164924373@sss.pgh.pa.us>
Content-Type: text/plain;
	charset="iso-8859-1"
Date: Fri, 01 Dec 2006 08:42:23 +0000
Message-ID: <1164962544.3778.847.camel@silverbirch.site>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

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