Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger? - Mailing list pgsql-hackers

From Pavan Deolasee
Subject Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?
Date
Msg-id CABOikdMv6mAbvg1c6S_YDw23rU9Lo0wsPtNUThRQx8ZYCZD6Cg@mail.gmail.com
Whole thread Raw
In response to missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?  (Andres Freund <andres@2ndquadrant.com>)
Responses Re: missing LockBuffer(buffer, BUFFER_LOCK_SHARE) in trigger.c GetTupleForTrigger?  (Andres Freund <andres@2ndquadrant.com>)
List pgsql-hackers


On Fri, Nov 30, 2012 at 3:19 AM, Andres Freund <andres@2ndquadrant.com> wrote:
Hi,

while looking at trigger.c from the POV of foreign key locks I noticed
that GetTupleForTrigger commentless assumes it can just look at a pages
content without a
    LockBuffer(buffer, BUFFER_LOCK_SHARE);

That code path is only reached for AFTER ... FOR EACH ROW ... triggers,
so its fine it's not locking the tuple itself. That already needs to
have happened before.

The code in question:

if (newslot_which_is_passed_by_before_triggers)
        ...
else
{
        Page            page;
        ItemId          lp;

        buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));

        page = BufferGetPage(buffer);
        lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));

        Assert(ItemIdIsNormal(lp));

        tuple.t_data = (HeapTupleHeader) PageGetItem(page, lp);
        tuple.t_len = ItemIdGetLength(lp);
        tuple.t_self = *tid;
        tuple.t_tableOid = RelationGetRelid(relation);
}

result = heap_copytuple(&tuple);
ReleaseBuffer(buffer);

As can be seen in the excerpt above this is basically a very stripped
down heap_fetch(). But without any locking on the buffer we just read.

I can't believe this is safe? Just about anything but eviction could
happen to that buffer at that moment?

Am I missing something?


That seems to be safe to me. Anything thats been read above can't really change. The tuple is already locked, so a concurrent update/delete has to wait on us. We have a pin on the buffer, so VACUUM or HOT-prune can't happen either. I can't see any other operation that can really change those fields.

heap_fetch() though looks very similar has an important difference. It might be reading the tuple without lock on it and we need the buffer lock to protect against concurrent update/delete to the tuple. AFAIK once the tuple is passed through qualification checks etc, even callers of heap_fetch() can read the tuple data without any lock on the buffer itself.

Thanks,
Pavan
--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Bugs in CREATE/DROP INDEX CONCURRENTLY
Next
From: Bruce Momjian
Date:
Subject: initdb.c::main() too large