Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Alvaro Herrera
Andres Freund escribió: On 2012-11-30 08:52:18 +0530, Pavan Deolasee wrote: On Fri, Nov 30, 2012 at 3:19 AM, Andres Freund and...@2ndquadrant.comwrote: I can't believe this is safe? Just about anything but eviction could happen to that buffer at that moment? Yeah, it does seem fishy

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Simon Riggs
On 30 November 2012 11:58, Andres Freund and...@2ndquadrant.com wrote: We only get the pin right there, I don't see any preexisting pin. Seems easy enough to test with an Assert patch. If the Assert doesn't fail, we apply it as documentation of the requirement for a pin. If it fails, we fix

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Andres Freund
On 2012-11-30 12:50:06 +, Simon Riggs wrote: On 30 November 2012 11:58, Andres Freund and...@2ndquadrant.com wrote: We only get the pin right there, I don't see any preexisting pin. Seems easy enough to test with an Assert patch. If the Assert doesn't fail, we apply it as documentation

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Andres Freund
On 2012-11-30 13:57:46 +0100, Andres Freund wrote: On 2012-11-30 12:50:06 +, Simon Riggs wrote: On 30 November 2012 11:58, Andres Freund and...@2ndquadrant.com wrote: We only get the pin right there, I don't see any preexisting pin. Seems easy enough to test with an Assert patch.

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: But a failing Assert obviously proofs something. It doesn't prove that the code is unsafe though. I am suspicious that it is safe, because it's pretty darn hard to believe we'd not have seen field reports of problems with triggers otherwise. It's

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2012-11-30 10:42:21 -0500, Tom Lane wrote: I am suspicious that it is safe, because it's pretty darn hard to believe we'd not have seen field reports of problems with triggers otherwise. It's not like this is a seldom-executed code path. Thats

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Andres Freund
On 2012-11-30 12:53:27 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-11-30 10:42:21 -0500, Tom Lane wrote: I am suspicious that it is safe, because it's pretty darn hard to believe we'd not have seen field reports of problems with triggers otherwise. It's

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Tom Lane
BTW, I went looking for other places that might have a similar mistake. I found that contrib/pageinspect/btreefuncs.c pokes around in btree pages without any buffer lock, which seems pretty broken --- don't we want it to take share lock? regards, tom lane -- Sent via

Re: [HACKERS] Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?

2012-11-30 Thread Tom Lane
BTW, on further inspection, there's yet another reason why we've not heard about this from the field: even if all the wrong things happen and GetTupleForTrigger manages to copy bogus data for the old tuple, that data *is not passed to the trigger function*. The only thing we do with it is decide