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

On 2012-11-30 08:52:18 +0530, Pavan Deolasee wrote:
> 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.

We only get the pin right there, I don't see any preexisting pin. Which
means we might have just opened a page thats in the process of being
pruned/vacuumed by another backend.
I think a concurrent heap_(insert|update)/PageAddItem might actually be
already dangerous because it might move line pointers around

> 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.

Sure, but the page header isn't accessed there, just the tuple data
itself which always stays at the same place on the page.
(A normal heap_fetch shouldn't be worried about updates/deletions to its
tuple, MVCC to the rescue.)

Even if it turns out to be safe, I think this deserves at least a
comment...

Greetings,

Andres Freund

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