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

On Fri, Nov 30, 2012 at 3:19 AM, Andres Freund <andres(at)2ndquadrant(dot)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