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

30.07.2016 14:00, Andrew Borodin:
> Here is BRIN-compatible version of patch. Now function
> PageIndexTupleOverwrite consumes tuple size as a parameter, this
> behavior is coherent with PageAddItem.
> Also, i had to remove asserstion that item has a storage in the loop
> that adjusts line pointers. It would be great if someone could check
> that place (commented Assert(ItemIdHasStorage(ii)); in
> PageIndexTupleOverwrite). I suspect that there might be a case when
> linp's should not be adjusted.

Hi, I reviewed the code and I have couple of questions about
following code:
    /* may have negative size here if new tuple is larger */    size_diff = oldsize - alignednewsize;    offset =
ItemIdGetOffset(tupid);
    if (offset < phdr->pd_upper || (offset + size_diff) > 
phdr->pd_special ||        offset != MAXALIGN(offset) || size_diff != MAXALIGN(size_diff))        ereport(ERROR,
       (errcode(ERRCODE_DATA_CORRUPTED),                 errmsg("corrupted item offset: offset = %u, size = %u",
               offset, (unsigned int) size_diff)));
 

First of all, shouldn't we use MAXALIGN(oldsize) instead of oldsize?
Although, I'm quite sure that it was already aligned somewhere before.

I doubt that the check (size_diff != MAXALIGN(size_diff)) is necessary.
I'd rather add  the check: (offset+size_diff < pd_lower).

Besides that moment, the patch seems pretty good.

BTW, I'm very surprised that it improves performance so much.
And also size is reduced significantly. 89MB against 289MB without patch.
Could you explain in details, why does it happen?

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company