fix: the writing session scanned delete_vector sequentially, once per row group (#1146)

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).
 

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;

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
 

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

test/pytest/test_native_delete_vector_index.py

@@ -115,3 +115,76 @@ 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()")
+
+        # 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. 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")
+    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)")