On Fri, Dec 27, 2024 at 06:16:24PM +0300, Vitaly Davydov wrote: > In general, I like your solution to use FullTransactionId. I haven't > found any evident problems. I just would like to propose to create a > couple of new functions like RemoveTwoPhaseFileInCurrentEpoch, > TwoPhaseFilePathInCurrentEpoch which accept TransactionId instead > FullTransactionId. It may make the code clearer and result into less > boilerplate code. I tried to do some hand testing. It seems, the > problem is gone with the patch.
Thanks for the review. Agreed that it would be a good thing to limit the number of paths calling ReadNextFullTransactionId(), but I did not like much the suggestion TwoPhaseFilePathInCurrentEpoch(), feeling that it was more important to keep a single code path in charge of building the file names. Instead, I have gone with a new FullTransactionIdFromCurrentEpoch() that replaces AdjustToFullTransactionId(). It cleans up most of the calls to ReadNextFullTransactionId() compared to the previous patch. It is true that these couple with calls to ProcessTwoPhaseBuffer(), but the result felt OK this way in the scope of this fix. > As an idea, I would like to propose to store FullTransactionId in > global transaction state instead of TransactionId. I'm not sure, it > will consume significant additional memory, but it make the code > more clear and probably result into less number of locks. FWIW, I was wondering about doing the same thing. However, I have concluded that this some refactoring work out of the scope of fixing the primary issue we have here, as we are hit by the way the file names are built when we attempt to remove them. Note that the memory footprint of storing a FullTransactionId in twophase.c's GlobalTransactionData does not worry me much. It is more important to not increase the size of the two-phase state data, impacting files on disk and their WAL records. This size argument has been mentioned on the thread that has added epochs to the 2PC file names, as far as I recall. At the end, I have applied two patches, the first one down to 13 that took care of the "future" issue, with tests added in the v14~v16 range. v13 was lacking a perl routine, and it's not a big deal to not have coverage for this code path anyway. The second patch has been applied down to v17, to fix the epoch issue, with the more advanced tests. If you have a suggestion of patch to plug in a FullTransactionId into GlobalTransactionData rather than a TransactionId, feel free to propose something! Help is always welcome, and this would be HEAD-only work, making it less urgent to deal with. -- Michael
signature.asc
Description: PGP signature
