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