Tom Lane wrote:
> Hey Alvaro, can you comment on this bit in the proposed patch?
>
> + for (i = FirstOffsetNumber; i <= itemcount; i++)
> + {
> + ItemId ii = PageGetItemId(phdr, i);
> +
> + /* Normally here was following assertion
> + * Assert(ItemIdHasStorage(ii));
> + * This assertion was introduced in PageIndexTupleDelete
> + * But if this function will be used from BRIN index
> + * this assertion will fail. Thus, here we do not check that
> + * item has the storage.
> + */
> + if (ItemIdGetOffset(ii) <= offset)
> + ii->lp_off += size_diff;
> + }
> + }
>
> 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.
This is a tricky case to reproduce, but I think this should do it:
consider a table with pages_per_range=1. Page 1 is summarized by index
tuple 1, page 2 is summarized by itup 2, page 3 is summarized by index
tuple 3. On heap page 2 a new tuple is inserted and the summary data is
now much larger, such that the summary tuple no longer fits in the index
page, so it is moved to a new index page. Then index tuple 2 in the
first index page becomes unused. Revmap still points to lp 3, so it
can't be moved to lp 2. The lp_offs can be adjusted, as long as the lp
itself is not moved from its original position.
Now if this loop is concerned only with live lps and does not move lps,
then it should be fine to add the assertion.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
