On Friday, December 27, 2024 10:37 MSK, Michael Paquier <mich...@paquier.xyz>
wrote:
> So please see the attached. You will note that RemoveTwoPhaseFile(),
> ProcessTwoPhaseBuffer() and TwoPhaseFilePath() now require a
> FullTransactionId, hence callers need to think about the epoch to use.
> That should limit future errors compared to passing the file name as
> optional argument.
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. Thank you!
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.
With best regards,
Vitaly
From 1e74e01a2485e2b3a59f880a5eaf86af1af81cc5 Mon Sep 17 00:00:00 2001
From: Vitaly Davydov <v.davy...@postgrespro.ru>
Date: Fri, 27 Dec 2024 18:03:28 +0300
Subject: [PATCH] Some cosmetic fixes
---
src/backend/access/transam/twophase.c | 92 +++++++++++----------------
1 file changed, 37 insertions(+), 55 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 01f5d728be..9418fd74ee 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -228,6 +228,7 @@ static void MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid,
const char *gid, TimestampTz prepared_at, Oid owner,
Oid databaseid);
static void RemoveTwoPhaseFile(FullTransactionId fxid, bool giveWarning);
+static void RemoveTwoPhaseFileInCurrentEpoch(TransactionId xid, bool giveWarning);
static void RecreateTwoPhaseFile(TransactionId xid, void *content, int len);
/*
@@ -934,6 +935,22 @@ TwoPhaseFilePath(char *path, FullTransactionId fxid)
XidFromFullTransactionId(fxid));
}
+static inline int
+TwoPhaseFilePathInCurrentEpoch(char *path, TransactionId xid)
+{
+ uint32 epoch;
+ FullTransactionId fxid;
+ FullTransactionId nextFullXid;
+
+ /* Use current epoch */
+ nextFullXid = ReadNextFullTransactionId();
+ epoch = EpochFromFullTransactionId(nextFullXid);
+
+ fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
+
+ return TwoPhaseFilePath(path, fxid);
+}
+
/*
* 2PC state file format:
*
@@ -1279,16 +1296,8 @@ ReadTwoPhaseFile(TransactionId xid, bool missing_ok)
pg_crc32c calc_crc,
file_crc;
int r;
- FullTransactionId fxid;
- FullTransactionId nextFullXid;
- uint32 epoch;
-
- /* get current epoch */
- nextFullXid = ReadNextFullTransactionId();
- epoch = EpochFromFullTransactionId(nextFullXid);
- fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
- TwoPhaseFilePath(path, fxid);
+ TwoPhaseFilePathInCurrentEpoch(path, xid);
fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
@@ -1656,18 +1665,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
* be from the current epoch.
*/
if (ondisk)
- {
- uint32 epoch;
- FullTransactionId nextFullXid;
- FullTransactionId fxid;
-
- /* get current epoch */
- nextFullXid = ReadNextFullTransactionId();
- epoch = EpochFromFullTransactionId(nextFullXid);
-
- fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
- RemoveTwoPhaseFile(fxid, true);
- }
+ RemoveTwoPhaseFileInCurrentEpoch(xid, true);
MyLockedGxact = NULL;
@@ -1723,6 +1721,21 @@ RemoveTwoPhaseFile(FullTransactionId fxid, bool giveWarning)
errmsg("could not remove file \"%s\": %m", path)));
}
+static void
+RemoveTwoPhaseFileInCurrentEpoch(TransactionId xid, bool giveWarning)
+{
+ uint32 epoch;
+ FullTransactionId nextFullXid;
+ FullTransactionId fxid;
+
+ /* get current epoch */
+ nextFullXid = ReadNextFullTransactionId();
+ epoch = EpochFromFullTransactionId(nextFullXid);
+
+ fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
+ RemoveTwoPhaseFile(fxid, true);
+}
+
/*
* Recreates a state file. This is used in WAL replay and during
* checkpoint creation.
@@ -1735,21 +1748,13 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len)
char path[MAXPGPATH];
pg_crc32c statefile_crc;
int fd;
- FullTransactionId fxid;
- FullTransactionId nextFullXid;
- uint32 epoch;
/* Recompute CRC */
INIT_CRC32C(statefile_crc);
COMP_CRC32C(statefile_crc, content, len);
FIN_CRC32C(statefile_crc);
- /* Use current epoch */
- nextFullXid = ReadNextFullTransactionId();
- epoch = EpochFromFullTransactionId(nextFullXid);
-
- fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
- TwoPhaseFilePath(path, fxid);
+ TwoPhaseFilePathInCurrentEpoch(path, xid);
fd = OpenTransientFile(path,
O_CREAT | O_TRUNC | O_WRONLY | PG_BINARY);
@@ -2286,7 +2291,6 @@ ProcessTwoPhaseBuffer(FullTransactionId fxid,
ereport(WARNING,
(errmsg("removing stale two-phase state file for transaction %u",
xid)));
-
RemoveTwoPhaseFile(fxid, true);
}
else
@@ -2573,16 +2577,8 @@ PrepareRedoAdd(char *buf, XLogRecPtr start_lsn,
if (!XLogRecPtrIsInvalid(start_lsn))
{
char path[MAXPGPATH];
- uint32 epoch;
- FullTransactionId fxid;
- FullTransactionId nextFullXid;
- /* Use current epoch */
- nextFullXid = ReadNextFullTransactionId();
- epoch = EpochFromFullTransactionId(nextFullXid);
-
- fxid = FullTransactionIdFromEpochAndXid(epoch, hdr->xid);
- TwoPhaseFilePath(path, fxid);
+ TwoPhaseFilePathInCurrentEpoch(path, hdr->xid);
if (access(path, F_OK) == 0)
{
@@ -2677,21 +2673,7 @@ PrepareRedoRemove(TransactionId xid, bool giveWarning)
*/
elog(DEBUG2, "removing 2PC data for transaction %u", xid);
if (gxact->ondisk)
- {
- uint32 epoch;
- FullTransactionId nextFullXid;
- FullTransactionId fxid;
-
- /*
- * We should deal with a file at the current epoch here, so
- * grab it.
- */
- nextFullXid = ReadNextFullTransactionId();
- epoch = EpochFromFullTransactionId(nextFullXid);
- fxid = FullTransactionIdFromEpochAndXid(epoch, xid);
-
- RemoveTwoPhaseFile(fxid, giveWarning);
- }
+ RemoveTwoPhaseFileInCurrentEpoch(xid, giveWarning);
RemoveGXact(gxact);
}
--
2.34.1
