From 150b7e9d9ef215f66e7111b2d901d9bc558bb036 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 22 Sep 2026 21:35:38 +0000 Subject: [PATCH 1/2] fix: the writing session scanned delete_vector sequentially, once per row group (#1146) `delete_vector` gained a `_pkey` and its readers were switched to it, but `PgColumnarUpsertDeleteVector` kept passing `InvalidOid`. It runs once per row group, so a DELETE touching twenty groups took twenty sequential scans of the catalog -- the cost the index switch existed to remove, still paid by the session that did the writing. ATTRIBUTED WITH AN ELOG PROBE AT EVERY delete_vector SCAN SITE, not inferred from the totals. During the DELETE, bracketed by two log marks: PgColumnarUpsertDeleteVector 20 PgColumnarReadDeleteVectorList 40 PgColumnarReadDeleteVectorsForStorage 1 against seq_scan=20 idx_scan=41 for the catalog. 40 + 1 are the indexed reads and the twenty are that one site, with nothing left over. Afterwards: before seq_scan=20 idx_scan=41 after seq_scan=0 idx_scan=61 41 + 20 = 61, so every scan that was sequential is now indexed and none was lost. THE REGRESSION ARM HAS TO BE IN THE PYTEST HARNESS. Every `q` in the shell harness is its own psql, so the scan it measures is always made by a session that did no writing; the writing-session path is unobservable there whatever the arm asserts. The new test holds one connection across the DELETE and reads the counters as a delta over it, and resets BEFORE the DELETE rather than after -- resetting afterwards is what attributed these scans to the following SELECT when they were first measured. The arm is red on the unfixed source for the intended reason (got 20 want 0) and green on the fixed one. Its zero is guarded: a premise asserts the DELETE wrote one delete_vector row per group, because a DELETE that reached no delete_vector row scans nothing and would report success over a fixture that never reached the path. TESTS.md said this file "asserts nothing about the writing session either way". That is no longer true, so it is corrected here rather than left for a reader. SnapshotSelf is unchanged: the upsert has to see its own uncommitted row, and systable_beginscan applies the same snapshot to an index scan. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP --- CHANGELOG.md | 35 +++++++++++ src/columnar_metadata.c | 20 ++++++- test/pytest/TESTS.md | 13 ++++- test/pytest/expected_tests.txt | 2 +- .../pytest/test_native_delete_vector_index.py | 58 +++++++++++++++++++ 5 files changed, 123 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3aa59aa7..93841bcb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,41 @@ true until the next version shipped. ### Fixed +- The session that deleted rows still scanned `delete_vector` sequentially, once per row + group (#1146). + + `delete_vector` gained a `_pkey` and its readers were switched to it, but + `PgColumnarUpsertDeleteVector` kept passing `InvalidOid`. It runs once per row group, so + a DELETE touching twenty groups took twenty sequential scans of the catalog -- the cost + the index switch existed to remove, still paid by the writer. + + Attributed with an elog probe at every `delete_vector` scan site rather than inferred + from the totals: + + | site | calls during the DELETE | + | --- | ---: | + | `PgColumnarUpsertDeleteVector` | 20 | + | `PgColumnarReadDeleteVectorList` | 40 | + | `PgColumnarReadDeleteVectorsForStorage` | 1 | + + against `seq_scan=20 idx_scan=41` for the catalog, so 40 + 1 are the indexed reads and + the twenty are that one site. Afterwards: + + | | seq_scan | idx_scan | + | --- | ---: | ---: | + | before | 20 | 41 | + | after | 0 | 61 | + + 41 + 20 = 61, so every scan that was sequential is now indexed and none was lost. + + **The regression arm has to be in the pytest harness.** Every `q` in the shell harness + is its own `psql`, so the scan it measures is always made by a session that did no + writing, and the writing-session path is unobservable there whatever the arm asserts. + The new test holds one connection across the DELETE and reads the counters as a delta + over it. + +### Fixed + - `docs/limitations.md` told a user that a range column has a collation, and it does not (#1144 review). diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index a18baea4..efe0daa7 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -1670,7 +1670,25 @@ PgColumnarUpsertDeleteVector(uint64 storageId, DeleteVectorMetadata *rm) ScanKeyInit(&key[1], Anum_delete_vector_group_number, BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum((int64) rm->groupNumber)); - scan = systable_beginscan(rel, InvalidOid, false, SnapshotSelf, 2, key); + /* + * THE INDEX, like every sibling catalog read. This ran once per row + * group with InvalidOid, so a DELETE touching twenty groups took twenty + * sequential scans of delete_vector -- the cost the index switch existed + * to remove, still being paid by the session that did the writing. + * + * Attributed with an elog probe at every delete_vector scan site (#1146): + * 20 here, 40 at ReadDeleteVectorList, 1 at ReadDeleteVectorsForStorage, + * against seq_scan=20 idx_scan=41. Afterwards seq_scan=0 idx_scan=61. + * + * SnapshotSelf is unchanged: the upsert has to see its own uncommitted + * row, and systable_beginscan applies the same snapshot to an index scan. + */ + { + Oid dvIdx = pgcolumnar_index_oid("delete_vector_pkey"); + + scan = systable_beginscan(rel, dvIdx, OidIsValid(dvIdx), + SnapshotSelf, 2, key); + } if (HeapTupleIsValid(existing = systable_getnext(scan))) { bool isnull; diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 0fd4e56a..4fa193f3 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -5125,15 +5125,22 @@ always made by a session that did not write. Measured on this fixture: | the session that wrote | 62 | **20** | | a fresh session | 21 | 0 | -Twenty sequential catalog scans, one per row group, survive in the writing session. This -file asserts the property the suite states, on a fresh connection, and asserts nothing -about the writing session either way; the observation is filed as #1146. +Twenty sequential catalog scans, one per row group, survived in the writing session, and +#1146 fixed that. `PgColumnarUpsertDeleteVector` ran once per row group passing +`InvalidOid`; attributed with an elog probe at every `delete_vector` scan site, 20 were +that call site against `seq_scan=20 idx_scan=41`, and afterwards `seq_scan=0 idx_scan=61`. + +This file now asserts BOTH sessions. The second test holds the connection across the +DELETE and reads the counters as a delta over it, which is the only way the writing-session +path is observable at all: every `q` in the shell harness is its own psql, so the scan it +measures is always made by a session that did no writing. ### Every arm | test | what it holds | | --- | --- | | `test_native_delete_vector_index` | every arm: the fixture's rows and row groups, one delete_vector row per group scoped to this storage, that the cache-building scan ran and was correct, that it used the index and did not sequentially scan, and that the deletes are still applied | +| `test_the_writing_session_does_not_sequentially_scan_the_delete_vector` | the session that DID the deleting: the fixture's row groups, that the DELETE wrote one delete_vector row per group so the zero is not vacuous, that it reached the catalog at all, and that it took no sequential scan | ## 65. test_native_delete_visibility_paths.py: a deleted row is invisible on every path diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index cb0f9c39..5dd97f69 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -479,4 +479,4 @@ guard_tests 402 # #1189). Re-derived by collection on this tree, never by adding one: # `464 tests collected`. `guard_tests` was re-derived in the same run and # did NOT move: 393. -cluster_tests 466 +cluster_tests 467 diff --git a/test/pytest/test_native_delete_vector_index.py b/test/pytest/test_native_delete_vector_index.py index 40d4fe01..d54d2817 100644 --- a/test/pytest/test_native_delete_vector_index.py +++ b/test/pytest/test_native_delete_vector_index.py @@ -115,3 +115,61 @@ def test_native_delete_vector_index(pgc_cluster, pgc_conn, expect): expect.num(_one(pgc_conn, "SELECT sum(v) FROM t"), LIVE_SUM, "the deletes are still applied (visibility unchanged by the index switch)") + + +def test_the_writing_session_does_not_sequentially_scan_the_delete_vector( + pgc_cluster, pgc_conn, expect): + """The session that DID the deleting must not sequentially scan either (#1146). + + The arm above resets the counters after the write and then reads, so the scan it + measures is always made by a session that did no writing. That is not a flaw in its + logic; it is the only thing the shell harness can observe, because every `q` there is + its own psql. Holding one connection across the DELETE and the read is what makes the + writing-session path visible at all. + + `PgColumnarUpsertDeleteVector` ran once per row group with `InvalidOid`, so a DELETE + touching twenty groups took twenty sequential scans of the catalog. Attributed with an + elog probe at every `delete_vector` scan site: 20 at that call site, 40 at + `ReadDeleteVectorList` and 1 at `ReadDeleteVectorsForStorage`, against + `seq_scan=20 idx_scan=41` -- so the twenty were that site and nothing else. + + RESET BEFORE THE DELETE, NOT AFTER. The counters are read as a delta across the + DELETE alone; resetting afterwards is what attributed these scans to the following + SELECT the first time they were measured. + """ + n, groups = 40000, 20 + with pgc_conn.cursor() as cur: + cur.execute("CREATE TABLE dvw (id int, v int) USING pgcolumnar") + cur.execute("SELECT pgcolumnar.set_options('dvw', stripe_row_limit => 2000," + " chunk_group_row_limit => 2000)") + cur.execute(f"INSERT INTO dvw SELECT g, g FROM generate_series(1, {n}) g") + cur.execute("SELECT count(*) FROM pgcolumnar.storage s" + " JOIN pgcolumnar.row_group rg USING (storage_id)" + " WHERE s.relation_oid = 'dvw'::regclass") + expect.num(cur.fetchone()[0], groups, + "premise: the fixture is laid out in the expected row groups") + + # The delta is across the DELETE and nothing else. + cur.execute("SELECT pg_stat_reset()") + cur.execute("DELETE FROM dvw WHERE id % 500 = 0") + cur.execute("SELECT pg_stat_force_next_flush()") + + # WITHOUT THIS THE ZERO IS VACUOUS. A DELETE that wrote no delete_vector row + # scans nothing, and seq_scan = 0 would report success for a fixture that never + # reached the path. + cur.execute("SELECT count(*) FROM pgcolumnar.delete_vector dv" + " JOIN pgcolumnar.storage s USING (storage_id)" + " WHERE s.relation_oid = 'dvw'::regclass") + expect.num(cur.fetchone()[0], groups, + "premise: the delete wrote one delete_vector row per group") + + cur.execute("SELECT coalesce(seq_scan, 0), coalesce(idx_scan, 0)" + " FROM pg_stat_all_tables WHERE relname = 'delete_vector'" + " AND schemaname = 'pgcolumnar'") + seq, idx = cur.fetchone() + print(f"-- delete_vector across the DELETE: seq_scan={seq} idx_scan={idx}") + + expect.at_least(idx, 1, + "premise: the writing session did reach the catalog at all") + expect.num(seq, 0, + "the writing session did NOT sequentially scan the catalog (seq_scan = 0)") From 826feefaa505661ee0f4fb31ebbe2aa17464e38c Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 22 Sep 2026 21:43:11 +0000 Subject: [PATCH 2/2] test: the zero-guard scanned the counter it guards (#1146 review) The premise that stops `seq_scan = 0` being vacuous counts `delete_vector` rows, and any read of `delete_vector` scans it -- the plan is Aggregate -> Hash Join -> Seq Scan on delete_vector. It ran BETWEEN `pg_stat_force_next_flush()` and the read of `pg_stat_all_tables`, so its own sequential scan was still pending when the counters were read. The arm passed on an accounting delay rather than on a property of the code. Found by @jdatcmd. Forcing a second flush after the premise: -- delete_vector across the DELETE: seq_scan=1 idx_scan=61 with `idx_scan` unchanged at 61, so nothing about the product fix moved; only the premise own scan became visible. NOT A FLAKE, WHICH IS WHY THE FIX IS ORDERING RATHER THAN A RETRY. A `time.sleep(2)` between the premise and the read still passed, and so did an intervening `SELECT 1`; only an explicit flush makes the pending scan visible. Two mechanism stories were refuted before the third discriminated, and both refutations are in the comment because they are what rule out a retry. The premise cannot be rewritten out of the problem -- any read of `delete_vector` scans it -- so it moves out of the window instead. The counters are read first, and the premise is asserted afterwards: it is a statement about the DELETE effect on the catalog, which does not expire. Sensitivity is unchanged, re-proved with the reordered arm: fix reverted seq_scan=20 idx_scan=41 got 20 want 0 fix restored 11 checks, 0 fail Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP --- .../pytest/test_native_delete_vector_index.py | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/test/pytest/test_native_delete_vector_index.py b/test/pytest/test_native_delete_vector_index.py index d54d2817..8895884d 100644 --- a/test/pytest/test_native_delete_vector_index.py +++ b/test/pytest/test_native_delete_vector_index.py @@ -154,19 +154,34 @@ def test_the_writing_session_does_not_sequentially_scan_the_delete_vector( cur.execute("DELETE FROM dvw WHERE id % 500 = 0") cur.execute("SELECT pg_stat_force_next_flush()") + # THE COUNTERS FIRST, BEFORE ANYTHING ELSE READS THE CATALOG. The zero-guard + # below counts delete_vector rows, and ANY read of delete_vector scans it -- + # the plan is Aggregate -> Hash Join -> Seq Scan on delete_vector. Asking it + # between the flush and this read leaves its own sequential scan pending, so + # the arm passes on an accounting delay rather than on a property of the code. + # Found by @jdatcmd: forcing a second flush after the premise gives + # seq_scan=1 idx_scan=61, with idx_scan unchanged, so the 1 is the premise's + # own scan and nothing about the fix moved. + # + # NOT A FLAKE, WHICH IS WHY ORDERING IS THE FIX RATHER THAN A RETRY. A + # time.sleep(2) between the premise and this read still passed, and so did an + # intervening SELECT 1; only an explicit flush makes the pending scan visible. + # The premise cannot be rewritten out of the problem -- any read of + # delete_vector scans it -- so it moves out of the window instead. + cur.execute("SELECT coalesce(seq_scan, 0), coalesce(idx_scan, 0)" + " FROM pg_stat_all_tables WHERE relname = 'delete_vector'" + " AND schemaname = 'pgcolumnar'") + seq, idx = cur.fetchone() + # WITHOUT THIS THE ZERO IS VACUOUS. A DELETE that wrote no delete_vector row # scans nothing, and seq_scan = 0 would report success for a fixture that never - # reached the path. + # reached the path. Asserted AFTER the counters are read: it is a statement + # about the DELETE's effect on the catalog, which does not expire. cur.execute("SELECT count(*) FROM pgcolumnar.delete_vector dv" " JOIN pgcolumnar.storage s USING (storage_id)" " WHERE s.relation_oid = 'dvw'::regclass") expect.num(cur.fetchone()[0], groups, "premise: the delete wrote one delete_vector row per group") - - cur.execute("SELECT coalesce(seq_scan, 0), coalesce(idx_scan, 0)" - " FROM pg_stat_all_tables WHERE relname = 'delete_vector'" - " AND schemaname = 'pgcolumnar'") - seq, idx = cur.fetchone() print(f"-- delete_vector across the DELETE: seq_scan={seq} idx_scan={idx}") expect.at_least(idx, 1,