diff --git a/CHANGELOG.md b/CHANGELOG.md index 461f23a..5055b79 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/testing.md b/docs/testing.md index 897a0f0..9707649 100644 --- a/docs/testing.md +++ b/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 diff --git a/src/columnar.h b/src/columnar.h index 0736c2d..9ee4096 100644 --- a/src/columnar.h +++ b/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. */ diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 8ef2d6c..9162d4e 100644 --- a/src/columnar_metadata.c +++ b/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))) { diff --git a/src/columnar_reader.c b/src/columnar_reader.c index 262d11b..0214e1c 100644 --- a/src/columnar_reader.c +++ b/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); /* diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index f21305d..e3854c8 100644 --- a/test/run_all_versions.sh +++ b/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) # --------------------------------------------------------------------------- diff --git a/test/zonemap_estimate_sample.sh b/test/zonemap_estimate_sample.sh new file mode 100644 index 0000000..ac897f9 --- /dev/null +++ b/test/zonemap_estimate_sample.sh @@ -0,0 +1,174 @@ +#!/usr/bin/env bash +# +# The planner's zone-map sample is one-based, complete, and per-column. +# +# pgcolumnar_zonemap_survival prices a restricted columnar scan by sampling row +# groups and asking the reader's own skip predicates how many survive. Two things +# were wrong with how it sampled. +# +# ONE-BASED. The stride was g = i * ngroups / nsample for i in [0, nsample), so it +# ran over [0, ngroups). A row group number is the stripe id reserved from the +# metapage (columnar_write_state.c), and the metapage starts reservedStripeId at 1, +# so 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. When ngroups fits in +# PGCOLUMNAR_PRUNE_SAMPLE_GROUPS the loop is a census, and it was a census that +# omitted the newest group every time. The same predicate was therefore priced +# differently according to WHERE in the table its groups sat -- and the half that +# came out cheap was the recency predicate this engine is aimed at. +# +# PER-COLUMN. It called PgColumnarReadZoneMapList, which keys on (storage_id, +# group_number) against a four-column index, so it fetched every column's and every +# vector's row and then used one column's. pgcolumnar_native_group_can_match asks +# the same question with a three-key per-column probe (#314); the estimator now +# asks it the same way, so its reads no longer scale with table width. +# +# The instrument is pg_stat_all_tables.idx_tup_fetch for pgcolumnar.zone_map, split +# into PLANNING (EXPLAIN, which runs the estimator and no executor) and EXECUTION, +# and made readable with pg_stat_force_next_flush. The per-table stripe_row_limit +# option is used rather than the session GUC, so the suite does not depend on the +# separate question of which limit the survival estimate reads (#817). +# +# Removal proof, both halves measured: +# - restore the zero-based stride and the two CENSUS checks read 9 of 10 groups +# while the mirror pair splits 166.89 against 200.27 and the narrow pair 33.38 +# against 66.76, exactly half; +# - restore the whole-group probe and WIDTH reads 600 against 40. +# +# Usage: test/zonemap_estimate_sample.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}" + +q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null + +wcols=""; wvals="" +for i in $(seq 1 30); do + wcols="$wcols${wcols:+, }c$i int" + wvals="$wvals${wvals:+, }g" +done +q "CREATE TABLE w ($wcols) USING pgcolumnar; + CREATE TABLE n (c1 int, c2 int) USING pgcolumnar; + SELECT pgcolumnar.set_options('w'::regclass, stripe_row_limit => 2000); + SELECT pgcolumnar.set_options('n'::regclass, stripe_row_limit => 2000);" >/dev/null +q "INSERT INTO w SELECT $wvals FROM generate_series(1,20000) g;" >/dev/null +q "INSERT INTO n SELECT g, g FROM generate_series(1,20000) g;" >/dev/null + +sid() { q "SELECT pgcolumnar.get_storage_id('$1'::regclass);"; } + +# ---- premises: the fixture is the shape every number below assumes ---------- + +check "w loaded 20000 rows" "$(q 'SELECT count(*) FROM w;')" "20000" +check "n loaded 20000 rows" "$(q 'SELECT count(*) FROM n;')" "20000" +check "w carries the per-table stripe_row_limit, not the session GUC" \ + "$(q "SELECT stripe_row_limit FROM pgcolumnar.options WHERE regclass='w'::regclass;")" "2000" +check "w has 10 row groups numbered 1..10, so there is no group 0" \ + "$(q "SELECT count(DISTINCT group_number)||'/'||min(group_number)||'/'||max(group_number) + FROM pgcolumnar.zone_map WHERE storage_id = $(sid w);")" "10/1/10" +# The per-column probe stops at the whole-chunk row and zone_map_pkey orders +# vector_index ascending, so -1 is the first tuple it meets: one fetch per group. +check "the whole-chunk row sorts first, so one fetch ends a per-column probe" \ + "$(q "SELECT min(vector_index) FROM pgcolumnar.zone_map + WHERE storage_id = $(sid w) AND group_number = 1 AND column_index = 0;")" "-1" + +echo "-- zone_map rows per group: whole group w=$(q "SELECT count(*) FROM pgcolumnar.zone_map WHERE storage_id = $(sid w) AND group_number = 1;") n=$(q "SELECT count(*) FROM pgcolumnar.zone_map WHERE storage_id = $(sid n) AND group_number = 1;")" + +# ---- the sample: complete, and one column wide ------------------------------ + +# zone_map index tuples fetched by ONE statement. +zm() { + q "SELECT pg_stat_reset();" >/dev/null + q "$1" >/dev/null + q "SELECT pg_stat_force_next_flush();" >/dev/null + q "SELECT coalesce(idx_tup_fetch,0) FROM pg_stat_all_tables WHERE relname='zone_map' AND schemaname='pgcolumnar';" +} + +WP="$(zm "EXPLAIN SELECT count(*) FROM w WHERE c1 >= 0;")" +WF="$(zm "SELECT count(*) FROM w WHERE c1 >= 0;")" +NP="$(zm "EXPLAIN SELECT count(*) FROM n WHERE c1 >= 0;")" +NF="$(zm "SELECT count(*) FROM n WHERE c1 >= 0;")" +echo "-- zone_map idx_tup_fetch, w (30 col): planning=$WP full=$WF execution=$((WF - WP))" +echo "-- zone_map idx_tup_fetch, n ( 2 col): planning=$NP full=$NF execution=$((NF - NP))" + +check "planning probes the zone map at all" \ + "$([ "$WP" -gt 0 ] && echo yes || echo no)" "yes" +check "CENSUS: planning reads one column across all 10 groups (w)" "$WP" "10" +check "CENSUS: planning reads one column across all 10 groups (n)" "$NP" "10" +check "WIDTH: the planner's zone-map reads do not scale with table width" "$WP" "$NP" +check "the executor's reads stay width-independent too" "$((WF - WP))" "$((NF - NP))" + +# ---- the estimator's own session ------------------------------------------- + +# #744 resolves zone_map once per read session instead of once per probe. The +# estimator now holds one too, and its DEBUG1 report is labelled "estimate" so it +# can be told from a scan's "read" -- 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. +est="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -At -c "SET client_min_messages=debug1; + EXPLAIN SELECT count(*) FROM w WHERE c1 >= 0;" 2>&1 | + grep -oE 'zone map estimate: probes=[0-9]+ opens=[0-9]+' | tail -1)" +echo "-- the planner's zone-map session: ${est:-}" +check "the estimator reports a session of its own, distinct from a scan's" \ + "$([ -n "$est" ] && echo yes || echo no)" "yes" +check "it probes once per group across all 10 groups" \ + "$(printf '%s' "$est" | grep -oE 'probes=[0-9]+' | grep -oE '[0-9]+')" "10" +check "and opens zone_map once for the whole sample, not once per probe" \ + "$(printf '%s' "$est" | grep -oE 'opens=[0-9]+' | grep -oE '[0-9]+')" "1" + +# ---- the consequence: position must not change the price -------------------- + +check "c1 > 17000 matches 3000 rows" "$(q 'SELECT count(*) FROM w WHERE c1 > 17000;')" "3000" +check "c1 <= 4000 matches 4000 rows" "$(q 'SELECT count(*) FROM w WHERE c1 <= 4000;')" "4000" +check "c1 > 8000 matches 12000 rows" "$(q 'SELECT count(*) FROM w WHERE c1 > 8000;')" "12000" +check "c1 <= 12000 matches 12000 rows" "$(q 'SELECT count(*) FROM w WHERE c1 <= 12000;')" "12000" + +scancost() { + q "EXPLAIN SELECT count(*) FROM w WHERE $1;" | + sed -n 's/.*Custom Scan (PgColumnarScan) on w (cost=[0-9.]*\.\.\([0-9.]*\) .*/\1/p' +} +scanrows() { + q "EXPLAIN SELECT count(*) FROM w WHERE $1;" | sed -n 's/.*PgColumnarScan.*rows=\([0-9]*\).*/\1/p' +} + +# The MIRROR PAIR isolates position and nothing else: one clause each, the same +# operator family, the same default selectivity (no ANALYZE has run, so every +# range clause is estimated alike), and the same SIX matching groups -- 5..10 for +# one, 1..6 for the other. `run` is therefore identical and any cost difference is +# survival alone. The NARROW PAIR repeats it at two groups, where the omission is +# half the sample rather than a sixth. +HI="$(scancost 'c1 > 8000')" # groups 5..10, the six NEWEST +LO="$(scancost 'c1 <= 12000')" # groups 1..6, the six OLDEST +TOP="$(scancost 'c1 > 17000')" # groups 9,10, the two NEWEST +BOT="$(scancost 'c1 <= 4000')" # groups 1,2, the two OLDEST +echo "-- Custom Scan total cost, six of ten groups: newest=$HI oldest=$LO" +echo "-- Custom Scan total cost, two of ten groups: newest=$TOP oldest=$BOT" + +check "a cost was extracted, so the checks below are not comparing two blanks" \ + "$([ -n "$HI" ] && [ -n "$LO" ] && [ -n "$TOP" ] && [ -n "$BOT" ] && echo yes || echo no)" "yes" +check "the mirror pair shares a row estimate, so only survival differs" \ + "$(scanrows 'c1 > 8000')" "$(scanrows 'c1 <= 12000')" +check "the narrow pair shares a row estimate, so only survival differs" \ + "$(scanrows 'c1 > 17000')" "$(scanrows 'c1 <= 4000')" +check "MIRROR: the six newest groups cost what the six oldest cost" "$HI" "$LO" +check "NARROW: the two newest groups cost what the two oldest cost" "$TOP" "$BOT" + +# The floor survives the change. A sample that excludes every group reports a +# survival of zero, and a scan matching anything at all must still read the group +# its match is in, so the discount is floored at one group's share (#171). A +# predicate matching NO group and one matching exactly the LAST group must both +# come out at that floor -- and the last group is one this sample previously never +# looked at. +NONE="$(scancost 'c1 > 99999')" +LAST="$(scancost 'c1 > 19000')" +echo "-- Custom Scan total cost at the floor: matches-nothing=$NONE last-group-only=$LAST" +check "a predicate matching nothing is floored, not priced at zero" \ + "$([ -n "$NONE" ] && [ "${NONE%%.*}" -gt 0 ] && echo yes || echo no)" "yes" +check "matching only the last group costs the same one-group share" "$LAST" "$NONE" +check "a pruning predicate still returns the right rows" \ + "$(q 'SELECT count(*) FROM w WHERE c1 > 19000;')" "1000" +check "a predicate matching nothing still returns no rows" \ + "$(q 'SELECT count(*) FROM w WHERE c1 > 99999;')" "0" + +pgc_summary