v7-0001-Remove-NoMovementScanDirection-inconsistencies.patch

From 7b3359a4ddad7083602d607f3ab95210f86eb2ae Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 6 Jan 2023 12:39:38 -0500
Subject: [PATCH v7 1/6] 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         | 74 ++++--------------------
 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, 88 insertions(+), 92 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e6024a980b..75f68b4e79 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -483,6 +483,7 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
 	scan->rs_ntuples = ntup;
 }
 
+
 /* ----------------
  *		heapgettup - fetch next heap tuple
  *
@@ -523,6 +524,9 @@ heapgettup(HeapScanDesc scan,
 	int			linesleft;
 	ItemId		lpp;
 
+	Assert(dir == ForwardScanDirection ||
+		   dir == BackwardScanDirection);
+
 	/*
 	 * calculate next starting lineoff, given scan direction
 	 */
@@ -583,7 +587,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 +657,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 +837,9 @@ heapgettup_pagemode(HeapScanDesc scan,
 	int			linesleft;
 	ItemId		lpp;
 
+	Assert(dir == ForwardScanDirection ||
+		   dir == BackwardScanDirection);
+
 	/*
 	 * calculate next starting lineindex, given scan direction
 	 */
@@ -918,7 +897,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 +957,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 +1246,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 35c23bd27d..fbbf28cf06 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 e9b784bcab..721a075201 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 f2bf68d33b..07f047cd8d 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