Re: An improvement of ProcessTwoPhaseBuffer logic

Hi Michael,

Thank you for the explanation and the patch! I'm happy, that I seem to be on the right way.

On Wednesday, December 25, 2024 08:04 MSK, Michael Paquier <michael@paquier.xyz> wrote:
> Vitaly, have you seen that in the wild as an effect of future 2PC files?

I haven't heard about this problem in production, but I've encountered it when I did some development in two-phase
functionality.I fixed it by swapping the if blocks and it solved my problem. 

It is pretty easy to reproduce it on the master branch:

1. Start an instance with enabled prepared transactions
2. Create a simple prepared transaction
3. Do checkpoint to write the transaction two-phase state into a file in pg_twophase subdirectory
4. Copy the created file, change its name to reflect a future xid (in my case: cp 00000000000002E8 00000000000FF2E8)
5. Commit the prepared transaction
6. Stop the instance with -m immediate
7. Start the instance

After starting, you can get an error like "could not start server". In the log file you can find a message like:

LOG:  database system was not properly shut down; automatic recovery in progress
FATAL:  could not access status of transaction 1045224
DETAIL:  Could not read from file "pg_xact/0000" at offset 253952: read too few bytes.
2024-12-25 18:38:30.606 MSK [795557] LOG:  startup process (PID 795560) exited with exit code 1

I tried your patch and it seems the server is started successfully. But I've found another problem in my synthetic test
-it can not remove the file with the following message: 

LOG:  database system was not properly shut down; automatic recovery in progress
WARNING:  removing future two-phase state file for transaction 1045224
WARNING:  could not remove file "pg_twophase/FFFFFFFF000FF2E8": No such file or directory
LOG:  redo starts at 0/1762978

The fill will never be removed automatically.
I guess, it is because we incorrectly calculate the two-phase file name using TwoPhaseFilePath in RemoveTwoPhaseFile in
thisscenario. It can be fixed if to pass file path directly from RecoverPreparedTransactions or
StandbyRecoverPreparedTransactioninto ProcessTwoPhaseBuffer -> RemoveTwoPhaseFile. I did it in the proposed patch
https://www.postgresql.org/message-id/cedbe-65e0c000-1-6db17700%40133269862(it is incomplete now). 

With best regards,
Vitaly