Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC] - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]
Date
Msg-id 20160908220223.GA65203@alvherre.pgsql
Whole thread Raw
In response to Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]
List pgsql-hackers
Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> That comment seems utterly wrong to me, because both PageIndexTupleDelete
> >> and PageIndexMultiDelete certainly contain assertions that every item on
> >> the page has storage.  Are you expecting that any BRIN index items
> >> wouldn't?  If they wouldn't, is adjusting their lp_off as if they did
> >> have storage sensible?
> 
> > It is possible in BRIN to have empty intermediate tuples; for example it
> > is possible for lp 1 and 3 to contain index tuples, while lp 2 does not.
> 
> Hm.  So apparently, the only reason this stuff works at all is that
> BRIN isn't using either PageIndexTupleDelete or PageIndexMultiDelete.

Yes, this is why the NoCompact variant was introduced in the first
place.

> > Now if this loop is concerned only with live lps and does not move lps,
> > then it should be fine to add the assertion.
> 
> No, it iterates over all lps on the page.  I'm inclined to think it
> should be written like
> 
>     if (ItemIdHasStorage(ii) && ItemIdGetOffset(ii) <= offset)
>         ii->lp_off += size_diff;
> 
> because futzing with the lp_off field in an item that isn't really
> pointing at storage feels wrong.  We might need to do that to
> PageIndexTupleDelete and/or PageIndexMultiDelete someday, too.

I suppose it is conceivable that we start using lp_off for other
purposes in the future, so I don't disagree.  I don't think index pages
currently do any funny business with it.

> I notice that PageIndexDeleteNoCompact, which seems to express what
> BRIN is expecting in a rather underdocumented way, forces any
> items without storage into "unused" state.  I don't really think
> it's bufpage.c's place to do that, though.  Do you think that code
> is actually supposed to fire, or is it just there for lack of a
> better idea?

I just put it there only because I didn't see any reason not to, really.
I don't think BRIN relies on it.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



pgsql-hackers by date:

Previous
From: Claudio Freire
Date:
Subject: Re: Is tuplesort_heap_siftup() a misnomer?
Next
From: Alvaro Herrera
Date:
Subject: Re: CVE-2016-1238 fix breaks (at least) pg_rewind tests