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

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: amborodin(at)acm(dot)org, Andrew Borodin <borodin(at)octonica(dot)com>, Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Oleg Bartunov <obartunov(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Sergey Mirvoda <sergey(at)mirvoda(dot)com>
Subject: Re: Re: GiST optimizing memmoves in gistplacetopage for fixed-size updates [PoC]
Date: 2016-09-08 22:02:23
Message-ID: 20160908220223.GA65203@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2016-09-08 22:04:58 Re: CVE-2016-1238 fix breaks (at least) pg_rewind tests
Previous Message Claudio Freire 2016-09-08 21:55:09 Re: Is tuplesort_heap_siftup() a misnomer?