Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC] - Mailing list pgsql-hackers

06.08.2016 19:51, Andrew Borodin:
> Anastasia , thank you for your attentive code examine.
>
> 2016-08-05 21:19 GMT+05:00 Anastasia Lubennikova <a.lubennikova@postgrespro.ru>:
>> 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).
> Actually, that's a tricky question. There may be different code expectations.
> 1. If we expect that not-maxaligned tuple may be placed by any other
> routine, we should remove check (size_diff != MAXALIGN(size_diff)),
> since it will fail for not-maxaligned tuple.
> 2. If we expect that noone should use PageIndexTupleOverwrite with
> not-maxaligned tuples, than current checks are OK: we will break
> execution if we find non-maxaligned tuples. With an almost correct
> message.
> I suggest that this check may be debug-only assertion: in a production
> code routine will work with not-maxaligned tuples if they already
> reside on the page, in a dev build it will inform dev that something
> is going wrong. Is this behavior Postgres-style compliant?
>
>
> I agree that pd_lower check makes sense.

Pointing out to this comparison I worried about possible call of
MAXALIGN for negative size_diff value. Although I don't sure if it can 
cause any problem.

Tuples on a page are always maxaligned.
But, as far as I recall,  itemid->lp_len can contain non-maxaligned value.

So, I'd suggest you to perform MAXALIGN(oldsize) before computing size_diff: size_diff = oldsize - alignednewsize;

Since both arguments are maxaligned the check of size_diff is not needed.

>> 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?
> Size reduction is unexpected for me.
> There might be 4 plausible explanations. I'll list them ordered by
> descending of probability:
> 1. Before this patch every update was throwing recently updated tuple
> to the end of a page. Thus, in some-how ordered data, recent best-fit
> will be discovered last. This can cause increase of MBB's overlap in
> spatial index and slightly higher tree. But 3 times size decrease is
> unlikely.
> How did you obtained those results? Can I look at a test case?

Nothing special, I've just run the test you provided in this thread.
And check index size via select pg_size_pretty(pg_relation_size('idx'));

> 2. Bug in PageIndexTupleDelete causing unused space emersion. I've
> searched for it, unsuccessfully.
> 3. Bug in PageIndexTupleOVerwrite. I cannot imagine nature of such a
> bug. May be we are not placing something not very important and big on
> a page?
> 4. Magic.
> I do not see what one should do with the R-tree to change it's size by
> a factor of 3. First three explanations are not better that forth,
> actually.
> Those 89 MB, they do not include WAL, right?

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