| From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
|---|---|
| To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
| 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 21:08:36 |
| Message-ID: | 5108.1473368916@sss.pgh.pa.us |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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?
regards, tom lane
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tom Lane | 2016-09-08 21:09:34 | Re: Is tuplesort_heap_siftup() a misnomer? |
| Previous Message | Andres Freund | 2016-09-08 21:04:40 | Re: CVE-2016-1238 fix breaks (at least) pg_rewind tests |