fix: charge a capped per-row term on clustered index fetches (#1039, rebased + ledger)

CHANGELOG.md

@@ -615,18 +615,26 @@ true until the next version shipped.
   This is the third arm in this file to be repaired for counting a string across a
   whole file. The `deltuples` comment 15 lines above records the first, fixed by
   scoping; these two were left as whole-file counts and did the same thing again.
-- A table-AM parallel scan was a single claimer.
-
-  `pgcolumnar_read_start` treated `phs_nallocated` as a first-wins flag: the
-  first participant loaded every row group and the others marked themselves
-  exhausted. Workers launched, then sat idle while one backend (usually the
-  leader) read the table. The custom-scan path already claims distinct groups
-  from a shared counter; the AM path now uses `phs_nallocated` the same way,
-  as a group index, not a mutex.
-
-  Measured with the custom scan off, two workers, and leader participation
-  off: both workers produced rows (19000 and 31000 of 50000). Restoring
-  first-wins returns one worker to 0.
+- A fetching index scan on a correlated key stayed cheaper than the custom scan
+  through ~50,000 rows, while doing about 27x the work (#913).
+
+  The index-fetch penalty prices distinct row-group decodes. On a clustered key
+  that count is ceil(rows / stripe), so it does not grow through the first group.
+  Core's heap-fetch cost grows with rows; the extra columnar work of reconstructing
+  each fetched row after the group is cached did not. The 50,000-row range then
+  stayed on the index (cost 2243 against the custom scan's 2504) while the point
+  lookup was already correctly on it.
+
+  The per-row term is `cpu_tuple_cost * rows * decodeUnits`, the same units #503
+  uses for a projection. It is not a conversion from heap instructions-per-cost:
+  #766 showed that conversion predicts the wrong winner. Uncapped it grows with
+  the whole table and costs a clustered ORDER BY off its index (#355). Cap it at
+  half a group: that is enough to move the 50,000-row range and small enough to
+  leave the ordered scan on the index.
+
+  Plan choice is the property, not a cost number: at 50,000 rows the planner now
+  picks the custom scan; a point lookup still uses the index; a clustered ORDER BY
+  stays on the index; both paths return the same aggregate.
 
 - `compare_to_bash.py`'s corpus arm called a WRAPPED name fabricated. A name too long
   for one line is written as adjacent literals, and Python joins them at parse time,

src/columnar_customscan.c

@@ -1892,6 +1892,20 @@ pgcolumnar_scan_decode_shape(RelOptInfo *rel, Index rti, Oid relid,
  *
  * decode_per_group is one group's cost: its pages read once, plus the per-value
  * decode of R rows across the columns the scan needs.
+ *
+ * That is not the whole fetch. Even after the group is in the statement-scoped
+ * cache, each row still pays tuple reconstruction and group location that a heap
+ * fetch does not. Without a per-row term the clustered penalty is independent of
+ * how many rows are fetched, so a 50,000-row correlated range stays on the index
+ * while doing about 27x the work of the custom scan (#913). cpu_tuple_cost is the
+ * same unit core already uses for a heap tuple; decodeUnits is the projection in
+ * #503's 4-byte-column units. Do not scale this from heap instructions-per-cost:
+ * #766 showed that conversion predicts the wrong winner.
+ *
+ * Cap the per-row term at half a group. Uncapped it grows with the whole table
+ * and costs a clustered ORDER BY off its index, which #355 must not do. Half a
+ * group is enough to move the 50,000-row range and small enough to leave the
+ * ordered scan on the index.
  */
 static Cost
 pgcolumnar_index_fetch_penalty(RelOptInfo *rel, Oid relid, double rows, double rho,
@@ -1954,7 +1968,22 @@ pgcolumnar_index_fetch_penalty(RelOptInfo *rel, Oid relid, double rows, double r
 
 	if (groups_decoded < 0)
 		groups_decoded = 0;
-	return groups_decoded * decode_per_group;
+	/*
+	 * Per fetched row, after the group decode is paid once (#913). On a
+	 * clustered key, groups_decoded is ceil(rows/R) and does not grow through
+	 * the first group, so this is the term that moves a 50,000-row range off
+	 * the index without costing a point lookup out of it. Cap at half a
+	 * group so a clustered ORDER BY of the whole table stays on the index
+	 * (#355).
+	 */
+	{
+		Cost		per_row = cpu_tuple_cost * rows * decodeUnits;
+		Cost		half_group = 0.5 * decode_per_group;
+
+		if (per_row > half_group)
+			per_row = half_group;
+		return groups_decoded * decode_per_group + per_row;
+	}
 }
 
 /*

test/check_ledger.tsv

@@ -1169,6 +1169,12 @@ harness_selftest	490-a-one-line-body-ends-at-its-own-brace	control: and the reco
 harness_selftest	490-a-one-line-body-ends-at-its-own-brace	premise: the skip-loop tool is present	15;16;17;18;19	never	-
 harness_selftest	490-a-one-line-body-ends-at-its-own-brace	premise: the tool answers with an emitter set, so 'q is absent' is about q	15;16;17;18;19	never	-
 harness_selftest	490-a-one-line-body-ends-at-its-own-brace	premise: the tool reports an unclosed count at all, so a zero below means zero	15;16;17;18;19	never	-
+index_fetch_penalty_crossover	index_fetch_penalty_crossover	a 50000-row correlated range uses the custom scan, not a fetching index	15;16;17;18;19	never	-
+index_fetch_penalty_crossover	index_fetch_penalty_crossover	a selective point lookup still uses the index	15;16;17;18;19	never	-
+index_fetch_penalty_crossover	index_fetch_penalty_crossover	both paths return the same aggregate at 50000	15;16;17;18;19	never	-
+index_fetch_penalty_crossover	index_fetch_penalty_crossover	premise: the btree on id exists	15;16;17;18;19	never	-
+index_fetch_penalty_crossover	index_fetch_penalty_crossover	premise: the table holds all 1000000 rows	15;16;17;18;19	never	-
+index_fetch_penalty_crossover	index_fetch_penalty_crossover	the fetch penalty leaves a clustered ORDER BY on its index	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	a sequential scan of the same poisoned chunk is refused (XX001)	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	an index fetch of a chunk whose page_length is 2^32 too large is refused (XX001)	15;16;17;18;19	never	-
 native_chunk_length_bound	native_chunk_length_bound	backend survived the sequential refusal	15;16;17;18;19	never	-

test/check_ledger_budget.txt

@@ -58,4 +58,4 @@ suites_not_covered 249
 # that is not this one. Neither survives. Re-derived by COUNTING on the merged tree,
 # which is the only resolution this number has:
 #     awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -l
-checks_never_observed_red 1249
+checks_never_observed_red 1255

test/index_fetch_penalty_crossover.sh

@@ -0,0 +1,114 @@
+#!/usr/bin/env bash
+#
+# pgColumnar: a fetching index scan on a correlated key is priced below the
+# custom scan through ~50,000 rows, while it does about 27x the work (#913).
+#
+# The penalty term exists for this. The measurement says it is too small. This
+# suite asserts the PLAN, not a cost number: costs drift with the constants, the
+# chosen node is the property. Split from #766, which closed on the opposite
+# question (custom scan vs heap). Raising the custom-scan cost would enlarge the
+# wrong-plan region.
+#
+# Independent of test/pytest/test_index_fetch_penalty_crossover.py: same public
+# seam, own fixture, own observations.
+#
+# Usage:  test/index_fetch_penalty_crossover.sh [PG_CONFIG]
+# Written fresh for pgColumnar.
+
+set -uo pipefail
+. "$(dirname "${BASH_SOURCE[0]}")/lib.sh"
+pgc_setup "${1:-/usr/local/pg17/bin/pg_config}"
+
+N=1000000
+K_RANGE=50000
+
+psql_run "CREATE TABLE ifc (id int, a int, b int) USING pgcolumnar;"
+psql_run "INSERT INTO ifc SELECT g, g % 10, g % 100 FROM generate_series(1,$N) g;"
+psql_run "CREATE INDEX ifc_id ON ifc(id);"
+psql_run "ANALYZE ifc;"
+
+q1() {
+	env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -Atq \
+		-c "$1" 2>&1 | tail -1
+}
+
+SETS="SET enable_seqscan=off;
+SET enable_bitmapscan=off;
+SET enable_indexonlyscan=off;
+SET max_parallel_workers_per_gather=0;
+SET jit=off;
+SET pgcolumnar.enable_vectorization=off;
+SET pgcolumnar.enable_ungrouped_vector_agg=off;
+SET pgcolumnar.enable_group_vectorization=off;"
+
+plan_of() {
+	env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -Atq \
+		-c "$SETS $1" 2>&1
+}
+
+top_node() {
+	# First scan/join node in COSTS OFF text, which is the chosen path.
+	grep -m1 -oE 'Index Scan|Index Only Scan|Bitmap Heap Scan|Custom Scan|Seq Scan' <<<"$1"
+}
+
+check "premise: the table holds all $N rows" "$(q1 "SELECT count(*) FROM ifc")" "$N"
+check "premise: the btree on id exists" \
+	"$(q1 "SELECT count(*) FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid WHERE i.indrelid = 'ifc'::regclass AND c.relname = 'ifc_id'")" \
+	"1"
+
+PLAN_PT="$(plan_of "EXPLAIN (COSTS OFF) SELECT sum(a) FROM ifc WHERE id = 1")"
+echo "-- point lookup: $(printf '%s\n' "$PLAN_PT" | grep -m1 -E 'Scan')"
+check "a selective point lookup still uses the index" \
+	"$(top_node "$PLAN_PT")" "Index Scan"
+
+PLAN_50="$(plan_of "EXPLAIN (COSTS OFF) SELECT sum(a) FROM ifc WHERE id <= $K_RANGE")"
+echo "-- ${K_RANGE}-row range: $(printf '%s\n' "$PLAN_50" | grep -m1 -E 'Scan')"
+check "a ${K_RANGE}-row correlated range uses the custom scan, not a fetching index" \
+	"$(top_node "$PLAN_50")" "Custom Scan"
+
+IDX_SUM="$(q1 "SET enable_seqscan=off; SET enable_bitmapscan=off; SET pgcolumnar.enable_custom_scan=off; SELECT sum(a) FROM ifc WHERE id <= $K_RANGE")"
+CS_SUM="$(q1 "SET enable_indexscan=off; SET enable_bitmapscan=off; SELECT sum(a) FROM ifc WHERE id <= $K_RANGE")"
+check "both paths return the same aggregate at $K_RANGE" "$IDX_SUM" "$CS_SUM"
+
+# #355: a clustered ORDER BY of the whole table must stay on the index. A
+# per-row term that grows with rows costs this path off onto a Sort. The
+# 50,000-row range above projects two ints; this table carries a payload so
+# SELECT * is the same shape that #355 must not over-fire on.
+N_ORD=300000
+psql_run "CREATE TABLE ifc_cl (id int, payload text) USING pgcolumnar;"
+psql_run "INSERT INTO ifc_cl SELECT g, repeat('x', 48) FROM generate_series(1,$N_ORD) g;"
+psql_run "CREATE INDEX ifc_cl_id ON ifc_cl(id);"
+psql_run "ANALYZE ifc_cl;"
+
+SETS_ORD="SET max_parallel_workers_per_gather=0; SET random_page_cost=1.0;"
+PLAN_ORD="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -Atq \
+	-c "$SETS_ORD EXPLAIN (COSTS OFF) SELECT * FROM ifc_cl ORDER BY id" 2>&1)"
+echo "-- clustered ORDER BY: $(printf '%s\n' "$PLAN_ORD" | grep -m1 -E 'Scan|Sort')"
+# THIS ARM IS THE CAP'S REMOVAL PROOF, and nothing else in the suite is.
+#
+# The per-row term is capped at half a group. Nothing here NAMES the cap, so a
+# reader asking "is that cap load-bearing, or can it be simplified away?" finds
+# no arm mentioning it and concludes nothing protects it. That conclusion is
+# wrong, and it was reached in writing during review of this PR before anyone
+# mutated the code.
+#
+# Measured, deleting the cap and leaving everything else:
+#
+#     as written   6 passed + 0 failed
+#     uncapped     FAIL  the fetch penalty leaves a clustered ORDER BY on its
+#                        index: got [no (Sort)] want [yes]
+#
+# per_row = cpu_tuple_cost * rows * decodeUnits grows with the whole table on an
+# ordered scan, so this IS the saturation case: uncapped it costs the ordered
+# scan off its index, which is the #355 regression the cap exists to prevent.
+#
+# The arm above it is the other side. Together they bound the cap in both
+# directions -- too small and the 50,000-row range stays on the index, too large
+# and the ordered scan leaves it. Removing either leaves the cap pinned on one
+# side only, which is the easy miss.
+check "the fetch penalty leaves a clustered ORDER BY on its index" \
+	"$(grep -q 'Index Scan using ifc_cl_id' <<<"$PLAN_ORD" && echo yes \
+		|| echo "no ($(printf '%s' "$PLAN_ORD" | grep -m1 -E 'Scan|Sort'))")" \
+	"yes"
+
+pgc_summary

test/pytest/TESTS.md

@@ -92,6 +92,7 @@ behaviour, the source of that number is named.
 - [44. test_native_chunk_length_bound.py: a truncated chunk length cannot fetch](#44-test_native_chunk_length_boundpy-a-truncated-chunk-length-cannot-fetch)
 - [45. test_native_fetch_coalesce.py: index fetch I/O is not per-column](#45-test_native_fetch_coalescepy-index-fetch-io-is-not-per-column)
 - [46. test_parallel_am_scan.py: a table-AM parallel scan must share work](#46-test_parallel_am_scanpy-a-table-am-parallel-scan-must-share-work)
+- [47. test_index_fetch_penalty_crossover.py: the correlated range must not fetch](#47-test_index_fetch_penalty_crossoverpy-the-correlated-range-must-not-fetch)
 
 ## 1. How to read a test in here
 
@@ -4354,11 +4355,6 @@ This file asserts the SQLSTATE, not a cost number. The poison is a catalog
 UPDATE; the property is that a fetch raises XX001 and the backend survives.
 
 Independent of `test/native_chunk_length_bound.sh`. Same public seam, own
-fixture, own observations. Assertion names match the shell suite.
-
-| test | what it asserts |
-| --- | --- |
-| `test_native_chunk_length_bound` | a point lookup uses the index and returns the row; after `page_length` grows by 2^32, both the fetch and a sequential scan raise XX001 and the backend survives each |
 ## 45. test_native_fetch_coalesce.py: index fetch I/O is not per-column
 
 Index fetch used to pin once per column: validity bitmap, then the value stream,
@@ -4400,3 +4396,21 @@ this file uses `ampar`, 80000 rows, groups of 200. Assertion names match.
 The load-bearing assertion is `workers share the table-AM scan, it is not a
 single claimer`. It is unreachable while `phs_nallocated` is first-wins, and
 reachable only when each worker claims its own row groups.
+| `test_index_fetch_penalty_crossover` | a 50,000-row correlated range uses the custom scan; a point lookup still uses the index; both paths agree on the aggregate; a clustered ORDER BY stays on the index |
+## 47. test_index_fetch_penalty_crossover.py: the correlated range must not fetch
+
+#913. A fetching index scan on a correlated key is priced below the custom scan
+through ~50,000 rows, while it does about 27x the work. The penalty term exists
+for this; the measurement says it is too small. Split from #766, which closed
+on the opposite question.
+
+This file asserts the PLAN, not a cost number. Costs drift with the constants.
+The chosen node is the property.
+
+Independent of `test/index_fetch_penalty_crossover.sh`. Same public seam, own
+fixture, own observations. Assertion names match the shell suite.
+
+| test | what it asserts |
+| --- | --- |
+| `test_native_chunk_length_bound` | a point lookup uses the index and returns the row; after `page_length` grows by 2^32, both the fetch and a sequential scan raise XX001 and the backend survives each |
+

test/pytest/expected_tests.txt

@@ -238,4 +238,4 @@ guard_tests 346
 # the expected answer for a file that needs a cluster, and checking it was the point
 # rather than assuming it.
 # cluster_tests re-derived by collection on the rebased tree, never by adding a delta measured on another tree.
-cluster_tests 415
+cluster_tests 416

test/pytest/test_compare_to_bash.py

@@ -89,7 +89,8 @@
             "native_chunk_length_bound", "native_fetch_coalesce", "native_ownership", "native_projection", "parallel_am_scan",
             "projection_privilege",
             "projections",
-            "sorted_pathkeys", "stats_privilege", "zonemap_boundaries"]
+            "sorted_pathkeys", "stats_privilege",
+            "index_fetch_penalty_crossover", "zonemap_boundaries"]
 
 # stem -> why it does not yet reach zero. Empty today, and an entry here is a claim
 # about the PORT rather than a licence: the standing arm does not grade it, so the

test/pytest/test_index_fetch_penalty_crossover.py

@@ -0,0 +1,110 @@
+"""A fetching index scan on a correlated key is priced below the custom scan
+through ~50,000 rows, while it does about 27x the work (#913).
+
+The penalty term exists for this. The measurement says it is too small. This
+file asserts the PLAN, not a cost number: costs drift with the constants, the
+chosen node is the property.
+
+Independent of test/index_fetch_penalty_crossover.sh: same public seam, own
+fixture, own observations. Assertion names match the shell suite so the two
+can be compared by name, not by importing each other.
+"""
+
+
+def _scan_node(plan):
+    """First scan node in the tree, matching the shell suite's grep."""
+    stack = [plan[0]["Plan"]]
+    while stack:
+        node = stack.pop(0)
+        t = node["Node Type"]
+        if t in (
+            "Index Scan",
+            "Index Only Scan",
+            "Bitmap Heap Scan",
+            "Custom Scan",
+            "Seq Scan",
+        ):
+            return t
+        stack.extend(node.get("Plans", ()))
+    return ""
+
+
+def _plan(conn, sql):
+    with conn.cursor() as cur:
+        cur.execute("SET enable_seqscan=off")
+        cur.execute("SET enable_bitmapscan=off")
+        cur.execute("SET enable_indexonlyscan=off")
+        cur.execute("SET max_parallel_workers_per_gather=0")
+        cur.execute("SET jit=off")
+        cur.execute("SET pgcolumnar.enable_vectorization=off")
+        cur.execute("SET pgcolumnar.enable_ungrouped_vector_agg=off")
+        cur.execute("SET pgcolumnar.enable_group_vectorization=off")
+        cur.execute(f"EXPLAIN (FORMAT JSON, COSTS OFF) {sql}")
+        return cur.fetchone()[0]
+
+
+def test_index_fetch_penalty_crossover(pgc_conn, expect):
+    n = 1_000_000
+    k_range = 50_000
+    with pgc_conn.cursor() as cur:
+        cur.execute("CREATE TABLE ifc (id int, a int, b int) USING pgcolumnar")
+        cur.execute(
+            f"INSERT INTO ifc SELECT g, g % 10, g % 100 FROM generate_series(1, {n}) g"
+        )
+        cur.execute("CREATE INDEX ifc_id ON ifc(id)")
+        cur.execute("ANALYZE ifc")
+        cur.execute("SELECT count(*) FROM ifc")
+        expect.num(cur.fetchone()[0], n, f"premise: the table holds all {n} rows")
+        cur.execute(
+            "SELECT count(*) FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid "
+            "WHERE i.indrelid = 'ifc'::regclass AND c.relname = 'ifc_id'"
+        )
+        expect.num(cur.fetchone()[0], 1, "premise: the btree on id exists")
+
+    point = _plan(pgc_conn, "SELECT sum(a) FROM ifc WHERE id = 1")
+    expect.text(
+        _scan_node(point),
+        "Index Scan",
+        "a selective point lookup still uses the index",
+    )
+
+    ranged = _plan(pgc_conn, f"SELECT sum(a) FROM ifc WHERE id <= {k_range}")
+    expect.text(
+        _scan_node(ranged),
+        "Custom Scan",
+        f"a {k_range}-row correlated range uses the custom scan, not a fetching index",
+    )
+
+    with pgc_conn.cursor() as cur:
+        cur.execute("SET enable_seqscan=off")
+        cur.execute("SET enable_bitmapscan=off")
+        cur.execute("SET pgcolumnar.enable_custom_scan=off")
+        cur.execute(f"SELECT sum(a) FROM ifc WHERE id <= {k_range}")
+        idx_sum = cur.fetchone()[0]
+        cur.execute("SET pgcolumnar.enable_custom_scan=on")
+        cur.execute("SET enable_indexscan=off")
+        cur.execute(f"SELECT sum(a) FROM ifc WHERE id <= {k_range}")
+        cs_sum = cur.fetchone()[0]
+    expect.num(idx_sum, cs_sum, f"both paths return the same aggregate at {k_range}")
+
+    n_ord = 300_000
+    with pgc_conn.cursor() as cur:
+        cur.execute("RESET ALL")
+        cur.execute("CREATE TABLE ifc_cl (id int, payload text) USING pgcolumnar")
+        cur.execute(
+            "INSERT INTO ifc_cl SELECT g, repeat('x', 48) "
+            f"FROM generate_series(1, {n_ord}) g"
+        )
+        cur.execute("CREATE INDEX ifc_cl_id ON ifc_cl(id)")
+        cur.execute("ANALYZE ifc_cl")
+        cur.execute("SET max_parallel_workers_per_gather=0")
+        cur.execute("SET random_page_cost=1.0")
+        cur.execute(
+            "EXPLAIN (FORMAT JSON, COSTS OFF) SELECT * FROM ifc_cl ORDER BY id"
+        )
+        ordered = cur.fetchone()[0]
+    expect.text(
+        _scan_node(ordered),
+        "Index Scan",
+        "the fetch penalty leaves a clustered ORDER BY on its index",
+    )

test/run_all_versions.sh

@@ -115,6 +115,7 @@ SUITES=(
 	import_exclusion
 	import_export_privilege
 	index_delete_liveness
+	index_fetch_penalty_crossover
 	index_fetch_penalty_width
 	index_only
 	inheritance