Hi, I have written the patch to remove the unreachable code in heapgettup_pagemode]().
On Wed, Jan 25, 2023 at 10:02 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > I wonder if we couldn't also get rid of this confusingly-inconsistent > alternative usage in the planner: > > * '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.) > > While that comment's claim is plausible, I think it's been wrong for > years. AFAICS indxpath.c makes pathkeys before it ever does this: > > index_is_ordered ? > ForwardScanDirection : > NoMovementScanDirection, > > and nothing depends on that later either. So I think we could > simplify this to something like "indexscandir is either > ForwardScanDirection or BackwardScanDirection. (Unordered > index types need not support BackwardScanDirection.)" > I also did what I *think* Tom is suggesting here -- make index scan's scan direction always forward or backward... Maybe the set should be two patches...dunno. - Melanie
From be8119af72499aec03e59200c9db44f8520ccebf Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Fri, 27 Jan 2023 14:02:03 -0500 Subject: [PATCH v1] Remove NoMovementScanDirection inconsistencies Remove use of NoMovementScanDirection for index scans. Unordered indexes will now always use ForwardScanDirection. Remove unreachable code in heapgettup() and heapgettup_pagemode() handling NoMovementScanDirection. Add assertions in case table AMs try to use scan directions other than forward and backward. Discussion: https://www.postgresql.org/message-id/flat/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY%3D_T8QEqZqOL02rpi2bw%40mail.gmail.com --- src/backend/access/heap/heapam.c | 73 ++++-------------------- src/backend/commands/explain.c | 3 - src/backend/executor/nodeIndexonlyscan.c | 17 +++--- src/backend/executor/nodeIndexscan.c | 17 +++--- src/backend/optimizer/path/indxpath.c | 8 +-- src/backend/optimizer/util/pathnode.c | 5 +- src/include/access/tableam.h | 6 ++ src/test/regress/expected/hash_index.out | 27 +++++++++ src/test/regress/sql/hash_index.sql | 23 ++++++++ 9 files changed, 87 insertions(+), 92 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index e6024a980b..be0555f5c4 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -523,6 +523,9 @@ heapgettup(HeapScanDesc scan, int linesleft; ItemId lpp; + Assert(dir == ForwardScanDirection || + dir == BackwardScanDirection); + /* * calculate next starting lineoff, given scan direction */ @@ -583,7 +586,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 +656,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 +836,9 @@ heapgettup_pagemode(HeapScanDesc scan, int linesleft; ItemId lpp; + Assert(dir == ForwardScanDirection || + dir == BackwardScanDirection); + /* * calculate next starting lineindex, given scan direction */ @@ -918,7 +896,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 +956,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 @@ -1299,6 +1245,9 @@ heap_getnext(TableScanDesc sscan, ScanDirection direction) { HeapScanDesc scan = (HeapScanDesc) sscan; + Assert(direction == ForwardScanDirection || + direction == BackwardScanDirection); + /* * This is still widely used directly, without going through table AM, so * add a safety check. It's possible we should, at a later point, 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..de7c71890a 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -70,15 +70,7 @@ 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 = estate->es_direction * ((IndexOnlyScan *) node->ss.ps.plan)->indexorderdir; scandesc = node->ioss_ScanDesc; econtext = node->ss.ps.ps_ExprContext; slot = node->ss.ss_ScanTupleSlot; @@ -503,6 +495,13 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags) indexstate->ss.ps.state = estate; indexstate->ss.ps.ExecProcNode = ExecIndexOnlyScan; + /* + * Only forward and backward are valid scan directions. Unordered indexes + * will always have ForwardScanDirection. + */ + Assert(node->indexorderdir == ForwardScanDirection || + node->indexorderdir == BackwardScanDirection); + /* * Miscellaneous initialization * diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index f1ced9ff0f..403ace9c90 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -90,15 +90,7 @@ 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 = estate->es_direction * ((IndexScan *) node->ss.ps.plan)->indexorderdir; scandesc = node->iss_ScanDesc; econtext = node->ss.ps.ps_ExprContext; slot = node->ss.ss_ScanTupleSlot; @@ -916,6 +908,13 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags) indexstate->ss.ps.state = estate; indexstate->ss.ps.ExecProcNode = ExecIndexScan; + /* + * Only forward and backward are valid scan directions. Unordered indexes + * will always have ForwardScanDirection. + */ + Assert(node->indexorderdir == ForwardScanDirection || + node->indexorderdir == BackwardScanDirection); + /* * Miscellaneous initialization * 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/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/tableam.h b/src/include/access/tableam.h index 3fb184717f..28abe819b0 100644 --- a/src/include/access/tableam.h +++ b/src/include/access/tableam.h @@ -1033,6 +1033,9 @@ extern void table_scan_update_snapshot(TableScanDesc scan, Snapshot snapshot); static inline bool table_scan_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *slot) { + Assert(direction == ForwardScanDirection || + direction == BackwardScanDirection); + slot->tts_tableOid = RelationGetRelid(sscan->rs_rd); /* @@ -1096,6 +1099,9 @@ static inline bool table_scan_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *slot) { + Assert(direction == ForwardScanDirection || + direction == BackwardScanDirection); + /* Ensure table_beginscan_tidrange() was used. */ Assert((sscan->rs_flags & SO_TYPE_TIDRANGESCAN) != 0); diff --git a/src/test/regress/expected/hash_index.out b/src/test/regress/expected/hash_index.out index a2036a1597..fde395a7b6 100644 --- a/src/test/regress/expected/hash_index.out +++ b/src/test/regress/expected/hash_index.out @@ -40,6 +40,33 @@ CREATE INDEX hash_name_index ON hash_name_heap USING hash (random name_ops); CREATE INDEX hash_txt_index ON hash_txt_heap USING hash (random text_ops); CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random float8_ops) WITH (fillfactor=60); +CREATE OR REPLACE FUNCTION scan_dir(query text, scan_type text) +RETURNS TEXT LANGUAGE plpgsql +AS +$$ +DECLARE + plan json; + node json; +BEGIN + FOR plan IN + EXECUTE 'EXPLAIN (FORMAT ''json'') ' || query + LOOP + node := json_extract_path(plan, '0', 'Plan'); + IF node->>'Node Type' = scan_type THEN + RETURN node->>'Scan Direction'; + END IF; + END LOOP; + RETURN NULL; +END; +$$; +-- Hash Indexes are unordered and thus only forward scan direction makes sense. +SELECT 'Forward' = scan_dir( + $$ SELECT random FROM hash_i4_heap WHERE random = 1345971420; $$, 'Index Scan'); + ?column? +---------- + t +(1 row) + -- -- Also try building functional, expressional, and partial indexes on -- tables that already contain data. diff --git a/src/test/regress/sql/hash_index.sql b/src/test/regress/sql/hash_index.sql index 527024f710..8a96f53612 100644 --- a/src/test/regress/sql/hash_index.sql +++ b/src/test/regress/sql/hash_index.sql @@ -53,6 +53,29 @@ CREATE INDEX hash_txt_index ON hash_txt_heap USING hash (random text_ops); CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random float8_ops) WITH (fillfactor=60); +CREATE OR REPLACE FUNCTION scan_dir(query text, scan_type text) +RETURNS TEXT LANGUAGE plpgsql +AS +$$ +DECLARE + plan json; + node json; +BEGIN + FOR plan IN + EXECUTE 'EXPLAIN (FORMAT ''json'') ' || query + LOOP + node := json_extract_path(plan, '0', 'Plan'); + IF node->>'Node Type' = scan_type THEN + RETURN node->>'Scan Direction'; + END IF; + END LOOP; + RETURN NULL; +END; +$$; + +-- Hash Indexes are unordered and thus only forward scan direction makes sense. +SELECT 'Forward' = scan_dir( + $$ SELECT random FROM hash_i4_heap WHERE random = 1345971420; $$, 'Index Scan'); -- -- Also try building functional, expressional, and partial indexes on -- tables that already contain data. -- 2.37.2
