diff --git a/CHANGELOG.md b/CHANGELOG.md index f4e171c8..bb6f2186 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -339,6 +339,13 @@ true until the next version shipped. `pgc_on_exit`; its cleanup now calls `pgc_on_exit` first. Selftest part 620 fails any suite that calls `pgc_setup` and arms an EXIT trap reaching neither `pgc_on_exit` nor `pgc_teardown`. Reported by @OffgridwithJD. +- `COMMIT PREPARED` (or `ROLLBACK PREPARED`) in the same session that prepared a + pgcolumnar write no longer crashes the backend (#1323). PREPARE frees the + transaction's memory without reaching the code that forgets the pending write + states and delete vectors, so the session's next transaction walked freed + lists. They are now let go of at PREPARE. Committing from another session, + which is what `parallel_copy` does, was never affected. `test/prepared_writes.sh` + and an independent `test_prepared_writes.py`. Reported by @OffgridwithJD. - An `ALTER TABLE` on a partitioned columnar table that does not recurse no longer opens the other partitions without a lock. `DETACH PARTITION`, diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 54df9b2a..b90e90cc 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -2669,6 +2669,20 @@ pgcolumnar_xact_callback(XactEvent event, void *arg) PgColumnarFlushAllPendingWrites(); PgColumnarFlushAllDeleteVectors(); + /* + * PREPARE frees TopTransactionContext and never reaches the + * COMMIT/ABORT arms below, so the pending write states and delete + * vectors -- children of that context, now flushed -- must be let go + * of here. Without this the session's next transaction walked the + * freed lists at its PRE_COMMIT, and a COMMIT PREPARED in the same + * session died of SIGSEGV (#1323). + */ + if (event == XACT_EVENT_PREPARE) + { + PgColumnarDiscardAllPendingWrites(); + PgColumnarDiscardAllDeleteVectors(); + } + /* * PREPARE deletes TopTransactionContext without ever reaching the * COMMIT/ABORT arms below, so a memo slot populated outside any diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 53fe01b4..cf690552 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -1846,6 +1846,17 @@ partman_columnar partman_columnar premise: the rows went in 15;16;17;18;19 2026- partman_columnar partman_columnar premise: with default_table_access_method every partition pg_partman makes is columnar 15;16;17;18;19 never - partman_columnar partman_columnar premise: without default_table_access_method pg_partman makes heap partitions 15;16;17;18;19 never - partman_columnar partman_columnar retention leaves no pgcolumnar storage row behind 15;16;17;18;19 2026-10-12 the drop hook stops deleting the storage row, so a partition pg_partman drops leaves it behind +prepared_writes prepared_writes a delete prepared and committed in one session leaves the session answering 15;16;17;18;19 2026-10-13 PREPARE letting go of neither (main before #1323);PREPARE without letting go of the delete-vector buffers +prepared_writes prepared_writes an index fetch before PREPARE, and the same row fetched first after COMMIT PREPARED, in one session 15;16;17;18;19 never - +prepared_writes prepared_writes an insert prepared and committed in one session leaves the session answering 15;16;17;18;19 2026-10-13 PREPARE letting go of neither (main before #1323);PREPARE without letting go of the pending write states +prepared_writes prepared_writes an insert prepared and rolled back in one session leaves the session answering 15;16;17;18;19 2026-10-13 PREPARE letting go of neither (main before #1323);PREPARE without letting go of the pending write states +prepared_writes prepared_writes an update prepared and committed in one session leaves the session answering 15;16;17;18;19 2026-10-13 PREPARE letting go of neither (main before #1323);PREPARE without letting go of the delete-vector buffers;PREPARE without letting go of the pending write states +prepared_writes prepared_writes and no prepared transaction is left behind 15;16;17;18;19 never - +prepared_writes prepared_writes and the session's next write, after COMMIT PREPARED, lands 15;16;17;18;19 2026-10-13 PREPARE letting go of neither (main before #1323);PREPARE without letting go of the pending write states +prepared_writes prepared_writes no backend was terminated by a signal 15;16;17;18;19 2026-10-13 PREPARE letting go of neither (main before #1323);PREPARE without letting go of the delete-vector buffers;PREPARE without letting go of the pending write states +prepared_writes prepared_writes premise: prepared transactions are enabled 15;16;17;18;19 never - +prepared_writes prepared_writes premise: the fetch is an index scan, which is what reaches the fetch cache 15;16;17;18;19 never - +prepared_writes prepared_writes premise: the server log is where a crash would be recorded 15;16;17;18;19 never - projection_parallel projection_parallel a covering projection can be a parallel scan 15;16;17;18;19 2026-09-18 - projection_parallel projection_parallel a page cost that makes I/O dominate costs the parallel covering path 15;16;17;18;19 2026-09-23 the partial covering total amortises I/O as well as CPU: (ioRunProj + cpuRunProj) / divisor projection_parallel projection_parallel a parallel covering projection returns the covering rows once 15;16;17;18;19 2026-10-12 a parallel worker never wires the shared row-group counter, so every worker scans every group diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index 1d76df24..9a35a789 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -1476,4 +1476,21 @@ suites_not_covered 247 # never 1701 + 33 = 1734 # dated 373 + 38 = 411 # 1734 + 411 == 2145 -checks_never_observed_red 1734 +# +# #1323, a write prepared and finished in one session (OffgridwithJD): 1734 -> +# 1739. prepared_writes enters the ledger with 11 checks from a five-major run +# (11/11 on each of 15-19). Six are dated by three mutations of the fix (each +# discard removed, and both, which is main); no premise is among them -- the +# index-scan premise read 0 under a mutation until it waited for the server the +# previous arm had crashed, and the run was redone. Five arrive `never`: three +# premises (prepared transactions on, the server log present, the plan an index +# scan) and two arms -- the index fetch across COMMIT PREPARED, which holds +# without any discard (one at PREPARE changed no arm, so the fix has none), and +# no prepared transaction left behind, which a crash during COMMIT PREPARED +# does not leave either. +# +# ledger rows 2145 + 11 = 2156 +# never 1734 + 5 = 1739 +# dated 411 + 6 = 417 +# 1739 + 417 == 2156 +checks_never_observed_red 1739 diff --git a/test/prepared_writes.sh b/test/prepared_writes.sh new file mode 100755 index 00000000..4a20de8a --- /dev/null +++ b/test/prepared_writes.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# +# A pgcolumnar write prepared and then finished in the SAME session (#1323). +# +# PREPARE deletes TopTransactionContext without ever reaching the COMMIT/ABORT +# arms of the transaction callback, and the pending write states and the +# delete-vector buffers live in children of it. Their statics were reset only by +# those arms, so the session's next transaction -- the COMMIT PREPARED itself -- +# walked a freed list at its PRE_COMMIT and the backend died of SIGSEGV. +# COMMIT PREPARED from another session never touched them, which is the path +# parallel_copy takes, so nothing in the corpus had reached it. +# +# Each arm runs its statements in ONE psql session (several -c, each its own +# transaction), and asserts what the session returns. A crash shows twice: the +# session's last answer is missing, and the server log records a backend +# terminated by a signal. Not pg_postmaster_start_time: after a backend crash the +# postmaster reinitializes without restarting, and the time does not move -- the +# first version of this suite asserted it, and it passed on a run that crashed. +# Each arm waits for the server to accept connections first, so one crash is +# reported by its own arm and not by every arm after it. +# +# Usage: test/prepared_writes.sh [PG_CONFIG] + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +export PGC_EXTRA_CONF='max_prepared_transactions=4' +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +q "CREATE EXTENSION IF NOT EXISTS pgcolumnar;" >/dev/null +check "premise: prepared transactions are enabled" "$(q "SHOW max_prepared_transactions;")" "4" +check "premise: the server log is where a crash would be recorded" \ + "$([ -f "$PGC_LOGFILE" ] && echo present || echo "absent ($PGC_LOGFILE)")" "present" + +ready() { # wait, up to 30 s, until the server answers a query + local i + for i in $(seq 1 60); do + [ "$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \ + -X -At -c "SELECT 1" 2>/dev/null)" = "1" ] && return + sleep 0.5 + done +} + +session() { # session SQL... -> the last line the one session printed + local args=() + ready + for s in "$@"; do args+=(-c "$s"); done + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -X -At \ + "${args[@]}" 2>&1 | tail -1 +} + +q "CREATE TABLE pw (id int, v text) USING pgcolumnar; + INSERT INTO pw SELECT g, 'base' FROM generate_series(1, 100) g;" >/dev/null + +check "an insert prepared and committed in one session leaves the session answering" \ + "$(session "BEGIN" "INSERT INTO pw SELECT g, 'two-phase' FROM generate_series(101, 3100) g" \ + "PREPARE TRANSACTION 'pw_ins'" "COMMIT PREPARED 'pw_ins'" "SELECT count(*) FROM pw")" "3100" + +check "a delete prepared and committed in one session leaves the session answering" \ + "$(session "BEGIN" "DELETE FROM pw WHERE id <= 50" \ + "PREPARE TRANSACTION 'pw_del'" "COMMIT PREPARED 'pw_del'" "SELECT count(*) FROM pw")" "3050" + +check "an update prepared and committed in one session leaves the session answering" \ + "$(session "BEGIN" "UPDATE pw SET v = 'changed' WHERE id BETWEEN 51 AND 150" \ + "PREPARE TRANSACTION 'pw_upd'" "COMMIT PREPARED 'pw_upd'" \ + "SELECT count(*) FILTER (WHERE v = 'changed') FROM pw")" "100" + +check "an insert prepared and rolled back in one session leaves the session answering" \ + "$(session "BEGIN" "INSERT INTO pw SELECT g, 'gone' FROM generate_series(1, 2000) g" \ + "PREPARE TRANSACTION 'pw_rb'" "ROLLBACK PREPARED 'pw_rb'" "SELECT count(*) FROM pw WHERE v = 'gone'")" "0" + +check "and the session's next write, after COMMIT PREPARED, lands" \ + "$(session "BEGIN" "INSERT INTO pw SELECT g, 'later' FROM generate_series(1, 10) g" \ + "PREPARE TRANSACTION 'pw_next'" "COMMIT PREPARED 'pw_next'" \ + "INSERT INTO pw SELECT g, 'after' FROM generate_series(1, 7) g" \ + "SELECT count(*) FROM pw WHERE v = 'after'")" "7" + +# The index-fetch cache is the third per-transaction cache COMMIT and ABORT reset. +# An index fetch on each side of COMMIT PREPARED, in one session, same rows: it +# works without a discard at PREPARE. Measured, not assumed -- a version of the +# fix that also discarded it at PREPARE changed no arm here, so it is not in the +# fix. The premise keeps this arm honest: with the custom scan on, the plan never +# reached the cache at all, and the arm passed by testing nothing. +ready # the arm before may have crashed the server; the premise below must not read that +q "CREATE INDEX pw_id ON pw (id);" >/dev/null +check "premise: the fetch is an index scan, which is what reaches the fetch cache" \ + "$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -X -At \ + -c "SET enable_seqscan = off" -c "SET enable_bitmapscan = off" -c "SET pgcolumnar.enable_custom_scan = off" \ + -c "EXPLAIN SELECT v FROM pw WHERE id = 120" 2>&1 | grep -c 'Index Scan using pw_id')" "1" +check "an index fetch before PREPARE, and the same row fetched first after COMMIT PREPARED, in one session" \ + "$(session "SET enable_seqscan = off" "SET enable_bitmapscan = off" "SET pgcolumnar.enable_custom_scan = off" \ + "BEGIN" "SELECT string_agg(v, ',') FROM pw WHERE id IN (120, 121, 122)" "PREPARE TRANSACTION 'pw_fetch'" \ + "COMMIT PREPARED 'pw_fetch'" "SELECT string_agg(v, ',') FROM pw WHERE id IN (120, 121, 122)")" "changed,changed,changed" + +ready +check "no backend was terminated by a signal" \ + "$(grep -c 'was terminated by signal' "$PGC_LOGFILE")" "0" +check "and no prepared transaction is left behind" "$(q "SELECT count(*) FROM pg_prepared_xacts;")" "0" + +pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 406d7dd5..b0483aac 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -146,6 +146,7 @@ behaviour, the source of that number is named. - [98. test_timeseries_recipes.py: the gap-fill, LOCF and interpolation recipes answer what they claim](#98-test_timeseries_recipespy-the-gap-fill-locf-and-interpolation-recipes-answer-what-they-claim) - [99. test_suite_exit_trap.py: a suite's own EXIT trap must still stop its cluster](#99-test_suite_exit_trappy-a-suites-own-exit-trap-must-still-stop-its-cluster) - [100. test_tier_partition.py: tier_partition moves a cold partition to Parquet, exactly once](#100-test_tier_partitionpy-tier_partition-moves-a-cold-partition-to-parquet-exactly-once) +- [101. test_prepared_writes.py: a write prepared and finished in one session leaves the backend alive](#101-test_prepared_writespy-a-write-prepared-and-finished-in-one-session-leaves-the-backend-alive) ## 1. How to read a test in here @@ -6684,3 +6685,17 @@ On a private database, because foreign servers are per database. A local path st | `test_tier_partition_moves_a_partition_exactly_once` | the move returns the rows moved; the parent's answers are the same; the cold range is a foreign partition with the same name and bound, reading the exported file; no pgcolumnar catalog rows are left; pruning keeps the ranges apart; an insert into the tiered range is refused; five refusals move nothing; the parent is held ACCESS EXCLUSIVE until commit, and a rollback leaves the rows local; a write in flight is exported; a fault after each of the five steps leaves the rows exactly once, still local | | `test_tier_partition_refuses_what_it_cannot_move_and_survives_concurrency` | at each isolation level a row committed between the snapshot and the lock survives, and only READ COMMITTED moves; a UNIQUE parent and four kinds of dependent are refused with no file left, and the partition moves once they are gone; grants and comment carry over; a racing insert leaves every row once; lock_wait times a held parent out with nothing attached; the caller's lock_timeout survives; a retry over a failed attempt's file moves the partition | | `test_tier_partition_moves_a_virtual_generated_column` | PostgreSQL 18+: a partition whose parent has a virtual generated column moves and reads back the same; below 18, `UNSUPPORTED_MAJOR`, listed in `expected_unrunnable.txt` | + +## 101. test_prepared_writes.py: a write prepared and finished in one session leaves the backend alive + +Port of `prepared_writes.sh` (#1323). PREPARE frees TopTransactionContext without reaching the COMMIT/ABORT arms of the transaction callback, and the pending write states and delete-vector buffers live in children of it. The session's next transaction, the COMMIT PREPARED itself, walked the freed lists at its PRE_COMMIT and died of SIGSEGV. Committing from another session never touched them, which is the path `parallel_copy` takes, so nothing in the corpus had reached this. + +Each arm runs in one connection, a transaction per statement, and asserts what that connection returns. A crash also shows as a "terminated by signal" line that the shared cluster's server log gained during this test; only lines written after the test began are counted, after a premise that the log is there. Independent of the shell suite: its own table, row counts and values. The check names match. + +Red on main, the first arm reading `lost`. Each of the two discards is red under its own removal. The index-fetch arm has a premise that the plan is an Index Scan, because with the custom scan on, the fetch never reaches the fetch cache. A discard of that cache at PREPARE changed no arm, so it is not in the fix. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_a_write_prepared_and_finished_in_one_session` | an insert, a delete and an update prepared and committed in one session, an insert prepared and rolled back, a write after COMMIT PREPARED, and an index fetch on each side of it, all answered by the same session; the server log gains no "terminated by signal"; no prepared transaction is left | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 5c8b4991..7357fd88 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -582,4 +582,7 @@ guard_tests 419 # # cluster_tests 513 -> 516 for #1295's test_tier_partition.py (three tests), by collection # on this branch rebased onto 2e34f90 (#1322). -cluster_tests 516 +# +# cluster_tests 516 -> 517 for #1323's test_prepared_writes.py, one test, by collection +# on this branch rebased onto 305e15e (#1317). +cluster_tests 517 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 24fdf935..6814e0de 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -121,6 +121,7 @@ "parallel_scan_cost", "partition_lifecycle", "partman_columnar", + "prepared_writes", "projection_drop_column", "projection_parallel", "projection_privilege", diff --git a/test/pytest/test_prepared_writes.py b/test/pytest/test_prepared_writes.py new file mode 100644 index 00000000..38ddae94 --- /dev/null +++ b/test/pytest/test_prepared_writes.py @@ -0,0 +1,109 @@ +"""A pgcolumnar write prepared and then finished in the same session (#1323). + +PREPARE frees TopTransactionContext without reaching the COMMIT/ABORT arms of the +transaction callback, and the pending write states and delete-vector buffers live +in children of it. Their statics were reset only by those arms, so the session's +next transaction -- the COMMIT PREPARED itself -- walked a freed list at its +PRE_COMMIT and the backend died of SIGSEGV. COMMIT PREPARED from another session +never touched them, which is the path parallel_copy takes. + +Every arm runs in ONE connection, a transaction per statement, and asserts what +that connection returns; a crash also shows as a "terminated by signal" line the +server log gained during this test. The cluster is shared, so only the lines +written after this test began are counted. + +ON A PRIVATE DATABASE (`pgc_own_db`). Independent of `test/prepared_writes.sh`: its +own table, row counts and values; the check names match. +""" + +import pathlib +import time + +import psycopg + + +def _session(dsn, *sqls): + """Run the statements in one fresh connection, autocommit; -> the last row's + first column as text, or 'lost' when the connection dies.""" + try: + with psycopg.connect(dsn, autocommit=True) as c: + row = None + for sql in sqls: + cur = c.execute(sql) + row = cur.fetchone() if cur.description else row + return "" if row is None else str(row[0]) + except psycopg.OperationalError: + return "lost" + + +def _ready(dsn): + """Wait, up to 30 s, until the server answers: an arm before may have crashed it.""" + for _ in range(60): + if _session(dsn, "SELECT 1") == "1": + return + time.sleep(0.5) + + +def test_a_write_prepared_and_finished_in_one_session(pgc_own_db, pgc_cluster, expect): + conn = pgc_own_db + dsn = pgc_cluster.dsn(dbname=conn.execute("SELECT current_database()").fetchone()[0]) + log = pathlib.Path(pgc_cluster.datadir) / "server.log" + expect.text("present" if log.is_file() else f"absent ({log})", "present", + "premise: the server log is where a crash would be recorded") + seen = log.stat().st_size + expect.at_least(int(conn.execute("SHOW max_prepared_transactions").fetchone()[0]), 1, + "premise: prepared transactions are enabled") + + conn.execute("CREATE TABLE pq (k int, tag text) USING pgcolumnar") + conn.execute("INSERT INTO pq SELECT g, 'seed' FROM generate_series(1, 240) g") + + expect.text(_session(dsn, "BEGIN", "INSERT INTO pq SELECT g, 'tpc' FROM generate_series(241, 2640) g", + "PREPARE TRANSACTION 'pq_ins'", "COMMIT PREPARED 'pq_ins'", + "SELECT count(*) FROM pq"), "2640", + "an insert prepared and committed in one session leaves the session answering") + expect.text(_session(dsn, "BEGIN", "DELETE FROM pq WHERE k <= 40", + "PREPARE TRANSACTION 'pq_del'", "COMMIT PREPARED 'pq_del'", + "SELECT count(*) FROM pq"), "2600", + "a delete prepared and committed in one session leaves the session answering") + expect.text(_session(dsn, "BEGIN", "UPDATE pq SET tag = 'moved' WHERE k BETWEEN 41 AND 300", + "PREPARE TRANSACTION 'pq_upd'", "COMMIT PREPARED 'pq_upd'", + "SELECT count(*) FILTER (WHERE tag = 'moved') FROM pq"), "260", + "an update prepared and committed in one session leaves the session answering") + expect.text(_session(dsn, "BEGIN", "INSERT INTO pq SELECT g, 'dropped' FROM generate_series(1, 1800) g", + "PREPARE TRANSACTION 'pq_rb'", "ROLLBACK PREPARED 'pq_rb'", + "SELECT count(*) FROM pq WHERE tag = 'dropped'"), "0", + "an insert prepared and rolled back in one session leaves the session answering") + expect.text(_session(dsn, "BEGIN", "INSERT INTO pq SELECT g, 'first' FROM generate_series(1, 12) g", + "PREPARE TRANSACTION 'pq_next'", "COMMIT PREPARED 'pq_next'", + "INSERT INTO pq SELECT g, 'second' FROM generate_series(1, 9) g", + "SELECT count(*) FROM pq WHERE tag = 'second'"), "9", + "and the session's next write, after COMMIT PREPARED, lands") + + # The index-fetch cache: an index fetch on each side of COMMIT PREPARED, same rows. + _ready(dsn) + _session(dsn, "CREATE INDEX pq_k ON pq (k)") + with psycopg.connect(dsn, autocommit=True) as c: + for s in ("SET enable_seqscan = off", "SET enable_bitmapscan = off", + "SET pgcolumnar.enable_custom_scan = off"): + c.execute(s) + plan = "\n".join(r[0] for r in c.execute("EXPLAIN SELECT tag FROM pq WHERE k = 77").fetchall()) + expect.num(plan.count("Index Scan using pq_k"), 1, + "premise: the fetch is an index scan, which is what reaches the fetch cache") + expect.text(_session(dsn, "SET enable_seqscan = off", "SET enable_bitmapscan = off", + "SET pgcolumnar.enable_custom_scan = off", "BEGIN", + "SELECT string_agg(tag, ',') FROM pq WHERE k IN (77, 78, 79)", + "PREPARE TRANSACTION 'pq_fetch'", "COMMIT PREPARED 'pq_fetch'", + "SELECT string_agg(tag, ',') FROM pq WHERE k IN (77, 78, 79)"), + "moved,moved,moved", + "an index fetch before PREPARE, and the same row fetched first after COMMIT PREPARED, in one session") + + # Wait for the server to be back if anything above crashed it, then read the log. + # Its own connections: after a crash the fixture's connection is gone too. + _ready(dsn) + with log.open(errors="replace") as f: + f.seek(seen) + gained = f.read() + expect.num(sum(1 for line in gained.splitlines() if "was terminated by signal" in line), 0, + "no backend was terminated by a signal") + expect.text(_session(dsn, "SELECT count(*) FROM pg_prepared_xacts WHERE gid LIKE 'pq_%'"), "0", + "and no prepared transaction is left behind") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 5bc262fa..fa6cf7d5 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -261,6 +261,7 @@ SUITES=( phase6 planner_choice_quality preimage_rewrite + prepared_writes projection_drop_column projection_parallel projection_privilege