Thread: heap_update temporary release of buffer lock
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. 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? The reason I care is because I need to do something to this code for the FOR KEY SHARE stuff I'm working on (not yet sure what). (I CC both Robert and Heikki because I don't remember whose work it was on the VM stuff). -- Álvaro Herrera <alvherre@alvh.no-ip.org>
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
On Tue, Sep 20, 2011 at 2:28 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > 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. Yeah, I think he's right, too. :-( The easiest fix seems to be (as you suggest) to add "goto l2" after reacquiring the lock. Can we get away with (and is there any benefit to) doing that only if xmax has changed? > 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 think our usual assumption is that the disk might write in chunks as small as 512 bytes. IIUC, tuples are only guaranteed to have 8-byte alignment, and xmax is at offset 4, while the infomask is at offset 20. So that doesn't seem safe. If we were doing this over again we could rearrange things to put them in the same eight-byte aligned chunk by (i.e. swap t_xmax and t_field3, move t_infomask before t_ctid), but it's a bit late for that now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mar sep 20 16:04:03 -0300 2011: > On Tue, Sep 20, 2011 at 2:28 PM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: > > 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. > > Yeah, I think he's right, too. :-( > > The easiest fix seems to be (as you suggest) to add "goto l2" after > reacquiring the lock. Can we get away with (and is there any benefit > to) doing that only if xmax has changed? Hmm ... I think that works, and it would suit my purposes too. Note this means you have to recheck infomask too (otherwise consider that IS_MULTI could be set the first time, and not set the second time, and that makes the Xmax have a different meaning.) OTOH if you just do it always, it is simpler. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Robert Haas's message of mar sep 20 16:04:03 -0300 2011:
>>> 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.
>> The easiest fix seems to be (as you suggest) to add "goto l2" after
>> reacquiring the lock. Can we get away with (and is there any benefit
>> to) doing that only if xmax has changed?
> Hmm ... I think that works, and it would suit my purposes too. Note
> this means you have to recheck infomask too (otherwise consider that
> IS_MULTI could be set the first time, and not set the second time, and
> that makes the Xmax have a different meaning.) OTOH if you just do it
> always, it is simpler.
Yeah, I think a "goto l2" is correct and sufficient. As the comment
already notes, this need not be a high-performance path, so why spend
extra code (with extra risk of bugs)?
regards, tom lane
On Tue, Sep 20, 2011 at 3:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> Excerpts from Robert Haas's message of mar sep 20 16:04:03 -0300 2011: >>>> 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. > >>> The easiest fix seems to be (as you suggest) to add "goto l2" after >>> reacquiring the lock. Can we get away with (and is there any benefit >>> to) doing that only if xmax has changed? > >> Hmm ... I think that works, and it would suit my purposes too. Note >> this means you have to recheck infomask too (otherwise consider that >> IS_MULTI could be set the first time, and not set the second time, and >> that makes the Xmax have a different meaning.) OTOH if you just do it >> always, it is simpler. > > Yeah, I think a "goto l2" is correct and sufficient. As the comment > already notes, this need not be a high-performance path, so why spend > extra code (with extra risk of bugs)? Done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company