On Thu, Dec 26, 2024 at 07:42:34AM +0900, Michael Paquier wrote: > I suspect that this can be still dangerous as-is while complicating > the code with more possible paths for the removal of the 2PC files, > because we may still build a file name from an XID, and that's what's > causing this issue..
Here are two patches to address both issues: - 0001 for the epoch calculation, down to 17, which would take care of the underflow problem when having a 2PC file that has an XID in the future at epoch 0. It is true that it could be smarter when dealing with files from other epochs, and that this is a problem in v17~. I think that this should integrate better with FullTransactionIds moving forward rather than pass the file names. That would be something quite invasive, only for HEAD. At least that's my impression. - 0002, to take care of the future file issue, down to 13. This includes a TAP test to demonstrate the problem. The test needs a tweak for the 2PC file name in v17~. -- Michael
From d45d0c95a0ddee51563b5e94ea046ce4c5cf1baf Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 26 Dec 2024 14:06:01 +0900 Subject: [PATCH v1 1/2] Fix failure with incorrect epoch for future 2PC files at recovery A two-phase file in the future would be able to trigger an assertion failure in AdjustToFullTransactionId() as the epoch counter would underflow if the checkpoint record uses an epoch of 0. In non-assert builds, this would create a WARNING message referring to a 2PC file with an epoch of "FFFFFFFF" (or UINT32_MAX), as an effect of the underflow calculation. Issue introduced by 5a1dfde8334b. Reported-by: Vitaly Davydov Discussion: https://postgr.es/m/13b5b6-676c3080-4d-531db900@47931709 Backpatch-through: 17 --- src/backend/access/transam/twophase.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 49be1df91c..d2649264f9 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -947,10 +947,9 @@ AdjustToFullTransactionId(TransactionId xid) nextXid = XidFromFullTransactionId(nextFullXid); epoch = EpochFromFullTransactionId(nextFullXid); - if (unlikely(xid > nextXid)) + if (unlikely(xid > nextXid) && epoch > 0) { /* Wraparound occurred, must be from a prev epoch. */ - Assert(epoch > 0); epoch--; } -- 2.45.2
From 31ebbadbca629abf8b7cf982d573ad2c838bf27f Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 26 Dec 2024 14:15:51 +0900 Subject: [PATCH v1 2/2] Fix handling of orphaned 2PC files in the future at recovery Issue introduced by 728bd991c3c4, that has added support for 2PC files in shared memory at recovery. Before this feature was introduced, files in the future of the transaction ID horizon were checked first, followed by a check if a transaction ID is aborted or committed which could involve a pg_xact lookup. After this commit, these checks were done in a reversed order, but files in the future do not have a state that can be checked in pg_xact, hence this caused recovery to fail abruptly should an orphaned 2PC file in the future of the transaction horizon exist in pg_twophase at the beginning of recovery. A test is added to check this scenario, using an empty 2PC with a transaction ID large enough to be in the future when running the test. Author: Vitaly Davydov, Michael Paquier Discussion: https://postgr.es/m/11e597-676ab680-8d-374f23c0@145466129 Backpatch-through: 13 --- src/backend/access/transam/twophase.c | 40 ++++++++++----------- src/test/recovery/t/009_twophase.pl | 51 ++++++++++----------------- 2 files changed, 38 insertions(+), 53 deletions(-) diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index d2649264f9..4b5a3146f1 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -2205,26 +2205,6 @@ ProcessTwoPhaseBuffer(TransactionId xid, if (!fromdisk) Assert(prepare_start_lsn != InvalidXLogRecPtr); - /* Already processed? */ - if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid)) - { - if (fromdisk) - { - ereport(WARNING, - (errmsg("removing stale two-phase state file for transaction %u", - xid))); - RemoveTwoPhaseFile(xid, true); - } - else - { - ereport(WARNING, - (errmsg("removing stale two-phase state from memory for transaction %u", - xid))); - PrepareRedoRemove(xid, true); - } - return NULL; - } - /* Reject XID if too new */ if (TransactionIdFollowsOrEquals(xid, origNextXid)) { @@ -2245,6 +2225,26 @@ ProcessTwoPhaseBuffer(TransactionId xid, return NULL; } + /* Already processed? */ + if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid)) + { + if (fromdisk) + { + ereport(WARNING, + (errmsg("removing stale two-phase state file for transaction %u", + xid))); + RemoveTwoPhaseFile(xid, true); + } + else + { + ereport(WARNING, + (errmsg("removing stale two-phase state from memory for transaction %u", + xid))); + PrepareRedoRemove(xid, true); + } + return NULL; + } + if (fromdisk) { /* Read and validate file */ diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_twophase.pl index 4b3e0f77dc..854910ea68 100644 --- a/src/test/recovery/t/009_twophase.pl +++ b/src/test/recovery/t/009_twophase.pl @@ -534,42 +534,27 @@ is( $psql_out, qq{27|issued to paris}, "Check expected t_009_tbl2 data on standby"); +############################################################################### +# Check handling of orphaned 2PC files at recovery. +############################################################################### -# Exercise the 2PC recovery code in StartupSUBTRANS, which is concerned with -# ensuring that enough pg_subtrans pages exist on disk to cover the range of -# prepared transactions at server start time. There's not much we can verify -# directly, but let's at least get the code to run. -$cur_standby->stop(); -configure_and_reload($cur_primary, "synchronous_standby_names = ''"); +$cur_primary->teardown_node; -$cur_primary->safe_psql('postgres', "CHECKPOINT"); +# Grab location in logs of primary +my $log_offset = -s $cur_primary->logfile; + +# Create a fake file with a transaction ID large enough to be in the future, +# then check that the primary is able to start and remove this file at +# recovery. + +my $future_2pc_file = $cur_primary->data_dir . '/pg_twophase/0000000000FFFFFF'; +append_to_file $future_2pc_file, ""; -my $start_lsn = - $cur_primary->safe_psql('postgres', 'select pg_current_wal_insert_lsn()'); -$cur_primary->safe_psql('postgres', - "CREATE TABLE test(); BEGIN; CREATE TABLE test1(); PREPARE TRANSACTION 'foo';" -); -my $osubtrans = $cur_primary->safe_psql('postgres', - "select 'pg_subtrans/'||f, s.size from pg_ls_dir('pg_subtrans') f, pg_stat_file('pg_subtrans/'||f) s" -); -$cur_primary->pgbench( - '--no-vacuum --client=5 --transactions=1000', - 0, - [], - [], - 'pgbench run to cause pg_subtrans traffic', - { - '009_twophase.pgb' => 'insert into test default values' - }); -# StartupSUBTRANS is exercised with a wide range of visible XIDs in this -# stop/start sequence, because we left a prepared transaction open above. -# Also, setting subtransaction_buffers to 32 above causes to switch SLRU -# bank, for additional code coverage. -$cur_primary->stop; $cur_primary->start; -my $nsubtrans = $cur_primary->safe_psql('postgres', - "select 'pg_subtrans/'||f, s.size from pg_ls_dir('pg_subtrans') f, pg_stat_file('pg_subtrans/'||f) s" -); -isnt($osubtrans, $nsubtrans, "contents of pg_subtrans/ have changed"); +$cur_primary->log_check( + "future two-phase file removed at recovery", + $log_offset, + log_like => + [qr/removing future two-phase state file for transaction 16777215/]); done_testing(); -- 2.45.2
From 854c87ae2ec2a44457143f24bc457158d2dac3ac Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 26 Dec 2024 13:26:46 +0900 Subject: [PATCH v1] Fix handling of orphaned 2PC files in the future at recovery Issue introduced by 728bd991c3c4, that has added support for 2PC files in shared memory at recovery. Before this feature was introduced, files in the future of the transaction ID horizon were checked first, followed by a check if a transaction ID is aborted or committed which could involve a pg_xact lookup. After this commit, these checks were done in a reversed order, but files in the future do not have a state that can be checked in pg_xact, hence this caused recovery to fail abruptly should an orphaned 2PC file in the future of the transaction horizon exist in pg_twophase at the beginning of recovery. A test is added to check this scenario, using an empty 2PC with a transaction ID large enough to be in the future when running the test. Author: Vitaly Davydov, Michael Paquier Discussion: https://postgr.es/m/11e597-676ab680-8d-374f23c0@145466129 Backpatch-through: 13 --- src/backend/access/transam/twophase.c | 40 +++++++++++++-------------- src/test/recovery/t/009_twophase.pl | 23 +++++++++++++++ 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index bca653e485..5adb98163a 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -2183,26 +2183,6 @@ ProcessTwoPhaseBuffer(TransactionId xid, if (!fromdisk) Assert(prepare_start_lsn != InvalidXLogRecPtr); - /* Already processed? */ - if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid)) - { - if (fromdisk) - { - ereport(WARNING, - (errmsg("removing stale two-phase state file for transaction %u", - xid))); - RemoveTwoPhaseFile(xid, true); - } - else - { - ereport(WARNING, - (errmsg("removing stale two-phase state from memory for transaction %u", - xid))); - PrepareRedoRemove(xid, true); - } - return NULL; - } - /* Reject XID if too new */ if (TransactionIdFollowsOrEquals(xid, origNextXid)) { @@ -2223,6 +2203,26 @@ ProcessTwoPhaseBuffer(TransactionId xid, return NULL; } + /* Already processed? */ + if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid)) + { + if (fromdisk) + { + ereport(WARNING, + (errmsg("removing stale two-phase state file for transaction %u", + xid))); + RemoveTwoPhaseFile(xid, true); + } + else + { + ereport(WARNING, + (errmsg("removing stale two-phase state from memory for transaction %u", + xid))); + PrepareRedoRemove(xid, true); + } + return NULL; + } + if (fromdisk) { /* Read and validate file */ diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_twophase.pl index fe7e8e7980..7642e4f0b3 100644 --- a/src/test/recovery/t/009_twophase.pl +++ b/src/test/recovery/t/009_twophase.pl @@ -528,4 +528,27 @@ is( $psql_out, qq{27|issued to paris}, "Check expected t_009_tbl2 data on standby"); +############################################################################### +# Check handling of orphaned 2PC files at recovery. +############################################################################### + +$cur_primary->teardown_node; + +# Grab location in logs of primary +my $log_offset = -s $cur_primary->logfile; + +# Create a fake file with a transaction ID large enough to be in the future, +# then check that the primary is able to start and remove this file at +# recovery. + +my $future_2pc_file = $cur_primary->data_dir . '/pg_twophase/00FFFFFF'; +append_to_file $future_2pc_file, ""; + +$cur_primary->start; +$cur_primary->log_check( + "future two-phase file removed at recovery", + $log_offset, + log_like => + [qr/removing future two-phase state file for transaction 16777215/]); + done_testing(); -- 2.45.2
signature.asc
Description: PGP signature
