Re: [HACKERS] heap_update temporary release of buffer lock

2011-09-27 Thread Robert Haas
On Tue, Sep 20, 2011 at 3:47 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Alvaro Herrera alvhe...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] heap_update temporary release of buffer lock

2011-09-20 Thread Alvaro Herrera

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 alvhe...@alvh.no-ip.org

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] heap_update temporary release of buffer lock

2011-09-20 Thread Heikki Linnakangas

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] heap_update temporary release of buffer lock

2011-09-20 Thread Robert Haas
On Tue, Sep 20, 2011 at 2:28 PM, Heikki Linnakangas
heikki.linnakan...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] heap_update temporary release of buffer lock

2011-09-20 Thread Alvaro Herrera

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.linnakan...@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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] heap_update temporary release of buffer lock

2011-09-20 Thread Tom Lane
Alvaro Herrera alvhe...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers