From 487b1723888a3a95effbebbb1670df614c16b639 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Wed, 26 Mar 2025 18:21:27 -0400
Subject: [PATCH v32 3/5] Improve skip scan primitive scan scheduling.

Fixes a few remaining cases where affected skip scans never quite manage
to reach the point of being able to apply the "passed first page"
heuristic added by commit 9a2e2a28.  They only need to manage to get
there once to converge on full index scan behavior, but it was still
possible for that to never happen, with the wrong workload.

Author: Peter Geoghegan <pg@bowt.ie>
---
 src/include/access/nbtree.h           |  3 +-
 src/backend/access/nbtree/nbtsearch.c |  1 +
 src/backend/access/nbtree/nbtutils.c  | 46 +++++++++++++++++++++++----
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index c8708f2fd..02da56995 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -1118,10 +1118,11 @@ typedef struct BTReadPageState
 
 	/*
 	 * Private _bt_checkkeys state used to manage "look ahead" optimization
-	 * (only used during scans with array keys)
+	 * and primscan scheduling (only used during scans with array keys)
 	 */
 	int16		rechecks;
 	int16		targetdistance;
+	int16		nskipadvances;
 
 } BTReadPageState;
 
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index 08cfbc45f..cec6676cc 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -1655,6 +1655,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
 	pstate.continuescan = true; /* default assumption */
 	pstate.rechecks = 0;
 	pstate.targetdistance = 0;
