From 235aa69985016c27c18147109fb21e1d066b3802 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 14 Oct 2026 17:57:21 +0000 Subject: [PATCH] fix: a DELETE whose qual deletes another row of the same table no longer fails (#1394) A DELETE whose qual called a function that deleted another row of the same columnar table failed with "attempted to update invisible tuple"; heap deletes both. The inner DELETE's scan start writes the outer statement's buffered delete marks, and its statement end writes its own mark to the same row group, both in one command. The second upsert found the delete_vector row the first had written in that command, and heap_update refuses a version written by the current command. PgColumnarUpsertDeleteVector now advances the command counter before updating a row version the current command wrote. Red first: native_delete_visibility_paths gains the arm in both twins, columnar against heap (main: 55000|20|210 against heap's ok|1|1). The suite enters the ledger with its eight checks, the new one dated. cluster_tests 541 -> 542. Fixes #1394. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019VvWuX1zUwk6Y513waRd4N --- src/columnar_metadata.c | 14 ++++++++++ test/check_ledger.tsv | 8 ++++++ test/check_ledger_budget.txt | 16 +++++++++-- test/native_delete_visibility_paths.sh | 23 +++++++++++++++ test/pytest/TESTS.md | 1 + test/pytest/expected_tests.txt | 4 ++- .../test_native_delete_visibility_paths.py | 28 +++++++++++++++++++ 7 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/columnar_metadata.c b/src/columnar_metadata.c index 21efd3add..6c3263945 100644 --- a/src/columnar_metadata.c +++ b/src/columnar_metadata.c @@ -1854,6 +1854,7 @@ PgColumnarUpsertDeleteVector(uint64 storageId, DeleteVectorMetadata *rm) int deletedCount = rm->deletedCount; ItemPointerData replaceTid; bool haveReplace = false; + bool writtenThisCommand = false; delete_vector_lock_chunk_group(storageId, rm->groupNumber, 0); @@ -1945,6 +1946,17 @@ PgColumnarUpsertDeleteVector(uint64 storageId, DeleteVectorMetadata *rm) replaceTid = existing->t_self; haveReplace = true; + + /* + * TWO WRITES OF ONE GROUP IN ONE COMMAND (#1394). A statement that starts a + * scan inside another -- a DELETE in a function the outer DELETE's qual calls + * -- writes the outer marks at its scan start and its own at its end, in the + * same command. heap_update refuses a version written by the current command + * ("attempted to update invisible tuple"), so advance the command first. + */ + writtenThisCommand = + TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(existing->t_data)) && + HeapTupleHeaderGetCmin(existing->t_data) >= GetCurrentCommandId(false); } systable_endscan(scan); @@ -1962,6 +1974,8 @@ PgColumnarUpsertDeleteVector(uint64 storageId, DeleteVectorMetadata *rm) if (haveReplace) { + if (writtenThisCommand) + CommandCounterIncrement(); tuple->t_self = replaceTid; CatalogTupleUpdate(rel, &replaceTid, tuple); } diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 79611db67..6c59a597d 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1887,6 +1887,14 @@ native_chunk_length_bound native_chunk_length_bound backend survived the sequent native_chunk_length_bound native_chunk_length_bound backend survived the truncated-length fetch 15;16;17;18;19 never - native_chunk_length_bound native_chunk_length_bound premise: a point lookup uses the index, not a sequential columnar scan 15;16;17;18;19 never - native_chunk_length_bound native_chunk_length_bound premise: that fetch returns the row 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths a DELETE whose qual deletes another row of the table succeeds and leaves heap's rows (#1394) 15;16;17;18;19 2026-10-14 PgColumnarUpsertDeleteVector without the command advance (main before #1394's fix) +native_delete_visibility_paths native_delete_visibility_paths a deleted row is invisible via the index 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths a live row is visible via the index 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths a row deleted in-transaction is invisible in the same transaction 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths aggregate over the whole table sees the live set 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths index / bitmap scan sees the live set 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths index-only scan sees the live set 15;16;17;18;19 never - +native_delete_visibility_paths native_delete_visibility_paths sequential scan sees the live set (row-emitting path) 15;16;17;18;19 never - native_fetch_coalesce native_fetch_coalesce a wide index fetch does not pin once per column 15;16;17;18;19 2026-10-13 the coalesced fetch merging no adjacent ranges, so each column is read on its own native_fetch_coalesce native_fetch_coalesce premise: a point lookup uses the index 15;16;17;18;19 never - native_fetch_coalesce native_fetch_coalesce premise: fetching every projected column touched a measurable number of buffers 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index a9d17b994..c726b914c 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -15,7 +15,7 @@ # # Adding a check to a suite that is already covered does not move it, which is # what makes it safe to bound. -suites_not_covered 243 +suites_not_covered 242 # # checks_never_observed_red -- A CENSUS. NOT a ceiling, and it must not become # one. @@ -1979,4 +1979,16 @@ suites_not_covered 243 # never 1895 + 23 = 1918 # dated 577 + 2 = 579 # 1918 + 579 == 2497 -checks_never_observed_red 1918 +# +# #1394, a DELETE whose qual deletes another row of the same table (jdatcmd): 1918 -> +# 1925, and suites_not_covered 243 -> 242. native_delete_visibility_paths had no rows; +# it enters with all eight checks from five-major runs (8/8 on each of 15-19). The new +# arm is dated by main's upsert, which failed with "attempted to update invisible +# tuple" (55000). The other seven arrive never: seeding a suite moves the census up by +# its undated rows. +# +# ledger rows 2497 + 8 = 2505 +# never 1918 + 7 = 1925 +# dated 579 + 1 = 580 +# 1925 + 580 == 2505 +checks_never_observed_red 1925 diff --git a/test/native_delete_visibility_paths.sh b/test/native_delete_visibility_paths.sh index 170b422cc..6cddc1c45 100755 --- a/test/native_delete_visibility_paths.sh +++ b/test/native_delete_visibility_paths.sh @@ -58,4 +58,27 @@ check "a live row is visible via the index" \ check "a row deleted in-transaction is invisible in the same transaction" \ "$(q 'BEGIN; DELETE FROM t WHERE id = 8; SELECT count(*) FROM t WHERE id = 8; ROLLBACK;' | grep -E '^[0-9]+$' | head -1)" "0" +# A DELETE whose qual deletes another row of the same table (#1394). The inner DELETE's +# scan start writes the outer statement's buffered marks, and its statement end writes +# its own mark to the same row group, both in one command. The second write updated a +# delete_vector row the first had written in that command, and failed with "attempted +# to update invisible tuple". Run on heap too; the columnar answer must be heap's. +psql_run "CREATE FUNCTION del_next(i int, t text) RETURNS int LANGUAGE plpgsql AS \$\$ +BEGIN EXECUTE format('DELETE FROM %s WHERE id = %s', t, i + 1); RETURN 1; END \$\$;" +for am in pgcolumnar heap; do + psql_run "CREATE TABLE dn_$am (id int, v int) USING $am; + INSERT INTO dn_$am SELECT g, 0 FROM generate_series(1, 20) g;" +done +dn() { # dn AM -> ok, or the SQLSTATE the DELETE fails with + local out + out=$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ + -X -At -v ON_ERROR_STOP=1 -v VERBOSITY=sqlstate \ + -c "DELETE FROM dn_$1 WHERE id % 2 = 0 AND del_next(id, 'dn_$1') IS NOT NULL" 2>&1 >/dev/null) + [ -z "$out" ] && { echo ok; return; } + echo "$out" | sed -n 's/^ERROR: *\([0-9A-Z]\{5\}\).*/\1/p' | head -1 +} +dn_after() { q "SELECT count(*) || '|' || coalesce(sum(id), 0) FROM dn_$1"; } +check "a DELETE whose qual deletes another row of the table succeeds and leaves heap's rows (#1394)" \ + "$(dn pgcolumnar)|$(dn_after pgcolumnar)" "$(dn heap)|$(dn_after heap)" + pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index fca414a22..98acbe2b3 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -5250,6 +5250,7 @@ its row in place, so the in-transaction arm cannot be satisfied by a delete that | test | what it holds | | --- | --- | | `test_native_delete_visibility_paths` | every arm: the row count and group span, then the live set through a Seq Scan, an Index Scan, an Index Only Scan and the columnar aggregate — each with its plan asserted — then one deleted and one live row through the index, and a delete read back inside its own transaction | +| `test_a_delete_whose_qual_deletes_another_row_matches_heap` | a DELETE whose qual calls a function that deletes the next row of the same table succeeds and leaves heap's rows (#1394). Its inner DELETE writes the outer marks at scan start and its own at statement end, in one command, and the second write failed with "attempted to update invisible tuple" | ## 66. test_temporal.py: a columnar table enforces temporal constraints like a heap diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 85e77a583..6bc6c4a00 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -642,4 +642,6 @@ guard_tests 433 # cluster_tests 539 -> 541 for #1298's test_a_catalog_reading_takes_in_none_of_another_sessions_reads # in test_catalog_delete_index.py and test_a_storage_reading_takes_in_none_of_another_sessions_reads # in test_catalog_plan_index.py, by collection over the cluster files on this tree. -cluster_tests 541 +# cluster_tests 541 -> 542 for #1394's test_a_delete_whose_qual_deletes_another_row_matches_heap +# in test_native_delete_visibility_paths.py, by collection over the cluster files on this tree. +cluster_tests 542 diff --git a/test/pytest/test_native_delete_visibility_paths.py b/test/pytest/test_native_delete_visibility_paths.py index 81d9c2bba..b01072285 100644 --- a/test/pytest/test_native_delete_visibility_paths.py +++ b/test/pytest/test_native_delete_visibility_paths.py @@ -36,6 +36,8 @@ the count. The shell suite is unchanged -- reported rather than edited, since this pull request adds no bash check. """ +import psycopg + ROWS = 5000 GROUP = 1000 LIVE = ROWS - ROWS // 7 # 5000 - 714 @@ -154,3 +156,29 @@ def test_native_delete_visibility_paths(pgc_conn, expect): cur.execute("SELECT count(*) FROM t WHERE id = 8") expect.num(cur.fetchone()[0], 1, "premise: the rolled-back delete left the row in place") + + +def test_a_delete_whose_qual_deletes_another_row_matches_heap(pgc_conn, expect): + """A DELETE whose qual deletes another row of the same table (#1394). The inner + DELETE's scan start writes the outer statement's buffered marks, and its statement + end writes its own mark to the same row group, both in one command. The second + write updated a delete_vector row the first had written in that command, and + failed with "attempted to update invisible tuple". Run on heap too; the columnar + answer must be heap's.""" + with pgc_conn.cursor() as cur: + cur.execute("""CREATE FUNCTION del_next(i int, t text) RETURNS int LANGUAGE plpgsql AS $$ +BEGIN EXECUTE format('DELETE FROM %s WHERE id = %s', t, i + 1); RETURN 1; END $$""") + got = {} + for am in ("pgcolumnar", "heap"): + with pgc_conn.cursor() as cur: + cur.execute(f"CREATE TABLE dn_{am} (id int, v int) USING {am}") + cur.execute(f"INSERT INTO dn_{am} SELECT g, 0 FROM generate_series(1, 20) g") + try: + cur.execute(f"DELETE FROM dn_{am} WHERE id % 2 = 0 AND del_next(id, 'dn_{am}') IS NOT NULL") + state = "ok" + except psycopg.Error as e: + state = e.sqlstate or "?" + cur.execute(f"SELECT count(*) || '|' || coalesce(sum(id), 0) FROM dn_{am}") + got[am] = f"{state}|{cur.fetchone()[0]}" + expect.text(got["pgcolumnar"], got["heap"], + "a DELETE whose qual deletes another row of the table succeeds and leaves heap's rows (#1394)")