fix: sample every row group, and only the predicate columns, when estimating pruning (#817)

CHANGELOG.md

@@ -79,6 +79,70 @@ true until the next version shipped.
 
 ### Fixed
 
+- The planner's zone-map sample is one-based, so it no longer under-prices a
+  scan for matching the **newest** rows (#817).
+
+  `pgcolumnar_zonemap_survival` samples row groups and asks the reader's own skip
+  predicates how many survive. It walked `g = i * ngroups / nsample` for `i` in
+  `[0, nsample)`, so it sampled `[0, ngroups)`. A row group number is the stripe
+  id reserved from the metapage, and the metapage starts `reservedStripeId` at 1:
+  **group 0 exists on no table.** The first probe was always spent on a number
+  that could not exist, and group `ngroups` was never probed at all.
+
+  The wasted probe was harmless -- an absent group narrows the sample rather than
+  biasing it. The missing one was not. When the group count fits in the sample
+  target the loop is a census, and it was a census that omitted the newest group
+  every time, so the same predicate was priced differently according to where in
+  the table its groups sat. Measured on ten groups of 2,000 rows, one clause
+  each, identical row estimates, the only difference being position:
+
+  | predicate | groups matched | before | after |
+  | --- | --- | ---: | ---: |
+  | `c1 > 8000` | 5..10, the six newest | 166.89 | 180.24 |
+  | `c1 <= 12000` | 1..6, the six oldest | 200.27 | 180.24 |
+  | `c1 > 17000` | 9,10, the two newest | 33.38 | 60.08 |
+  | `c1 <= 4000` | 1,2, the two oldest | 66.76 | 60.08 |
+
+  Exactly half, in the narrow pair. The under-priced half is the recency
+  predicate this engine is aimed at: on batch-loaded time-series, `WHERE ts >
+  now() - interval '1 hour'` selects the groups the sample never looked at.
+
+- The planner's zone-map sample reads only the predicate columns, as the executor
+  already does (#817).
+
+  It called `PgColumnarReadZoneMapList`, which keys on `(storage_id,
+  group_number)` against the four-column `zone_map_pkey`, so it fetched every
+  column's and every vector's row from the heap and then used one column's.
+  `pgcolumnar_native_group_can_match` has asked the per-column question with a
+  three-key probe since #314; the estimator now asks it the same way, through the
+  same `PgColumnarReadZoneMapForColumn` and the same session cache. Planning-time
+  `zone_map` fetches on a 30-column table go from 540 to 10, and no longer scale
+  with table width: 30 columns and 2 columns both read 10.
+
+  The estimator holds a #744 read session of its own, so it resolves `zone_map`
+  once for the whole sample rather than once per probe. Its `DEBUG1` report is
+  labelled `zone map estimate:` where a scan's stays `zone map read:`, because
+  the two interleave in one backend's log and `native_zonemap_session` counts
+  scan reports to prove that a scan around an aborted one opens for itself. A
+  scan's line is byte-identical to what #744 shipped.
+
+  **The two halves could not ship apart.** Fixing only the one-based sample makes
+  the whole-group probe reachable at the default `stripe_row_limit`, where the
+  single wasted probe had been hiding it, and `native_zonemap_narrow` correctly
+  reddens at wide 90 against narrow 34.
+
+- `PgColumnarReadZoneMapForColumn` no longer discards the index oid it just
+  cached (#817). #744 resolves `zone_map_pkey` once per read session and stores
+  it; an unconditional second `pgcolumnar_index_oid("zone_map_pkey")` stood
+  immediately before `systable_beginscan` and overwrote both that value and the
+  no-session branch's own lookup. The session's `opens` counter never noticed,
+  because it counts relation opens, which really were saved. Shown by poisoning
+  the cached value to `InvalidOid`: with the dead store present the poison is
+  inert (40 index fetches, 0 sequential scans, identical to the unpoisoned
+  control), and with it removed the poison reaches the scan and forces 20
+  sequential scans of `zone_map`. A dead store draws no compiler warning, which
+  is how it survived.
+
 - The cost model reads the row-group geometry a table was **written** with,
   not the geometry a write in the planning session would produce (#806).
   `pgcolumnar.storage.row_group_limit` records what the writer used and nothing

docs/testing.md

@@ -52,6 +52,7 @@ test/native_writer.sh    /path/to/pg_config  # native format catalog output
 test/native_roundtrip.sh /path/to/pg_config  # native write then read round-trip
 test/native_encoding.sh  /path/to/pg_config  # native per-vector encoding cascade
 test/native_zonemap.sh   /path/to/pg_config  # native zone maps
+test/zonemap_estimate_sample.sh /path/to/pg_config  # the planner's zone-map sample
 test/write_minmax_fastpath.sh /path/to/pg_config # direct zone min/max comparison
 test/native_skip.sh      /path/to/pg_config  # native chunk and vector skipping
 test/native_agg.sh       /path/to/pg_config  # native aggregate paths

src/columnar.h

@@ -493,6 +493,15 @@ typedef struct PgColumnarZoneMapSession
 	Oid			idxOid;
 	uint64		probes;
 	uint64		opens;
+	/*
+	 * What this session was opened for, used only in the DEBUG1 report. The
+	 * planner's survival estimate holds a session of its own, and its report has
+	 * to be told apart from a scan's: they interleave in one backend's log, and
+	 * native_zonemap_session counts scan reports to prove a scan around an
+	 * aborted one opens for itself. NULL reads as "read", so an executor scan's
+	 * line is byte-identical to what #744 shipped.
+	 */
+	const char *what;
 } PgColumnarZoneMapSession;
 
 /* sess may be NULL, which is the old open-per-probe behaviour. */

src/columnar_metadata.c

@@ -355,7 +355,8 @@ PgColumnarCloseZoneMapSession(PgColumnarZoneMapSession *sess)
 	}
 	sess->idxOid = InvalidOid;
 	if (message_level_is_interesting(DEBUG1))
-		elog(DEBUG1, "pgcolumnar zone map read: probes=%lu opens=%lu",
+		elog(DEBUG1, "pgcolumnar zone map %s: probes=%lu opens=%lu",
+			 sess->what != NULL ? sess->what : "read",
 			 (unsigned long) sess->probes, (unsigned long) sess->opens);
 }
 
@@ -2937,7 +2938,19 @@ PgColumnarReadZoneMapForColumn(uint64 storageId, uint64 groupNumber,
 				F_INT8EQ, Int64GetDatum((int64) groupNumber));
 	ScanKeyInit(&key[2], Anum_zone_map_column_index, BTEqualStrategyNumber,
 				F_INT2EQ, Int16GetDatum((int16) columnIndex));
-	idxOid = pgcolumnar_index_oid("zone_map_pkey");
+	/*
+	 * idxOid is already resolved above -- from the session when there is one,
+	 * by lookup when there is not. A second unconditional
+	 * pgcolumnar_index_oid("zone_map_pkey") stood here and overwrote both, so
+	 * #744's cache stored an index oid that every probe discarded and
+	 * re-derived. The `opens` counter never noticed, because it counts relation
+	 * opens, which the session really did save; the cache read as wholly
+	 * effective while half of what it cached was thrown away. A dead store
+	 * draws no compiler warning, which is how it survived.
+	 *
+	 * The sibling PgColumnarReadZoneMapVectorsForColumn was checked and has one
+	 * lookup, not two.
+	 */
 	scan = systable_beginscan(rel, idxOid, OidIsValid(idxOid), snapshot, 3, key);
 	while (HeapTupleIsValid(tuple = systable_getnext(scan)))
 	{

src/columnar_reader.c

@@ -1597,15 +1597,21 @@ PgColumnarEstimatePruneSurvival(uint64 storageId, TupleDesc tupdesc, List *qual,
 	int			nkeys = 0;
 	SkipPredicate *preds;
 	int			npreds;
+	NativeZoneMapMetadata **byCol;
+	bool	   *lookedUp;
 	Snapshot	snap;
 	MemoryContext cx;
 	MemoryContext old;
+	PgColumnarZoneMapSession sess;
 	int			nsample;
 	int			i;
 	int			examined = 0;
 	int			survived = 0;
 	double		survival;
 
+	memset(&sess, 0, sizeof(sess));
+	sess.what = "estimate";
+
 	if (ngroups == 0 || tupdesc == NULL || qual == NIL)
 		return 1.0;
 	if (!pgcolumnar_enable_qual_pushdown)
@@ -1646,47 +1652,97 @@ PgColumnarEstimatePruneSurvival(uint64 storageId, TupleDesc tupdesc, List *qual,
 		return 1.0;
 	}
 
+	byCol = palloc0(sizeof(NativeZoneMapMetadata *) * tupdesc->natts);
+	lookedUp = palloc0(sizeof(bool) * tupdesc->natts);
+
 	for (i = 0; i < nsample; i++)
 	{
-		uint64		g = (uint64) (((double) i * (double) ngroups) / (double) nsample);
-		List	   *zones;
-		NativeZoneMapMetadata **byCol;
-		ListCell   *lc;
+		/*
+		 * Row group numbers are ONE-based, so the sample must be too.
+		 *
+		 * A group number is the stripe id reserved from the metapage when the
+		 * stripe began buffering (columnar_write_state.c, "the row group number
+		 * is the stripe id"), and the metapage starts reservedStripeId at 1.
+		 * There is no group 0 on any table, so the plain stride
+		 * i * ngroups / nsample spent its first probe on a number that cannot
+		 * exist and never reached group ngroups at all.
+		 *
+		 * The wasted probe was harmless -- an absent group narrows the sample
+		 * rather than biasing it -- but the missing one was not. When ngroups
+		 * fits in sampleTarget this loop is a CENSUS, and it was a census that
+		 * omitted the newest group every time, so the same predicate was priced
+		 * differently depending on WHERE in the table its groups sat. Measured
+		 * on ten groups of 2,000 rows, one clause, identical row estimates:
+		 * the six newest groups were priced at 166.89 and the six oldest at
+		 * 200.27; the two newest at 33.38 and the two oldest at 66.76, exactly
+		 * half. The under-priced half is the recency predicate this engine is
+		 * aimed at.
+		 */
+		uint64		g = 1 + (uint64) (((double) i * (double) ngroups) / (double) nsample);
 		bool		canMatch = true;
+		bool		present = false;
 		int			p;
 
 		CHECK_FOR_INTERRUPTS();
 
-		zones = PgColumnarReadZoneMapList(storageId, g, snap);
-		if (zones == NIL)
-			continue;			/* no such group: narrow the sample, do not bias it */
-
-		examined++;
-
-		byCol = palloc0(sizeof(NativeZoneMapMetadata *) * tupdesc->natts);
-		foreach(lc, zones)
-		{
-			NativeZoneMapMetadata *z = (NativeZoneMapMetadata *) lfirst(lc);
-
-			if (z->columnIndex >= 0 && z->columnIndex < tupdesc->natts)
-				byCol[z->columnIndex] = z;
-		}
+		memset(byCol, 0, sizeof(NativeZoneMapMetadata *) * tupdesc->natts);
+		memset(lookedUp, 0, sizeof(bool) * tupdesc->natts);
 
 		for (p = 0; p < npreds; p++)
 		{
 			Form_pg_attribute att = TupleDescAttr(tupdesc, preds[p].attidx);
+			int			a = preds[p].attidx;
 
-			if (native_zone_excludes(&preds[p], att, byCol[preds[p].attidx], cx))
+			/*
+			 * One probe per PREDICATE COLUMN, not one per group.
+			 *
+			 * This asked PgColumnarReadZoneMapList for the whole group and then
+			 * dereferenced byCol[preds[p].attidx] alone. That call keys on
+			 * (storage_id, group_number) against a four-column index, so it
+			 * fetched every column's and every vector's row from the heap and
+			 * discarded the vector rows afterwards: 60 tuples per group on a
+			 * 30-column table to read one column's min and max.
+			 *
+			 * pgcolumnar_native_group_can_match already asks the per-column
+			 * question with a three-key probe (#314), and the estimator must ask
+			 * the same question the same way -- a discount taken by a different
+			 * rule than the one that priced it is how a plan gets chosen for a
+			 * saving it never realises. lookedUp records the fetch, not the
+			 * result, so two predicates on one column probe once and a column
+			 * with no zone map is not re-probed.
+			 */
+			if (!lookedUp[a])
+			{
+				byCol[a] = PgColumnarReadZoneMapForColumn(storageId, g, a,
+														  snap, &sess);
+				lookedUp[a] = true;
+			}
+			if (byCol[a] != NULL)
+				present = true;
+
+			if (native_zone_excludes(&preds[p], att, byCol[a], cx))
 			{
 				canMatch = false;
 				break;
 			}
 		}
 
+		/*
+		 * Absent group, decided by the predicate columns rather than by the
+		 * whole group's row list. When the group does not exist every probe
+		 * returns NULL; when it exists but the predicate columns carry no zone
+		 * map, nothing can prune on it and both readings give a survival of
+		 * 1.0, so the two cases need not be told apart.
+		 */
+		if (!present)
+			continue;			/* no such group: narrow the sample, do not bias it */
+
+		examined++;
 		if (canMatch)
 			survived++;
 	}
 
+	PgColumnarCloseZoneMapSession(&sess);
 	MemoryContextSwitchTo(old);
 
 	/*

test/run_all_versions.sh

@@ -269,7 +269,8 @@ SUITES=(
 	wal_envelope
 	write_fsst_compressed
 	write_minmax_fastpath
-	zonemap_cost)
+	zonemap_cost
+	zonemap_estimate_sample)
 
 
 # ---------------------------------------------------------------------------