+	pstate.nskipadvances = 0;
 
 	if (ScanDirectionIsForward(dir))
 	{
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 01beaa4c8..8f9e6050d 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -26,6 +26,7 @@
 
 #define LOOK_AHEAD_REQUIRED_RECHECKS 	3
 #define LOOK_AHEAD_DEFAULT_DISTANCE 	5
+#define NSKIPADVANCES_THRESHOLD			3
 
 static inline int32 _bt_compare_array_skey(FmgrInfo *orderproc,
 										   Datum tupdatum, bool tupnull,
@@ -41,7 +42,8 @@ static void _bt_array_set_low_or_high(Relation rel, ScanKey skey,
 									  BTArrayKeyInfo *array, bool low_not_high);
 static bool _bt_array_decrement(Relation rel, ScanKey skey, BTArrayKeyInfo *array);
 static bool _bt_array_increment(Relation rel, ScanKey skey, BTArrayKeyInfo *array);
-static bool _bt_advance_array_keys_increment(IndexScanDesc scan, ScanDirection dir);
+static bool _bt_advance_array_keys_increment(IndexScanDesc scan, ScanDirection dir,
+											 bool *skip_array_set);
 static void _bt_rewind_nonrequired_arrays(IndexScanDesc scan, ScanDirection dir);
 static bool _bt_tuple_before_array_skeys(IndexScanDesc scan, ScanDirection dir,
 										 IndexTuple tuple, TupleDesc tupdesc, int tupnatts,
@@ -969,7 +971,8 @@ _bt_array_increment(Relation rel, ScanKey skey, BTArrayKeyInfo *array)
  * advanced (every array remains at its final element for scan direction).
  */
 static bool
-_bt_advance_array_keys_increment(IndexScanDesc scan, ScanDirection dir)
+_bt_advance_array_keys_increment(IndexScanDesc scan, ScanDirection dir,
+								 bool *skip_array_set)
 {
 	Relation	rel = scan->indexRelation;
 	BTScanOpaque so = (BTScanOpaque) scan->opaque;
@@ -984,6 +987,9 @@ _bt_advance_array_keys_increment(IndexScanDesc scan, ScanDirection dir)
 		BTArrayKeyInfo *array = &so->arrayKeys[i];
 		ScanKey		skey = &so->keyData[array->scan_key];
 
+		if (array->num_elems == -1)
+			*skip_array_set = true;
+
 		if (ScanDirectionIsForward(dir))
 		{
 			if (_bt_array_increment(rel, skey, array))
@@ -1459,6 +1465,7 @@ _bt_advance_array_keys(IndexScanDesc scan, BTReadPageState *pstate,
 	ScanDirection dir = so->currPos.dir;
 	int			arrayidx = 0;
 	bool		beyond_end_advance = false,
+				skip_array_advanced = false,
 				has_required_opposite_direction_only = false,
 				all_required_satisfied = true,
 				all_satisfied = true;
@@ -1755,6 +1762,7 @@ _bt_advance_array_keys(IndexScanDesc scan, BTReadPageState *pstate,
 				/* Skip array's new element is tupdatum (or MINVAL/MAXVAL) */
 				_bt_skiparray_set_element(rel, cur, array, result,
 										  tupdatum, tupnull);
+				skip_array_advanced = true;
 			}
 			else if (array->cur_elem != set_elem)
 			{
@@ -1771,11 +1779,19 @@ _bt_advance_array_keys(IndexScanDesc scan, BTReadPageState *pstate,
 	 * higher-order arrays (might exhaust all the scan's arrays instead, which
 	 * ends the top-level scan).
 	 */
-	if (beyond_end_advance && !_bt_advance_array_keys_increment(scan, dir))
+	if (beyond_end_advance &&
+		!_bt_advance_array_keys_increment(scan, dir, &skip_array_advanced))
 		goto end_toplevel_scan;
 
 	Assert(_bt_verify_keys_with_arraykeys(scan));
 
+	/*
+	 * Maintain a page-level count of the number of times the scan's array
+	 * keys advanced in a way that affected at least one skip array
+	 */
+	if (sktrig_required && skip_array_advanced)
+		pstate->nskipadvances++;
+
 	/*
 	 * Does tuple now satisfy our new qual?  Recheck with _bt_check_compare.
 	 *
@@ -2058,13 +2074,31 @@ new_prim_scan:
 	 * This will in turn encourage _bt_readpage to apply the pstate.startikey
 	 * optimization more often.
 	 *
-	 * Note: This heuristic isn't as aggressive as you might think.  We're
+	 * Also continue the ongoing primitive index scan when it is still on the
+	 * first page if there have been more than NSKIPADVANCES_THRESHOLD calls
+	 * here that each advanced at least one of the scan's skip arrays
+	 * (deliberately ignore advancements that only affected SAOP arrays here).
+	 * A page that cycles through this many skip array elements is quite
+	 * likely to neighbor similar pages, that we'll also need to read.
+	 *
+	 * Note: These heuristics aren't as aggressive as you might think.  We're
 	 * conservative about allowing a primitive scan to step from the first
 	 * leaf page it reads to the page's sibling page (we only allow it on
-	 * first pages whose finaltup strongly suggests that it'll work out).
+	 * first pages whose finaltup strongly suggests that it'll work out, as
+	 * well as first pages that have a large number of skip array advances).
 	 * Clearing this first page finaltup hurdle is a strong signal in itself.
+	 *
+	 * Note: The NSKIPADVANCES_THRESHOLD heuristic exists only to avoid
+	 * pathological cases: cases where a skip scan should just behave like a
+	 * traditional full index scan, but ends up "skipping" again and again,
+	 * descending to the prior leaf page's direct sibling leaf page each time.
+	 * This misbehavior would otherwise be possible during scans that never
+	 * quite manage to "clear the first page finaltup hurdle".  Workloads like
+	 * this (that are at risk of never converging on full index scan behavior)
+	 * aren't very common, but they are a real concern.  No individual scan
+	 * should ever be allowed to do the wrong thing ceaselessly.
 	 */
-	if (!pstate->firstpage)
+	if (!pstate->firstpage || pstate->nskipadvances > NSKIPADVANCES_THRESHOLD)
 	{
 		/* Schedule a recheck once on the next (or previous) page */
 		so->scanBehind = true;
-- 
2.49.0

