From: Vitaly Davydov <v.davydov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: An improvement of ProcessTwoPhaseBuffer logic
Date: Fri, 27 Dec 2024 18:16:24 +0300
Message-ID: <16aa85-676ec500-3d-23db9b40@59039559> (raw)
In-Reply-To: <Z25ZHgw220Pt0uIH@paquier.xyz>
References: <13b5b6-676c3080-4d-531db900@47931709>
<Z2yKWtB4iB8BdEiI@paquier.xyz>
<Z2zq-GrOpIDPOHUj@paquier.xyz>
<152dda-676d7200-61-659acf0@102179597>
<Z25ZHgw220Pt0uIH@paquier.xyz>
On Friday, December 27, 2024 10:37 MSK, Michael Paquier <michael@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
Attachments:
[text/x-patch] 0001-Some-cosmetic-fixes.patch (5.1K, ../16aa85-676ec500-3d-23db9b40@59039559/2-0001-Some-cosmetic-fixes.patch)
download | inline diff:
From 1e74e01a2485e2b3a59f880a5eaf86af1af81cc5 Mon Sep 17 00:00:00 2001
From: Vitaly Davydov <v.davydov@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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: v.davydov@postgrespro.ru, michael@paquier.xyz
Subject: Re: An improvement of ProcessTwoPhaseBuffer logic
In-Reply-To: <16aa85-676ec500-3d-23db9b40@59039559>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox