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

On 2012-11-30 10:42:21 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > But a failing Assert obviously proofs something.
>
> It doesn't prove that the code is unsafe though.

Hehe.

> 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 true, but I think due to the fact that the plain DELETEs do *not*
trigger the Assert we might just not have noticed it. A make check with
the error checking in place doesn't error out in fact.
Also I think the wrong data caused by it aren't that likely to be
noticed. Its just the OLD in triggers so its not going to be looked at
all the time all too closely and we might return data thats somewhat
validly looking.

I think most executor paths actually hold a separate pin "by accident"
while this is executing. I think that my example is hitting that case is
due to the ReleaseBuffer() after ExecStoreTuple() in TidNext(). Seqscans
have another pin via scan->rs_cbuf, indexscans one via ->xs_cbuf. Many
of the other nodes that are likely below ExecUpdate/Delete probably work
similar.

I think most cases with high throughput (which you probably need to
actually hit the relatively small window) won't use plans that are
complex enough to trigger the bug.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services