From 4d194ee9aa8cacdc9f8f80ec0e2c716c9a2e2349 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 2 May 2024 11:18:44 -0400
Subject: [PATCH v3 2/4] Rationalize behavior of enable_indexscan and
 enable_indexonlyscan.

Previously, index-scan paths were still generated even when
enable_indexscan=false, but we added disable-cost to the cost of
both index scan plans and index-only scan plans. It doesn't make sense
for enable_indexscan to affect the whether index-only scans are chosen
given that we also have a GUC called enable_indexonlyscan.

With this commit, enable_indexscan and enable_indexonlyscan work
the same way: each one prevents consideration of paths of the
appropriate type, and neither has any affect on the cost of the
generate paths. This requires some updates to the regression tests,
which previously relied on enable_indexscan=false to also disable
index-only scans.

Note that when enable_indexonlyscan=false and enable_indexscan=true,
we will generate index-scan paths that would have not have been
generated if both had been set to true. That's because generating
both an index-scan path and an index-only path would be a waste
of cycles, since the index-only path should always win. In effect,
the index-scan plan shape was still being considered; we just
rejected it before actually constructing a path.
---
 src/backend/optimizer/path/costsize.c         |  4 ---
 src/backend/optimizer/path/indxpath.c         | 26 ++++++++++++++++---
 src/test/regress/expected/btree_index.out     |  3 +++
 src/test/regress/expected/create_index.out    |  2 ++
 src/test/regress/expected/select.out          |  1 +
 src/test/regress/expected/select_parallel.out |  2 ++
 src/test/regress/expected/tuplesort.out       |  2 ++
 src/test/regress/sql/btree_index.sql          |  5 ++++
 src/test/regress/sql/create_index.sql         |  2 ++
 src/test/regress/sql/select.sql               |  1 +
 src/test/regress/sql/select_parallel.sql      |  2 ++
 src/test/regress/sql/tuplesort.sql            |  2 ++
 12 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 2021c481b4..74fc5aab56 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -603,10 +603,6 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count,
 											  path->indexclauses);
 	}
 
-	if (!enable_indexscan)
-		startup_cost += disable_cost;
-	/* we don't need to check enable_indexonlyscan; indxpath.c does that */
-
 	/*
 	 * Call index-access-method-specific code to estimate the processing cost
 	 * for scanning the index, as well as the selectivity of the index (ie,
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index c0fcc7d78d..8887231cf9 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -742,7 +742,13 @@ get_index_paths(PlannerInfo *root, RelOptInfo *rel,
 		IndexPath  *ipath = (IndexPath *) lfirst(lc);
 
 		if (index->amhasgettuple)
-			add_path(rel, (Path *) ipath);
+		{
+			if (ipath->path.pathtype == T_IndexScan && enable_indexscan)
+				add_path(rel, (Path *) ipath);
+			else if (ipath->path.pathtype == T_IndexOnlyScan &&
+					 enable_indexonlyscan)
+				add_path(rel, (Path *) ipath);
+		}
 
 		if (index->amhasgetbitmap &&
 			(ipath->path.pathkeys == NIL ||
@@ -831,6 +837,8 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
 		case ST_INDEXSCAN:
 			if (!index->amhasgettuple)
 				return NIL;
+			if (!enable_indexscan && !enable_indexonlyscan)
+				return NIL;
 			break;
 		case ST_BITMAPSCAN:
 			if (!index->amhasgetbitmap)
@@ -978,7 +986,8 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
 		 */
 		if (index->amcanparallel &&
 			rel->consider_parallel && outer_relids == NULL &&
-			scantype != ST_BITMAPSCAN)
+			scantype != ST_BITMAPSCAN &&
+			(index_only_scan ? enable_indexonlyscan : enable_indexscan))
 		{
 			ipath = create_index_path(root, index,
 									  index_clauses,
@@ -1028,7 +1037,8 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
 			/* If appropriate, consider parallel index scan */
 			if (index->amcanparallel &&
 				rel->consider_parallel && outer_relids == NULL &&
-				scantype != ST_BITMAPSCAN)
+				scantype != ST_BITMAPSCAN &&
+				(index_only_scan ? enable_indexonlyscan : enable_indexscan))
 			{
 				ipath = create_index_path(root, index,
 										  index_clauses,
@@ -1735,7 +1745,15 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
 	ListCell   *lc;
 	int			i;
 
-	/* Index-only scans must be enabled */
+	/*
+	 * Index-only scans must be enabled.
+	 *
+	 * NB: Returning false here means that an index scan will be considered
+	 * instead, so setting enable_indexscan=false causes to consider paths
+	 * that we wouldn't have considered otherwise. That seems OK, because our
+	 * only reason for not generating the index-scan paths is that we expect
+	 * them to lose on cost.
+	 */
 	if (!enable_indexonlyscan)
 		return false;
 
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index 510646cbce..f15db99771 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -247,6 +247,7 @@ select thousand from tenk1 where thousand in (364, 366,380) and tenthous = 20000
 --
 set enable_seqscan to false;
 set enable_indexscan to true;
+set enable_indexonlyscan to true;
 set enable_bitmapscan to false;
 explain (costs off)
 select proname from pg_proc where proname like E'RI\\_FKey%del' order by 1;
@@ -290,6 +291,7 @@ select proname from pg_proc where proname ilike 'ri%foo' order by 1;
 (2 rows)
 
 set enable_indexscan to false;
+set enable_indexonlyscan to false;
 set enable_bitmapscan to true;
 explain (costs off)
 select proname from pg_proc where proname like E'RI\\_FKey%del' order by 1;
@@ -330,6 +332,7 @@ select proname from pg_proc where proname ilike '00%foo' order by 1;
 ---------
 (0 rows)
 
+reset enable_indexonlyscan;
 explain (costs off)
 select proname from pg_proc where proname ilike 'ri%foo' order by 1;
                            QUERY PLAN                            
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index cf6eac5734..ec69bafd40 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -618,6 +618,7 @@ SELECT point(x,x), (SELECT f1 FROM gpolygon_tbl ORDER BY f1 <-> point(x,x) LIMIT
 -- Now check the results from bitmap indexscan
 SET enable_seqscan = OFF;
 SET enable_indexscan = OFF;
+SET enable_indexonlyscan = OFF;
 SET enable_bitmapscan = ON;
 EXPLAIN (COSTS OFF)
 SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1 <-> '0,1';
@@ -643,6 +644,7 @@ SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1 <-> '0
 
 RESET enable_seqscan;
 RESET enable_indexscan;
+RESET enable_indexonlyscan;
 RESET enable_bitmapscan;
 --
 -- GIN over int[] and text[]
diff --git a/src/test/regress/expected/select.out b/src/test/regress/expected/select.out
index 33a6dceb0e..6445815741 100644
--- a/src/test/regress/expected/select.out
+++ b/src/test/regress/expected/select.out
@@ -844,6 +844,7 @@ select unique2 from onek2 where unique2 = 11 and stringu1 < 'C';
 
 -- partial index implies clause, but bitmap scan must recheck predicate anyway
 SET enable_indexscan TO off;
+SET enable_indexonlyscan TO off;
 explain (costs off)
 select unique2 from onek2 where unique2 = 11 and stringu1 < 'B';
                          QUERY PLAN                          
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 87273fa635..f79eda79f6 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -522,6 +522,7 @@ reset enable_indexscan;
 -- test parallel bitmap heap scan.
 set enable_seqscan to off;
 set enable_indexscan to off;
+set enable_indexonlyscan to off;
 set enable_hashjoin to off;
 set enable_mergejoin to off;
 set enable_material to off;
@@ -622,6 +623,7 @@ select * from explain_parallel_sort_stats();
 (14 rows)
 
 reset enable_indexscan;
+reset enable_indexonlyscan;
 reset enable_hashjoin;
 reset enable_mergejoin;
 reset enable_material;
diff --git a/src/test/regress/expected/tuplesort.out b/src/test/regress/expected/tuplesort.out
index 6dd97e7427..87b05a22cb 100644
--- a/src/test/regress/expected/tuplesort.out
+++ b/src/test/regress/expected/tuplesort.out
@@ -362,6 +362,7 @@ ORDER BY v.a DESC;
 -- in-memory
 BEGIN;
 SET LOCAL enable_indexscan = false;
+SET LOCAL enable_indexonlyscan = false;
 -- unfortunately can't show analyze output confirming sort method,
 -- the memory used output wouldn't be stable
 EXPLAIN (COSTS OFF) DECLARE c SCROLL CURSOR FOR SELECT noabort_decreasing FROM abbrev_abort_uuids ORDER BY noabort_decreasing;
@@ -458,6 +459,7 @@ COMMIT;
 -- disk based
 BEGIN;
 SET LOCAL enable_indexscan = false;
+SET LOCAL enable_indexonlyscan = false;
 SET LOCAL work_mem = '100kB';
 -- unfortunately can't show analyze output confirming sort method,
 -- the memory used output wouldn't be stable
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index 0d2a33f370..bc99f44dda 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -157,6 +157,7 @@ select thousand from tenk1 where thousand in (364, 366,380) and tenthous = 20000
 
 set enable_seqscan to false;
 set enable_indexscan to true;
+set enable_indexonlyscan to true;
 set enable_bitmapscan to false;
 explain (costs off)
 select proname from pg_proc where proname like E'RI\\_FKey%del' order by 1;
@@ -168,6 +169,7 @@ explain (costs off)
 select proname from pg_proc where proname ilike 'ri%foo' order by 1;
 
 set enable_indexscan to false;
+set enable_indexonlyscan to false;
 set enable_bitmapscan to true;
 explain (costs off)
 select proname from pg_proc where proname like E'RI\\_FKey%del' order by 1;
@@ -175,6 +177,9 @@ select proname from pg_proc where proname like E'RI\\_FKey%del' order by 1;
 explain (costs off)
 select proname from pg_proc where proname ilike '00%foo' order by 1;
 select proname from pg_proc where proname ilike '00%foo' order by 1;
+
+reset enable_indexonlyscan;
+
 explain (costs off)
 select proname from pg_proc where proname ilike 'ri%foo' order by 1;
 
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index e296891cab..04dea5225e 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -246,6 +246,7 @@ SELECT point(x,x), (SELECT f1 FROM gpolygon_tbl ORDER BY f1 <-> point(x,x) LIMIT
 -- Now check the results from bitmap indexscan
 SET enable_seqscan = OFF;
 SET enable_indexscan = OFF;
+SET enable_indexonlyscan = OFF;
 SET enable_bitmapscan = ON;
 
 EXPLAIN (COSTS OFF)
@@ -254,6 +255,7 @@ SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1 <-> '0
 
 RESET enable_seqscan;
 RESET enable_indexscan;
+RESET enable_indexonlyscan;
 RESET enable_bitmapscan;
 
 --
diff --git a/src/test/regress/sql/select.sql b/src/test/regress/sql/select.sql
index 019f1e7673..a0c7417dec 100644
--- a/src/test/regress/sql/select.sql
+++ b/src/test/regress/sql/select.sql
@@ -218,6 +218,7 @@ select unique2 from onek2 where unique2 = 11 and stringu1 < 'C';
 select unique2 from onek2 where unique2 = 11 and stringu1 < 'C';
 -- partial index implies clause, but bitmap scan must recheck predicate anyway
 SET enable_indexscan TO off;
+SET enable_indexonlyscan TO off;
 explain (costs off)
 select unique2 from onek2 where unique2 = 11 and stringu1 < 'B';
 select unique2 from onek2 where unique2 = 11 and stringu1 < 'B';
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 20376c03fa..3f003e2e71 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -201,6 +201,7 @@ reset enable_indexscan;
 -- test parallel bitmap heap scan.
 set enable_seqscan to off;
 set enable_indexscan to off;
+set enable_indexonlyscan to off;
 set enable_hashjoin to off;
 set enable_mergejoin to off;
 set enable_material to off;
@@ -248,6 +249,7 @@ $$;
 select * from explain_parallel_sort_stats();
 
 reset enable_indexscan;
+reset enable_indexonlyscan;
 reset enable_hashjoin;
 reset enable_mergejoin;
 reset enable_material;
diff --git a/src/test/regress/sql/tuplesort.sql b/src/test/regress/sql/tuplesort.sql
index 8476e594e6..95ac8ec04c 100644
--- a/src/test/regress/sql/tuplesort.sql
+++ b/src/test/regress/sql/tuplesort.sql
@@ -162,6 +162,7 @@ ORDER BY v.a DESC;
 -- in-memory
 BEGIN;
 SET LOCAL enable_indexscan = false;
+SET LOCAL enable_indexonlyscan = false;
 -- unfortunately can't show analyze output confirming sort method,
 -- the memory used output wouldn't be stable
 EXPLAIN (COSTS OFF) DECLARE c SCROLL CURSOR FOR SELECT noabort_decreasing FROM abbrev_abort_uuids ORDER BY noabort_decreasing;
@@ -192,6 +193,7 @@ COMMIT;
 -- disk based
 BEGIN;
 SET LOCAL enable_indexscan = false;
+SET LOCAL enable_indexonlyscan = false;
 SET LOCAL work_mem = '100kB';
 -- unfortunately can't show analyze output confirming sort method,
 -- the memory used output wouldn't be stable
-- 
2.39.3 (Apple Git-145)

