On 20.09.2011 20:42, Alvaro Herrera wrote:
> I notice that heap_update releases the buffer lock, after checking the
> HeapTupleSatifiesUpdate result, and before marking the tuple as updated,
> to pin the visibility map page -- heapam.c lines 2638ff in master branch.
>
> Is this not a bug? I imagine that while this code releases the lock,
> someone else could acquire it and grab a FOR SHARE lock on the tuple; if
> the update later ends up modifying the tuple completely, this could
> cause an FK to be broken, for example.
Yeah, I think you're right. Not only might someone grab a FOR SHARE lock
on the tuple, but someone might even update it under your nose.
> The other piece of that routine that releases the buffer lock, to toast
> the tuple, is careful enough to set the Xmax to itself before releasing
> the lock, which seems to me the right thing to do, because then the
> prospective locker would have to wait until this transaction finishes
> before being able to grab the lock. Is this not necessary in the other
> path? If so, why?
Yeah, we could do the same when relocking to pin the VM page. Or just
add a "goto l2" there to start over.
BTW, I think we're playing a bit fast and loose with that
set-xmax-before-unlocking trick. We haven't WAL-logged anything at that
point yet, so it's possible that while we're busy toasting the tuple,
the page is flushed from shared buffers with the xmax and the infomask
already update. Now, the system crashes, and you get a torn page, so
that the xmax is already updated, but the HEAP_XMAX_COMMITTED flag was
*not* cleared, so it's still set. Oops. Highly unlikely in practice,
because xmax and infomask are very close to each other, but it violates
the principles of what we usually expect from the underlying system wrt.
torn pages.
> (I CC both Robert and Heikki because I don't remember whose work it was
> on the VM stuff).
Fortunately, this race was in Robert's patch for 9.2, so this doesn't
need to be back-patched.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com