From b9021c498bb273055f8cf8809030c4abc7848737 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 1 Jan 2024 21:50:47 +0100 Subject: [PATCH v20240103 2/2] switch to StreamingRead-like API --- src/backend/access/heap/heapam_handler.c | 2 +- src/backend/access/index/README.prefetch | 3 + src/backend/access/index/genam.c | 4 +- src/backend/access/index/indexam.c | 254 ++++++++++++----------- src/backend/executor/execIndexing.c | 6 +- src/backend/executor/execReplication.c | 9 +- src/backend/executor/nodeIndexonlyscan.c | 149 +++++++------ src/backend/executor/nodeIndexscan.c | 105 ++++++---- src/backend/optimizer/plan/createplan.c | 27 ++- src/backend/utils/adt/selfuncs.c | 2 +- src/include/access/genam.h | 68 ++++-- src/include/nodes/execnodes.h | 4 +- src/include/nodes/plannodes.h | 2 + 13 files changed, 382 insertions(+), 253 deletions(-) create mode 100644 src/backend/access/index/README.prefetch diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 26d3ec20b63..7c28dafb728 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -792,7 +792,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap, if (indexScan != NULL) { - if (!index_getnext_slot(indexScan, ForwardScanDirection, slot, NULL)) + if (!index_getnext_slot(indexScan, ForwardScanDirection, slot)) break; /* Since we used no scan keys, should never need to recheck */ diff --git a/src/backend/access/index/README.prefetch b/src/backend/access/index/README.prefetch new file mode 100644 index 00000000000..2a6ac4a0eea --- /dev/null +++ b/src/backend/access/index/README.prefetch @@ -0,0 +1,3 @@ +- index heap prefetch overview +- +- callback - decision whether to prefetch, possibility to keep data diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index 72e7c9f206c..4ca12006843 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -509,7 +509,7 @@ systable_getnext(SysScanDesc sysscan) if (sysscan->irel) { - if (index_getnext_slot(sysscan->iscan, ForwardScanDirection, sysscan->slot, NULL)) + if (index_getnext_slot(sysscan->iscan, ForwardScanDirection, sysscan->slot)) { bool shouldFree; @@ -713,7 +713,7 @@ systable_getnext_ordered(SysScanDesc sysscan, ScanDirection direction) HeapTuple htup = NULL; Assert(sysscan->irel); - if (index_getnext_slot(sysscan->iscan, direction, sysscan->slot, NULL)) + if (index_getnext_slot(sysscan->iscan, direction, sysscan->slot)) htup = ExecFetchSlotHeapTuple(sysscan->slot, false, NULL); /* See notes in systable_getnext */ diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c index f96aeba1b39..cdad3f4c6f9 100644 --- a/src/backend/access/index/indexam.c +++ b/src/backend/access/index/indexam.c @@ -49,7 +49,6 @@ #include "access/relscan.h" #include "access/tableam.h" #include "access/transam.h" -#include "access/visibilitymap.h" #include "access/xlog.h" #include "catalog/index.h" #include "catalog/pg_amproc.h" @@ -64,6 +63,7 @@ #include "utils/lsyscache.h" #include "utils/ruleutils.h" #include "utils/snapmgr.h" +#include "utils/spccache.h" #include "utils/syscache.h" @@ -111,13 +111,16 @@ static IndexScanDesc index_beginscan_internal(Relation indexRelation, int nkeys, int norderbys, Snapshot snapshot, ParallelIndexScanDesc pscan, bool temp_snap); -static void index_prefetch_tids(IndexScanDesc scan, ScanDirection direction, - IndexPrefetch *prefetch); -static ItemPointer index_prefetch_get_tid(IndexScanDesc scan, ScanDirection direction, - IndexPrefetch *prefetch, bool *all_visible); -static void index_prefetch(IndexScanDesc scan, IndexPrefetch *prefetch, - ItemPointer tid, bool skip_all_visible, bool *all_visible); - +/* index prefetching of heap pages */ +static void index_prefetch_tids(IndexScanDesc scan, + IndexPrefetch *prefetch, + ScanDirection direction); +static IndexPrefetchEntry *index_prefetch_get_entry(IndexScanDesc scan, + IndexPrefetch *prefetch, + ScanDirection direction); +static void index_prefetch_heap_page(IndexScanDesc scan, + IndexPrefetch *prefetch, + IndexPrefetchEntry *entry); /* ---------------------------------------------------------------- * index_ interface functions @@ -546,8 +549,8 @@ index_beginscan_parallel(Relation heaprel, Relation indexrel, int nkeys, * or NULL if no more matching tuples exist. * ---------------- */ -static ItemPointer -index_getnext_tid_internal(IndexScanDesc scan, ScanDirection direction) +ItemPointer +index_getnext_tid(IndexScanDesc scan, ScanDirection direction) { bool found; @@ -643,24 +646,22 @@ index_fetch_heap(IndexScanDesc scan, TupleTableSlot *slot) * Note: caller must check scan->xs_recheck, and perform rechecking of the * scan keys if required. We do not do that here because we don't have * enough information to do it efficiently in the general case. + * + * XXX This does not support prefetching of heap pages. When such prefetching is + * desirable, use index_getnext_tid(). * ---------------- */ bool -index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot *slot, - IndexPrefetch *prefetch) +index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot *slot) { for (;;) { - /* Do prefetching (if requested/enabled). */ - index_prefetch_tids(scan, direction, prefetch); - if (!scan->xs_heap_continue) { - ItemPointer tid; - bool all_visible; + ItemPointer tid; /* Time to fetch the next TID from the index */ - tid = index_prefetch_get_tid(scan, direction, prefetch, &all_visible); + tid = index_getnext_tid(scan, direction); /* If we're out of index entries, we're done */ if (tid == NULL) @@ -1339,13 +1340,9 @@ index_prefetch_add_cache(IndexPrefetch *prefetch, BlockNumber block) * value once in a while, and see what happens. */ static void -index_prefetch(IndexScanDesc scan, IndexPrefetch *prefetch, - ItemPointer tid, bool skip_all_visible, bool *all_visible) +index_prefetch_heap_page(IndexScanDesc scan, IndexPrefetch *prefetch, IndexPrefetchEntry *entry) { - BlockNumber block; - - /* by default not all visible (or we didn't check) */ - *all_visible = false; + BlockNumber block = ItemPointerGetBlockNumber(&entry->tid); /* * No heap relation means bitmap index scan, which does prefetching at the @@ -1355,6 +1352,8 @@ index_prefetch(IndexScanDesc scan, IndexPrefetch *prefetch, * XXX But in this case we should have prefetchMaxTarget=0, because in * index_bebinscan_bitmap() we disable prefetching. So maybe we should * just check that. + * + * XXX Comment/check seems obsolete. */ if (!prefetch) return; @@ -1362,37 +1361,10 @@ index_prefetch(IndexScanDesc scan, IndexPrefetch *prefetch, /* * If we got here, prefetching is enabled and it's a node that supports * prefetching (i.e. it can't be a bitmap index scan). - */ - Assert(scan->heapRelation); - - block = ItemPointerGetBlockNumber(tid); - - /* - * When prefetching for IOS, we want to only prefetch pages that are not - * marked as all-visible (because not fetching all-visible pages is the - * point of IOS). * - * XXX This is not great, because it releases the VM buffer for each TID - * we consider to prefetch. We should reuse that somehow, similar to the - * actual IOS code. Ideally, we should use the same ioss_VMBuffer (if - * we can propagate it here). Or at least do it for a bulk of prefetches, - * although that's not very useful - after the ramp-up we will prefetch - * the pages one by one anyway. - * - * XXX Ideally we'd also propagate this to the executor, so that the - * nodeIndexonlyscan.c doesn't need to repeat the same VM check (which - * is measurable). But the index_getnext_tid() is not really well - * suited for that, so the API needs a change.s + * XXX Comment/check seems obsolete. */ - if (skip_all_visible) - { - *all_visible = VM_ALL_VISIBLE(scan->heapRelation, - block, - &prefetch->vmBuffer); - - if (*all_visible) - return; - } + Assert(scan->heapRelation); /* * Do not prefetch the same block over and over again, @@ -1412,42 +1384,12 @@ index_prefetch(IndexScanDesc scan, IndexPrefetch *prefetch, prefetch->countAll++; } -/* ---------------- - * index_getnext_tid - get the next TID from a scan - * - * The result is the next TID satisfying the scan keys, - * or NULL if no more matching tuples exist. - * - * FIXME not sure this handles xs_heapfetch correctly. - * ---------------- +/* + * index_prefetch_tids + * Fill the prefetch queue and issue necessary prefetch requests. */ -ItemPointer -index_getnext_tid(IndexScanDesc scan, ScanDirection direction, - IndexPrefetch *prefetch) -{ - bool all_visible; /* ignored */ - - /* Do prefetching (if requested/enabled). */ - index_prefetch_tids(scan, direction, prefetch); - - /* Read the TID from the queue (or directly from the index). */ - return index_prefetch_get_tid(scan, direction, prefetch, &all_visible); -} - -ItemPointer -index_getnext_tid_vm(IndexScanDesc scan, ScanDirection direction, - IndexPrefetch *prefetch, bool *all_visible) -{ - /* Do prefetching (if requested/enabled). */ - index_prefetch_tids(scan, direction, prefetch); - - /* Read the TID from the queue (or directly from the index). */ - return index_prefetch_get_tid(scan, direction, prefetch, all_visible); -} - static void -index_prefetch_tids(IndexScanDesc scan, ScanDirection direction, - IndexPrefetch *prefetch) +index_prefetch_tids(IndexScanDesc scan, IndexPrefetch *prefetch, ScanDirection direction) { /* * If the prefetching is still active (i.e. enabled and we still @@ -1473,43 +1415,46 @@ index_prefetch_tids(IndexScanDesc scan, ScanDirection direction, */ while (!PREFETCH_FULL(prefetch)) { - ItemPointer tid; - bool all_visible; + IndexPrefetchEntry *entry = prefetch->next_cb(scan, prefetch, direction); - /* Time to fetch the next TID from the index */ - tid = index_getnext_tid_internal(scan, direction); - - /* - * If we're out of index entries, we're done (and we mark the - * the prefetcher as inactive). - */ - if (tid == NULL) + /* no more entries in this index scan */ + if (entry == NULL) { prefetch->prefetchDone = true; - break; + return; } - Assert(ItemPointerEquals(tid, &scan->xs_heaptid)); + Assert(ItemPointerEquals(&entry->tid, &scan->xs_heaptid)); - /* - * Issue the actuall prefetch requests for the new TID. - * - * XXX index_getnext_tid_prefetch is only called for IOS (for now), - * so skip prefetching of all-visible pages. - */ - index_prefetch(scan, prefetch, tid, prefetch->indexonly, &all_visible); + /* store the entry and then maybe issue the prefetch request */ + prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueEnd++)] = *entry; - prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueEnd)].tid = *tid; - prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueEnd)].all_visible = all_visible; - prefetch->queueEnd++; + /* issue the prefetch request? */ + if (entry->prefetch) + index_prefetch_heap_page(scan, prefetch, entry); } } } -static ItemPointer -index_prefetch_get_tid(IndexScanDesc scan, ScanDirection direction, - IndexPrefetch *prefetch, bool *all_visible) +/* + * index_prefetch_get_entry + * Get the next entry from the prefetch queue (or from the index directly). + * + * If prefetching is enabled, get next entry from the prefetch queue (unless + * queue is empty). With prefetching disabled, read an entry directly from the + * index scan. + * + * XXX not sure this correctly handles xs_heap_continue - see index_getnext_slot, + * maybe nodeIndexscan needs to do something more to handle this? Although, that + * should be in the indexscan next_cb callback, probably. + * + * XXX If xs_heap_continue=true, we need to return the last TID. + */ +static IndexPrefetchEntry * +index_prefetch_get_entry(IndexScanDesc scan, IndexPrefetch *prefetch, ScanDirection direction) { + IndexPrefetchEntry *entry = NULL; + /* * With prefetching enabled (even if we already finished reading * all TIDs from the index scan), we need to return a TID from the @@ -1522,25 +1467,98 @@ index_prefetch_get_tid(IndexScanDesc scan, ScanDirection direction, if (PREFETCH_DONE(prefetch)) return NULL; - scan->xs_heaptid = prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueIndex)].tid; - *all_visible = prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueIndex)].all_visible; + entry = palloc(sizeof(IndexPrefetchEntry)); + + entry->tid = prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueIndex)].tid; + entry->data = prefetch->queueItems[PREFETCH_QUEUE_INDEX(prefetch->queueIndex)].data; + prefetch->queueIndex++; + + scan->xs_heaptid = entry->tid; } else /* not prefetching, just do the regular work */ { ItemPointer tid; /* Time to fetch the next TID from the index */ - tid = index_getnext_tid_internal(scan, direction); - *all_visible = false; + tid = index_getnext_tid(scan, direction); /* If we're out of index entries, we're done */ if (tid == NULL) return NULL; Assert(ItemPointerEquals(tid, &scan->xs_heaptid)); + + entry = palloc(sizeof(IndexPrefetchEntry)); + + entry->tid = scan->xs_heaptid; + entry->data = NULL; } - /* Return the TID of the tuple we found. */ - return &scan->xs_heaptid; + return entry; +} + +int +index_heap_prefetch_target(Relation heapRel, double plan_rows, bool allow_prefetch) +{ + /* + * XXX No prefetching for direct I/O. + * + * XXX Shouldn't we do prefetching even for direct I/O? We would only pretend + * doing it now, ofc, because we'd not do posix_fadvise(), but once the code + * starts loading into shared buffers, that'd work. + */ + if ((io_direct_flags & IO_DIRECT_DATA) == 0) + return 0; + + /* disable prefetching for cursors etc. */ + if (!allow_prefetch) + return 0; + + /* + * Determine number of heap pages to prefetch for this index. This is + * essentially just effective_io_concurrency for the table (or the + * tablespace it's in). + * + * XXX Should this also look at plan.plan_rows and maybe cap the target + * to that? Pointless to prefetch more than we expect to use. Or maybe + * just reset to that value during prefetching, after reading the next + * index page (or rather after rescan)? + * + * XXX Maybe reduce the value with parallel workers? + */ + return Min(get_tablespace_io_concurrency(heapRel->rd_rel->reltablespace), + plan_rows); +} + +IndexPrefetch * +IndexPrefetchAlloc(IndexPrefetchNextCB next_cb, int prefetch_max, void *data) +{ + IndexPrefetch *prefetch = palloc0(sizeof(IndexPrefetch)); + + prefetch->queueIndex = 0; + prefetch->queueStart = 0; + prefetch->queueEnd = 0; + + prefetch->prefetchTarget = 0; + prefetch->prefetchMaxTarget = prefetch_max; + + /* + * Customize the prefetch to also check visibility map and keep + * the result so that IOS does not need to repeat it. + */ + prefetch->next_cb = next_cb; + prefetch->data = data; + + return prefetch; +} + +IndexPrefetchEntry * +IndexPrefetchNext(IndexScanDesc scan, IndexPrefetch *prefetch, ScanDirection direction) +{ + /* Do prefetching (if requested/enabled). */ + index_prefetch_tids(scan, prefetch, direction); + + /* Read the TID from the queue (or directly from the index). */ + return index_prefetch_get_entry(scan, prefetch, direction); } diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 0a136db6712..2fa2118f3c2 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -777,11 +777,7 @@ retry: index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts, 0); index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0); - /* - * XXX Would be nice to also benefit from prefetching here. All we need to - * do is instantiate the prefetcher, I guess. - */ - while (index_getnext_slot(index_scan, ForwardScanDirection, existing_slot, NULL)) + while (index_getnext_slot(index_scan, ForwardScanDirection, existing_slot)) { TransactionId xwait; XLTW_Oper reason_wait; diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 9498b00fa64..81f27042bc4 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -212,13 +212,8 @@ retry: index_rescan(scan, skey, skey_attoff, NULL, 0); - /* - * Try to find the tuple - * - * XXX Would be nice to also benefit from prefetching here. All we need to - * do is instantiate the prefetcher, I guess. - */ - while (index_getnext_slot(scan, ForwardScanDirection, outslot, NULL)) + /* Try to find the tuple */ + while (index_getnext_slot(scan, ForwardScanDirection, outslot)) { /* * Avoid expensive equality check if the index is primary key or diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index a7eadaf3db2..af7dd364f33 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -43,12 +43,13 @@ #include "storage/predicate.h" #include "utils/memutils.h" #include "utils/rel.h" -#include "utils/spccache.h" static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node); static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc); - +static IndexPrefetchEntry *IndexOnlyPrefetchNext(IndexScanDesc scan, + IndexPrefetch *prefetch, + ScanDirection direction); /* ---------------------------------------------------------------- * IndexOnlyNext @@ -66,7 +67,7 @@ IndexOnlyNext(IndexOnlyScanState *node) TupleTableSlot *slot; ItemPointer tid; IndexPrefetch *prefetch; - bool all_visible; + IndexPrefetchEntry *entry; /* * extract necessary information from index scan node @@ -76,6 +77,12 @@ IndexOnlyNext(IndexOnlyScanState *node) /* * Determine which direction to scan the index in based on the plan's scan * direction and the current direction of execution. + * + * XXX Could this be an issue for the prefetching? What if we prefetch something + * but the direction changes before we get to the read? If that could happen, + * maybe we should discard the prefetched data and go back? But can we even + * do that, if we already fetched some TIDs from the index? I don't think + * indexorderdir can't change, but es_direction maybe can? */ direction = ScanDirectionCombine(estate->es_direction, ((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir); @@ -119,10 +126,15 @@ IndexOnlyNext(IndexOnlyScanState *node) /* * OK, now that we have what we need, fetch the next tuple. */ - while ((tid = index_getnext_tid_vm(scandesc, direction, prefetch, &all_visible)) != NULL) + while ((entry = IndexPrefetchNext(scandesc, prefetch, direction)) != NULL) { + bool *all_visible = NULL; bool tuple_from_heap = false; + /* unpack the entry */ + tid = &entry->tid; + all_visible = (bool *) entry->data; /* result of visibility check */ + CHECK_FOR_INTERRUPTS(); /* @@ -161,7 +173,7 @@ IndexOnlyNext(IndexOnlyScanState *node) * * XXX Skip if we already know the page is all visible from prefetcher. */ - if (!all_visible && + if (!(all_visible && *all_visible) && !VM_ALL_VISIBLE(scandesc->heapRelation, ItemPointerGetBlockNumber(tid), &node->ioss_VMBuffer)) @@ -367,6 +379,9 @@ ExecReScanIndexOnlyScan(IndexOnlyScanState *node) prefetch->queueIndex = 0; prefetch->queueStart = 0; prefetch->queueEnd = 0; + + prefetch->prefetchDone = false; + prefetch->prefetchTarget = 0; } ExecScanReScan(&node->ss); @@ -401,6 +416,8 @@ ExecEndIndexOnlyScan(IndexOnlyScanState *node) { IndexPrefetch *prefetch = node->ioss_prefetch; + Buffer *buffer = (Buffer *) prefetch->data; + /* XXX some debug info */ elog(LOG, "index prefetch stats: requests " UINT64_FORMAT " prefetches " UINT64_FORMAT " (%f) skip cached " UINT64_FORMAT " sequential " UINT64_FORMAT, prefetch->countAll, @@ -409,10 +426,10 @@ ExecEndIndexOnlyScan(IndexOnlyScanState *node) prefetch->countSkipCached, prefetch->countSkipSequential); - if (prefetch->vmBuffer != InvalidBuffer) + if (*buffer != InvalidBuffer) { - ReleaseBuffer(prefetch->vmBuffer); - prefetch->vmBuffer = InvalidBuffer; + ReleaseBuffer(*buffer); + *buffer = InvalidBuffer; } } @@ -512,6 +529,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags) Relation currentRelation; LOCKMODE lockmode; TupleDesc tupDesc; + int prefetch_max; /* * create state structure @@ -641,61 +659,33 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags) } /* - * Also initialize index prefetcher. + * Also initialize index prefetcher. We do this even when prefetching is + * not done (see index_heap_prefetch_calculate_target), because the + * prefetcher is used for all index reads. + * + * We reach here if the index only scan is not parallel, or if we're + * serially executing an index only scan that was planned to be + * parallel. * - * XXX No prefetching for direct I/O. + * XXX Maybe we should enable prefetching, but prefetch only pages that + * are not all-visible (but checking that from the index code seems like + * a violation of layering etc). + * + * XXX This might lead to IOS being slower than plain index scan, if the + * table has a lot of pages that need recheck. + * + * Remember this is index-only scan, because of prefetching. Not the most + * elegant way to pass this info. + * + * XXX Maybe rename the object to "index reader" or something? */ - if ((io_direct_flags & IO_DIRECT_DATA) == 0) - { - int prefetch_max; - Relation heapRel = indexstate->ss.ss_currentRelation; - - /* - * Determine number of heap pages to prefetch for this index. This is - * essentially just effective_io_concurrency for the table (or the - * tablespace it's in). - * - * XXX Should this also look at plan.plan_rows and maybe cap the target - * to that? Pointless to prefetch more than we expect to use. Or maybe - * just reset to that value during prefetching, after reading the next - * index page (or rather after rescan)? - * - * XXX Maybe reduce the value with parallel workers? - */ - prefetch_max = Min(get_tablespace_io_concurrency(heapRel->rd_rel->reltablespace), - indexstate->ss.ps.plan->plan_rows); - - /* - * We reach here if the index only scan is not parallel, or if we're - * serially executing an index only scan that was planned to be - * parallel. - * - * XXX Maybe we should enable prefetching, but prefetch only pages that - * are not all-visible (but checking that from the index code seems like - * a violation of layering etc). - * - * XXX This might lead to IOS being slower than plain index scan, if the - * table has a lot of pages that need recheck. - * - * Remember this is index-only scan, because of prefetching. Not the most - * elegant way to pass this info. - */ - if (prefetch_max > 0) - { - IndexPrefetch *prefetch = palloc0(sizeof(IndexPrefetch)); + prefetch_max = index_heap_prefetch_target(indexstate->ss.ss_currentRelation, + indexstate->ss.ps.plan->plan_rows, + node->allow_prefetch); - prefetch->queueIndex = 0; - prefetch->queueStart = 0; - prefetch->queueEnd = 0; - - prefetch->prefetchTarget = 0; - prefetch->prefetchMaxTarget = prefetch_max; - prefetch->vmBuffer = InvalidBuffer; - prefetch->indexonly = true; - - indexstate->ioss_prefetch = prefetch; - } - } + indexstate->ioss_prefetch = IndexPrefetchAlloc(IndexOnlyPrefetchNext, + prefetch_max, + palloc0(sizeof(Buffer))); /* * all done. @@ -808,3 +798,42 @@ ExecIndexOnlyScanInitializeWorker(IndexOnlyScanState *node, node->ioss_ScanKeys, node->ioss_NumScanKeys, node->ioss_OrderByKeys, node->ioss_NumOrderByKeys); } + +/* + * When prefetching for IOS, we want to only prefetch pages that are not + * marked as all-visible (because not fetching all-visible pages is the + * point of IOS). + * + * The buffer used by the VM_ALL_VISIBLE() check is reused, similarly to + * ioss_VMBuffer (maybe we could/should use it here too?). We also keep + * the result of the all_visible flag, so that the main loop does not to + * do it again. + */ +static IndexPrefetchEntry * +IndexOnlyPrefetchNext(IndexScanDesc scan, IndexPrefetch *prefetch, ScanDirection direction) +{ + IndexPrefetchEntry *entry = NULL; + ItemPointer tid; + + if ((tid = index_getnext_tid(scan, direction)) != NULL) + { + BlockNumber blkno = ItemPointerGetBlockNumber(tid); + + bool all_visible = VM_ALL_VISIBLE(scan->heapRelation, + blkno, + (Buffer *) prefetch->data); + + entry = palloc0(sizeof(IndexPrefetchEntry)); + + entry->tid = *tid; + + /* prefetch only if not all visible */ + entry->prefetch = !all_visible; + + /* store the all_visible flag in the private part of the entry */ + entry->data = palloc(sizeof(bool)); + *(bool *) entry->data = all_visible; + } + + return entry; +} diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index b3282ec5a75..bd65337270c 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -43,7 +43,6 @@ #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/rel.h" -#include "utils/spccache.h" /* * When an ordering operator is used, tuples fetched from the index that @@ -70,6 +69,9 @@ static void reorderqueue_push(IndexScanState *node, TupleTableSlot *slot, Datum *orderbyvals, bool *orderbynulls); static HeapTuple reorderqueue_pop(IndexScanState *node); +static IndexPrefetchEntry *IndexScanPrefetchNext(IndexScanDesc scan, + IndexPrefetch *prefetch, + ScanDirection direction); /* ---------------------------------------------------------------- * IndexNext @@ -87,6 +89,7 @@ IndexNext(IndexScanState *node) IndexScanDesc scandesc; TupleTableSlot *slot; IndexPrefetch *prefetch; + IndexPrefetchEntry *entry; /* * extract necessary information from index scan node @@ -131,10 +134,19 @@ IndexNext(IndexScanState *node) /* * ok, now that we have what we need, fetch the next tuple. */ - while (index_getnext_slot(scandesc, direction, slot, prefetch)) + while ((entry = IndexPrefetchNext(scandesc, prefetch, direction)) != NULL) { CHECK_FOR_INTERRUPTS(); + /* + * Fetch the next (or only) visible heap tuple for this index entry. + * If we don't find anything, loop around and grab the next TID from + * the index. + */ + Assert(ItemPointerIsValid(&scandesc->xs_heaptid)); + if (!index_fetch_heap(scandesc, slot)) + continue; + /* * If the index was lossy, we have to recheck the index quals using * the fetched tuple. @@ -180,7 +192,6 @@ IndexNextWithReorder(IndexScanState *node) Datum *lastfetched_vals; bool *lastfetched_nulls; int cmp; - IndexPrefetch *prefetch; estate = node->ss.ps.state; @@ -197,7 +208,6 @@ IndexNextWithReorder(IndexScanState *node) Assert(ScanDirectionIsForward(estate->es_direction)); scandesc = node->iss_ScanDesc; - prefetch = node->iss_prefetch; econtext = node->ss.ps.ps_ExprContext; slot = node->ss.ss_ScanTupleSlot; @@ -264,7 +274,7 @@ IndexNextWithReorder(IndexScanState *node) * Fetch next tuple from the index. */ next_indextuple: - if (!index_getnext_slot(scandesc, ForwardScanDirection, slot, prefetch)) + if (!index_getnext_slot(scandesc, ForwardScanDirection, slot)) { /* * No more tuples from the index. But we still need to drain any @@ -601,6 +611,9 @@ ExecReScanIndexScan(IndexScanState *node) prefetch->queueIndex = 0; prefetch->queueStart = 0; prefetch->queueEnd = 0; + + prefetch->prefetchDone = false; + prefetch->prefetchTarget = 0; } ExecScanReScan(&node->ss); @@ -917,6 +930,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags) IndexScanState *indexstate; Relation currentRelation; LOCKMODE lockmode; + int prefetch_max; /* * create state structure @@ -1095,43 +1109,33 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags) } /* - * Also initialize index prefetcher. + * Also initialize index prefetcher. We do this even when prefetching is + * not done (see index_heap_prefetch_calculate_target), because the + * prefetcher is used for all index reads. + * + * We reach here if the index only scan is not parallel, or if we're + * serially executing an index only scan that was planned to be + * parallel. + * + * XXX Maybe we should enable prefetching, but prefetch only pages that + * are not all-visible (but checking that from the index code seems like + * a violation of layering etc). * - * XXX No prefetching for direct I/O. + * XXX This might lead to IOS being slower than plain index scan, if the + * table has a lot of pages that need recheck. + * + * Remember this is index-only scan, because of prefetching. Not the most + * elegant way to pass this info. + * + * XXX Maybe rename the object to "index reader" or something? */ - if ((io_direct_flags & IO_DIRECT_DATA) == 0) - { - int prefetch_max; - Relation heapRel = indexstate->ss.ss_currentRelation; - - /* - * Determine number of heap pages to prefetch for this index scan. This - * is essentially just effective_io_concurrency for the table (or the - * tablespace it's in). - * - * XXX Should this also look at plan.plan_rows and maybe cap the target - * to that? Pointless to prefetch more than we expect to use. Or maybe - * just reset to that value during prefetching, after reading the next - * index page (or rather after rescan)? - */ - prefetch_max = Min(get_tablespace_io_concurrency(heapRel->rd_rel->reltablespace), - indexstate->ss.ps.plan->plan_rows); - - if (prefetch_max > 0) - { - IndexPrefetch *prefetch = palloc0(sizeof(IndexPrefetch)); - - prefetch->queueIndex = 0; - prefetch->queueStart = 0; - prefetch->queueEnd = 0; + prefetch_max = index_heap_prefetch_target(indexstate->ss.ss_currentRelation, + indexstate->ss.ps.plan->plan_rows, + node->allow_prefetch); - prefetch->prefetchTarget = 0; - prefetch->prefetchMaxTarget = prefetch_max; - prefetch->vmBuffer = InvalidBuffer; - - indexstate->iss_prefetch = prefetch; - } - } + indexstate->iss_prefetch = IndexPrefetchAlloc(IndexScanPrefetchNext, + prefetch_max, + NULL); /* * all done. @@ -1795,3 +1799,26 @@ ExecIndexScanInitializeWorker(IndexScanState *node, node->iss_ScanKeys, node->iss_NumScanKeys, node->iss_OrderByKeys, node->iss_NumOrderByKeys); } + +/* + * XXX not sure this correctly handles xs_heap_continue - see index_getnext_slot, + * maybe nodeIndexscan needs to do something more to handle this? + */ +static IndexPrefetchEntry * +IndexScanPrefetchNext(IndexScanDesc scan, IndexPrefetch *prefetch, ScanDirection direction) +{ + IndexPrefetchEntry *entry = NULL; + ItemPointer tid; + + if ((tid = index_getnext_tid(scan, direction)) != NULL) + { + entry = palloc0(sizeof(IndexPrefetchEntry)); + + entry->tid = *tid; + + /* prefetch always */ + entry->prefetch = true; + } + + return entry; +} diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 34ca6d4ac21..0abbcd31ddd 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -184,13 +184,15 @@ static IndexScan *make_indexscan(List *qptlist, List *qpqual, Index scanrelid, Oid indexid, List *indexqual, List *indexqualorig, List *indexorderby, List *indexorderbyorig, List *indexorderbyops, - ScanDirection indexscandir); + ScanDirection indexscandir, + bool allow_prefetch); static IndexOnlyScan *make_indexonlyscan(List *qptlist, List *qpqual, Index scanrelid, Oid indexid, List *indexqual, List *recheckqual, List *indexorderby, List *indextlist, - ScanDirection indexscandir); + ScanDirection indexscandir, + bool allow_prefetch); static BitmapIndexScan *make_bitmap_indexscan(Index scanrelid, Oid indexid, List *indexqual, List *indexqualorig); @@ -3161,6 +3163,13 @@ create_indexscan_plan(PlannerInfo *root, } } + /* + * XXX Only allow index prefetching when parallelModeOK=true. This is a bit + * of a misuse of the flag, but we need to disable prefetching for cursors + * (which might change direction), and parallelModeOK does that. But maybe + * we might (or should) have a separate flag. + */ + /* Finally ready to build the plan node */ if (indexonly) scan_plan = (Scan *) make_indexonlyscan(tlist, @@ -3171,7 +3180,8 @@ create_indexscan_plan(PlannerInfo *root, stripped_indexquals, fixed_indexorderbys, indexinfo->indextlist, - best_path->indexscandir); + best_path->indexscandir, + root->glob->parallelModeOK); else scan_plan = (Scan *) make_indexscan(tlist, qpqual, @@ -3182,7 +3192,8 @@ create_indexscan_plan(PlannerInfo *root, fixed_indexorderbys, indexorderbys, indexorderbyops, - best_path->indexscandir); + best_path->indexscandir, + root->glob->parallelModeOK); copy_generic_path_info(&scan_plan->plan, &best_path->path); @@ -5522,7 +5533,8 @@ make_indexscan(List *qptlist, List *indexorderby, List *indexorderbyorig, List *indexorderbyops, - ScanDirection indexscandir) + ScanDirection indexscandir, + bool allow_prefetch) { IndexScan *node = makeNode(IndexScan); Plan *plan = &node->scan.plan; @@ -5539,6 +5551,7 @@ make_indexscan(List *qptlist, node->indexorderbyorig = indexorderbyorig; node->indexorderbyops = indexorderbyops; node->indexorderdir = indexscandir; + node->allow_prefetch = allow_prefetch; return node; } @@ -5552,7 +5565,8 @@ make_indexonlyscan(List *qptlist, List *recheckqual, List *indexorderby, List *indextlist, - ScanDirection indexscandir) + ScanDirection indexscandir, + bool allow_prefetch) { IndexOnlyScan *node = makeNode(IndexOnlyScan); Plan *plan = &node->scan.plan; @@ -5568,6 +5582,7 @@ make_indexonlyscan(List *qptlist, node->indexorderby = indexorderby; node->indextlist = indextlist; node->indexorderdir = indexscandir; + node->allow_prefetch = allow_prefetch; return node; } diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index b5c79359425..e11d022827a 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -6297,7 +6297,7 @@ get_actual_variable_endpoint(Relation heapRel, index_rescan(index_scan, scankeys, 1, NULL, 0); /* Fetch first/next tuple in specified direction */ - while ((tid = index_getnext_tid(index_scan, indexscandir, NULL)) != NULL) + while ((tid = index_getnext_tid(index_scan, indexscandir)) != NULL) { BlockNumber block = ItemPointerGetBlockNumber(tid); diff --git a/src/include/access/genam.h b/src/include/access/genam.h index c0c46d7a05f..f3452e8a799 100644 --- a/src/include/access/genam.h +++ b/src/include/access/genam.h @@ -129,7 +129,7 @@ typedef struct IndexOrderByDistance bool isnull; } IndexOrderByDistance; - +/* index prefetching - probably should be somewhere else, outside indexam */ /* * Cache of recently prefetched blocks, organized as a hash table of @@ -159,6 +159,8 @@ typedef struct IndexPrefetchCacheEntry { /* * Used to detect sequential patterns (and disable prefetching). + * + * XXX seems strange to have two separate values */ #define PREFETCH_QUEUE_HISTORY 8 #define PREFETCH_SEQ_PATTERN_BLOCKS 4 @@ -166,9 +168,38 @@ typedef struct IndexPrefetchCacheEntry { typedef struct IndexPrefetchEntry { ItemPointerData tid; - bool all_visible; + + /* should we prefetch heap page for this TID? */ + bool prefetch; + + /* + * If a callback is specified, it may store per-tid information. The + * data has to be a single palloc-ed piece of data, so that it can + * be easily pfreed. + * + * XXX We could relax this by providing another cleanup callback, but + * that seems unnecessarily complex - we expect the information to be + * very simple, like bool flags or something. Easy to do in a simple + * struct, and perhaps even reuse without pfree/palloc. + */ + void *data; } IndexPrefetchEntry; +/* needs to be before IndexPrefetchCallback typedef */ +typedef struct IndexPrefetch IndexPrefetch; + +/* + * custom callback, allowing the user code to determine which TID to read + * + * If there is no TID to prefetch, the return value is expected to be NULL. + * + * Otherwise the "tid" field is expected to contain the TID to prefetch, and + * "data" may be set to custom information the callback needs to pass outside. + */ +typedef IndexPrefetchEntry *(*IndexPrefetchNextCB) (IndexScanDesc scan, + IndexPrefetch *state, + ScanDirection direction); + typedef struct IndexPrefetch { /* @@ -187,9 +218,18 @@ typedef struct IndexPrefetch uint64 countSkipSequential; uint64 countSkipCached; - /* used when prefetching index-only scans */ - bool indexonly; - Buffer vmBuffer; + /* + * If a callback is specified, it may store global state (for all TIDs). + * For example VM buffer may be kept during IOS. This is similar to the + * data field in IndexPrefetchEntry, but that's per-TID. + */ + void *data; + + /* + * Callback to customize the prefetch (decide which block need to be + * prefetched, etc.) + */ + IndexPrefetchNextCB next_cb; /* * Queue of TIDs to prefetch. @@ -224,14 +264,22 @@ typedef struct IndexPrefetch } IndexPrefetch; +IndexPrefetch *IndexPrefetchAlloc(IndexPrefetchNextCB next_cb, + int prefetch_max, void *data); + +IndexPrefetchEntry *IndexPrefetchNext(IndexScanDesc scan, IndexPrefetch *state, ScanDirection direction); + #define PREFETCH_QUEUE_INDEX(a) ((a) % (MAX_IO_CONCURRENCY)) #define PREFETCH_QUEUE_EMPTY(p) ((p)->queueEnd == (p)->queueIndex) #define PREFETCH_ENABLED(p) ((p) && ((p)->prefetchMaxTarget > 0)) #define PREFETCH_FULL(p) ((p)->queueEnd - (p)->queueIndex == (p)->prefetchTarget) #define PREFETCH_DONE(p) ((p) && ((p)->prefetchDone && PREFETCH_QUEUE_EMPTY(p))) + +/* XXX easy to confuse with PREFETCH_ACTIVE */ #define PREFETCH_ACTIVE(p) (PREFETCH_ENABLED(p) && !(p)->prefetchDone) #define PREFETCH_BLOCK_INDEX(v) ((v) % PREFETCH_QUEUE_HISTORY) +int index_heap_prefetch_target(Relation heapRel, double plan_rows, bool allow_prefetch); /* * generalized index_ interface routines (in indexam.c) @@ -278,17 +326,11 @@ extern IndexScanDesc index_beginscan_parallel(Relation heaprel, Relation indexrel, int nkeys, int norderbys, ParallelIndexScanDesc pscan); extern ItemPointer index_getnext_tid(IndexScanDesc scan, - ScanDirection direction, - IndexPrefetch *prefetch); -extern ItemPointer index_getnext_tid_vm(IndexScanDesc scan, - ScanDirection direction, - IndexPrefetch *prefetch, - bool *all_visible); + ScanDirection direction); struct TupleTableSlot; extern bool index_fetch_heap(IndexScanDesc scan, struct TupleTableSlot *slot); extern bool index_getnext_slot(IndexScanDesc scan, ScanDirection direction, - struct TupleTableSlot *slot, - IndexPrefetch *prefetch); + struct TupleTableSlot *slot); extern int64 index_getbitmap(IndexScanDesc scan, TIDBitmap *bitmap); extern IndexBulkDeleteResult *index_bulk_delete(IndexVacuumInfo *info, diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 8745453a5b4..cc891d4fccf 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -1529,7 +1529,6 @@ typedef struct bool *elem_nulls; /* array of num_elems is-null flags */ } IndexArrayKeyInfo; - /* ---------------- * IndexScanState information * @@ -1582,6 +1581,7 @@ typedef struct IndexScanState int16 *iss_OrderByTypLens; Size iss_PscanLen; + /* prefetching */ IndexPrefetch *iss_prefetch; } IndexScanState; @@ -1621,6 +1621,8 @@ typedef struct IndexOnlyScanState TupleTableSlot *ioss_TableSlot; Buffer ioss_VMBuffer; Size ioss_PscanLen; + + /* prefetching */ IndexPrefetch *ioss_prefetch; } IndexOnlyScanState; diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index d40af8e59fe..bc1029982cf 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -454,6 +454,7 @@ typedef struct IndexScan List *indexorderbyorig; /* the same in original form */ List *indexorderbyops; /* OIDs of sort ops for ORDER BY exprs */ ScanDirection indexorderdir; /* forward or backward or don't care */ + bool allow_prefetch; /* allow prefetching of heap pages */ } IndexScan; /* ---------------- @@ -496,6 +497,7 @@ typedef struct IndexOnlyScan List *indexorderby; /* list of index ORDER BY exprs */ List *indextlist; /* TargetEntry list describing index's cols */ ScanDirection indexorderdir; /* forward or backward or don't care */ + bool allow_prefetch; /* allow prefetching of heap pages */ } IndexOnlyScan; /* ---------------- -- 2.43.0