From d6aae3a73864aaaf84b4dc00ff299c2e8b4a5729 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 30 Jan 2023 09:49:54 -0500 Subject: [PATCH v2] remove nomovement scandir --- src/backend/access/heap/heapam.c | 71 ++---------------------- src/backend/commands/explain.c | 3 - src/backend/executor/nodeIndexonlyscan.c | 11 +--- src/backend/executor/nodeIndexscan.c | 11 +--- src/backend/optimizer/path/indxpath.c | 8 +-- src/backend/optimizer/plan/createplan.c | 14 +++++ src/backend/optimizer/util/pathnode.c | 5 +- src/include/access/sdir.h | 11 +++- src/include/nodes/pathnodes.h | 6 +- 9 files changed, 41 insertions(+), 99 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index e6024a980b..10aaeb14aa 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -490,9 +490,6 @@ heapgetpage(TableScanDesc sscan, BlockNumber block) * tuple as indicated by "dir"; return the next tuple in scan->rs_ctup, * or set scan->rs_ctup.t_data = NULL if no more tuples. * - * dir == NoMovementScanDirection means "re-fetch the tuple indicated - * by scan->rs_ctup". - * * Note: the reason nkeys/key are passed separately, even though they are * kept in the scan descriptor, is that the caller may not want us to check * the scankeys. @@ -523,6 +520,8 @@ heapgettup(HeapScanDesc scan, int linesleft; ItemId lpp; + Assert(dir != NoMovementScanDirection); + /* * calculate next starting lineoff, given scan direction */ @@ -583,7 +582,7 @@ heapgettup(HeapScanDesc scan, linesleft = lines - lineoff + 1; } - else if (backward) + else { /* backward parallel scan not supported */ Assert(scan->rs_base.rs_parallel == NULL); @@ -653,34 +652,6 @@ heapgettup(HeapScanDesc scan, linesleft = lineoff; } - else - { - /* - * ``no movement'' scan direction: refetch prior tuple - */ - if (!scan->rs_inited) - { - Assert(!BufferIsValid(scan->rs_cbuf)); - tuple->t_data = NULL; - return; - } - - block = ItemPointerGetBlockNumber(&(tuple->t_self)); - if (block != scan->rs_cblock) - heapgetpage((TableScanDesc) scan, block); - - /* Since the tuple was previously fetched, needn't lock page here */ - page = BufferGetPage(scan->rs_cbuf); - TestForOldSnapshot(snapshot, scan->rs_base.rs_rd, page); - lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self)); - lpp = PageGetItemId(page, lineoff); - Assert(ItemIdIsNormal(lpp)); - - tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp); - tuple->t_len = ItemIdGetLength(lpp); - - return; - } /* * advance the scan until we find a qualifying tuple or run out of stuff @@ -861,6 +832,8 @@ heapgettup_pagemode(HeapScanDesc scan, int linesleft; ItemId lpp; + Assert(dir != NoMovementScanDirection); + /* * calculate next starting lineindex, given scan direction */ @@ -918,7 +891,7 @@ heapgettup_pagemode(HeapScanDesc scan, linesleft = lines - lineindex; } - else if (backward) + else { /* backward parallel scan not supported */ Assert(scan->rs_base.rs_parallel == NULL); @@ -978,38 +951,6 @@ heapgettup_pagemode(HeapScanDesc scan, linesleft = lineindex + 1; } - else - { - /* - * ``no movement'' scan direction: refetch prior tuple - */ - if (!scan->rs_inited) - { - Assert(!BufferIsValid(scan->rs_cbuf)); - tuple->t_data = NULL; - return; - } - - block = ItemPointerGetBlockNumber(&(tuple->t_self)); - if (block != scan->rs_cblock) - heapgetpage((TableScanDesc) scan, block); - - /* Since the tuple was previously fetched, needn't lock page here */ - page = BufferGetPage(scan->rs_cbuf); - TestForOldSnapshot(scan->rs_base.rs_snapshot, scan->rs_base.rs_rd, page); - lineoff = ItemPointerGetOffsetNumber(&(tuple->t_self)); - lpp = PageGetItemId(page, lineoff); - Assert(ItemIdIsNormal(lpp)); - - tuple->t_data = (HeapTupleHeader) PageGetItem(page, lpp); - tuple->t_len = ItemIdGetLength(lpp); - - /* check that rs_cindex is in sync */ - Assert(scan->rs_cindex < scan->rs_ntuples); - Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]); - - return; - } /* * advance the scan until we find a qualifying tuple or run out of stuff diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index a0311ce9dc..d38311fa7e 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -3746,9 +3746,6 @@ ExplainIndexScanDetails(Oid indexid, ScanDirection indexorderdir, case BackwardScanDirection: scandir = "Backward"; break; - case NoMovementScanDirection: - scandir = "NoMovement"; - break; case ForwardScanDirection: scandir = "Forward"; break; diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index 8c7da9ee60..e75b3bb8e1 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -70,15 +70,10 @@ IndexOnlyNext(IndexOnlyScanState *node) * extract necessary information from index scan node */ estate = node->ss.ps.state; - direction = estate->es_direction; + /* flip direction if this is an overall backward scan */ - if (ScanDirectionIsBackward(((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir)) - { - if (ScanDirectionIsForward(direction)) - direction = BackwardScanDirection; - else if (ScanDirectionIsBackward(direction)) - direction = ForwardScanDirection; - } + direction = ScanDirectionCombine(estate->es_direction, + ((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir); scandesc = node->ioss_ScanDesc; econtext = node->ss.ps.ps_ExprContext; slot = node->ss.ss_ScanTupleSlot; diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index f1ced9ff0f..1c1c558b22 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -90,15 +90,10 @@ IndexNext(IndexScanState *node) * extract necessary information from index scan node */ estate = node->ss.ps.state; - direction = estate->es_direction; + /* flip direction if this is an overall backward scan */ - if (ScanDirectionIsBackward(((IndexScan *) node->ss.ps.plan)->indexorderdir)) - { - if (ScanDirectionIsForward(direction)) - direction = BackwardScanDirection; - else if (ScanDirectionIsBackward(direction)) - direction = ForwardScanDirection; - } + direction = ScanDirectionCombine(estate->es_direction, + ((IndexScan *) node->ss.ps.plan)->indexorderdir); scandesc = node->iss_ScanDesc; econtext = node->ss.ps.ps_ExprContext; slot = node->ss.ss_ScanTupleSlot; diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index e13c8f1914..751fa2edfe 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -1015,9 +1015,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel, orderbyclauses, orderbyclausecols, useful_pathkeys, - index_is_ordered ? - ForwardScanDirection : - NoMovementScanDirection, + ForwardScanDirection, index_only_scan, outer_relids, loop_count, @@ -1037,9 +1035,7 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel, orderbyclauses, orderbyclausecols, useful_pathkeys, - index_is_ordered ? - ForwardScanDirection : - NoMovementScanDirection, + ForwardScanDirection, index_only_scan, outer_relids, loop_count, diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index cd68942af0..1302d25e54 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -5512,6 +5512,13 @@ make_indexscan(List *qptlist, IndexScan *node = makeNode(IndexScan); Plan *plan = &node->scan.plan; + /* + * Only forward and backward index scan directions are meaningful to the + * executor. Unordered indexes will always have ForwardScanDirection. + */ + Assert(indexscandir == ForwardScanDirection || + indexscandir == BackwardScanDirection); + plan->targetlist = qptlist; plan->qual = qpqual; plan->lefttree = NULL; @@ -5542,6 +5549,13 @@ make_indexonlyscan(List *qptlist, IndexOnlyScan *node = makeNode(IndexOnlyScan); Plan *plan = &node->scan.plan; + /* + * Only forward and backward index scan directions are meaningful to the + * executor. Unordered indexes will always have ForwardScanDirection. + */ + Assert(indexscandir == ForwardScanDirection || + indexscandir == BackwardScanDirection); + plan->targetlist = qptlist; plan->qual = qpqual; plan->lefttree = NULL; diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 4478036bb6..dd5236682f 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -982,9 +982,8 @@ create_samplescan_path(PlannerInfo *root, RelOptInfo *rel, Relids required_outer * 'indexorderbycols' is an integer list of index column numbers (zero based) * the ordering operators can be used with. * 'pathkeys' describes the ordering of the path. - * 'indexscandir' is ForwardScanDirection or BackwardScanDirection - * for an ordered index, or NoMovementScanDirection for - * an unordered index. + * 'indexscandir' is either ForwardScanDirection or BackwardScanDirection. + * Unordered index types need not support BackwardScanDirection. * 'indexonly' is true if an index-only scan is wanted. * 'required_outer' is the set of outer relids for a parameterized path. * 'loop_count' is the number of repetitions of the indexscan to factor into diff --git a/src/include/access/sdir.h b/src/include/access/sdir.h index 16cb06c709..bd79d977ec 100644 --- a/src/include/access/sdir.h +++ b/src/include/access/sdir.h @@ -16,8 +16,8 @@ /* - * ScanDirection was an int8 for no apparent reason. I kept the original - * values because I'm not sure if I'll break anything otherwise. -ay 2/95 + * These enum values were originally int8 values. Using -1, 0, and 1 as their + * values conveniently mirrors their semantic value when used during execution. */ typedef enum ScanDirection { @@ -26,6 +26,13 @@ typedef enum ScanDirection ForwardScanDirection = 1 } ScanDirection; +/* + * Determine the net effect of two direction specifications. + * This relies on having ForwardScanDirection = +1, BackwardScanDirection = -1, + * and will probably not do what you want if applied to any other values. + */ +#define ScanDirectionCombine(a, b) ((a) * (b)) + /* * ScanDirectionIsValid * True iff scan direction is valid. diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 2d1d8f4bcd..cdb2a06409 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1611,10 +1611,8 @@ typedef struct Path * 'indexscandir' is one of: * ForwardScanDirection: forward scan of an ordered index * BackwardScanDirection: backward scan of an ordered index - * NoMovementScanDirection: scan of an unordered index, or don't care - * (The executor doesn't care whether it gets ForwardScanDirection or - * NoMovementScanDirection for an indexscan, but the planner wants to - * distinguish ordered from unordered indexes for building pathkeys.) + * NoMovementScanDirection for indexscandir is meaningless for the executor, so + * unordered indexes will always set ForwardScanDirection. * * 'indextotalcost' and 'indexselectivity' are saved in the IndexPath so that * we need not recompute them when considering using the same index in a -- 2.37.2