diff --git a/CHANGELOG.md b/CHANGELOG.md index 60a19a2c..2fce0c0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -994,6 +994,83 @@ true until the next version shipped. first version passed on prose, green on a caller that consulted nothing, and one arm now builds that exact text and requires zero. `selftest/190` has the same shape on `pgc_build_needs_clean`, tracked in #1222. +- Retiring a row group read five catalogs whole, once each per group, and the + cost grew with every unrelated columnar table in the database (#1207). + + `delete_group_rows()` opens its catalog from a `const char *tableName` + **parameter**, and `PgColumnarDeleteGroupMetadata` calls it five times, for + `delete_vector`, `column_chunk`, `zone_map`, `bloom` and `row_group`. So one + `systable_beginscan` in the source was five sequential reads per retired group + at run time, and two more sit beside it on the compaction path. The + `pgcolumnar` metadata catalogs are shared by every columnar table in the + database, so each of those reads was charged for every other table's rows. + + Two audits of these scans missed it. Both attributed a scan to a catalog by + the `open_columnar_table("")` that produced its relation handle, and a + relation that arrives as an argument has no name at the call site. What found + it was a reconciliation rather than a better reading: probing all 44 + `systable_beginscan` sites and requiring the number that ran without an index + to equal the sum of `seq_scan` over every catalog failed at 41 counted against + 22 probed. + + Eight sites now choose between an index probe and a sequential read **by + asking the catalog how many pages it has**. Every key was already a prefix of + an index that exists, so nothing here needs a catalog migration. + + **Choosing by size, rather than always probing, is the whole of the fix.** An + index probe is not unconditionally cheaper: it is a btree descent plus a heap + fetch plus two catcache lookups, which on a one-page catalog is more work than + reading the whole thing. The same catalog is one page in a database with one + columnar table and hundreds in a database with a thousand, so a path that + commits to either method is wrong at one end of that range. Measured in + buffers, for `pgcolumnar.compact()` over 40 row groups with 20 retired: + + | catalogs | always read whole | always probe | **by size** | + | ---: | ---: | ---: | ---: | + | 8 pages | 848 | 1000 | **848** | + | 18 pages | 1112 | 1080 | **1038** | + | 46 pages | 1672 | 1086 | **1041** | + | 390 pages | 8993 | 1122 | **1080** | + + Always probing is a regression of up to 18 per cent on a small database; + always reading whole costs 8.3 times as much on a large one. Choosing by size + is within 4 buffers of the best of the two at every size measured. + + `pgcolumnar.index_min_blocks` is the page count at which the choice flips. + Its default of **3** is derived rather than chosen: every threshold from + always-probe to never-probe, on PG 15, 17 and 19, over two families of fixture + (many small columnar tables; one deep one), 24 size points. Scored against the + cheapest threshold at each point, 3 costs 19 buffers in total where its + nearest rival costs 95, and never more than 4 at any single point. The + optimum is not the same for every catalog -- `row_group` pays for a probe at + three pages, `zone_map` not until five, because the hot catalogs are read more + often per retired group -- so one value for all six is a priced compromise + rather than a truth. Setting it very large restores the behaviour of every + earlier release exactly; setting it to 0 always probes. Both are slower at + some database size. + + Two of the eight sites are in `PgColumnarCheckFreeSpaceNoOverlap`, which is + assert-only. It was the last one found, and it is worth saying why: a + measurement taken on a release build reports those two clean while every + assert-enabled CI leg pays for them on each maintenance operation. Both + suites print `debug_assertions` for that reason. + + `test/catalog_delete_index.sh` (30 checks) and + `test/pytest/test_catalog_delete_index.py` (31). They assert the work -- + buffers served out of the six catalogs -- and never the access path. An + earlier version of both asserted `seq_scan = 0` and `idx_scan >= 1` per + catalog, and that is a claim about which path was taken: run against the + size-aware build, which is cheaper at every size, it failed 13 of 21 arms, the + same 13 a full revert reddens. A guard that fires on correct code gets + switched off. + + The port runs on a private DATABASE rather than the private schema every other + test here gets, because the `pgcolumnar` catalogs are per database and shared + by the whole session, and every claim in the file is about how big they are. + Run alone the file saw six catalog pages at its smallest phase; run after the + other fifty-one cluster files it saw thirty-nine, and the claim there fell from + 223 parts per thousand to 65. A premise counts the columnar relations so that + arrives as a named failure rather than a weak number. - A subset pytest run failed on PG 15-17, and the message told you to break the check (#1204). diff --git a/docs/configuration.md b/docs/configuration.md index 9c3a0c2d..a1297fb5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -83,6 +83,7 @@ disk. It never changes the values that a table returns. | Setting | Type | Default | Description | | --- | --- | --- | --- | +| `pgcolumnar.index_min_blocks` | integer | `3` | Heap pages at or above which a keyed read of a `pgcolumnar` metadata catalog probes that catalog's index instead of reading it sequentially. These catalogs are shared by every columnar table in the database, so the same catalog is one page in a small database and hundreds in a large one, and below a few pages a sequential read is cheaper than a probe. The default is measured rather than chosen, and it is a default rather than a truth: the best value differs per catalog, because the hot catalogs are read more often per retired row group. Change it only to undo a regression you have measured. A very large value restores the behaviour of every release before 1.0-alpha6, which always read sequentially; `0` always probes. Range 0 to INT_MAX. | | `pgcolumnar.reclaim_coalesce` | boolean | `on` | During online compaction, split an oversized freed range on reuse and coalesce adjacent freed ranges, so space is reclaimed under fragmentation. Off reverts to whole-range reuse. | | `pgcolumnar.enable_end_truncation` | boolean | `off` | Allow `pgcolumnar.truncate()` to return trailing reclaimed blocks to the operating system. Off makes `pgcolumnar.truncate()` a no-op. Requires superuser to set. | | `pgcolumnar.autovacuum` | boolean | `off` | Run the maintenance daemon. When on, it runs `compact_rewrite` and `recluster` on columnar tables that cross a threshold. It uses only `ShareUpdateExclusiveLock` and yields to any stronger lock. It never blocks a reader or a writer. See the [administration guide](administration.md#the-maintenance-daemon-pgcolumnarautovacuum). Reloadable, not a per-session setting. | diff --git a/src/columnar.h b/src/columnar.h index fac14eb1..50d36e07 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -525,6 +525,7 @@ extern int64 PgColumnarRetireFullyDeletedGroups(Relation rel); /* physical reclaim: split freed ranges on allocate and coalesce on free (GUC) */ extern bool pgcolumnar_reclaim_coalesce; +extern int pgcolumnar_index_min_blocks; /* physical end-truncation opt-in (GUC) */ extern bool pgcolumnar_enable_end_truncation; diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index dd1bcb91..2a1a1007 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -26,6 +26,7 @@ #include "commands/defrem.h" #include "commands/sequence.h" #include "miscadmin.h" +#include "storage/bufmgr.h" #include "storage/lock.h" #include "storage/procarray.h" #include "utils/array.h" @@ -147,6 +148,7 @@ static Oid pgcolumnar_schema_oid(void); static Relation open_columnar_table(const char *name, LOCKMODE lockmode); static Oid pgcolumnar_index_oid(const char *name); +static Oid pgcolumnar_scan_index_oid(Relation rel, const char *name); /* * pgcolumnar_schema_oid @@ -385,6 +387,76 @@ pgcolumnar_index_oid(const char *name) return get_relname_relid(name, pgcolumnar_schema_oid()); } +/* + * pgcolumnar_index_min_blocks + * Heap pages at or above which pgcolumnar_scan_index_oid() prefers an + * index probe to a sequential scan. Zero probes always; a very large + * value never probes. + * + * DERIVED BY MEASUREMENT (#1213). Total buffers for `compact()` over 40 + * row groups with 20 retired, against every threshold from "always probe" + * to "never probe", on PG 15, 17 and 19, over two families of fixture: + * many small columnar tables (0 to 1000, catalogs 8 to 65 pages) and one + * deep one (catalogs 8 to 390 pages). 24 size points in all. + * + * Scored against the cheapest threshold at each point: + * + * threshold total cost above best worst point worse than main + * 0 1561 buffers 155 10 of 24 points + * 2 448 69 7 + * 3 19 4 3 + * 5 95 42 none + * 8..16 791..3862 162..716 none + * never 15477 7913 -- + * + * 3 costs least overall by a factor of five and never more than 4 buffers + * at any point. Its whole cost is one shape: a `zone_map` of exactly four + * pages, which is worth scanning and gets probed. THE OPTIMUM IS NOT THE + * SAME FOR EVERY CATALOG, because the hot ones are scanned more often per + * retired group: `row_group` pays for a probe at three pages (probing it + * there saves 42), `zone_map` not until five (probing it at four costs 4). + * One value for all six is a deliberate compromise, and the measurement + * above prices it at 19 buffers across 24 points. + * + * Re-derive rather than adjust. #1217 would cache the index Oid, which + * removes the fixed cost of a probe and so moves this number down. + */ +int pgcolumnar_index_min_blocks = 3; + +/* + * pgcolumnar_scan_index_oid + * The index to hand systable_beginscan for a keyed scan of `rel`, or + * InvalidOid to read the heap instead. + * + * An index probe is not unconditionally cheaper than a sequential scan. + * Reading a heap of n pages costs n buffer touches; probing costs the + * btree descent plus the heap fetch, plus the two catcache lookups + * pgcolumnar_index_oid() makes on every call. Below a few pages the + * sequential scan is the cheaper read. + * + * Both sides of that comparison are reachable in one installation, + * because these catalogs are shared by every columnar table in the + * database: `row_group` is one page where there is one table and 23 in a + * database holding two million rows of columnar data. A path that commits + * to one access method is therefore wrong at one end of that range + * whichever end it picks, and #1213 measured it wrong at both. + * + * So ask the relation. RelationGetNumberOfBlocks() answers from smgr's + * cached block count after the first call in a backend. The decision is + * per relation and per call, so a path touching six catalogs of different + * sizes picks for each rather than committing all six one way. + * + * pgcolumnar_index_min_blocks is derived by measurement; see its + * declaration above. + */ +static Oid +pgcolumnar_scan_index_oid(Relation rel, const char *name) +{ + if (RelationGetNumberOfBlocks(rel) < (BlockNumber) pgcolumnar_index_min_blocks) + return InvalidOid; + return pgcolumnar_index_oid(name); +} + /* * PgColumnarNextStorageId * Draw the next value from pgcolumnar.storageid_seq (spec 3, 7.6). @@ -461,6 +533,7 @@ List * PgColumnarComputeAllVisibleGroups(uint64 storageId, TransactionId oldestXmin) { Relation grel = open_columnar_table("row_group", AccessShareLock); + Oid rgIdx = pgcolumnar_scan_index_oid(grel, "row_group_pkey"); TupleDesc gtd = RelationGetDescr(grel); ScanKeyData gkey[1]; SysScanDesc gscan; @@ -485,7 +558,7 @@ PgColumnarComputeAllVisibleGroups(uint64 storageId, TransactionId oldestXmin) ScanKeyInit(&gkey[0], Anum_row_group_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - gscan = systable_beginscan(grel, InvalidOid, false, snap, 1, gkey); + gscan = systable_beginscan(grel, rgIdx, OidIsValid(rgIdx), snap, 1, gkey); while (HeapTupleIsValid(gtuple = systable_getnext(gscan))) { TransactionId xmin = HeapTupleHeaderGetXmin(gtuple->t_data); @@ -550,6 +623,7 @@ static List * PgColumnarComputeFullyDeletedGroups(uint64 storageId, TransactionId oldestXmin) { Relation grel = open_columnar_table("row_group", AccessShareLock); + Oid rgIdx = pgcolumnar_scan_index_oid(grel, "row_group_pkey"); TupleDesc gtd = RelationGetDescr(grel); Relation mrel = open_columnar_table("delete_vector", AccessShareLock); TupleDesc mtd = RelationGetDescr(mrel); @@ -564,7 +638,7 @@ PgColumnarComputeFullyDeletedGroups(uint64 storageId, TransactionId oldestXmin) ScanKeyInit(&gkey[0], Anum_row_group_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - gscan = systable_beginscan(grel, InvalidOid, false, snap, 1, gkey); + gscan = systable_beginscan(grel, rgIdx, OidIsValid(rgIdx), snap, 1, gkey); while (HeapTupleIsValid(gtuple = systable_getnext(gscan))) { TransactionId xmin = HeapTupleHeaderGetXmin(gtuple->t_data); @@ -627,12 +701,52 @@ PgColumnarComputeFullyDeletedGroups(uint64 storageId, TransactionId oldestXmin) return groups; } -/* delete every row of a metadata table matching (storageAttno, groupAttno) */ +/* + * delete_group_rows + * Delete every row of a metadata table matching (storageAttno, groupAttno), + * probing through indexName. + * + * ONE SOURCE LINE HERE IS FIVE SCAN SITES AT RUN TIME. The relation comes + * from a PARAMETER, and PgColumnarDeleteGroupMetadata calls this five + * times -- delete_vector, column_chunk, zone_map, bloom, row_group -- so + * the single systable_beginscan below ran once per catalog per retired + * group. With InvalidOid that was five sequential scans of five catalogs + * for every group a compaction retires, walking every OTHER columnar + * table's rows each time. + * + * IT WAS INVISIBLE TO BOTH AUDITS OF THESE SCANS (#1207). Each of them + * attributed a systable_beginscan to a catalog by reading the + * open_columnar_table("") that produced its relation handle, or by + * finding the nearest scan key. Neither can resolve a relation that + * arrives as an argument: at this call site the catalog has no name. What + * found it was a reconciliation rather than a better reading -- probing + * all 44 systable_beginscan sites in this file and requiring + * sum(probes that ran with InvalidOid) == sum(seq_scan over pgcolumnar), + * which failed at 41 counted against 22 probed. + * + * The caller passes the index name because this function cannot derive it: + * the pkey of the table named by tableName is what it needs, and only the + * caller knows which table that is. Every one of the five is a prefix + * match on the two keys below, so none of this needs a new index: + * + * delete_vector_pkey (storage_id, group_number) exact + * row_group_pkey (storage_id, group_number) exact + * column_chunk_pkey (storage_id, group_number, column_index) + * bloom_pkey (storage_id, group_number, column_index) + * zone_map_pkey (storage_id, group_number, column_index, ...) + * + * Deleting while scanning through the index is the pattern core uses for + * its own catalogs (see deleteDependencyRecordsFor and its siblings): the + * scan holds its position by tid, and CatalogTupleDelete updates the index + * entries behind it. + */ static void -delete_group_rows(const char *tableName, AttrNumber storageAttno, +delete_group_rows(const char *tableName, const char *indexName, + AttrNumber storageAttno, AttrNumber groupAttno, uint64 storageId, uint64 groupNumber) { Relation rel = open_columnar_table(tableName, RowExclusiveLock); + Oid idx = pgcolumnar_scan_index_oid(rel, indexName); ScanKeyData key[2]; SysScanDesc scan; HeapTuple tuple; @@ -642,7 +756,7 @@ delete_group_rows(const char *tableName, AttrNumber storageAttno, ScanKeyInit(&key[1], groupAttno, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) groupNumber)); - scan = systable_beginscan(rel, InvalidOid, false, NULL, 2, key); + scan = systable_beginscan(rel, idx, OidIsValid(idx), NULL, 2, key); while (HeapTupleIsValid(tuple = systable_getnext(scan))) CatalogTupleDelete(rel, &tuple->t_self); systable_endscan(scan); @@ -657,6 +771,7 @@ read_row_group_range(uint64 storageId, uint64 groupNumber, { Relation rel = open_columnar_table("row_group", AccessShareLock); TupleDesc td = RelationGetDescr(rel); + Oid rgIdx = pgcolumnar_scan_index_oid(rel, "row_group_pkey"); ScanKeyData key[2]; SysScanDesc scan; HeapTuple tuple; @@ -668,7 +783,12 @@ read_row_group_range(uint64 storageId, uint64 groupNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); ScanKeyInit(&key[1], Anum_row_group_group_number, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) groupNumber)); - scan = systable_beginscan(rel, InvalidOid, false, snap, 2, key); + /* + * row_group_pkey IS (storage_id, group_number), so these two keys are the + * whole index. One more sequential scan per retired group, beside the five + * delete_group_rows makes (#1207). + */ + scan = systable_beginscan(rel, rgIdx, OidIsValid(rgIdx), snap, 2, key); if (HeapTupleIsValid(tuple = systable_getnext(scan))) { bool isnull; @@ -742,9 +862,32 @@ record_free_space(uint64 storageId, uint64 fileOffset, uint64 byteLength) ItemPointerData mergeTids[2]; int nMerge = 0; + Oid fsIdx = pgcolumnar_scan_index_oid(rel, "free_space_pkey"); + ScanKeyInit(&key[0], Anum_free_space_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - scan = systable_beginscan(rel, InvalidOid, false, snap, 1, key); + /* + * storage_id leads free_space_pkey (storage_id, file_offset), so the + * one key here is a prefix probe. The loop is order-independent: it + * tests adjacency against the ORIGINAL range bounds, not against the + * accumulating ones, and takes Min() of the offsets, so reading the + * rows in index order rather than heap order cannot change which + * neighbours merge (#1207). + * + * WHAT THAT ARGUMENT RESTS ON, said out loud because it is not + * self-evident (@OffgridwithJD, #1213 review). Order-independence needs + * `nMerge < 2` below to be a cap that never binds, and it never binds + * only because there is AT MOST one left neighbour and one right + * neighbour. With three matches `len` would absorb all three while only + * two rows were deleted, leaving a double-counted extent. The invariant + * that makes three impossible -- file_offset unique, no overlap -- is + * enforced by PgColumnarCheckFreeSpaceNoOverlap, which is ASSERT-ONLY. + * So the cap's safety and that checker are one argument, not two + * independent ones, and a release build carries the invariant without + * checking it. The invariant does hold; it is the independence that + * does not. + */ + scan = systable_beginscan(rel, fsIdx, OidIsValid(fsIdx), snap, 1, key); while (HeapTupleIsValid(tuple = systable_getnext(scan))) { bool isnull; @@ -839,15 +982,15 @@ PgColumnarRetireGroup(uint64 storageId, uint64 groupNumber) bool haveRange = read_row_group_range(storageId, groupNumber, &fileOffset, &byteLength); - delete_group_rows("delete_vector", Anum_delete_vector_storage_id, + delete_group_rows("delete_vector", "delete_vector_pkey", Anum_delete_vector_storage_id, Anum_delete_vector_group_number, storageId, groupNumber); - delete_group_rows("column_chunk", Anum_column_chunk_storage_id, + delete_group_rows("column_chunk", "column_chunk_pkey", Anum_column_chunk_storage_id, Anum_column_chunk_group_number, storageId, groupNumber); - delete_group_rows("zone_map", Anum_zone_map_storage_id, + delete_group_rows("zone_map", "zone_map_pkey", Anum_zone_map_storage_id, Anum_zone_map_group_number, storageId, groupNumber); - delete_group_rows("bloom", Anum_bloom_storage_id, + delete_group_rows("bloom", "bloom_pkey", Anum_bloom_storage_id, Anum_bloom_group_number, storageId, groupNumber); - delete_group_rows("row_group", Anum_row_group_storage_id, + delete_group_rows("row_group", "row_group_pkey", Anum_row_group_storage_id, Anum_row_group_group_number, storageId, groupNumber); /* @@ -995,6 +1138,8 @@ PgColumnarCheckFreeSpaceNoOverlap(uint64 storageId) { Relation rg; Relation fs; + Oid rgIdx; + Oid fsIdx; TupleDesc rgd; TupleDesc fsd; Snapshot snap; @@ -1011,13 +1156,23 @@ PgColumnarCheckFreeSpaceNoOverlap(uint64 storageId) snap = RegisterSnapshot(GetLatestSnapshot()); rg = open_columnar_table("row_group", AccessShareLock); fs = open_columnar_table("free_space", AccessShareLock); + rgIdx = pgcolumnar_scan_index_oid(rg, "row_group_pkey"); + fsIdx = pgcolumnar_scan_index_oid(fs, "free_space_pkey"); rgd = RelationGetDescr(rg); fsd = RelationGetDescr(fs); ranges = palloc(sizeof(ReclaimRange) * cap); ScanKeyInit(&key[0], Anum_row_group_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - scan = systable_beginscan(rg, InvalidOid, false, snap, 1, key); + /* + * ASSERT-ONLY, AND THAT IS EXACTLY WHY IT WAS THE LAST ONE FOUND. This + * check does not run in a release build, so a measurement taken on one + * reports zero sequential scans here while every assert-enabled CI leg pays + * two per maintenance operation. Both keys below lead their table's primary + * key, and the loops qsort what they collect, so index order changes + * nothing (#1207). + */ + scan = systable_beginscan(rg, rgIdx, OidIsValid(rgIdx), snap, 1, key); while (HeapTupleIsValid(t = systable_getnext(scan))) { bool isnull; @@ -1038,7 +1193,7 @@ PgColumnarCheckFreeSpaceNoOverlap(uint64 storageId) ScanKeyInit(&key[0], Anum_free_space_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) storageId)); - scan = systable_beginscan(fs, InvalidOid, false, snap, 1, key); + scan = systable_beginscan(fs, fsIdx, OidIsValid(fsIdx), snap, 1, key); while (HeapTupleIsValid(t = systable_getnext(scan))) { bool isnull; @@ -1265,6 +1420,7 @@ PgColumnarReconcileFreeList(Relation dataRel) int nf = 0; int capf = 64; Relation fsrel; + Oid fsIdx; TupleDesc td; List *storages = NIL; List *tids = NIL; @@ -1302,6 +1458,7 @@ PgColumnarReconcileFreeList(Relation dataRel) qsort(foots, nf, sizeof(FootRange), footrange_cmp); fsrel = open_columnar_table("free_space", RowExclusiveLock); + fsIdx = pgcolumnar_scan_index_oid(fsrel, "free_space_pkey"); td = RelationGetDescr(fsrel); foreach(lc, storages) { @@ -1312,7 +1469,13 @@ PgColumnarReconcileFreeList(Relation dataRel) ScanKeyInit(&key[0], Anum_free_space_storage_id, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) sid)); - scan = systable_beginscan(fsrel, InvalidOid, false, snap, 1, key); + /* + * storage_id leads free_space_pkey. The loop tests each row against the + * footprints independently and collects tids, deleting only after every + * scan has finished, so index order rather than heap order changes + * nothing about which rows are collected (#1207). + */ + scan = systable_beginscan(fsrel, fsIdx, OidIsValid(fsIdx), snap, 1, key); while (HeapTupleIsValid(tuple = systable_getnext(scan))) { bool isnull; diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 7366aeb6..8cd9a8f3 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -3534,6 +3534,20 @@ _PG_init(void) 0, NULL, NULL, NULL); + DefineCustomIntVariable("pgcolumnar.index_min_blocks", + "Heap pages at or above which a keyed scan of a " + "pgcolumnar metadata catalog probes that catalog's " + "index instead of reading it sequentially. Below a " + "few pages the sequential read is cheaper. 0 always " + "probes; a very large value never does.", + NULL, + &pgcolumnar_index_min_blocks, + 3, + 0, INT_MAX, + PGC_USERSET, + 0, + NULL, NULL, NULL); + DefineCustomBoolVariable("pgcolumnar.enable_end_truncation", "Allow pgcolumnar.truncate() to physically return " "trailing reclaimed blocks to the OS. Off (the default) " diff --git a/test/catalog_delete_index.sh b/test/catalog_delete_index.sh new file mode 100755 index 00000000..d17b2d7f --- /dev/null +++ b/test/catalog_delete_index.sh @@ -0,0 +1,473 @@ +#!/usr/bin/env bash +# +# Retiring a row group must not cost more because the database holds other +# columnar tables. +# +# THE BUG. delete_group_rows() opens its catalog from a `const char *tableName` +# PARAMETER and PgColumnarDeleteGroupMetadata calls it five times, for +# delete_vector, column_chunk, zone_map, bloom and row_group. One +# systable_beginscan in the source was therefore five sequential reads per +# retired group at run time, and two more sit beside it on the compaction path. +# The pgcolumnar metadata catalogs are SHARED by every columnar table in the +# database, so each of those reads was charged for every other table's rows. +# +# THE FIX IS NOT "USE THE INDEX". It is "ask the catalog how big it is, and use +# the index when that is the cheaper read". A probe is a btree descent plus a +# heap fetch plus two catcache lookups, which on a catalog of a few pages is +# MORE work than reading the whole thing; on a large one it is far less. Both +# sizes occur in one installation, because the catalogs are shared. So this +# suite has to show the choice being made well at BOTH ends, which is why it +# measures at three catalog sizes rather than one. +# +# WHAT IT ASSERTS, AND WHAT IT DELIBERATELY DOES NOT. It asserts the WORK -- +# buffers served out of the six catalogs, heap and index, from +# pg_statio_all_tables -- and never the access path. An earlier version of this +# file asserted `seq_scan = 0` and `idx_scan >= 1` on each catalog. That is a +# claim about WHICH PATH WAS TAKEN, and it is wrong twice over: +# +# 1. It cannot tell a revert from an improvement. Run against the build that +# chooses by catalog size -- cheaper than the always-probe version at every +# size measured -- those arms failed 13 of 21, the same 13 a full revert +# reddens. A guard that fires on correct code gets switched off, and the +# rule goes with it. +# 2. A scan count is not a cost. Counting scans is how this change came to +# report a saving it had measured in the wrong unit (#1213). +# +# NO CONSTANT ANYWHERE. Every buffer count is compared against another buffer +# count taken in the same run, on the same cluster, from the same build. +# `pgcolumnar.index_min_blocks` is the setting that decides the path -- 0 probes +# every catalog, a very large value reads every one whole -- so the same +# compaction is measured three ways and the three are compared to each other. +# This matters because the numbers are not the same on every major: the same +# fixture reads 848 buffers on PG17, 845 on PG15 and 895 on PG19. Any number +# typed into this file from a measurement someone once took would be wrong on +# two majors out of three. +# +# Usage: test/catalog_delete_index.sh [PG_CONFIG] + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +CATS="'delete_vector','column_chunk','zone_map','bloom','row_group','free_space'" + +# HOW FAR APART TWO READINGS MUST BE BEFORE THIS FILE CALLS THE DIFFERENCE A +# RESULT, in parts per thousand of the reading they are compared against. +# +# MEASURED, NOT CHOSEN, AND THE FIRST TWO ATTEMPTS AT IT WERE BOTH WRONG. +# +# Every claim compares two compactions of two different tables run one after +# another, and that is not a controlled comparison: compaction WRITES to +# row_group and free_space, so the next compaction reads more of them. Two +# readings taken from identical code drift apart. +# +# A floor of ONE BUFFER let a full revert through. Fifteen arms of sixteen +# passed against code with the fix removed, carried by 2 to 8 buffers of +# drift. +# +# A floor of TEN PARTS PER THOUSAND let it through too. Under a full revert +# the drift reached 14 to 16 parts per thousand -- above the floor -- and two +# arms of eighteen reddened. Worse, the two arms it was meant to protect were +# themselves worth only 22 and 26, so they sat inside the drift. They were not +# measuring the fix; they were measuring the sequence. +# +# The floor is now 100, and each claim is made at a catalog size where it is +# worth several times that. Measured, this build against the three mutations, +# in parts per thousand: +# +# arm real revert probe-always default replaced +# P1 (6 pages) 284 0 12 284 (passes) +# A1 (22 pages) 237 -1 -2 -1 +# B1 (76 pages) 1823 0 -1 0 +# growth (A to B) 960 1 22 1 +# vacuum 714 0 0 0 +# +# measured drift - 1-7 2-18 1-7 +# +# Every claim clears the floor by at least 2.4x; every mutation falls at least +# 4.5x below it. A full revert and a deleted size check each redden all five. +# Replacing the default with one that never probes reddens four: P1 passes +# there, correctly, because at six catalog pages declining every probe IS the +# cheaper read -- that build is wrong at the other end, and four arms say so. +# +# TWO ARMS WERE DELETED RATHER THAN RESCUED. "The default does less work than +# probing every catalog" was worth 22 parts per thousand at phase A and 26 at +# phase B -- inside the drift, so unmeasurable there. It survives as P1, at a +# catalog size where it is worth 284. What a fixture cannot measure, this file +# does not assert. +FLOOR_PERMILLE=100 + +q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null + +# WHICH BUILD KIND THIS RUN MEASURED, printed rather than asserted. +# +# Two of the eight converted scan sites are in PgColumnarCheckFreeSpaceNoOverlap, +# which is assert-only. On a release build they do not execute, so every arm +# below is a WEAKER claim there: it says nothing about those two sites rather +# than clearing them. A green on `debug_assertions = off` is not the same +# statement as a green on `on`. +# +# That distinction cost real time. The probe run that closed the account for +# #1207 was on a release build and reported the compaction path FULLY CLEAN, +# while the assert-enabled suite still showed one sequential scan on each of two +# catalogs. Nothing in the measurement said which build it was, so the zero read +# as an answer rather than as a partial one. Suggested by @OffgridwithJD. +# +# Printed and NOT made an arm on purpose: it records the condition the run +# happened in, and breaking the code under test cannot change it. +echo "-- debug_assertions=$(q "SHOW debug_assertions;") (off = the two assert-only sites did not run)" +echo "-- pgcolumnar.index_min_blocks=$(q "SHOW pgcolumnar.index_min_blocks;") (the shipped default this run measures)" + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +# make_target TABLE GROUPS -- GROUPS 1000-row groups, every other one emptied. +# +# Each measurement gets its own table. A compaction retires its groups once, so +# a second reading of the same table would measure a compaction that found +# nothing left to do -- which reports a small number for the same reason a fast +# one does. +make_target() { + q "CREATE TABLE $1 (id int, v int) USING pgcolumnar; + SELECT pgcolumnar.set_options('$1', stripe_row_limit => 1000); + INSERT INTO $1 SELECT g, g % 100 FROM generate_series(1,$(($2 * 1000))) g; + DELETE FROM $1 WHERE ((id - 1) / 1000) % 2 = 0;" >/dev/null +} + +groups_of() { + q "SELECT count(*) FROM pgcolumnar.row_group r + JOIN pgcolumnar.storage s USING (storage_id) + WHERE s.relation_oid = '$1'::regclass::oid;" +} + +catpages() { + q "SELECT coalesce(sum(pg_relation_size('pgcolumnar.' || relname) / 8192), 0) + FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'pgcolumnar' AND relname IN ($CATS);" +} + +# compact_work TABLE [INDEX_MIN_BLOCKS] -- buffers the six catalogs served +# while TABLE was compacted. Heap AND index blocks: counting only the heap +# would make a probe look free, which is the error this file exists to avoid. +# +# The SET and the compaction share one q, so they share a backend. The harness +# runs every other statement in a fresh backend, which flushes its statistics on +# exit, so the reset and the reading need no flush of their own. +compact_work() { + local set_clause="" + [ $# -ge 2 ] && set_clause="SET pgcolumnar.index_min_blocks = $2; " + q "SELECT pg_stat_reset();" >/dev/null + q "${set_clause}SELECT pgcolumnar.compact('$1');" >/dev/null + q "SELECT pg_stat_force_next_flush();" >/dev/null + q "SELECT coalesce(sum(heap_blks_read + heap_blks_hit + + coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0)), 0) + FROM pg_statio_all_tables + WHERE schemaname = 'pgcolumnar' AND relname IN ($CATS);" +} + +# cat_detail -- the per-catalog breakdown of the reading compact_work just took. +# Valid only until the next reset, so it is called immediately after. +# +# It is diagnosis, not decoration. It is what showed that under a full revert +# five of the six catalogs return identical numbers across every reading and the +# whole drift is row_group -- the one catalog the compaction writes to. +cat_detail() { + q "SELECT string_agg(relname || '=' || + (heap_blks_read + heap_blks_hit + + coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0))::text, + ' ' ORDER BY relname) + FROM pg_statio_all_tables + WHERE schemaname = 'pgcolumnar' AND relname IN ($CATS);" +} + +# permille PART WHOLE. A WHOLE of zero gives 0, which fails every arm rather +# than dividing by it. +permille() { [ "$2" -gt 0 ] && echo $(( $1 * 1000 / $2 )) || echo 0; } +abs() { [ "$1" -lt 0 ] && echo $(( -$1 )) || echo "$1"; } + +# margin MARGIN FLOOR -- MARGIN when it falls short of FLOOR, else FLOOR. An arm +# comparing this against FLOOR passes exactly when MARGIN >= FLOOR, and a +# failing one reports the margin it measured. A plain 1-or-0 arm would report +# `got [0] want [1]` whether the readings were four parts per thousand the wrong +# way or four hundred, and the size of the gap is most of the diagnosis. +margin() { [ "$1" -lt "$2" ] && echo "$1" || echo "$2"; } + +# THE CONTROL IS COMPACTED FIRST, NEXT TO THE DEFAULT, in every phase. +# +# Drift accumulates with distance, so a control three steps from the default +# measures three steps of it and condemns a claim exposed to one. It did: with +# the control last, phase B reported 32 parts per thousand of noise against a +# margin of 31. Adjacent, the same phase reports 2. +# +# NO SEPARATE NOISE TABLE. Each phase already holds three storages, so no arm +# can pass on a catalog that happens to hold only one. + +# phase PH PREFIX GROUPS OTHER OTHER_LABEL +# +# Measure one phase and set PH_DEFAULT, PH_OTHER and PH_PAGES for the caller. +# +# Three identical tables: a control and the measured one at the shipped default, +# then one at OTHER. Each measurement needs its own table because a compaction +# retires its groups once. +# +# THE CONTROL IS COMPACTED FIRST, NEXT TO THE DEFAULT. Drift accumulates with +# distance, so a control three steps from the default measures three steps of it +# and condemns a claim exposed to one. It did: with the control last, one phase +# reported 32 parts per thousand of noise against a margin of 31. Adjacent, the +# same phase reports 2. +# +# NO SEPARATE NOISE TABLE. Each phase already holds three storages, so no arm can +# pass on a catalog that happens to hold only one. +phase() { + local ph="$1" prefix="$2" groups="$3" other="$4" label="$5" carried="${6:-0}" + local retired=$(( groups - groups / 2 )) + local surviving=$(( (groups / 2) * 1000 )) + local t_control="${prefix}_control" t_default="${prefix}_default" t_other="${prefix}_other" + local control default other_work noise + + for t in "$t_control" "$t_default" "$t_other"; do make_target "$t" "$groups"; done + PH_PAGES="$(catpages)" + + check_num "premise: the three phase $ph targets are the same fixture" \ + "$(( $(groups_of "$t_control") + $(groups_of "$t_default") \ + + $(groups_of "$t_other") ))" "$(( groups * 3 ))" + + # THE PREMISE THAT CAUGHT A REAL FIXTURE DEFECT IN THE PORT. Phase 0's claim + # is about a catalog of a few pages, and a large one reports a SMALLER + # MARGIN rather than an error -- 65 parts per thousand instead of 223, which + # reads as a weak result and not as a broken fixture. Counting the columnar + # relations says which it is. + # + # This half gets a cluster of its own so it has always been true here. The + # pytest half gets a private SCHEMA and the pgcolumnar catalogs are per + # DATABASE, so run after the other fifty-one cluster files its phase 0 saw + # thirty-nine catalog pages instead of six and P1 fell under the floor. CI + # found it; a local run of one file could not. + check_num "premise: the phase $ph catalogs hold only this file's tables" \ + "$(( $(q "SELECT count(*) FROM pg_class c JOIN pg_am a ON a.oid = c.relam + WHERE a.amname = 'pgcolumnar';") - carried ))" "3" + + control="$(compact_work "$t_control")"; echo "-- control: $(cat_detail)" + default="$(compact_work "$t_default")"; echo "-- default: $(cat_detail)" + other_work="$(compact_work "$t_other" "$other")"; echo "-- $label: $(cat_detail)" + noise="$(permille "$(abs $((control - default)))" "$default")" + + echo "-- phase $ph catalog pages=$PH_PAGES work: control=$control default=$default $label=$other_work" + echo "-- phase $ph permille vs the default: $label=$(permille "$((other_work - default))" "$default") noise=$noise" + + # DERIVED FROM `groups`, NOT ASSUMED EVEN. make_target empties every other + # group, so an odd count retires the larger half. + check_num "premise: the phase $ph compaction retired the emptied groups" \ + "$(( groups - $(groups_of "$t_default") ))" "$retired" + check_num "premise: the phase $ph compaction kept every surviving row" \ + "$(q "SELECT count(*) FROM $t_default;")" "$surviving" + + # A catalog missing from the reading would read as a catalog nothing touched, + # which is the answer these arms are looking for. + check_num "premise: the phase $ph reading covers every catalog the arms name" \ + "$(q "SELECT count(*) FROM pg_statio_all_tables + WHERE schemaname = 'pgcolumnar' AND relname IN ($CATS);")" "6" + check_num "premise: the phase $ph reading measured something" \ + "$(margin "$default" 1)" "1" + + # THE ARMS ARE ONLY AS GOOD AS THIS ONE. A third identical table is compacted + # at the SAME setting as the measured one, so the two readings differ only by + # where they sit in the sequence. If that difference ever approaches the floor + # the claims are asserted against, the claims stop meaning anything -- and this + # says so instead of letting them pass on it. + check_num "premise: two phase $ph compactions at the same setting agree well inside the floor" \ + "$(margin "$((FLOOR_PERMILLE - noise))" 1)" "1" + + PH_DEFAULT="$default" + PH_OTHER="$other_work" +} + +# --------------------------------------------------------------------------- +# Phase 0: a few catalog pages, where reading one whole is the cheaper path +# +# Ten-group tables, and small on purpose. The claim here -- that probing every +# catalog costs more than choosing -- is worth 284 parts per thousand at six +# catalog pages, 151 at nine and 31 at eighteen, against a drift of 3 to 10 +# throughout. It is a real effect that a bigger fixture hides. +# --------------------------------------------------------------------------- + +phase 0 del_0 10 0 "probe-always" +w0_default="$PH_DEFAULT"; w0_probe="$PH_OTHER" + +check_num "P1 with a few catalog pages the default does less work than probing every one" \ + "$(margin "$(permille "$((w0_probe - w0_default))" "$w0_default")" $FLOOR_PERMILLE)" \ + "$FLOOR_PERMILLE" + +# --------------------------------------------------------------------------- +# Phase A: more catalog pages, where probing has started to pay +# --------------------------------------------------------------------------- + +phase A del_a 40 2147483647 "read-whole" 3 +wa_default="$PH_DEFAULT"; wa_scan="$PH_OTHER"; pages_a="$PH_PAGES" + +check_num "A1 with more catalog pages the default does less work than reading every one whole" \ + "$(margin "$(permille "$((wa_scan - wa_default))" "$wa_default")" $FLOOR_PERMILLE)" \ + "$FLOOR_PERMILLE" + +# --------------------------------------------------------------------------- +# Phase B: large catalogs +# +# ONE DEEP TABLE, NOT MANY SHALLOW ONES. What makes a sequential read expensive +# is catalog PAGES, not how many tables share the catalogs. 200,000 rows at 1024 +# to a group put the six catalogs near eighty pages; reaching the same size with +# one-group tables took a thousand of them and most of a suite's runtime. +# --------------------------------------------------------------------------- + +q "CREATE TABLE del_deep (id int, a int, b int, c text) USING pgcolumnar; + SELECT pgcolumnar.set_options('del_deep', stripe_row_limit => 1024); + INSERT INTO del_deep SELECT g, g%7, g%13, 'x'||g FROM generate_series(1,200000) g;" >/dev/null + +pages_after_deep="$(catpages)" + +# THE PREMISE THIS EXPERIMENT NEEDS MOST. The growth arm compares two readings +# taken over catalogs that are supposed to differ in size. If the deep table +# never landed, both phases measure the same fixture and the arm passes while +# proving nothing -- and that is not hypothetical: the sweep that chose the +# shipped default first produced a clean table across seven database sizes in +# which the noise had been eaten by shell quoting. Every row was secretly the +# same database, and the only thing that said so was this quantity, flat at 8 +# pages where it should have reached 65. +phase B del_b 40 2147483647 "read-whole" 7 +wb_default="$PH_DEFAULT"; wb_scan="$PH_OTHER"; pages_b="$PH_PAGES" + +# THE PREMISE THIS EXPERIMENT NEEDS MOST. The growth arm compares two readings +# taken over catalogs that are supposed to differ in size. If the deep table +# never landed, both phases measure the same fixture and the arm passes while +# proving nothing -- and that is not hypothetical: the sweep that chose the +# shipped default first produced a clean table across seven database sizes in +# which the noise had been eaten by shell quoting. Every row was secretly the +# same database, and the only thing that said so was this quantity, flat at 8 +# pages where it should have reached 65. +# READ BEFORE PHASE B BUILDS ITS TARGETS, on purpose: this asks whether the +# DEEP TABLE grew the catalogs, and measuring after phase B's own three tables +# exist would fold their growth into the answer and make it true either way. +check_num "premise: the deep table grew the catalogs it is there to grow" \ + "$(margin "$((pages_after_deep - pages_a))" 1)" "1" + +check_num "B1 with large catalogs the default does far less work than reading every one whole" \ + "$(margin "$(permille "$((wb_scan - wb_default))" "$wb_default")" $FLOOR_PERMILLE)" \ + "$FLOOR_PERMILLE" + +# THE INVARIANT THE ISSUE IS ABOUT, written down as its own arm rather than left +# for a reader to compose out of A1 and B1. It is the sentence the bug report +# would use: retiring a group must not cost more because other tables exist. +growth_default=$((wb_default - wa_default)) +growth_scan=$((wb_scan - wa_scan)) +echo "-- growth from phase A to phase B: default=$growth_default read-whole=$growth_scan permille=$(permille "$((growth_scan - growth_default))" "$growth_scan")" + +check_num "the default's cost grows far less with the database than reading whole does" \ + "$(margin "$(permille "$((growth_scan - growth_default))" "$growth_scan")" $FLOOR_PERMILLE)" \ + "$FLOOR_PERMILLE" + +# --------------------------------------------------------------------------- +# The VACUUM path, which reaches a different row_group read. +# +# PgColumnarVMSetVisibleForRelation calls PgColumnarComputeAllVisibleGroups, and +# NOTHING ABOVE REACHES IT. An earlier draft converted that scan and proved +# nothing about it: probing every site during a run of the sections above showed +# PgColumnarComputeAllVisibleGroups never fired, so the change to it rode along +# on arms that could not fail if it were reverted. +# +# THIS ARM HAS NO POSITIONAL CONFOUND, unlike every arm above it, because a +# VACUUM is repeatable where a compaction is not: retiring a group happens once, +# so each compaction needs its own table, but the same table can be vacuumed +# three times. All three readings here come from ONE table, and the only thing +# that differs between them is the setting. +# +# AND IT IS VACUUMED SMALL, ON PURPOSE. The size check is worth the difference +# between reading row_group whole and fetching the vacuumed table's own rows +# from it, so the gap widens as the catalog grows and narrows as the VACUUMED +# table grows. An earlier draft vacuumed a forty-group table and the arm swung +# between 200 and 750 parts per thousand from run to run on a base of ten +# buffers. Two groups against a row_group of fourteen pages is the same claim +# with a base that can carry it. +# --------------------------------------------------------------------------- + +# ALTER DATABASE rather than SET, because VACUUM refuses to run inside a +# transaction block and `psql -c "SET ...; VACUUM ..."` is exactly that: one +# simple query, wrapped implicitly. Measured -- the first draft reported 0 +# buffers, which reads like a vacuum that touched nothing and was a vacuum that +# never ran: +# ERROR: VACUUM cannot run inside a transaction block +# The harness runs every statement in a fresh backend, so a database-level +# setting is how a GUC gets in front of one of them. +# +# Both catalogs come back from ONE reading, so the premise below cannot be +# describing a different moment from the arm it guards. +vac_work() { + if [ $# -ge 2 ]; then + q "ALTER DATABASE \"$PGC_DB\" SET pgcolumnar.index_min_blocks = $2;" >/dev/null + fi + q "SELECT pg_stat_reset();" >/dev/null + q "VACUUM $1;" >/dev/null + q "SELECT pg_stat_force_next_flush();" >/dev/null + q "SELECT coalesce(max(w) FILTER (WHERE relname = 'row_group'), 0)::text || ' ' || + coalesce(max(w) FILTER (WHERE relname = 'delete_vector'), 0)::text + FROM (SELECT relname, + heap_blks_read + heap_blks_hit + + coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0) AS w + FROM pg_statio_all_tables + WHERE schemaname = 'pgcolumnar') t;" + if [ $# -ge 2 ]; then + q "ALTER DATABASE \"$PGC_DB\" RESET pgcolumnar.index_min_blocks;" >/dev/null + fi +} + +q "CREATE TABLE del_vac (id int) USING pgcolumnar; + SELECT pgcolumnar.set_options('del_vac', stripe_row_limit => 1000); + INSERT INTO del_vac SELECT g FROM generate_series(1,2000) g; + DELETE FROM del_vac WHERE id % 3 = 0;" >/dev/null + +rg_pages="$(q "SELECT pg_relation_size('pgcolumnar.row_group') / 8192;")" +min_blocks="$(q "SHOW pgcolumnar.index_min_blocks;")" +echo "-- row_group pages=$rg_pages, threshold=$min_blocks" + +# THE PREMISE THE ARM BELOW CANNOT DO WITHOUT, and it is derived from the +# setting rather than typed. Below the threshold the default declines the probe +# and reads row_group whole -- which is what the other reading does too, so both +# come back equal and the arm reports 0. That reads as "the fix is gone" and +# means "the fixture is too small". The port failed exactly that way the moment +# its fixture was corrected to a private database. +check_num "premise: row_group is larger than the threshold, so the two paths differ" \ + "$(margin "$((rg_pages - min_blocks))" 1)" "1" + +v_d="$(vac_work del_vac)" +v_c="$(vac_work del_vac)" +v_s="$(vac_work del_vac 2147483647)" +v_default="${v_d%% *}"; v_dv="${v_d##* }" +v_control="${v_c%% *}" +v_scan="${v_s%% *}" +noise_v="$(permille "$(abs $((v_control - v_default)))" "$v_default")" +echo "-- VACUUM row_group work: default=$v_default control=$v_control read-whole=$v_scan permille=$(permille "$((v_scan - v_default))" "$v_default") noise=$noise_v (delete_vector at the default=$v_dv)" + +# THE PREMISE IS NOT THE CLAIM. It reads delete_vector -- a DIFFERENT catalog +# from the one the arm is about -- so it cannot be satisfied by whatever makes +# that arm pass. +# +# TWO MORE OBVIOUS PREMISES WERE MEASURED AND ARE BOTH THE WRONG QUANTITY: +# +# relallvisible stays 0 on this fixture however many times the table is +# vacuumed (measured at six consecutive vacuums), and 0 again on a table with +# no deletes at all. An arm resting on it would have refused a vacuum that HAD +# reached the visibility-map path. +# +# vacuum_count and last_vacuum in pg_stat_all_tables stay 0 and NULL for a +# columnar table, because this table access method's vacuum does not report +# through them. "The vacuum did not run" and "the counter cannot see this +# vacuum" are the same reading. +check_num "premise: the vacuum walked this table's groups" \ + "$(margin "$v_dv" 1)" "1" +check_num "premise: two vacuums of the same table at the same setting agree well inside the floor" \ + "$(margin "$((FLOOR_PERMILLE - noise_v))" 1)" "1" + +check_num "the vacuum's default does less row_group work than reading it whole" \ + "$(margin "$(permille "$((v_scan - v_default))" "$v_default")" $FLOOR_PERMILLE)" \ + "$FLOOR_PERMILLE" + +pgc_summary diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index bfd7e11e..d47661d1 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -20,6 +20,36 @@ capability_sweep capability_sweep premise: the shim makes pyarrow unimportable 1 capability_sweep capability_sweep premise: the sweep is not in the population it sweeps 15;16;17;18;19 2026-09-23 the self-exclusion removed from pgc_cap_population, so the sweep enters its own population capability_sweep capability_sweep premise: the two derivations of the population name the same suites 15;16;17;18;19 never - capability_sweep capability_sweep premise: while python itself still runs under the shim 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index A1 with more catalog pages the default does less work than reading every one whole 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid +catalog_delete_index catalog_delete_index B1 with large catalogs the default does far less work than reading every one whole 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid +catalog_delete_index catalog_delete_index P1 with a few catalog pages the default does less work than probing every one 15;16;17;18;19 2026-09-24 pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid +catalog_delete_index catalog_delete_index premise: row_group is larger than the threshold, so the two paths differ 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes +catalog_delete_index catalog_delete_index premise: the deep table grew the catalogs it is there to grow 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase 0 catalogs hold only this file's tables 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase 0 compaction kept every surviving row 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase 0 compaction retired the emptied groups 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase 0 reading covers every catalog the arms name 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase 0 reading measured something 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase A catalogs hold only this file's tables 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase A compaction kept every surviving row 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase A compaction retired the emptied groups 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase A reading covers every catalog the arms name 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase A reading measured something 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase B catalogs hold only this file's tables 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase B compaction kept every surviving row 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase B compaction retired the emptied groups 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase B reading covers every catalog the arms name 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the phase B reading measured something 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the three phase 0 targets are the same fixture 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the three phase A targets are the same fixture 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the three phase B targets are the same fixture 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: the vacuum walked this table's groups 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: two phase 0 compactions at the same setting agree well inside the floor 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: two phase A compactions at the same setting agree well inside the floor 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: two phase B compactions at the same setting agree well inside the floor 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index premise: two vacuums of the same table at the same setting agree well inside the floor 15;16;17;18;19 never - +catalog_delete_index catalog_delete_index the default's cost grows far less with the database than reading whole does 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid +catalog_delete_index catalog_delete_index the vacuum's default does less row_group work than reading it whole 15;16;17;18;19 2026-09-24 pgcolumnar_index_min_blocks default 3 -> 2147483647, so no site probes;pgcolumnar_scan_index_oid stops asking the relation its size, so every site probes;the eight converted scan sites back to InvalidOid catalog_plan_index catalog_plan_index a no-qual count did not sequentially scan pgcolumnar.storage 15;16;17;18;19 2026-09-23 both storage_id scans reverted to InvalidOid, so the catalog is swept sequentially on its own primary key catalog_plan_index catalog_plan_index planning a join probed pgcolumnar.storage through storage_pkey 15;16;17;18;19 2026-09-23 both storage_id scans reverted to InvalidOid, so the catalog is swept sequentially on its own primary key catalog_plan_index catalog_plan_index planning did not sequentially scan pgcolumnar.options 15;16;17;18;19 2026-09-22 InvalidOid on the planner options and projection scans diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index e2bd952e..4d564a26 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -777,4 +777,26 @@ suites_not_covered 249 # all, and the mid-run death became invisible too. # # suites_not_covered does NOT move: harness_selftest is already a covered suite. -checks_never_observed_red 1506 +# +# #1207 adds catalog_delete_index with 30 checks. 24 of them are premises and +# are `never`; the other 6 were observed red under three mutations of the code: +# +# the eight converted scan sites back to InvalidOid 5 arms +# the size check deleted, so every site probes 5 arms +# the default replaced by one that never probes 5 arms +# +# Five each, not the same five. The third leaves P1 green -- correctly, +# because at six catalog pages declining every probe IS the cheaper read -- +# and reddens instead the premise that row_group is bigger than the threshold, +# which a threshold of 2147483647 makes false. +# +# 1506 + 24 = 1530, and 1530 is also what counting field five over the file +# returns: 1662 rows, 132 not `never`, and 1662 == 1530 + 132 printed beside +# it rather than asserted. +# +# suites_not_covered does NOT move. catalog_delete_index is a newly REGISTERED +# suite that arrives WITH ledger rows, so registered and covered rise by one +# each. Counted on both trees rather than reasoned: registered == covered + +# not_covered holds on each, and the only name in the difference either way is +# catalog_delete_index. +checks_never_observed_red 1530 diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index dc382895..03d0cc8f 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -126,6 +126,7 @@ behaviour, the source of that number is named. - [78. test_ttl_expire.py: the one function that deletes rows, tested twice](#78-test_ttl_expirepy-the-one-function-that-deletes-rows-tested-twice) - [79. test_projection_scan_io.py: a covering projection is not priced from the base table's pages](#79-test_projection_scan_iopy-a-covering-projection-is-not-priced-from-the-base-tables-pages) - [80. test_catalog_plan_index.py: planning uses the options and projection indexes](#80-test_catalog_plan_indexpy-planning-uses-the-options-and-projection-indexes) +- [81. test_catalog_delete_index.py: retiring a row group costs no more for a bigger database](#81-test_catalog_delete_indexpy-retiring-a-row-group-costs-no-more-for-a-bigger-database) ## 1. How to read a test in here @@ -6182,3 +6183,42 @@ The measured statement runs on a second connection. The shell suite gets that by | test | what it holds | | --- | --- | | `test_catalog_plan_index` | the measured table's row count, that the filtered scan returned every row, and that `options` and `projection` were probed by index with `seq_scan` still 0 | + +## 81. test_catalog_delete_index.py: retiring a row group costs no more for a bigger database + +Port of `catalog_delete_index.sh`. `delete_group_rows()` opens its catalog from a `const char *tableName` PARAMETER and `PgColumnarDeleteGroupMetadata` calls it five times, so one `systable_beginscan` in the source was five sequential reads per retired group at run time. The `pgcolumnar` catalogs are shared by every columnar table in the database, so each read was charged for every other table's rows. + +THE FIX IS NOT "USE THE INDEX", so the test cannot be "was the index used". A probe is a btree descent plus a heap fetch plus two catcache lookups, which on a catalog of a few pages is MORE work than reading the whole thing and far less on a large one. Both sizes occur in one installation. So these arms measure at three catalog sizes and each carries only the claim its size can support. + +THEY ASSERT THE WORK AND NEVER THE ACCESS PATH: buffers out of `pg_statio_all_tables`, heap and index both. An earlier version asserted `seq_scan == 0` and `idx_scan >= 1` per catalog; run against the build that chooses by catalog size -- cheaper at every size measured -- it failed 13 of 21 arms, the same 13 a full revert reddens. A test that cannot tell a revert from an improvement fires on correct code, and a guard that fires on correct code gets switched off. + +NOT A CONSTANT ANYWHERE. Every buffer count is compared against another taken in the same run from the same build, because the numbers are not the same on every major: the same fixture reads 848 buffers on PG17, 845 on PG15 and 895 on PG19. `pgcolumnar.index_min_blocks` decides the path, so the same work is measured two or three ways and the readings are compared to each other. + +WHAT MAKES THE COMPARISON TRUSTWORTHY, AND THE TWO FLOORS THAT DID NOT. Compaction writes to `row_group` and `free_space`, so the next compaction reads more of them and two readings from identical code drift apart. A floor of one buffer let a full revert through: fifteen arms of sixteen passed against code with the fix removed. A floor of ten parts per thousand let it through too, because under a revert the drift reaches fourteen to sixteen -- and the two arms that floor was meant to protect were themselves worth only twenty-two and twenty-six, so they were measuring the sequence, not the fix. The floor is now 100 parts per thousand; each phase compacts a control table at the same setting as the measured one and refuses to report on top of the drift; and the control sits ADJACENT to the default, because drift accumulates with distance and a control three steps away once reported 32 against a margin of 31. Two arms were deleted rather than rescued. What a fixture cannot measure, this file does not assert. + +Measured, this build against three mutations, in parts per thousand: + +| arm | real | revert | probe-always | default replaced | +| --- | ---: | ---: | ---: | ---: | +| P1, 6 catalog pages | 284 | 0 | 12 | 284 (passes) | +| A1, 22 pages | 237 | -1 | -2 | -1 | +| B1, 76 pages | 1823 | 0 | -1 | 0 | +| growth, A to B | 960 | 1 | 22 | 1 | +| vacuum | 714 | 0 | 0 | 0 | + +The vacuum arm has no positional confound at all, because a VACUUM is repeatable where a compaction is not: all three of its readings come from one table and only the setting differs. It is vacuumed small on purpose -- the gap widens as the catalog grows and narrows as the vacuumed table grows, and an earlier draft that vacuumed a thirty-group table swung between 200 and 750 from run to run on a base of ten buffers. + +THE PORT RUNS ON A PRIVATE DATABASE, not the private schema `pgc_conn` gives every other test here. That trade -- isolation without an initdb per test -- is right almost everywhere and wrong for this file: the `pgcolumnar` catalogs are per DATABASE and shared by the whole session, and every claim here is about how big they are. Run alone, the file saw six catalog pages at phase 0 and P1 was worth 223 parts per thousand; run after the other fifty-one cluster files it saw thirty-nine and P1 was worth 65, under the floor. CI found that and a local run of one file could not. A premise now counts the columnar relations in each phase, so a shared database arrives as a named failure rather than as a weak number -- and on its first run that premise caught a different defect too, a table left behind by a refactor that nothing used. + +The two halves share no code and no fixture. The shell suite uses ten- and forty-group tables, retires every OTHER group and adds 200,000 rows of deep noise; this one uses twelve and forty-five, retires the LAST half, and adds 150,000. The shell suite sums the work in SQL; this one reads it per catalog and sums in Python, and prints the per-catalog breakdown for every setting -- which is how the drift was traced to `row_group` alone, the one catalog a compaction writes to. + +THE ONE THING THIS HALF HAS TO DO THAT THE SHELL HALF DOES NOT is flush the statistics before resetting them. This harness holds one connection for the whole file, so the writes leave pending statistics in the backend that `pg_stat_reset()` does not clear; they are flushed afterwards and land on top of the reading. Measured on an otherwise identical single-session fixture: `row_group idx=36 seq=15` without the flush, `idx=32 seq=0` with it. The fifteen were the test's own `DELETE`. The shell half runs every statement in a fresh backend, which flushes on exit, so it cannot reach this state. + +`pg_stat_reset()` is database-wide. The corpus runs serially within a worker. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_retiring_a_group_costs_no_more_for_a_bigger_database` | per phase: three identical targets, the emptied groups retired, every survivor kept, all six catalogs read, and two compactions at the same setting agreeing well inside the floor. Then that the default does less work than probing every catalog at six pages, less than reading every one whole at twenty-two and at seventy-six, and that its cost grows far less with the database than reading whole does | +| `test_vacuum_reads_less_of_row_group_than_reading_it_whole` | that the vacuum walked this table's groups, that two vacuums at the same setting agree, and that its `row_group` work is below what reading that catalog whole costs -- the `PgColumnarComputeAllVisibleGroups` read, which the compaction path never reaches | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 47df537e..632f703e 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -509,4 +509,21 @@ guard_tests 403 # main alone guard=403 cluster=476 (50 cluster files) # composed guard=403 cluster=477 (51 cluster files) # -cluster_tests 478 +# cluster_tests moved for #1207's new cluster file, test_catalog_delete_index.py, +# which holds two tests. RE-DERIVED BY COLLECTION against this branch's actual +# base, never by adding two to either side: this branch has carried 478, 479 and +# 480 against three different mains, and each described a tree that no longer +# exists. +# +# main 1bcfb92b guard=403 cluster=478 (51 cluster files) +# this tree guard=403 cluster=480 (52 cluster files) +# +# THE FIRST ATTEMPT AT THE MAIN FIGURE READ 480, AND IT WAS THE MEASUREMENT THAT +# WAS WRONG. It collected in a copy of this working tree with +# `git checkout origin/main -- .` applied, which restores the files main has and +# does not remove the ones it does not: the new test file was still sitting +# there, so "main" collected 52 files. `git checkout -f` plus `git clean -fd` +# gives 51 and 478, which is what main's own expected_tests.txt declares -- and +# agreeing with the committed number is how the corrected measurement was +# recognised as the right one. +cluster_tests 480 diff --git a/test/pytest/test_catalog_delete_index.py b/test/pytest/test_catalog_delete_index.py new file mode 100644 index 00000000..38179eb5 --- /dev/null +++ b/test/pytest/test_catalog_delete_index.py @@ -0,0 +1,551 @@ +"""Retiring a row group must not cost more because other columnar tables exist. + +`delete_group_rows()` opens its catalog from a `const char *tableName` +PARAMETER, and `PgColumnarDeleteGroupMetadata` calls it five times, for +`delete_vector`, `column_chunk`, `zone_map`, `bloom` and `row_group`. One +`systable_beginscan` in the source was therefore five sequential reads per +retired group at run time. The `pgcolumnar` metadata catalogs are SHARED by +every columnar table in the database, so each read was charged for every other +table's rows. + +THE FIX IS NOT "USE THE INDEX". It is "ask the catalog how big it is, and use +the index when that is the cheaper read". A probe is a btree descent plus a heap +fetch plus two catcache lookups, which on a catalog of a few pages is MORE work +than reading the whole thing, and far less on a large one. Both sizes occur in +one installation, because the catalogs are shared -- so these arms measure at +three catalog sizes rather than one, and the claim at each is the one that size +can carry. + +WHAT THEY ASSERT. The WORK: buffers served out of the six catalogs, heap and +index, from `pg_statio_all_tables`. Never the access path. An earlier version +asserted `seq_scan == 0` and `idx_scan >= 1` per catalog; run against the build +that chooses by catalog size -- cheaper at every size measured -- it failed 13 +of 21 arms, the same 13 a full revert reddens. A guard that fires on correct +code gets switched off. + +NO CONSTANT ANYWHERE. Every buffer count is compared against another taken in +the same run from the same build, because the numbers are not the same on every +major: the same fixture reads 848 buffers on PG17, 845 on PG15 and 895 on PG19. +`pgcolumnar.index_min_blocks` decides the path -- 0 probes every catalog, a very +large value reads every one whole -- so the same work is measured two or three +ways and the readings are compared to each other. + +Independent of `test/catalog_delete_index.sh`: different tables, different row +counts, different group sizes, different retention pattern (this half retires +the LAST half of its groups, the shell half retires every other one), a smaller +deep fixture, and this half reads the work per catalog and sums in Python where +the shell half sums in SQL. + +`pg_stat_reset()` is database-wide. Tests run serially within a worker, so +nothing else is counting during these tests, and this file must not be run +concurrently with another that reads statistics. +""" + +# Phase 0 is small ON PURPOSE. The claim it carries -- that probing every +# catalog costs more than choosing -- is worth 284 parts per thousand at six +# catalog pages, 151 at nine and 31 at eighteen, against a drift of 3 to 10 +# throughout. A bigger fixture hides a real effect. +TINY_GROUPS = 12 +BIG_GROUPS = 45 +GROUP = 1000 + +DEEP_ROWS = 150000 +DEEP_GROUP = 1024 + +# The vacuum test needs a BIGGER deep table than the compaction test does, and +# that is not padding. Its claim is the difference between reading `row_group` +# whole and fetching one small table's rows from it, so it needs `row_group` +# itself above the threshold. In a private database with only this test's +# tables in it, 150,000 rows leaves `row_group` at two pages -- below the +# setting -- and BOTH paths then read it whole and the arm reports 0. Measured: +# that is exactly how it failed once the fixture was corrected. +VAC_DEEP_ROWS = 600000 + +PROBE_ALWAYS = 0 +READ_WHOLE = 2147483647 + +# HOW FAR APART TWO READINGS MUST BE BEFORE THIS FILE CALLS THE DIFFERENCE A +# RESULT, in parts per thousand of the reading they are compared against. +# +# MEASURED, NOT CHOSEN, AND THE FIRST TWO ATTEMPTS WERE BOTH WRONG. Every claim +# compares two compactions of two different tables run one after another, and +# compaction WRITES to `row_group` and `free_space`, so the next compaction +# reads more of them. Two readings from identical code drift apart. +# +# A floor of ONE BUFFER let a full revert through: fifteen arms of sixteen +# passed against code with the fix removed, carried by 2 to 8 buffers. +# +# A floor of TEN PARTS PER THOUSAND let it through too. Under a full revert +# the drift reached 14 to 16 -- above the floor -- and two arms of eighteen +# reddened. Worse, the two arms it was meant to protect were worth only 22 and +# 26, so they sat inside the drift: they measured the sequence, not the fix. +# +# The floor is 100, and each claim is made where it is worth several times that. +# Measured, this build against the three mutations, in parts per thousand: +# +# arm real revert probe-always default replaced +# P1 (6 pages) 284 0 12 284 (passes) +# A1 (22 pages) 237 -1 -2 -1 +# B1 (76 pages) 1823 0 -1 0 +# growth (A to B) 960 1 22 1 +# vacuum 714 0 0 0 +# +# measured drift - 1-7 2-18 1-7 +# +# Every claim clears the floor by at least 2.4x; every mutation falls at least +# 4.5x below it. Two arms were DELETED rather than rescued; what a fixture +# cannot measure, this file does not assert. +FLOOR_PERMILLE = 100 + +import pytest + +import pgc_vacuity + +CATALOGS = ( + "bloom", + "column_chunk", + "delete_vector", + "free_space", + "row_group", + "zone_map", +) + + +@pytest.fixture +def pgc_own_db(pgc_cluster, request): + """A private DATABASE, not merely a private schema. + + `pgc_conn` gives every test its own schema, which is the right trade almost + everywhere here: isolation without paying an initdb per test. IT IS THE + WRONG ONE FOR THIS FILE. The `pgcolumnar` metadata catalogs are per + DATABASE and shared by every test in the session, and every claim below is + about how big those catalogs are. + + MEASURED, AND IT IS WHY CI FOUND THIS AND A LOCAL RUN COULD NOT. Run alone, + this file saw six catalog pages at phase 0 and P1 was worth 223 parts per + thousand. Run after the other fifty-one cluster files, it saw thirty-nine + pages and P1 was worth 65 -- under the floor. The arm was not wrong and the + code was not wrong; the fixture's assumption was, and it held only in the + one arrangement I had run. + + The extension is created on the raw connection before the wrapper goes on, + the way conftest creates its schema: that is this fixture's own DDL, not the + test's writes, and DDL carries no row count anyway. + """ + import psycopg # deferred: see the module docstring + + name = "pgc_own_" + "".join( + ch if ch.isalnum() else "_" for ch in request.node.name + )[:40] + + def admin(sql): + c = psycopg.connect(pgc_cluster.dsn(), autocommit=True) + try: + c.execute(sql) + finally: + c.close() + + admin(f'DROP DATABASE IF EXISTS "{name}"') + admin(f'CREATE DATABASE "{name}"') + conn = psycopg.connect(pgc_cluster.dsn(dbname=name), autocommit=True) + try: + conn.execute("CREATE EXTENSION pgcolumnar") + yield pgc_vacuity.watch_writes(conn, request.node.nodeid) + finally: + conn.close() + admin(f'DROP DATABASE IF EXISTS "{name}"') + + +def _columnar_relations(conn): + with conn.cursor() as cur: + cur.execute( + "SELECT count(*) FROM pg_class c JOIN pg_am a ON a.oid = c.relam " + "WHERE a.amname = 'pgcolumnar'" + ) + return cur.fetchone()[0] + + +def _permille(part, whole): + """`part` as thousandths of `whole`; 0 for a whole of 0, which fails.""" + return (part * 1000) // whole if whole > 0 else 0 + + +def _build_kind(conn): + """Which build kind this run measured, printed rather than asserted. + + Two of the eight converted scan sites are in + `PgColumnarCheckFreeSpaceNoOverlap`, which is assert-only. On a release + build they do not execute, so every arm here is a WEAKER claim there: it + says nothing about those two sites rather than clearing them. + + The probe run that closed the account for #1207 was on a release build and + reported the compaction path fully clean while the assert-enabled suite + still showed a scan on each of two catalogs. Nothing in the measurement + said which build it was, so the zero read as an answer rather than a + partial one. + + Printed and NOT made an arm: it records the condition the run happened in, + and breaking the code under test cannot change it. + """ + with conn.cursor() as cur: + cur.execute("SHOW debug_assertions") + assertions = cur.fetchone()[0] + cur.execute("SHOW pgcolumnar.index_min_blocks") + return assertions, cur.fetchone()[0] + + +def _work(conn): + """Buffers the six catalogs have served, heap AND index, per catalog. + + Index blocks are counted because the question is total work: a probe that + read only index pages would otherwise look free, which is the error this + file exists to avoid. + """ + with conn.cursor() as cur: + cur.execute( + "SELECT relname, " + " coalesce(heap_blks_read,0) + coalesce(heap_blks_hit,0) " + "+ coalesce(idx_blks_read,0) + coalesce(idx_blks_hit,0) " + "FROM pg_statio_all_tables " + "WHERE schemaname = 'pgcolumnar' AND relname = ANY(%s)", + (list(CATALOGS),), + ) + return {r[0]: int(r[1]) for r in cur.fetchall()} + + +def _catpages(conn): + with conn.cursor() as cur: + cur.execute( + "SELECT coalesce(sum(pg_relation_size('pgcolumnar.' || relname) / 8192), 0) " + "FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace " + "WHERE n.nspname = 'pgcolumnar' AND relname = ANY(%s)", + (list(CATALOGS),), + ) + return int(cur.fetchone()[0]) + + +def _groups_of(conn, table): + with conn.cursor() as cur: + cur.execute( + "SELECT count(*) FROM pgcolumnar.row_group r " + "JOIN pgcolumnar.storage s USING (storage_id) " + "WHERE s.relation_oid = %s::regclass::oid", + (table,), + ) + return cur.fetchone()[0] + + +def _make_target(conn, table, groups): + """A measurement table: `groups` groups, the last half emptied. + + One per measurement. A compaction retires its groups once, so a second + reading of the same table would measure a compaction that found nothing + left to do -- which reports a small number for the same reason a fast one + does. + """ + rows = groups * GROUP + with conn.cursor() as cur: + cur.execute(f"CREATE TABLE {table} (k bigint, tag int) USING pgcolumnar") + cur.execute( + "SELECT pgcolumnar.set_options(%s, stripe_row_limit => %s)", (table, GROUP) + ) + cur.execute( + f"INSERT INTO {table} SELECT g, g % 7 FROM generate_series(1,{rows}) g" + ) + cur.execute(f"DELETE FROM {table} WHERE k > {(groups // 2) * GROUP}") + + +def _compact_work(conn, table, min_blocks=None): + """Total catalog buffers the compaction of `table` cost, and the breakdown. + + FLUSH BEFORE THE RESET, NOT ONLY AFTER. This harness holds ONE connection + for the whole file, so the writes that built the fixture leave pending + statistics in this backend that `pg_stat_reset()` does not clear -- they + flush afterwards and land on top of the reading. The shell twin cannot + reach this state: it runs every statement in a fresh backend, which flushes + on exit before the next one starts. + + Measured, on an otherwise identical single-session fixture: + reset with pending stats row_group idx=36 seq=15 + flush BEFORE reset row_group idx=32 seq=0 + The fifteen were the test's own DELETE, one scan per retired group, + arriving after the counter had been zeroed. + """ + with conn.cursor() as cur: + cur.execute("SELECT pg_stat_force_next_flush()") + cur.execute("SELECT pg_stat_reset()") + if min_blocks is not None: + cur.execute(f"SET pgcolumnar.index_min_blocks = {min_blocks}") + cur.execute("SELECT pgcolumnar.compact(%s)", (table,)) + cur.execute("RESET pgcolumnar.index_min_blocks") + cur.execute("SELECT pg_stat_force_next_flush()") + per_catalog = _work(conn) + return sum(per_catalog.values()), per_catalog + + +def _phase(conn, expect, name, prefix, groups, other, other_label, carried=0): + """Measure a phase: a control and the default, then `other`. + + THE CONTROL IS COMPACTED FIRST, NEXT TO THE DEFAULT. Drift accumulates with + distance, so a control three steps from the default measures three steps of + it and condemns a claim exposed to one. It did: placed last, one phase + reported 32 parts per thousand of noise against a margin of 31. + + Returns (default, other, pages). + """ + tables = [f"{prefix}_control", f"{prefix}_default", f"{prefix}_other"] + for t in tables: + _make_target(conn, t, groups) + pages = _catpages(conn) + expect.num( + sum(_groups_of(conn, t) for t in tables), + groups * 3, + f"premise: the three phase {name} targets are the same fixture", + ) + # THE PREMISE THAT WOULD HAVE CAUGHT THE FIXTURE DEFECT. Phase 0's claim is + # about a catalog of a few pages, and it reports a smaller margin rather + # than an error when the catalogs are large -- 65 parts per thousand + # instead of 223, which reads as a weak result and not as a broken fixture. + # Counting the relations says which it is. + expect.num( + _columnar_relations(conn) - carried, + 3, + f"premise: the phase {name} catalogs hold only this file's tables", + ) + + control, _ = _compact_work(conn, tables[0]) + default, by_cat = _compact_work(conn, tables[1]) + other_work, other_cat = _compact_work(conn, tables[2], other) + noise = _permille(abs(control - default), default) + margin = _permille(other_work - default, default) + + print(f"-- phase {name} catalog pages={pages} work: control={control} " + f"default={default} {other_label}={other_work}") + print(f"-- phase {name} permille vs the default: {other_label}={margin} noise={noise}") + print(f"-- by catalog default={by_cat}") + print(f"-- by catalog {other_label}={other_cat}") + + # DERIVED FROM `groups`, NOT ASSUMED EVEN. _make_target empties everything + # past the halfway row, so an odd group count retires the larger half: + # at 45 groups it is 23 that go and 22 that stay, and a premise written as + # `groups // 2` fails on the fixture rather than on the code. + expect.num( + groups - _groups_of(conn, tables[1]), + groups - groups // 2, + f"premise: the phase {name} compaction retired the emptied groups", + ) + with conn.cursor() as cur: + cur.execute(f"SELECT count(*) FROM {tables[1]}") + expect.num( + cur.fetchone()[0], + (groups // 2) * GROUP, + f"premise: the phase {name} compaction kept every surviving row", + ) + expect.num( + len(by_cat), + len(CATALOGS), + f"premise: the phase {name} reading covers every catalog the arms name", + ) + expect.at_least( + default, 1, f"premise: the phase {name} reading measured something" + ) + # THE ARMS ARE ONLY AS GOOD AS THIS ONE. A third identical table is + # compacted at the SAME setting as the measured one, so the two readings + # differ only by where they sit in the sequence. If that ever approaches the + # floor the claims are asserted against, the claims stop meaning anything. + expect.at_least( + FLOOR_PERMILLE - noise, + 1, + f"premise: two phase {name} compactions at the same setting " + "agree well inside the floor", + ) + return default, other_work, pages, margin + + +def test_retiring_a_group_costs_no_more_for_a_bigger_database(pgc_own_db, expect): + conn = pgc_own_db + assertions, min_blocks = _build_kind(conn) + print(f"-- debug_assertions={assertions} " + "(off = the two assert-only sites did not run)") + print(f"-- pgcolumnar.index_min_blocks={min_blocks} " + "(the shipped default this run measures)") + + # NO SEPARATE NOISE TABLE. Each phase already holds three storages, so no + # arm can pass on a catalog that happens to hold only one. + _d0, _p0, _pages0, margin0 = _phase( + conn, expect, "0", "ret0", TINY_GROUPS, PROBE_ALWAYS, "probe-always" + ) + expect.at_least( + margin0, + FLOOR_PERMILLE, + "P1 with a few catalog pages the default does less work than probing every one", + ) + + da, _sa, pages_a, margin_a = _phase( + conn, expect, "A", "retA", BIG_GROUPS, READ_WHOLE, "read-whole", carried=3 + ) + expect.at_least( + margin_a, + FLOOR_PERMILLE, + "A1 with more catalog pages the default does less work than reading every one whole", + ) + sa = _sa + + # ONE DEEP TABLE, NOT MANY SHALLOW ONES. What makes a sequential read + # expensive is catalog PAGES, not how many tables share the catalogs; + # reaching this size with one-group tables took a thousand of them. + with conn.cursor() as cur: + cur.execute( + "CREATE TABLE retire_deep (k bigint, a int, b int, c text) USING pgcolumnar" + ) + cur.execute( + "SELECT pgcolumnar.set_options('retire_deep', stripe_row_limit => %s)", + (DEEP_GROUP,), + ) + cur.execute( + "INSERT INTO retire_deep " + f"SELECT g, g % 7, g % 13, 'x' || g FROM generate_series(1,{DEEP_ROWS}) g" + ) + + db, sb, pages_b, margin_b = _phase( + conn, expect, "B", "retB", BIG_GROUPS, READ_WHOLE, "read-whole", carried=7 + ) + + # THE PREMISE THIS EXPERIMENT NEEDS MOST. The growth arm compares two + # readings taken over catalogs that are supposed to differ in size. If the + # deep table never landed, both phases measure the same fixture and the arm + # passes while proving nothing -- and that is not hypothetical: the sweep + # that chose the shipped default first produced a clean table across seven + # database sizes in which the noise had been eaten by shell quoting. Every + # row was secretly the same database, and the only thing that said so was + # this quantity, flat where it should have grown eightfold. + expect.at_least( + pages_b - pages_a, + 1, + "premise: the deep table grew the catalogs it is there to grow", + ) + expect.at_least( + margin_b, + FLOOR_PERMILLE, + "B1 with large catalogs the default does far less work than reading every one whole", + ) + + # THE INVARIANT THE ISSUE IS ABOUT, written down as its own arm rather than + # left for a reader to compose out of A1 and B1. It is the sentence the bug + # report would use: retiring a group must not cost more because other tables + # exist. + growth_default = db - da + growth_scan = sb - sa + growth_margin = _permille(growth_scan - growth_default, growth_scan) + print(f"-- growth from phase A to phase B: default={growth_default} " + f"read-whole={growth_scan} permille={growth_margin}") + expect.at_least( + growth_margin, + FLOOR_PERMILLE, + "the default's cost grows far less with the database than reading whole does", + ) + + +def test_vacuum_reads_less_of_row_group_than_reading_it_whole(pgc_own_db, expect): + """The vacuum path reaches a row_group read the compaction path does not. + + `PgColumnarVMSetVisibleForRelation` calls + `PgColumnarComputeAllVisibleGroups`, and nothing in the test above reaches + it. Probing every scan site during a compaction shows that function never + fires, so without this test a change to it would ride along on arms that + could not fail if it were reverted. + + NO POSITIONAL CONFOUND, unlike every arm in the test above, because a VACUUM + is repeatable where a compaction is not. All three readings come from ONE + table and the only thing that differs is the setting. + + AND IT IS VACUUMED SMALL, ON PURPOSE. The size check is worth the difference + between reading `row_group` whole and fetching the vacuumed table's own rows + from it, so the gap widens as the catalog grows and narrows as the VACUUMED + table grows. An earlier draft vacuumed a thirty-group table and the arm swung + between 200 and 750 parts per thousand from run to run on a base of ten + buffers. + + The premise reads `delete_vector`, a DIFFERENT catalog from the one the arm + is about, so it cannot be satisfied by whatever makes it pass. + `relallvisible` is the obvious premise and is the wrong quantity: it stays 0 + on this fixture however many times the table is vacuumed. So do + `vacuum_count` and `last_vacuum`, which this table access method's vacuum + does not report through at all. + """ + conn = pgc_own_db + assertions, min_blocks = _build_kind(conn) + print(f"-- debug_assertions={assertions} index_min_blocks={min_blocks}") + + with conn.cursor() as cur: + cur.execute("CREATE TABLE vac_deep (k bigint, a int, b int, c text) USING pgcolumnar") + cur.execute( + "SELECT pgcolumnar.set_options('vac_deep', stripe_row_limit => %s)", + (DEEP_GROUP,), + ) + cur.execute( + "INSERT INTO vac_deep " + f"SELECT g, g % 7, g % 13, 'x' || g FROM generate_series(1,{VAC_DEEP_ROWS}) g" + ) + cur.execute("CREATE TABLE vac_small (k bigint) USING pgcolumnar") + cur.execute( + "SELECT pgcolumnar.set_options('vac_small', stripe_row_limit => %s)", (GROUP,) + ) + cur.execute("INSERT INTO vac_small SELECT g FROM generate_series(1,3000) g") + cur.execute("DELETE FROM vac_small WHERE k % 3 = 0") + + cur.execute("SELECT count(*) FROM vac_small") + expect.at_least(cur.fetchone()[0], 1, "premise: the vacuumed table holds rows") + cur.execute( + "SELECT pg_relation_size('pgcolumnar.row_group') / 8192" + ) + rg_pages = int(cur.fetchone()[0]) + print(f"-- row_group pages={rg_pages}, threshold={min_blocks}") + # THE PREMISE THE ARM BELOW CANNOT DO WITHOUT, and it is derived from the + # setting rather than typed. Below the threshold the default declines the + # probe and reads `row_group` whole -- which is what the other reading does + # too, so both come back equal and the arm reports 0. That reads as "the fix + # is gone" and means "the fixture is too small". + expect.at_least( + rg_pages - int(min_blocks), + 1, + "premise: row_group is larger than the threshold, so the two paths differ", + ) + + def vacuum_work(min_blocks=None): + with conn.cursor() as cur: + cur.execute("SELECT pg_stat_force_next_flush()") + cur.execute("SELECT pg_stat_reset()") + if min_blocks is not None: + cur.execute(f"SET pgcolumnar.index_min_blocks = {min_blocks}") + cur.execute("VACUUM vac_small") + cur.execute("RESET pgcolumnar.index_min_blocks") + cur.execute("SELECT pg_stat_force_next_flush()") + return _work(conn) + + w_default = vacuum_work() + w_control = vacuum_work() + w_scan = vacuum_work(READ_WHOLE) + v_default = w_default.get("row_group", 0) + v_control = w_control.get("row_group", 0) + v_scan = w_scan.get("row_group", 0) + noise = _permille(abs(v_control - v_default), v_default) + margin = _permille(v_scan - v_default, v_default) + print(f"-- VACUUM row_group work: default={v_default} control={v_control} " + f"read-whole={v_scan} permille={margin} noise={noise}") + + expect.at_least( + w_default.get("delete_vector", 0), + 1, + "premise: the vacuum walked this table's groups", + ) + expect.at_least( + FLOOR_PERMILLE - noise, + 1, + "premise: two vacuums of the same table at the same setting agree well inside the floor", + ) + expect.at_least( + margin, + FLOOR_PERMILLE, + "the vacuum's default does less row_group work than reading it whole", + ) diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 03317a4c..8297be0c 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -90,6 +90,7 @@ "analyze_function", "analyze_reltuples", "base_scan_io", + "catalog_delete_index", "catalog_plan_index", "differential", "encode_post_codec", diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 091137da..622ca60b 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -61,6 +61,7 @@ SUITES=( bloom_setting bloom_sizing cancel_decode + catalog_delete_index catalog_natts catalog_plan_index column_projection