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