heapgettup() with NoMovementScanDirection unused in core?

From be8119af72499aec03e59200c9db44f8520ccebf Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@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

From d6aae3a73864aaaf84b4dc00ff299c2e8b4a5729 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
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