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

On 2012-11-30 18:35:05 +0530, Pavan Deolasee wrote:
> On Fri, Nov 30, 2012 at 6:21 PM, Andres Freund <andres@2ndquadrant.com>wrote:
>
> > On 2012-11-30 18:09:49 +0530, Pavan Deolasee wrote:
> > > On Fri, Nov 30, 2012 at 5:28 PM, Andres Freund <andres@2ndquadrant.com
> > >wrote:
> > >
> > > >
> > > > > >
> > > > > 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.
> > > >
> > >
> > > Hmm. Yeah, you're right. That is a possible risky scenario. Even though
> > > cleanup lock waits for all pins to go away, it will work only if every
> > > reader takes at least a SHARE lock unless it was continuously holding a
> > pin
> > > on a buffer (in which case its OK to drop lock and read a tuple body
> > > without reacquiring it again). Otherwise, as you rightly pointed out, we
> > > could actually be reading a page which being actively cleaned up and
> > tuples
> > > are being moved around.
> >
> > Well, live (or recently dead) tuples can't be just moved arround unless
> > I miss something. That would cause problems with with HeapTuples
> > directly pointing into the page as youve noticed.

> I think we confusing with the terminology. The tuple headers and bodies
> (live or dead) can be moved around freely as long as you have a cleanup
> lock on the page. Thats how HOT-prune frees up fragmented space.

Need to read more code here. This is nothing I really have looked into
before.Youre probably right.

Up to now I thought hot pruning only removed intermediate & surely dead
tuples but didn't move stuff arround. And that page fragementation was
repaired separately. But it looks like I am wrong.

> > > > I think a concurrent heap_(insert|update)/PageAddItem might actually be
> > > > already dangerous because it might move line pointers around
> > > >
> > > >
> > > I don't we move the line pointers around ever because the indexes will be
> > > pointing to them.
> >
> > The indexes point to a tid, not to a line pointer. So reshuffling the
> > linepointer array - while keeping the tids valid - is entirely
> > fine. Right?

> I think its again terminology confusion :-) I thought TID and Line Pointers
> are the same and used interchangeably. Or at least I've done so for long.
> But I think you are reading line pointer as the thing stored in the ItemId
> structure and not the ItemId itself.

I always understood ItemIds to be line pointers and ItemPointers to be
tids. Line pointers are only meaningful within a single page, store the
actual offset within the page and so on. ItemPointers (cids) are longer
lasting and store (page, offset_number)…

> Also, PageAddItem() doesn't move things around normally. It does that only
> during recovery, at least for heao pages. Elsewhere it will either reuse an
> UNUSED pointer or add at the end. Isn't that how it is ?

Hm? It doesn't move the page contents around but it moves the ItemId
array during completely normal operation (c.f. needshuffle in
PageAddItem). Or am I missing something?

Greetings,

Andres Freund

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