Hi, This thread started on committers, at https://www.postgresql.org/message-id/20200818234532.uiafo5br5lo6zhya%40alap3.anarazel.de
In it I wanted to add a isolation test around prepared transactions: On 2020-08-18 16:45:32 -0700, Andres Freund wrote: > I think it's worth adding an isolation test. But it doesn't seem like > extending prepared-transactions.spec makes too much sense, it doesn't > fit in well. It's a lot easier to reproduce the issue without > SERIALIZABLE, for example. Generally the file seems more about > serializable than 2PC... > > So unless somebody disagrees I'm gonna add a new > prepared-transactions-2.spec. But I noticed that the already existing prepared transactions test wasn't in the normal schedule, since: commit ae55d9fbe3871a5e6309d9b91629f1b0ff2b8cba Author: Andrew Dunstan <and...@dunslane.net> Date: 2012-07-20 15:51:40 -0400 Remove prepared transactions from main isolation test schedule. There is no point in running this test when prepared transactions are disabled, which is the default. New make targets that include the test are provided. This will save some useless waste of cycles on buildfarm machines. Backpatch to 9.1 where these tests were introduced. diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 669c0f220c4..2184975dcb1 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -9,7 +9,6 @@ test: ri-trigger test: partial-index test: two-ids test: multiple-row-versions -test: prepared-transactions test: fk-contention test: fk-deadlock test: fk-deadlock2 The commit confuses me, cause I thought we explicitly enabled prepared transactions during tests well before that? See commit 8d4f2ecd41312e57422901952cbad234d293060b Author: Tom Lane <t...@sss.pgh.pa.us> Date: 2009-04-23 00:23:46 +0000 Change the default value of max_prepared_transactions to zero, and add documentation warnings against setting it nonzero unless active use of prepared transactions is intended and a suitable transaction manager has been installed. This should help to prevent the type of scenario we've seen several times now where a prepared transaction is forgotten and eventually causes severe maintenance problems (or even anti-wraparound shutdown). The only real reason we had the default be nonzero in the first place was to support regression testing of the feature. To still be able to do that, tweak pg_regress to force a nonzero value during "make check". Since we cannot force a nonzero value in "make installcheck", add a variant regression test "expected" file that shows the results that will be obtained when max_prepared_transactions is zero. Also, extend the HINT messages for transaction wraparound warnings to mention the possibility that old prepared transactions are causing the problem. All per today's discussion. And indeed, including the test in the schedule works for make check, not just an installcheck with explicitly enabled prepared xacts. ISTM we should just add an alternative output for disabled prepared xacts, and re-add the test? My new test, without the alternative output for now, is attached. Greetings, Andres Freund
diff --git c/src/test/isolation/expected/prepared-transactions-2.out i/src/test/isolation/expected/prepared-transactions-2.out new file mode 100644 index 00000000000..f7fd2e7f989 --- /dev/null +++ i/src/test/isolation/expected/prepared-transactions-2.out @@ -0,0 +1,119 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_ins_before s1_begin s1_txid s2_txid s1_update_in s1_ins_in s1_show s1_prepare s1_show s1_commit s1_show +step s1_ins_before: INSERT INTO preptest VALUES ('before'); +step s1_begin: BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; +step s1_txid: SELECT txid_current() IS NULL; +?column? + +f +step s2_txid: SELECT txid_current() IS NULL; +?column? + +f +step s1_update_in: UPDATE preptest SET data = 'in; '|| data; +step s1_ins_in: INSERT INTO preptest VALUES ('in'); +step s1_show: SELECT * FROM preptest +data + +in; before +in +step s1_prepare: PREPARE TRANSACTION 'prep'; +step s1_show: SELECT * FROM preptest +data + +before +step s1_commit: COMMIT PREPARED 'prep'; +step s1_show: SELECT * FROM preptest +data + +in; before +in + +starting permutation: s1_ins_before s1_begin s1_txid s2_txid s1_update_in s1_ins_in s1_show s1_prepare s2_show s1_commit s2_show +step s1_ins_before: INSERT INTO preptest VALUES ('before'); +step s1_begin: BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; +step s1_txid: SELECT txid_current() IS NULL; +?column? + +f +step s2_txid: SELECT txid_current() IS NULL; +?column? + +f +step s1_update_in: UPDATE preptest SET data = 'in; '|| data; +step s1_ins_in: INSERT INTO preptest VALUES ('in'); +step s1_show: SELECT * FROM preptest +data + +in; before +in +step s1_prepare: PREPARE TRANSACTION 'prep'; +step s2_show: SELECT * FROM preptest +data + +before +step s1_commit: COMMIT PREPARED 'prep'; +step s2_show: SELECT * FROM preptest +data + +in; before +in + +starting permutation: s1_ins_before s1_begin s1_txid s2_txid s1_update_in s1_ins_in s1_show s1_prepare s1_show s1_rollback s1_show +step s1_ins_before: INSERT INTO preptest VALUES ('before'); +step s1_begin: BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; +step s1_txid: SELECT txid_current() IS NULL; +?column? + +f +step s2_txid: SELECT txid_current() IS NULL; +?column? + +f +step s1_update_in: UPDATE preptest SET data = 'in; '|| data; +step s1_ins_in: INSERT INTO preptest VALUES ('in'); +step s1_show: SELECT * FROM preptest +data + +in; before +in +step s1_prepare: PREPARE TRANSACTION 'prep'; +step s1_show: SELECT * FROM preptest +data + +before +step s1_rollback: ROLLBACK PREPARED 'prep'; +step s1_show: SELECT * FROM preptest +data + +before + +starting permutation: s1_ins_before s1_begin s1_txid s2_txid s1_update_in s1_ins_in s1_show s1_prepare s2_show s1_rollback s2_show +step s1_ins_before: INSERT INTO preptest VALUES ('before'); +step s1_begin: BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; +step s1_txid: SELECT txid_current() IS NULL; +?column? + +f +step s2_txid: SELECT txid_current() IS NULL; +?column? + +f +step s1_update_in: UPDATE preptest SET data = 'in; '|| data; +step s1_ins_in: INSERT INTO preptest VALUES ('in'); +step s1_show: SELECT * FROM preptest +data + +in; before +in +step s1_prepare: PREPARE TRANSACTION 'prep'; +step s2_show: SELECT * FROM preptest +data + +before +step s1_rollback: ROLLBACK PREPARED 'prep'; +step s2_show: SELECT * FROM preptest +data + +before diff --git c/src/test/isolation/isolation_schedule i/src/test/isolation/isolation_schedule index 218c87b24bf..ec15ce784db 100644 --- c/src/test/isolation/isolation_schedule +++ i/src/test/isolation/isolation_schedule @@ -1,3 +1,5 @@ +test: prepared-transactions +test: prepared-transactions-2 test: read-only-anomaly test: read-only-anomaly-2 test: read-only-anomaly-3 diff --git c/src/test/isolation/specs/prepared-transactions-2.spec i/src/test/isolation/specs/prepared-transactions-2.spec new file mode 100644 index 00000000000..d5ed58e4cfc --- /dev/null +++ i/src/test/isolation/specs/prepared-transactions-2.spec @@ -0,0 +1,60 @@ +# This test checks against a bug found in the initial support for +# caching snapshots. The bug caused the snapshot from within the +# to-be-prepared transaction to be used after preparing, erroneously +# not including the prepared transaction's xid, thereby effectively +# treating it as aborted. +# +# See also https://postgr.es/m/E1k7tGP-0005V0-5k%40gemulon.postgresql.org + +setup +{ + CREATE TABLE preptest(data text); +} + +teardown +{ + DROP TABLE preptest; +} + +session "s1" +step "s1_begin" { BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED; } +step "s1_txid" { SELECT txid_current() IS NULL; } +step "s1_ins_before" { INSERT INTO preptest VALUES ('before'); } +step "s1_update_in" { UPDATE preptest SET data = 'in; '|| data; } +step "s1_ins_in" { INSERT INTO preptest VALUES ('in'); } +step "s1_prepare" { PREPARE TRANSACTION 'prep'; } +step "s1_commit" { COMMIT PREPARED 'prep'; } +step "s1_rollback" { ROLLBACK PREPARED 'prep'; } +step "s1_show" { SELECT * FROM preptest } + +session "s2" +step "s2_show" { SELECT * FROM preptest } +step "s2_txid" { SELECT txid_current() IS NULL; } + +# This permutation shows the bug +permutation "s1_ins_before" "s1_begin" "s1_txid" + # Without acquiring an xid here, s1's xid would be the snapshot's + # xmax, hiding the bug (as only xids < xmax are looked up in ->xip). + "s2_txid" + "s1_update_in" "s1_ins_in" "s1_show" "s1_prepare" + # With the bug the next select would find xmax neither committed nor in progress, + # and therefore mark xmax as invalid. Obviously breaking visibility. + "s1_show" "s1_commit" "s1_show" + +# The following are just variations of the theme that seem good to +# also test, even though they weren't affected by the bug. + +permutation "s1_ins_before" "s1_begin" "s1_txid" + "s2_txid" + "s1_update_in" "s1_ins_in" "s1_show" "s1_prepare" + "s2_show" "s1_commit" "s2_show" + +permutation "s1_ins_before" "s1_begin" "s1_txid" + "s2_txid" + "s1_update_in" "s1_ins_in" "s1_show" "s1_prepare" + "s1_show" "s1_rollback" "s1_show" + +permutation "s1_ins_before" "s1_begin" "s1_txid" + "s2_txid" + "s1_update_in" "s1_ins_in" "s1_show" "s1_prepare" + "s2_show" "s1_rollback" "s2_show"