Shortening the Scope of Critical Section in Creation of New MultiXacts
Hi all,
I have been going through the multixact code over the last few
weeks, and noticed a potential discrepancy between the need for critical
sections in the creation of new multixacts and the current code.
As per the comment in GetNewMultiXact:
/*
* Critical section from here until caller has written the data into the
* just-reserved SLRU space; we don't want to error out with a partly
* written MultiXact structure. (In particular, failing to write our
* start offset after advancing nextMXact would effectively corrupt the
* previous MultiXact.)
*/
START_CRIT_SECTION()
This makes sense, as we need the multixact state and multixact offset
data to be consistent, but once we write data into the offsets, we can
end the critical section. Currently we wait until the members data is
also written before we end the critical section.
I’ve attached a patch that moves the end of the critical section into
RecordNewMultiXact, right after we finish writing data into the offsets
cache.
This passes regression tests, as well as some custom testing around
injecting random failures while writing multixact members, and
restarting.
I would appreciate any feedback on this.
Sincerely,
Rishu Bagga, Amazon Web Services (AWS)
Attachments:
move_multixact_critical_section.patchapplication/octet-stream; name=move_multixact_critical_section.patchDownload
From fc63d9102f27e83ceb38f0fd3486f41a883a7b69 Mon Sep 17 00:00:00 2001
From: Rishu Bagga <bagrishu@amazon.com>
Date: Wed, 14 Dec 2022 00:01:30 +0000
Subject: [PATCH] move end of critical section to right after we finish
writing offsets
---
src/backend/access/transam/multixact.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index e1191a7564c..77d2b4e0d1c 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -840,9 +840,6 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
/* Now enter the information into the OFFSETs and MEMBERs logs */
RecordNewMultiXact(multi, offset, nmembers, members);
- /* Done with critical section */
- END_CRIT_SECTION();
-
/* Store the new MultiXactId in the local cache, too */
mXactCachePut(multi, nmembers, members);
@@ -889,6 +886,9 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
MultiXactOffsetCtl->shared->page_dirty[slotno] = true;
+ /* Done with critical section */
+ END_CRIT_SECTION();
+
/* Exchange our lock */
LWLockRelease(MultiXactOffsetSLRULock);
--
2.38.1
Hello.
In short, the code as-is looks correct.
At Wed, 14 Dec 2022 00:14:34 +0000, "Bagga, Rishu" <bagrishu@amazon.com> wrote in
* Critical section from here until caller has written the data into the
* just-reserved SLRU space; we don't want to error out with a partly
"the data" here means the whole this multi transaction, which includes
members. We shouldn't exit the critical section at least until the
very end of RecordNewMultiXact().
This makes sense, as we need the multixact state and multixact offset
data to be consistent, but once we write data into the offsets, we can
end the critical section. Currently we wait until the members data is
also written before we end the critical section.
Why do you think that the members are not a part of a
multitransaction? A multitransaction is not complete without them.
Addition to the above, we cannot simply move the END_CRIT_SECTION() to
there since RecordNewMultiXact() has another caller that doesn't start
a critical section.
By the way, I didn't tested this for myself but..
This passes regression tests
Maybe if you did the same with an assertion-build, you will get an
assertion-failure.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center