Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]

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.

> 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 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?
        regards, tom lane