Hi,
On 5/4/26 15:16, Tomas Vondra wrote:
> Hi,
>
> ... snip ...
>
> Let me briefly explain what the various TAP tests aim to do. From the
> very beginning, my main concern regarding this patch was race conditions
> when updating the shared state about effective data_checksum_version.
> Because the state is effectively split into about three or four places:
>
> * LocalDataChecksumVersion (local cache)
> * XLogCtl->data_checksum_version (XLogCtl->info_lck)
> * ControlFile->data_checksum_version (ControlFileLock)
> * state in control file on disk
>
> These pieces are protected by different locks, the protocol for updating
> and/or reading the various flags is not trivial (and some of the fixed
> issues were due to ControlFile->data_checksum_version being updated from
> a place that shouldn't have).
>
I still have a funny feeling about the "global checksum version" stored
in multiple places with separate locks, and the protocol when updating
them (e.g. which process does that, when, etc.). I haven't found any
fundamental correctness issue, though.
I kept looking for issues, and I realized StartupXLOG may not do the
right thing on the standby after promotion.
if (XLogCtl->data_checksum_version == PG_DATA_CHECKSUM_INPROGRESS_ON)
{
XLogChecksums(PG_DATA_CHECKSUM_OFF);
SpinLockAcquire(&XLogCtl->info_lck);
XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_OFF;
SetLocalDataChecksumState(XLogCtl->data_checksum_version);
SpinLockRelease(&XLogCtl->info_lck);
ereport(...);
}
Consider this sequence of steps:
1) primary/standby cluster has checksums OFF
2) primary starts enabling checksums
3) primary moves to inprogress-on
4) standby receives that and moves to inprogress-on too
5) primary crashes / shuts down / ...
6) standby gets promoted, and does the StartupXLOG thing
7) standby moves from inprogress-on back to off
However, as there's no EmitProcSignalBarrier(), existing backends on the
hot standby won't be notified about the "off" change. And so there will
be a somewhat inconsistent checksum state, with new backends knowing
it's "off", and old backends thinking it's still inprogress-on.
It's not difficult to reproduce this. I guess the existing TAP tests may
not catch this because they always open a new connection when checking
the state, and so see the new (and correct) version.
I believe a similar issue happens for the inprogress-off case a couple
lines later, but I haven't tried reproducing that one.
Ultimately, neither of these cases should cause checksum failures,
because the "correct" new state is "off", and the old backends are not
verifying checksums either.
I suppose this means we should not be updating the checksum state
without emitting the barrier? I think all other places do that.
>
> ... snip ...
>
> The TAP 012 tests checksum change with a concurrent checkpoint, followed
> by a crash, and tests the final state. It pauses the change at an
> injection point, does a checkpoint, proceeds to the next injection
> point, crashes and does recovery. The expectation is that the final
> state "flips" at some injection point, once it gets further enough, and
> stays there. But what actually happens is this:
>
> a) test_checksum_transition(
> 'disabled', 'enable', undef,
> 'datachecksums-enable-inprogress-checksums-end',
> 'datachecksums-enable-checksums-start',
> 'off');
>
> b) test_checksum_transition(
> 'disabled', 'enable', undef,
> 'datachecksums-enable-checksums-start',
> 'datachecksums-enable-checksums-after-xlog',
> 'on');
>
> c) test_checksum_transition(
> 'disabled', 'enable', 'datachecksums-enable-checksums-start',
> 'datachecksums-enable-checksums-after-xlogs',
> 'datachecksums-enable-checksums-after-xlogctl',
> 'off');
>
> This says that if the checkpoint happens after
> 'datachecksums-enable-inprogress-checksums-end' or after
> 'datachecksums-enable-checksums-after-xlog', we end up with 'off' (i.e.
> enabling checksums fails).
>
> But if the checkpoint happens after
> 'datachecksums-enable-checksums-start', we end up with "on" (after
> recovery).
>
> This is a bit surprising, because that injection point is before
> 'datachecksums-enable-checksums-after-xlog'. So the enabling process
> gets further and further, but the final state flips off -> on -> off,
> contradicting the expectation that it changes once.
>
> I haven't quite wrapped my head around it yet, but my understanding is
> this is due to a race condition between the checksum launcher (writing
> XLOG2_CHECKSUMS and updating the shmem state), and the checkpointer
> (reading the shmem state and generating REDO).
>
> The launcher does this sequence of steps:
>
> 1) write XLOG2_CHECKSUMS with new state
> 2) update XLogCtl->data_checksum_version
> 3) update ControlFile->data_checksum_version
> 4) UpdateControlFile()
> 5) emits barrier
>
> while the checkpointer (CreateCheckPoint) does this:
>
> A) read XLogCtl->data_checksum_version (while holding insert locks)
> B) insert XLOG_CHECKPOINT_REDO (reads XLogCtl->data_checksum_version)
> C) UpdateControlFile()
>
> The outcome depends on how exactly these two sequences interleave. For
> example, this can happen:
>
> 1) write XLOG2_CHECKSUMS with new state
> A) read XLogCtl->data_checksum_version (while holding insert locks)
> B) insert XLOG_CHECKPOINT_REDO (reads XLogCtl->data_checksum_version)
> C) UpdateControlFile()
> 2) update XLogCtl->data_checksum_version
> 3) update ControlFile->data_checksum_version
> 4) UpdateControlFile()
> 5) emits barrier
>
> Which means the XLOG_CHECKPOINT_REDO will be after XLOG2_CHECKSUMS (and
> so redo won't see it), but the checkpoint will still get the old
> checksum state from XLogCtl. And so the outcome is "off", per case (c).
>
> But it can also happen what case (b) does:
>
> A) read XLogCtl->data_checksum_version (while holding insert locks)
> B) insert XLOG_CHECKPOINT_REDO (reads XLogCtl->data_checksum_version)
> C) UpdateControlFile()
> 1) write XLOG2_CHECKSUMS with new state
> 2) update XLogCtl->data_checksum_version
> 3) update ControlFile->data_checksum_version
> 4) UpdateControlFile()
> 5) emits barrier
>
> In which case the REDO will have the old state, but the recovery will
> read the XLOG2_CHECKSUMS, and so end up with "on".
>
> This is the root cause of the surprising behavior in TAP 012, I think.
>
> I attempted to trigger these race conditions in TAP 013, but without
> much success. In the end I realized it probably needs more control,
> waiting for the other process to hit the next injection point before
> unpausing the current one. TAP 014 does that, and it shows that with the
> right interleaving of steps the (c) case can end up with both "on" and
> "off" final state.
>
> As I said, I don't claim I fully understand this yet. But I wouldn't
> call this "bug" - AFAICS it won't produce an incorrect final state (I
> haven't seen any such cases).
>
> Still, I wonder if there's a potential issue I failed to notice.
>
I kept thinking about this non-determinism, and I still think I'm right
about the root cause. The checkpointer and datachecksum worker may
interleave in different ways, affecting the final checksum state. The
existing interlock (or lack of of it) is not sufficient.
To verify this hypothesis, I've done a simple thing - I've introduced a
new LWLock, protecting the "critical part" so that the checkpoint_redo
can't happen in between XLogChecksums() and XLogCtl update. Patch
attached, but I don't propose this for commit, I'm not sure if it's
right (or safe), it's merely a PoC to confirm the issue. But it resolves
the weird cases, simply by prohibiting some of the step sequences (so
some of the tests needed to be commented out, as they'd block).
I'm still not sure if it really is an issue or just an annoyance,
because I've not been able to find a case where it'd lead to checksum
failures (or obviously incorrect final state after recovery).
>
> The other question I had when looking at this (concurrency with
> checkpoints) is what we get by doing
>
> MyProc->delayChkptFlags |= DELAY_CHKPT_START;
>
> whenever updating the state in SetDataChecksums... functions. Because
> the only thing that guarantees is the updates happen on one side of the
> checkpoint record. What does that give us, actually?
>
> It does not seem to prevent this surprising behavior, and it does not
> say the XLOG2_CHECKSUMS happens before/after the XLOG_CHECKPOINT_REDO.
>
I still don't understand why this needs DELAY_CHKPT_START ...
I also noticed a couple minor comment issues, per attached patch (this
may need pgindent).
regards
--
Tomas Vondra
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 483e3ea77f5..cb1a77aabb8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4772,6 +4772,8 @@ SetDataChecksumsOnInProgress(void)
INJECTION_POINT_CACHED("datachecksums-enable-inprogress-checksums-before-xlog", NULL);
+ LWLockAcquire(ChecksumStateLock, LW_EXCLUSIVE);
+
XLogChecksums(PG_DATA_CHECKSUM_INPROGRESS_ON);
INJECTION_POINT_CACHED("datachecksums-enable-inprogress-checksums-after-xlog", NULL);
@@ -4781,6 +4783,8 @@ SetDataChecksumsOnInProgress(void)
XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_INPROGRESS_ON;
SpinLockRelease(&XLogCtl->info_lck);
+ LWLockRelease(ChecksumStateLock);
+
elog(LOG, "SetDataChecksumsOnInProgress XLogCtl->data_checksum_version %u => %u",
data_checksum_version, PG_DATA_CHECKSUM_INPROGRESS_ON);
@@ -4875,6 +4879,8 @@ SetDataChecksumsOn(void)
INJECTION_POINT_CACHED("datachecksums-enable-checksums-before-xlog", NULL);
+ LWLockAcquire(ChecksumStateLock, LW_EXCLUSIVE);
+
XLogChecksums(PG_DATA_CHECKSUM_VERSION);
INJECTION_POINT_CACHED("datachecksums-enable-checksums-after-xlog", NULL);
@@ -4884,6 +4890,8 @@ SetDataChecksumsOn(void)
XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_VERSION;
SpinLockRelease(&XLogCtl->info_lck);
+ LWLockRelease(ChecksumStateLock);
+
elog(LOG, "SetDataChecksumsOn / XLogCtl->data_checksum_version %u => %u",
data_checksum_version, PG_DATA_CHECKSUM_VERSION);
@@ -4980,6 +4988,8 @@ SetDataChecksumsOff(void)
INJECTION_POINT_CACHED("datachecksums-disable-inprogress-checksums-before-xlog", NULL);
+ LWLockAcquire(ChecksumStateLock, LW_EXCLUSIVE);
+
XLogChecksums(PG_DATA_CHECKSUM_INPROGRESS_OFF);
INJECTION_POINT_CACHED("datachecksums-disable-inprogress-checksums-after-xlog", NULL);
@@ -4989,6 +4999,8 @@ SetDataChecksumsOff(void)
XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_INPROGRESS_OFF;
SpinLockRelease(&XLogCtl->info_lck);
+ LWLockRelease(ChecksumStateLock);
+
elog(LOG, "SetDataChecksumsOff / XLogCtl->data_checksum_version %u => %u",
data_checksum_version, PG_DATA_CHECKSUM_INPROGRESS_OFF);
@@ -5050,6 +5062,8 @@ SetDataChecksumsOff(void)
INJECTION_POINT_CACHED("datachecksums-disable-checksums-before-xlog", NULL);
+ LWLockAcquire(ChecksumStateLock, LW_EXCLUSIVE);
+
XLogChecksums(PG_DATA_CHECKSUM_OFF);
INJECTION_POINT_CACHED("datachecksums-disable-checksums-after-xlog", NULL);
@@ -5059,6 +5073,8 @@ SetDataChecksumsOff(void)
XLogCtl->data_checksum_version = PG_DATA_CHECKSUM_OFF;
SpinLockRelease(&XLogCtl->info_lck);
+ LWLockRelease(ChecksumStateLock);
+
elog(LOG, "SetDataChecksumsOff / XLogCtl->data_checksum_version %u => %u",
data_checksum_version, PG_DATA_CHECKSUM_OFF);
@@ -7771,6 +7787,9 @@ CreateCheckPoint(int flags)
INJECTION_POINT_CACHED("checkpoint-before-redo-checksums", NULL);
+ /* make XLogChecksums + XLogCtl update atomic */
+ LWLockAcquire(ChecksumStateLock, LW_EXCLUSIVE);
+
WALInsertLockAcquire();
redo_rec.wal_level = wal_level;
SpinLockAcquire(&XLogCtl->info_lck);
@@ -7785,6 +7804,8 @@ CreateCheckPoint(int flags)
XLogRegisterData(&redo_rec, sizeof(xl_checkpoint_redo));
(void) XLogInsert(RM_XLOG_ID, XLOG_CHECKPOINT_REDO);
+ LWLockRelease(ChecksumStateLock);
+
/*
* XLogInsertRecord will have updated XLogCtl->Insert.RedoRecPtr in
* shared memory and RedoRecPtr in backend-local memory, but we need
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 560659f9568..a9b1e213c7e 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -370,6 +370,7 @@ WaitLSN "Waiting to read or update shared Wait-for-LSN state."
LogicalDecodingControl "Waiting to read or update logical decoding status information."
DataChecksumsWorker "Waiting for data checksums worker."
AioWorkerControl "Waiting to update AIO worker information."
+ChecksumState "Waiting to update data checksum state."
#
# END OF PREDEFINED LWLOCKS (DO NOT CHANGE THIS LINE)
diff --git a/src/include/storage/lwlocklist.h b/src/include/storage/lwlocklist.h
index d7eb648bd27..2e566c4cbe3 100644
--- a/src/include/storage/lwlocklist.h
+++ b/src/include/storage/lwlocklist.h
@@ -89,6 +89,7 @@ PG_LWLOCK(54, WaitLSN)
PG_LWLOCK(55, LogicalDecodingControl)
PG_LWLOCK(56, DataChecksumsWorker)
PG_LWLOCK(57, AioWorkerControl)
+PG_LWLOCK(58, ChecksumState)
/*
* There also exist several built-in LWLock tranches. As with the predefined
diff --git a/src/test/modules/test_checksums/t/011_concurrent_checkpoint.pl b/src/test/modules/test_checksums/t/011_concurrent_checkpoint.pl
index 4cea74914d4..c6f7842caa9 100644
--- a/src/test/modules/test_checksums/t/011_concurrent_checkpoint.pl
+++ b/src/test/modules/test_checksums/t/011_concurrent_checkpoint.pl
@@ -245,13 +245,13 @@ sub test_checksum_transition
# concurrent enable + checkpoint, different injection points in the "enable" process
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-inprogress-checksums-start', 'on');
-test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlog', 'on');
+#test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlog', 'on');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlogctl', 'on');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-controlfile', 'on');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-inprogress-checksums-before-barrier-wait', 'on');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-inprogress-checksums-end', 'on');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-checksums-start', 'on');
-test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlog', 'on');
+#test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlog', 'on');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlogctl', 'on');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-controlfile', 'on');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-checksums-before-checkpoint', 'on');
@@ -260,12 +260,12 @@ test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-chec
# concurrent disable + checkpoint, different injection points in the "disable" process
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-inprogress-checksums-start', 'off');
-test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlog', 'off');
+#test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlog', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlogctl', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-controlfile', 'off');
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-inprogress-checksums-before-barrier-wait', 'off');
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-checksums-start', 'off');
-test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlog', 'off');
+#test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlog', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlogctl', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-controlfile', 'off');
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-checksums-before-checkpoint', 'off');
diff --git a/src/test/modules/test_checksums/t/012_concurrent_checkpoint_crash.pl b/src/test/modules/test_checksums/t/012_concurrent_checkpoint_crash.pl
index de169f47034..77c34b8b16d 100644
--- a/src/test/modules/test_checksums/t/012_concurrent_checkpoint_crash.pl
+++ b/src/test/modules/test_checksums/t/012_concurrent_checkpoint_crash.pl
@@ -251,13 +251,13 @@ sub test_checksum_transition
# concurrent enable + checkpoint, different injection points in the "enable" process
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlog', 'off');
-test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlog', 'datachecksums-enable-inprogress-checksums-after-xlogctl', 'off');
+#test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlog', 'datachecksums-enable-inprogress-checksums-after-xlogctl', 'off');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-xlogctl', 'datachecksums-enable-inprogress-checksums-after-controlfile', 'off');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-after-controlfile', 'datachecksums-enable-inprogress-checksums-before-barrier-wait', 'off');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-inprogress-checksums-before-barrier-wait', 'datachecksums-enable-inprogress-checksums-end', 'off');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-inprogress-checksums-end', 'datachecksums-enable-checksums-start', 'off');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlog', 'on');
-test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlog', 'datachecksums-enable-checksums-after-xlogctl', 'off');
+#test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlog', 'datachecksums-enable-checksums-after-xlogctl', 'off');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-xlogctl', 'datachecksums-enable-checksums-after-controlfile', 'on');
test_checksum_transition('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-after-controlfile', 'datachecksums-enable-checksums-before-checkpoint', 'on');
test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-checksums-before-checkpoint', 'datachecksums-enable-checksums-before-barrier-wait', 'on');
@@ -266,12 +266,12 @@ test_checksum_transition('disabled', 'enable', undef, 'datachecksums-enable-chec
# concurrent disable + checkpoint, different injection points in the "disable" process
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlog', 'off');
-test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlog', 'datachecksums-disable-inprogress-checksums-after-xlogctl', 'on');
+#test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlog', 'datachecksums-disable-inprogress-checksums-after-xlogctl', 'on');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-xlogctl', 'datachecksums-disable-inprogress-checksums-after-controlfile', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-inprogress-checksums-after-controlfile', 'datachecksums-disable-inprogress-checksums-before-barrier-wait', 'off');
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-inprogress-checksums-before-barrier-wait', 'datachecksums-disable-checksums-start', 'off');
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlog', 'off');
-test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlog', 'datachecksums-disable-checksums-after-xlogctl', 'off');
+#test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlog', 'datachecksums-disable-checksums-after-xlogctl', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-xlogctl', 'datachecksums-disable-checksums-after-controlfile', 'off');
test_checksum_transition('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-after-controlfile', 'datachecksums-disable-checksums-before-checkpoint', 'off');
test_checksum_transition('enabled', 'disable', undef, 'datachecksums-disable-checksums-before-checkpoint', 'datachecksums-disable-checksums-before-barrier-wait', 'off');
diff --git a/src/test/modules/test_checksums/t/013_async_checkpoint_crash.pl b/src/test/modules/test_checksums/t/013_async_checkpoint_crash.pl
index 4a310506482..0c59f6d4095 100644
--- a/src/test/modules/test_checksums/t/013_async_checkpoint_crash.pl
+++ b/src/test/modules/test_checksums/t/013_async_checkpoint_crash.pl
@@ -349,20 +349,20 @@ note('TEST INPROGRESS-ON/5');
datachecksums-enable-inprogress-checksums-after-controlfile);
test_checksum_sequence('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-end', 'off', @steps);
-note('TEST INPROGRESS-ON/6');
-@steps = qw(create-checkpoint-initial
- checkpoint-before-redo
- checkpoint-before-xlogctl-checksums
- checkpoint-after-xlogctl-checksums
- checkpoint-before-redo-checksums
- datachecksums-enable-inprogress-checksums-before-xlog
- datachecksums-enable-inprogress-checksums-after-xlog
- datachecksums-enable-inprogress-checksums-after-xlogctl
- datachecksums-enable-inprogress-checksums-after-controlfile
- checkpoint-before-redo-wal
- checkpoint-after-redo-wal
- checkpoint-before-old-wal-removal);
-test_checksum_sequence('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-end', 'off', @steps);
+#note('TEST INPROGRESS-ON/6');
+#@steps = qw(create-checkpoint-initial
+# checkpoint-before-redo
+# checkpoint-before-xlogctl-checksums
+# checkpoint-after-xlogctl-checksums
+# checkpoint-before-redo-checksums
+# datachecksums-enable-inprogress-checksums-before-xlog
+# datachecksums-enable-inprogress-checksums-after-xlog
+# datachecksums-enable-inprogress-checksums-after-xlogctl
+# datachecksums-enable-inprogress-checksums-after-controlfile
+# checkpoint-before-redo-wal
+# checkpoint-after-redo-wal
+# checkpoint-before-old-wal-removal);
+#test_checksum_sequence('disabled', 'enable', 'datachecksums-enable-inprogress-checksums-start', 'datachecksums-enable-inprogress-checksums-end', 'off', @steps);
## checksums ON
@@ -395,20 +395,20 @@ note('TEST ON/2');
datachecksums-enable-checksums-after-controlfile);
test_checksum_sequence('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-end', 'on', @steps);
-note('TEST ON/3');
-@steps = qw(create-checkpoint-initial
- checkpoint-before-redo
- checkpoint-before-xlogctl-checksums
- checkpoint-after-xlogctl-checksums
- checkpoint-before-redo-checksums
- datachecksums-enable-checksums-before-xlog
- datachecksums-enable-checksums-after-xlog
- checkpoint-before-redo-wal
- checkpoint-after-redo-wal
- checkpoint-before-old-wal-removal
- datachecksums-enable-checksums-after-xlogctl
- datachecksums-enable-checksums-after-controlfile);
-test_checksum_sequence('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-end', 'on', @steps);
+#note('TEST ON/3');
+#@steps = qw(create-checkpoint-initial
+# checkpoint-before-redo
+# checkpoint-before-xlogctl-checksums
+# checkpoint-after-xlogctl-checksums
+# checkpoint-before-redo-checksums
+# datachecksums-enable-checksums-before-xlog
+# datachecksums-enable-checksums-after-xlog
+# checkpoint-before-redo-wal
+# checkpoint-after-redo-wal
+# checkpoint-before-old-wal-removal
+# datachecksums-enable-checksums-after-xlogctl
+# datachecksums-enable-checksums-after-controlfile);
+#test_checksum_sequence('disabled', 'enable', 'datachecksums-enable-checksums-start', 'datachecksums-enable-checksums-end', 'on', @steps);
note('TEST ON/4');
@steps = qw(create-checkpoint-initial
@@ -462,39 +462,39 @@ note('TEST INPROGRESS-OFF/2');
datachecksums-disable-inprogress-checksums-before-barrier-wait);
test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-checksums-start', 'off', @steps);
-note('TEST INPROGRESS-OFF/3');
-@steps = qw(create-checkpoint-initial
- checkpoint-before-redo
- checkpoint-before-xlogctl-checksums
- checkpoint-after-xlogctl-checksums
- checkpoint-before-redo-checksums
- datachecksums-disable-inprogress-checksums-before-xlog
- datachecksums-disable-inprogress-checksums-after-xlog
- checkpoint-before-redo-wal
- checkpoint-after-redo-wal
- checkpoint-before-old-wal-removal
- datachecksums-disable-inprogress-checksums-after-xlogctl
- datachecksums-disable-inprogress-checksums-after-controlfile
- datachecksums-disable-inprogress-checksums-before-checkpoint
- datachecksums-disable-inprogress-checksums-before-barrier-wait);
-test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-checksums-start', 'off', @steps);
-
-note('TEST INPROGRESS-OFF/4');
-@steps = qw(create-checkpoint-initial
- checkpoint-before-redo
- checkpoint-before-xlogctl-checksums
- checkpoint-after-xlogctl-checksums
- checkpoint-before-redo-checksums
- datachecksums-disable-inprogress-checksums-before-xlog
- datachecksums-disable-inprogress-checksums-after-xlog
- datachecksums-disable-inprogress-checksums-after-xlogctl
- datachecksums-disable-inprogress-checksums-after-controlfile
- datachecksums-disable-inprogress-checksums-before-checkpoint
- checkpoint-before-redo-wal
- checkpoint-after-redo-wal
- checkpoint-before-old-wal-removal
- datachecksums-disable-inprogress-checksums-before-barrier-wait);
-test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-checksums-start', 'off', @steps);
+#note('TEST INPROGRESS-OFF/3');
+#@steps = qw(create-checkpoint-initial
+# checkpoint-before-redo
+# checkpoint-before-xlogctl-checksums
+# checkpoint-after-xlogctl-checksums
+# checkpoint-before-redo-checksums
+# datachecksums-disable-inprogress-checksums-before-xlog
+# datachecksums-disable-inprogress-checksums-after-xlog
+# checkpoint-before-redo-wal
+# checkpoint-after-redo-wal
+# checkpoint-before-old-wal-removal
+# datachecksums-disable-inprogress-checksums-after-xlogctl
+# datachecksums-disable-inprogress-checksums-after-controlfile
+# datachecksums-disable-inprogress-checksums-before-checkpoint
+# datachecksums-disable-inprogress-checksums-before-barrier-wait);
+#test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-checksums-start', 'off', @steps);
+
+#note('TEST INPROGRESS-OFF/4');
+#@steps = qw(create-checkpoint-initial
+# checkpoint-before-redo
+# checkpoint-before-xlogctl-checksums
+# checkpoint-after-xlogctl-checksums
+# checkpoint-before-redo-checksums
+# datachecksums-disable-inprogress-checksums-before-xlog
+# datachecksums-disable-inprogress-checksums-after-xlog
+# datachecksums-disable-inprogress-checksums-after-xlogctl
+# datachecksums-disable-inprogress-checksums-after-controlfile
+# datachecksums-disable-inprogress-checksums-before-checkpoint
+# checkpoint-before-redo-wal
+# checkpoint-after-redo-wal
+# checkpoint-before-old-wal-removal
+# datachecksums-disable-inprogress-checksums-before-barrier-wait);
+#test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-inprogress-checksums-start', 'datachecksums-disable-checksums-start', 'off', @steps);
## checksums OFF
@@ -532,22 +532,22 @@ note('TEST OFF/2');
datachecksums-disable-checksums-before-barrier-wait);
test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-end', 'off', @steps);
-note('TEST OFF/3');
-@steps = qw(create-checkpoint-initial
- checkpoint-before-redo
- checkpoint-before-xlogctl-checksums
- checkpoint-after-xlogctl-checksums
- checkpoint-before-redo-checksums
- datachecksums-disable-checksums-before-xlog
- datachecksums-disable-checksums-after-xlog
- checkpoint-before-redo-wal
- checkpoint-after-redo-wal
- datachecksums-disable-checksums-after-xlogctl
- datachecksums-disable-checksums-after-controlfile
- datachecksums-disable-checksums-before-checkpoint
- checkpoint-before-old-wal-removal
- datachecksums-disable-checksums-before-barrier-wait);
-test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-end', 'off', @steps);
+#note('TEST OFF/3');
+#@steps = qw(create-checkpoint-initial
+# checkpoint-before-redo
+# checkpoint-before-xlogctl-checksums
+# checkpoint-after-xlogctl-checksums
+# checkpoint-before-redo-checksums
+# datachecksums-disable-checksums-before-xlog
+# datachecksums-disable-checksums-after-xlog
+# checkpoint-before-redo-wal
+# checkpoint-after-redo-wal
+# datachecksums-disable-checksums-after-xlogctl
+# datachecksums-disable-checksums-after-controlfile
+# datachecksums-disable-checksums-before-checkpoint
+# checkpoint-before-old-wal-removal
+# datachecksums-disable-checksums-before-barrier-wait);
+#test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-end', 'off', @steps);
note('TEST OFF/4');
@steps = qw(create-checkpoint-initial
@@ -566,22 +566,22 @@ note('TEST OFF/4');
datachecksums-disable-checksums-before-barrier-wait);
test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-end', 'off', @steps);
-note('TEST OFF/5');
-@steps = qw(create-checkpoint-initial
- checkpoint-before-redo
- checkpoint-before-xlogctl-checksums
- checkpoint-after-xlogctl-checksums
- checkpoint-before-redo-checksums
- datachecksums-disable-checksums-before-xlog
- datachecksums-disable-checksums-after-xlog
- checkpoint-before-redo-wal
- checkpoint-after-redo-wal
- datachecksums-disable-checksums-after-xlogctl
- datachecksums-disable-checksums-after-controlfile
- datachecksums-disable-checksums-before-checkpoint
- checkpoint-before-old-wal-removal
- datachecksums-disable-checksums-before-barrier-wait);
-test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-end', 'off', @steps);
+#note('TEST OFF/5');
+#@steps = qw(create-checkpoint-initial
+# checkpoint-before-redo
+# checkpoint-before-xlogctl-checksums
+# checkpoint-after-xlogctl-checksums
+# checkpoint-before-redo-checksums
+# datachecksums-disable-checksums-before-xlog
+# datachecksums-disable-checksums-after-xlog
+# checkpoint-before-redo-wal
+# checkpoint-after-redo-wal
+# datachecksums-disable-checksums-after-xlogctl
+# datachecksums-disable-checksums-after-controlfile
+# datachecksums-disable-checksums-before-checkpoint
+# checkpoint-before-old-wal-removal
+# datachecksums-disable-checksums-before-barrier-wait);
+#test_checksum_sequence('enabled', 'disable', 'datachecksums-disable-checksums-start', 'datachecksums-disable-checksums-end', 'off', @steps);
note('TEST OFF/6');
@steps = qw(create-checkpoint-initial
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fecdf0d4b05..462414323a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4807,9 +4807,8 @@ SetDataChecksumsOn(void)
/*
* The only allowed state transition to "on" is from "inprogress-on" since
- * that state ensures that all pages will have data checksums written. No
- * such state transition exists, if it does happen it's likely due to a
- * programmer error.
+ * that state ensures that all pages will have data checksums written. Any
+ * other attempted state transition is likely due to a programmer error.
*/
if (XLogCtl->data_checksum_version != PG_DATA_CHECKSUM_INPROGRESS_ON)
{
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 9803a0ee2a1..ad4bf4bd2a8 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -88,7 +88,7 @@ AuxiliaryProcessMainCommon(void)
*
* The postmaster (which is what gets forked into the new child process)
* does not handle barriers, therefore it may not have the current value
- * of LocalDataChecksumVersion value (it'll have the value read from the
+ * of LocalDataChecksumState value (it'll have the value read from the
* control file, which may be arbitrarily old).
*
* NB: Even if the postmaster handled barriers, the value might still be
diff --git a/src/backend/postmaster/datachecksum_state.c b/src/backend/postmaster/datachecksum_state.c
index 33430147ff2..520eb8db16b 100644
--- a/src/backend/postmaster/datachecksum_state.c
+++ b/src/backend/postmaster/datachecksum_state.c
@@ -20,8 +20,8 @@
* When enabling checksums in an online cluster, data_checksums will be set to
* "inprogress-on" which signals that write operations MUST compute and write
* the checksum on the data page, but during reading the checksum SHALL NOT be
- * verified. This ensures that all objects created during when checksums are
- * being enabled will have checksums set, but reads won't fail due to missing or
+ * verified. This ensures that all objects created while checksums are being
+ * enabled will have checksums set, but reads won't fail due to missing or
* invalid checksums. Invalid checksums can be present in case the cluster had
* checksums enabled, then disabled them and updated the page while they were
* disabled.
@@ -299,10 +299,11 @@ typedef struct DataChecksumsStateStruct
bool launcher_running;
/*
- * Is a worker process currently running? This is set by the worker
- * launcher when it starts waiting for a worker process to finish.
+ * PID of the worker process, if it's currently running, of InvalidPid
+ * if none. This is set by the worker launcher when it starts waiting
+ * for a worker process to finish.
*/
- int worker_pid;
+ pid_t worker_pid;
/*
* These fields indicate the target state that the launcher is currently
@@ -320,12 +321,8 @@ typedef struct DataChecksumsStateStruct
int cost_limit;
/*
- * Signaling between the launcher and the worker process.
- *
- * As there is only a single worker, and the launcher won't read these
- * until the worker exits, they can be accessed without the need for a
- * lock. If multiple workers are supported then this will have to be
- * revisited.
+ * Signaling between the launcher and the worker process. Protected by
+ * DataChecksumsWorkerLock.
*/
/* result, set by worker before exiting */
@@ -599,9 +596,9 @@ StartDataChecksumsWorkerLauncher(DataChecksumsWorkerOperation op,
*
* If the launcher is currently busy enabling the checksums, and we want
* them disabled (or vice versa), the launcher will notice that at latest
- * when it's about to exit, and will loop back process the new request. So
- * if the launcher is already running, we don't need to do anything more
- * here to abort it.
+ * when it's about to exit, and will loop back to process the new request.
+ * So if the launcher is already running, we don't need to do anything
+ * more here to abort it.
*
* If you call pg_enable/disable_data_checksums() twice in a row, before
* the launcher has had a chance to start up, we still end up launching it
@@ -710,10 +707,7 @@ ProcessSingleRelationFork(Relation reln, ForkNumber forkNum, BufferAccessStrateg
UnlockReleaseBuffer(buf);
- /*
- * This is the only place where we check if we are asked to abort, the
- * abortion will bubble up from here.
- */
+ /* Check if we are asked to abort, the abortion will bubble up. */
Assert(operation == ENABLE_DATACHECKSUMS);
LWLockAcquire(DataChecksumsWorkerLock, LW_SHARED);
if (DataChecksumState->launch_operation == DISABLE_DATACHECKSUMS)
@@ -924,8 +918,8 @@ ProcessDatabase(DataChecksumsWorkerDatabase *db)
* performed checksum operations exits. A launcher process which is exiting due
* to a duplicate started launcher does not need to perform any cleanup and
* this function should not be called. Otherwise, we need to clean up the abort
- * flag to ensure that processing started again if it was previously aborted
- * (note: started again, *not* restarted from where it left off).
+ * flag to ensure that processing can be started again if it was previously
+ * aborted (note: started again, *not* restarted from where it left off).
*/
static void
launcher_exit(int code, Datum arg)
@@ -1434,7 +1428,7 @@ FreeDatabaseList(List *dblist)
* BuildRelationList
* Compile a list of relations in the database
*
- * Returns a list of OIDs for the request relation types. If temp_relations
+ * Returns a list of OIDs for the requested relation types. If temp_relations
* is True then only temporary relations are returned. If temp_relations is
* False then non-temporary relations which have data checksums are returned.
* If include_shared is True then shared relations are included as well in a
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 2460e550f96..c1457eb34f0 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -773,7 +773,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
*
* The postmaster (which is what gets forked into the new child process)
* does not handle barriers, therefore it may not have the current value
- * of LocalDataChecksumVersion value (it'll have the value read from the
+ * of LocalDataChecksumState value (it'll have the value read from the
* control file, which may be arbitrarily old).
*
* NB: Even if the postmaster handled barriers, the value might still be
