Thread: Wrong FOR UPDATE lock type
Hi, I'm about 99.666667% sure that the lock type choosen in the FOR UPDATE case (line 511 of parse_relation.c) should be RowExclusiveLock instead of RowShareLock. Actually I get "Deadlock risk" debug messages when selectingFOR UPDATE and then really UPDATE. Should I change it? Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
> I'm about 99.666667% sure that the lock type choosen in the > FOR UPDATE case (line 511 of parse_relation.c) should be > RowExclusiveLock instead of RowShareLock. Actually I get > "Deadlock risk" debug messages when selecting FOR UPDATE and > then really UPDATE. http://www.postgresql.org/users-lounge/docs/6.5/user/x3116.htm RowShareLock Acquired by SELECT FOR UPDATE and LOCK TABLE for IN ROW SHARE MODE statements. Conflicts with ExclusiveLock and AccessExclusiveLock modes. Vadim
Mikheev, Vadim wrote: > > I'm about 99.666667% sure that the lock type choosen in the > > FOR UPDATE case (line 511 of parse_relation.c) should be > > RowExclusiveLock instead of RowShareLock. Actually I get > > "Deadlock risk" debug messages when selecting FOR UPDATE and > > then really UPDATE. > > http://www.postgresql.org/users-lounge/docs/6.5/user/x3116.htm > > RowShareLock > Acquired by SELECT FOR UPDATE and LOCK TABLE for IN ROW SHARE MODE > statements. > > Conflicts with ExclusiveLock and AccessExclusiveLock modes. Tom, IIRC the "Deadlock risk" debug message is from you. I think it must get a little smarter. IMHO an application thatwant's to UPDATE something in a transaction but must SELECT the row(s) first to do it's own calculation on them, should use SELECT FOR UPDATE. Is that debug output really appropriate in this case (it raises from RowShareLock to RowExclusiveLock because of the UPDATE of the previous FOR UPDATE selected row)? Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck <janwieck@yahoo.com> writes:
> I'm about 99.666667% sure that the lock type choosen in the
> FOR UPDATE case (line 511 of parse_relation.c) should be
> RowExclusiveLock instead of RowShareLock. Actually I get
> "Deadlock risk" debug messages when selecting FOR UPDATE and
> then really UPDATE.
> Should I change it?
Not sure, but if you do change it, that's *not* the only place. I coded
that as RowShareLock because that was what was getting grabbed by the
executor for SELECT FOR UPDATE. I believe the rewriter may need changed
as well, since it can also be the first grabber of a lock for a rel.
Note also that the docs say SELECT FOR UPDATE gets RowShareLock!
The "deadlock risk" message is not very bright, and I wouldn't suggest
changing the code just because of that. I'm not even sure I want to
leave that check in the release version ...
regards, tom lane
Jan Wieck <janwieck@Yahoo.com> writes:
> Tom,
> IIRC the "Deadlock risk" debug message is from you. I think
> it must get a little smarter. IMHO an application that want's
> to UPDATE something in a transaction but must SELECT the
> row(s) first to do it's own calculation on them, should use
> SELECT FOR UPDATE. Is that debug output really appropriate in
> this case (it raises from RowShareLock to RowExclusiveLock
> because of the UPDATE of the previous FOR UPDATE selected
> row)?
Well, there is a theoretical chance of deadlock --- not against other
transactions doing the same thing, since RowShareLock and
RowExclusiveLock don't conflict, but you could construct deadlock
scenarios involving other transactions that grab ShareLock or
ShareRowExclusiveLock. So I don't think it's appropriate for the
"deadlock risk" check to ignore RowShareLock->RowExclusiveLock
upgrades.
But I'm not sure the check should be enabled in production releases
anyway. I just put it in as a quick and dirty debug check. Perhaps
it should be under an #ifdef that's not enabled by default.
regards, tom lane
> Well, there is a theoretical chance of deadlock --- not against other > transactions doing the same thing, since RowShareLock and > RowExclusiveLock don't conflict, but you could construct deadlock > scenarios involving other transactions that grab ShareLock or > ShareRowExclusiveLock. So I don't think it's appropriate for the > "deadlock risk" check to ignore RowShareLock->RowExclusiveLock > upgrades. There is theoretical chance of deadlock when two xactions lock tables in different order and we can check this only in deadlock detection code. > But I'm not sure the check should be enabled in production releases > anyway. I just put it in as a quick and dirty debug check. Perhaps > it should be under an #ifdef that's not enabled by default. No reason to learn users during transaction processing. We need in good deadlock detection code and documentation. Vadim