Thanks Amit. v19 addresses some of the comments below. On Thu, Mar 23, 2017 at 10:28 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 4:06 PM, Amit Kapila <amit.kapil...@gmail.com> > wrote: > > On Tue, Mar 21, 2017 at 6:47 PM, Pavan Deolasee > > <pavan.deola...@gmail.com> wrote: > >> > >>> > >> > >> Please find attached rebased patches. > >> > > > > Few comments on 0005_warm_updates_v18.patch: > > > > Few more comments on 0005_warm_updates_v18.patch: > 1. > @@ -234,6 +241,25 @@ index_beginscan(Relation heapRelation, > scan->heapRelation = heapRelation; > scan->xs_snapshot = snapshot; > > + /* > + * If the index supports recheck, > make sure that index tuple is saved > + * during index scans. Also build and cache IndexInfo which is used by > + * amrecheck routine. > + * > + * XXX Ideally, we should look at > all indexes on the table and check if > + * WARM is at all supported on the base table. If WARM is not supported > + * then we don't need to do any recheck. > RelationGetIndexAttrBitmap() does > + * do that and sets rd_supportswarm after looking at all indexes. But we > + * don't know if the function was called earlier in the > session when we're > + * here. We can't call it now because there exists a risk of causing > + * deadlock. > + */ > + if (indexRelation->rd_amroutine->amrecheck) > + { > +scan->xs_want_itup = true; > + scan->indexInfo = BuildIndexInfo(indexRelation); > + } > + > > Don't we need to do this rechecking during parallel scans? Also what > about bitmap heap scans? > > Yes, we need to handle parallel scans. Bitmap scans are not a problem because it can never return the same TID twice. I fixed this though by moving this inside index_beginscan_internal. > 2. > +++ b/src/backend/access/nbtree/nbtinsert.c > - > typedef struct > > Above change is not require. > > Sure. Fixed. > 3. > +_bt_clear_items(Page page, OffsetNumber *clearitemnos, uint16 nclearitems) > +void _hash_clear_items(Page page, OffsetNumber *clearitemnos, > + uint16 nclearitems) > > Both the above functions look exactly same, isn't it better to have a > single function like page_clear_items? If you want separation for > different index types, then we can have one common function which can > be called from different index types. > > Yes, makes sense. Moved that to bufpage.c. The reason why I originally had index-specific versions because I started by putting WARM flag in IndexTuple header. But since hash index does not have the bit free, moved everything to TID bit-flag. I still left index-specific wrappers, but they just call PageIndexClearWarmTuples. > 4. > - if (callback(htup, callback_state)) > + flags = ItemPointerGetFlags(&itup->t_tid); > + is_warm = ((flags & BTREE_INDEX_WARM_POINTER) != 0); > + > + if (is_warm) > + stats->num_warm_pointers++; > + else > + stats->num_clear_pointers++; > + > + result = callback(htup, is_warm, callback_state); > + if (result == IBDCR_DELETE) > + { > + if (is_warm) > + stats->warm_pointers_removed++; > + else > + stats->clear_pointers_removed++; > > The patch looks to be inconsistent in collecting stats for btree and > hash. I don't see above stats are getting updated in hash index code. > > Fixed. The hashbucketcleanup signature is just getting a bit too long. May be we should move some of these counters in a structure and pass that around. Not done here though. > 5. > +btrecheck(Relation indexRel, IndexInfo *indexInfo, IndexTuple indexTuple, > + Relation heapRel, HeapTuple heapTuple) > { > .. > + if (!datumIsEqual(values[i - 1], indxvalue, att->attbyval, > + att->attlen)) > .. > } > > Will this work if the index is using non-default collation? > > Not sure I understand that. Can you please elaborate? > 6. > +++ b/src/backend/access/nbtree/nbtxlog.c > @@ -390,83 +390,9 @@ btree_xlog_vacuum(XLogReaderState *record) > -#ifdef UNUSED > xl_btree_vacuum *xlrec = (xl_btree_vacuum *) XLogRecGetData(record); > > /* > - * This section of code is thought to be no longer needed, after analysis > - * of the calling paths. It is retained to allow the code to be reinstated > - * if a flaw is revealed in that thinking. > - * > .. > > Why does this patch need to remove the above code under #ifdef UNUSED > > Yeah, it isn't strictly necessary. But that dead code was coming in the way and hence I decided to strip it out. I can put it back if it's an issue or remove that as a separate commit first. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services