diff --git a/CHANGELOG.md b/CHANGELOG.md index 333c91c8..180387ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,23 @@ true until the next version shipped. ### Fixed +- The planner estimate counts live rows, and reads the delete count in one + catalog scan rather than one per row group. + + **`row_group.row_count` is physical occupancy.** It still counts rows that a + later `DELETE` marked in the delete vector, and the planner uses this callback + instead of `pg_class.reltuples`, so every scan of a heavily deleted table was + priced as if the deletes had not happened. + + The count now comes from one indexed scan summing + `delete_vector.deleted_count` over the storage. The earlier shape walked each + group's bitmap a bit at a time, once per row group, on every plan of a + columnar relation. + + Summing is exact rather than an approximation. `delete_vector` carries a + unique index on `(storage_id, group_number)`, so there is one row per group + and no two summands can count the same row. The comment that justified the + per-group fold said the opposite, and the schema forbids what it described. - Arrow import reads the temporal unit and carrier width the file declares, rather than assuming the ones our own exporter writes (#864, #865). diff --git a/src/columnar.h b/src/columnar.h index 9ee40969..f4ac62e9 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -552,6 +552,8 @@ extern Snapshot PgColumnarCatalogSnapshot(Snapshot base); extern List *PgColumnarReadDeleteVectorList(uint64 storageId, uint64 stripeId, Snapshot snapshot); extern void PgColumnarUpsertDeleteVector(uint64 storageId, DeleteVectorMetadata *rm); +extern uint64 PgColumnarGroupDeletedCount(uint64 storageId, NativeRowGroupMetadata *rg, + Snapshot snapshot); /* ------------------------------------------------------------------------- * writer (pgcolumnar_write_state.c) diff --git a/src/columnar_delete_vector.c b/src/columnar_delete_vector.c index f45e35ba..110fd633 100644 --- a/src/columnar_delete_vector.c +++ b/src/columnar_delete_vector.c @@ -497,3 +497,60 @@ PgColumnarDeleteVectorPromoteSubXact(SubTransactionId subid, SubTransactionId pa buf->subid = parent; } } + +/* + * PgColumnarGroupDeletedCount + * How many of this row group's rows are deleted, under the given catalog + * snapshot. Bits past the group's row count are ignored. + * + * This walks the group's mask the same way the reader does when it builds + * one (spec 7.5), which is why it lives beside that code rather than being + * reimplemented at each caller. + * + * It does NOT do so to avoid double-counting. An earlier version of this + * comment said a group can have several delete_vector rows whose bitmaps + * overlap, so summing deletedCount would count a row deleted twice. The + * catalog forbids it: delete_vector carries a unique index on + * (storage_id, group_number), so there is at most one row per group and + * nothing to OR together. The planner estimate now sums deleted_count over + * the storage in one scan for exactly that reason. + */ +uint64 +PgColumnarGroupDeletedCount(uint64 storageId, NativeRowGroupMetadata *rg, + Snapshot snapshot) +{ + uint32 want = (uint32) ((rg->rowCount + 7) / 8); + char *mask; + List *rml; + ListCell *mc; + uint64 deleted = 0; + uint32 b; + + rml = PgColumnarReadDeleteVectorList(storageId, rg->groupNumber, snapshot); + if (rml == NIL) + return 0; + + mask = palloc0(want > 0 ? want : 1); + foreach(mc, rml) + { + DeleteVectorMetadata *rm = (DeleteVectorMetadata *) lfirst(mc); + + if (rm->bitmap == NULL || rm->bitmapLen == 0) + continue; + for (b = 0; b < rm->bitmapLen && b < want; b++) + mask[b] |= rm->bitmap[b]; + } + + for (b = 0; b < want; b++) + { + uint64 base = (uint64) b * 8; + int i; + + for (i = 0; i < 8; i++) + if (base + i < rg->rowCount && ((mask[b] >> i) & 1)) + deleted++; + } + + pfree(mask); + return deleted; +} diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 1a01f1d4..856666a5 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -1423,6 +1423,55 @@ PgColumnarStorageHasDeleteVector(uint64 storageId, Snapshot snapshot) return found; } +/* + * PgColumnarStorageDeletedCount + * Total rows marked deleted across the whole storage, read from + * delete_vector.deleted_count. + * + * One index scan over the storage's delete_vector rows, summing a stored + * integer, rather than one scan and a bitmap walk per row group. The + * planner calls this on every plan of a columnar relation, so the cost is + * paid per plan and not per query. + * + * deleted_count is the number of set bits in that row's bitmap, maintained + * where the bitmap is written, so summing it is exact rather than an + * approximation -- the unique index on (storage_id, group_number) means + * one row per group, so no two summands can count the same row. + */ +uint64 +PgColumnarStorageDeletedCount(uint64 storageId, Snapshot snapshot) +{ + Relation rel = open_columnar_table("delete_vector", AccessShareLock); + ScanKeyData key[1]; + SysScanDesc scan; + HeapTuple tup; + TupleDesc tupdesc = RelationGetDescr(rel); + uint64 total = 0; + Oid dvIdx = pgcolumnar_index_oid("delete_vector_pkey"); + + ScanKeyInit(&key[0], Anum_delete_vector_storage_id, BTEqualStrategyNumber, + F_INT8EQ, Int64GetDatum((int64) storageId)); + scan = systable_beginscan(rel, dvIdx, OidIsValid(dvIdx), snapshot, 1, key); + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + bool isnull; + Datum d = heap_getattr(tup, Anum_delete_vector_deleted_count, + tupdesc, &isnull); + + if (!isnull) + { + int32 n = DatumGetInt32(d); + + if (n > 0) + total += (uint64) n; + } + } + systable_endscan(scan); + table_close(rel, AccessShareLock); + + return total; +} + /* * delete_vector_chunk_lock_key * Mix the identity of a chunk group into a 64-bit advisory-lock key. The diff --git a/src/columnar_metadata.h b/src/columnar_metadata.h index 0d6d5c65..739bf4e0 100644 --- a/src/columnar_metadata.h +++ b/src/columnar_metadata.h @@ -95,5 +95,6 @@ extern void PgColumnarDeleteProjectionDeclaration(Oid relid, const char *name); extern void PgColumnarDeleteProjectionDeclarationsForRel(Oid relid); extern bool PgColumnarStorageHasDeleteVector(uint64 storageId, Snapshot snapshot); +extern uint64 PgColumnarStorageDeletedCount(uint64 storageId, Snapshot snapshot); #endif /* PGCOLUMNAR_METADATA_H */ diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 6046f162..f228601e 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -841,10 +841,53 @@ pgcolumnar_relation_estimate_size(Relation rel, int32 *attr_widths, * planner from mis-costing scans (spec 6, 9). */ snapshot = ActiveSnapshotSet() ? GetActiveSnapshot() : GetTransactionSnapshot(); - rowGroupList = PgColumnarReadRowGroupList(storageId, PgColumnarCatalogSnapshot(snapshot)); + snapshot = PgColumnarCatalogSnapshot(snapshot); + rowGroupList = PgColumnarReadRowGroupList(storageId, snapshot); - foreach(lc, rowGroupList) - liveRows += (double) ((NativeRowGroupMetadata *) lfirst(lc))->rowCount; + /* + * row_group.row_count is the physical occupancy, including rows later + * marked in delete_vector. The planner uses this callback instead of + * pg_class.reltuples, so leaving those rows in *tuples prices every scan + * as if DELETE had not happened. + */ + { + uint64 physicalRows = 0; + uint64 deleted; + + foreach(lc, rowGroupList) + { + NativeRowGroupMetadata *rg = (NativeRowGroupMetadata *) lfirst(lc); + + physicalRows += rg->rowCount; + } + + /* + * One catalog scan summing delete_vector.deleted_count, not one scan + * and a bitmap walk per row group. This runs on every plan of a + * columnar relation, so a per-group fold is paid per plan. + * + * Summing is exact, not an approximation: the unique index on + * (storage_id, group_number) means one delete_vector row per group, so + * no two summands can count the same row. An earlier comment here + * justified the per-group fold by saying a group can have several + * delete_vector rows whose bitmaps overlap. The catalog forbids that, + * and the expensive path was defended by a premise the schema rules + * out. + * + * The clamp is against the storage total rather than per group, which + * is what the per-group clamp was doing in aggregate. It matters only + * if the count is ever inconsistent with the row groups, and an + * estimate must not go negative. + * + * Both reads take the same catalog snapshot as the row-group list + * above, deliberately, so the count cannot straddle two snapshots and + * report more deletes than there are rows. + */ + deleted = PgColumnarStorageDeletedCount(storageId, snapshot); + if (deleted > physicalRows) + deleted = physicalRows; + liveRows = (double) (physicalRows - deleted); + } *pages = Max(nblocks, 1); *tuples = Max(liveRows, 0); diff --git a/src/columnar_vector.c b/src/columnar_vector.c index 6f2ed530..49408d1b 100644 --- a/src/columnar_vector.c +++ b/src/columnar_vector.c @@ -2929,60 +2929,6 @@ pgcolumnar_agg_finalize(PgColumnarAggSpec *spec, bool *isnull) return (Datum) 0; } -/* - * pgcolumnar_group_deleted_count - * How many of this row group's rows are deleted, under the given catalog - * snapshot. A group can have several delete_vector rows, whose bitmaps - * overlap, so they are OR'd before counting rather than summed -- summing - * deletedCount across entries would double-count a row deleted twice (spec - * 7.5, and the same combining the reader does when it builds a group's mask). - * Bits past the group's row count are ignored. - */ -static uint64 -pgcolumnar_group_deleted_count(uint64 storageId, NativeRowGroupMetadata *rg, - Snapshot snap) -{ - uint32 want = (uint32) ((rg->rowCount + 7) / 8); - char *mask; - List *rml; - ListCell *mc; - uint64 deleted = 0; - uint32 b; - - rml = PgColumnarReadDeleteVectorList(storageId, rg->groupNumber, snap); - if (rml == NIL) - return 0; - - mask = palloc0(want > 0 ? want : 1); - foreach(mc, rml) - { - DeleteVectorMetadata *rm = (DeleteVectorMetadata *) lfirst(mc); - - if (rm->bitmap == NULL || rm->bitmapLen == 0) - continue; - for (b = 0; b < rm->bitmapLen && b < want; b++) - mask[b] |= rm->bitmap[b]; - } - - /* - * Count set bits only up to rowCount. The last byte of the bitmap can carry - * bits beyond the group's final row, and counting those would report more - * rows deleted than the group holds. - */ - for (b = 0; b < want; b++) - { - uint64 base = (uint64) b * 8; - int i; - - for (i = 0; i < 8; i++) - if (base + i < rg->rowCount && ((mask[b] >> i) & 1)) - deleted++; - } - - pfree(mask); - return deleted; -} - /* * pgcolumnar_fill_native_metadata_agg * Answer an ungrouped, unfiltered aggregate over a native (PGCN v1) table @@ -3065,7 +3011,7 @@ pgcolumnar_fill_native_metadata_agg(PgColumnarAggScanState *state, int *ndirty) int a; if (anyDeletes) - deleted = pgcolumnar_group_deleted_count(storageId, rg, snap); + deleted = PgColumnarGroupDeletedCount(storageId, rg, snap); if (deleted > 0) { diff --git a/test/estimate_deleted.sh b/test/estimate_deleted.sh new file mode 100755 index 00000000..97e02c09 --- /dev/null +++ b/test/estimate_deleted.sh @@ -0,0 +1,122 @@ +#!/usr/bin/env bash +# +# The planner must not count rows that delete_vector has marked. +# +# pgcolumnar_relation_estimate_size is the only row count the planner sees: +# core delegates, so pg_class.reltuples is ignored. The callback summed +# row_group.row_count and never subtracted delete_vector, so after +# +# INSERT 100000 rows; DELETE 99000 of them; ANALYZE +# +# reltuples was 1000 and the true count was 1000, but EXPLAIN still said +# rows=100000. That is the same class of defect as #507: the catalog looks +# right and the plan stays wrong. +# +# heap is the control for the shape (ANALYZE updates its estimate). Columnar +# is required to report the live count from metadata, not a sampled one, so +# the figure must be exact. +# +# Usage: test/estimate_deleted.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}" + +plan_rows() { + q "EXPLAIN (COSTS ON) $1" | grep -oiE 'rows=[0-9]+' | head -1 | cut -d= -f2 +} + +psql_run "CREATE TABLE ed_c (id int, v text) USING pgcolumnar;" +psql_run "CREATE TABLE ed_h (id int, v text);" +psql_run "INSERT INTO ed_c SELECT g, 'x' || g FROM generate_series(1, 100000) g;" +psql_run "INSERT INTO ed_h SELECT g, 'x' || g FROM generate_series(1, 100000) g;" +psql_run "ANALYZE ed_c; ANALYZE ed_h;" + +check_num "premise: columnar holds 100000 rows before delete" \ + "$(q 'SELECT count(*) FROM ed_c')" "100000" +before="$(plan_rows 'SELECT * FROM ed_c')" +echo "-- rows before delete: $before" +check "premise: the estimate before delete is the full table" \ + "$(awk -v v="$before" 'BEGIN { print (v+0 >= 90000) ? "full" : "low " v }')" "full" + +psql_run "DELETE FROM ed_c WHERE id > 1000;" +psql_run "DELETE FROM ed_h WHERE id > 1000;" +psql_run "ANALYZE ed_c; ANALYZE ed_h;" + +check_num "premise: 1000 live rows remain" \ + "$(q 'SELECT count(*) FROM ed_c')" "1000" +relt="$(q "SELECT reltuples::bigint FROM pg_class WHERE relname='ed_c'")" +echo "-- reltuples after ANALYZE: $relt" +check "premise: ANALYZE recorded the live count in reltuples" \ + "$(awk -v v="$relt" 'BEGIN { d = (v > 1000 ? v-1000 : 1000-v); print (d <= 50) ? "close" : "off " v }')" "close" + +got="$(plan_rows 'SELECT * FROM ed_c')" +echo "-- columnar EXPLAIN rows= after delete: $got" +check "the planner estimate after delete is the live count, not the physical occupancy" \ + "$(awk -v v="$got" 'BEGIN { print (v+0 == 1000) ? "live" : "stale " v }')" "live" + +hgot="$(plan_rows 'SELECT * FROM ed_h')" +echo "-- heap EXPLAIN rows= after delete: $hgot" +check "control: heap is also near the live count, so the probe reads EXPLAIN" \ + "$(awk -v v="$hgot" 'BEGIN { d = (v > 1000 ? v-1000 : 1000-v); print (d <= 50) ? "close" : "off " v }')" "close" + +# ---- more than one row group, and no ANALYZE between delete and measurement -- +# +# Two holes in the arms above, both of which let a broken estimate look right. +# +# 1. The fixture is 100,000 rows against a default stripe_row_limit of 150,000, +# so it holds ONE row group. Anything the estimate does per group is +# exercised for a single iteration, and a bug that handled only the first +# group would pass. +# +# 2. `ANALYZE` runs immediately before every measurement, so pg_class.reltuples +# is freshly correct at each one. That is not what the callback reads, but it +# means a fixture cannot distinguish "the estimate subtracts deletes" from +# "something else supplied a correct number". +# +# This section fixes both: a smaller stripe_row_limit to get several groups +# cheaply, deletes confined to the LAST group, and the measurement taken with a +# DELIBERATELY STALE reltuples -- ANALYZE runs before the delete and not after. + +psql_run "CREATE TABLE ed_m (id int, v text) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('ed_m', stripe_row_limit => 20000);" +psql_run "INSERT INTO ed_m SELECT g, 'x' || g FROM generate_series(1, 100000) g;" +psql_run "ANALYZE ed_m;" + +ed_m_groups="$(q "SELECT count(*) FROM pgcolumnar.row_group rg + JOIN pgcolumnar.storage s ON s.storage_id = rg.storage_id + WHERE s.relation_oid = 'ed_m'::regclass")" +check "premise: the fixture spans several row groups, not one" \ + "$(awk -v v="$ed_m_groups" 'BEGIN { print (v+0 >= 3) ? "several" : "only " v }')" "several" + +ed_m_stale="$(q "SELECT reltuples::bigint FROM pg_class WHERE relname = 'ed_m'")" +check "premise: reltuples is 100000 before the delete" \ + "$(awk -v v="$ed_m_stale" 'BEGIN { print (v+0 >= 90000) ? "full" : "low " v }')" "full" + +# Delete only from the HIGHEST row numbers, so a bug that subtracts deletes for +# the first group alone leaves the estimate untouched and this arm reddens. +psql_run "DELETE FROM ed_m WHERE id > 60000;" + +# NO ANALYZE here, on purpose. reltuples still says 100000. If the estimate +# reported reltuples, or summed row_count without subtracting, it would say +# ~100000. Only reading the delete vector gets near 60000. +ed_m_after="$(plan_rows 'SELECT * FROM ed_m')" +echo "-- ed_m: groups=$ed_m_groups stale reltuples=$ed_m_stale estimate after delete=$ed_m_after" +check "the estimate follows the deletes with a stale reltuples and several groups" \ + "$(awk -v v="$ed_m_after" 'BEGIN { + print (v+0 >= 55000 && v+0 <= 65000) ? "live" : "wrong " v }')" "live" + +check "control: and the live count really is 60000" \ + "$(q 'SELECT count(*) FROM ed_m')" "60000" + +# The other direction: with no deletes at all the estimate must be the full +# table, or "follows the deletes" is satisfied by an estimate that is simply low. +psql_run "CREATE TABLE ed_n (id int, v text) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('ed_n', stripe_row_limit => 20000);" +psql_run "INSERT INTO ed_n SELECT g, 'x' || g FROM generate_series(1, 100000) g;" +ed_n_est="$(plan_rows 'SELECT * FROM ed_n')" +check "control: an undeleted table of the same shape estimates its full size" \ + "$(awk -v v="$ed_n_est" 'BEGIN { print (v+0 >= 90000) ? "full" : "low " v }')" "full" + +pgc_summary diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 28d85fa0..8f6b7ab9 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -77,6 +77,7 @@ SUITES=( encode_effort encode_invariants entry_point_privilege + estimate_deleted export_sink fk_referencing fsst_margin