I read through the latest patch,
v31-0001-Support-checksum-enable-disable-in-a-running-clu.patch. Some
comments below:
On 19/01/2021 14:32, Daniel Gustafsson wrote:
+ /*
+ * Hold interrupts for the duration of xlogging to avoid the state of
data
+ * checksums changing during the processing which would later the
premise
+ * for xlogging hint bits.
+ */
Sentence sense does not make.
@@ -904,6 +916,7 @@ static void SetLatestXTime(TimestampTz xtime);
static void SetCurrentChunkStartTime(TimestampTz xtime);
static void CheckRequiredParameterValues(void);
static void XLogReportParameters(void);
+static void XlogChecksums(ChecksumType new_type);
static void checkTimeLineSwitch(XLogRecPtr lsn, TimeLineID newTLI,
TimeLineID
prevTLI);
static void LocalSetXLogInsertAllowed(void);
Spelling: make it "XLogChecksums" for consistency.
/*
* DataChecksumsNeedWrite
* Returns whether data checksums must be written or not
*
* Returns true iff data checksums are enabled or are in the process of being
* enabled. In case data checksums are currently being enabled we must write
* the checksum even though it's not verified during this stage. Interrupts
* need to be held off by the caller to ensure that the returned state is
* valid for the duration of the intended processing.
*/
bool
DataChecksumsNeedWrite(void)
{
Assert(InterruptHoldoffCount > 0);
return (LocalDataChecksumVersion == PG_DATA_CHECKSUM_VERSION ||
LocalDataChecksumVersion ==
PG_DATA_CHECKSUM_INPROGRESS_ON_VERSION ||
LocalDataChecksumVersion ==
PG_DATA_CHECKSUM_INPROGRESS_OFF_VERSION);
}
Can you be more precise on the "duration of the intended processing"? It
means, until you have actually written out the page, or something like
that, right? The similar explanation in DataChecksumsNeedVerify() is
easier to understand. The two functions are similar, it would be good to
phrase the comments similarly, so that you can quickly see the
difference between the two.
/*
* SetDataChecksumsOnInProgress
* Sets the data checksum state to "inprogress-on" to enable
checksums
*
* In order to start the process of enabling data checksums in a running
* cluster the data_checksum_version state must be changed to "inprogress-on".
* This state requires data checksums to be written but not verified. The state
* transition is performed in a critical section in order to provide crash
* safety, and checkpoints are held off. When the emitted procsignalbarrier
* has been absorbed by all backends we know that the cluster has started to
* enable data checksums.
*/
The two "in order" are superfluous, it's more concise to say just "To
start the process ..." (I was made aware of that by Jürgen Purtz's
recent patches that removed a couple of "in order"s from the docs as
unnecessary).
It's a bit confusing to talk about the critical section and
procsignalbarrier here. Does the caller need to wait for the
procsignalbarrier? No, that's just explaining what the function does
internally. Maybe move that explanation inside the function, and say
here something like "This function blocks until all processes have
acknowledged the state change" or something like that.
/*
* SetDataChecksumsOn
* Enables data checksums cluster-wide
*
* Enabling data checksums is performed using two barriers, the first one
* sets the checksums state to "inprogress-on" (which is performed by
* SetDataChecksumsOnInProgress()) and the second one to "on" (performed here).
* During "inprogress-on", checksums are written but not verified. When all
* existing pages are guaranteed to have checksums, and all new pages will be
* initiated with checksums, the state can be changed to "on".
*/
Perhaps the explanation for how these SetDataChecksumsOn() and
SetDataChecksumsOnInProgress() functions work together should be moved
to one place. For example, explain both functions here at
SetDataChecksumsOn(), and just have a "see SetDataChecksumsOn()" in the
other function, or no comment at all if they're kept next to each other.
As it stands, you have to read both comments and piece together the the
big picture in your head. Maybe add a "see datachecksumsworker.c" here,
since there's a longer explanation of the overall mechanism there.
+/*
+ * Disables checksums for the cluster, unless already disabled.
+ *
+ * Has immediate effect - the checksums are set to off right away.
+ */
+Datum
+disable_data_checksums(PG_FUNCTION_ARGS)
+{
+ if (!superuser())
+ ereport(ERROR,
+ (errmsg("must be superuser")));
+
+ StartDatachecksumsWorkerLauncher(false, 0, 0);
+
+ PG_RETURN_BOOL(true);
+}
The documentation says "Returns <literal>false</literal> in case data
checksums are disabled already", but the function always returns true.
The enable_data_checksum() function also returns a constant 'true'; why
not make it void?
The "has immediate effect" comment seems wrong, given that it actually
launches a worker process.
+ /*
+ * If the launcher is already started, the only operation we can perform
+ * is to cancel it iff the user requested for checksums to be disabled.
+ * That doesn't however mean that all other cases yield an error, as
some
+ * might be perfectly benevolent.
+ */
This comment is a bit hard to understand. Maybe something like
"If the launcher is already started, we cannot launch a new one. But if
the user requested for checksums to be disabled, we can cancel it."
+ if (DatachecksumsWorkerShmem->launcher_started)
+ {
+ if (DatachecksumsWorkerShmem->abort)
+ {
+ ereport(NOTICE,
+ (errmsg("data checksum processing is
concurrently being aborted, please retry")));
+
+ LWLockRelease(DatachecksumsWorkerLock);
+ return;
+ }
If it's being aborted, and the user requested to disable checksum, can
we leave out the NOTICE? I guess not, because the worker process won't
check the 'abort' flag, if it's already finished processing all data pages.
+ /*
+ * If the launcher is started data checksums cannot be on or
off, but
+ * it may be in an inprogress state. Since the state transition
may
+ * not have happened yet (in case of rapidly initiated checksum
enable
+ * calls for example) we inspect the target state of the
currently
+ * running launcher.
+ */
This comment contradicts itself. If the state transition has not
happened yet, then contrary to the first sentence, data checksums *are*
currently on or off.
+ if (enable_checksums)
+ {
+ /*
+ * If we are asked to enable checksums when they are
already being
+ * enabled, there is nothing to do so exit.
+ */
+ if (DatachecksumsWorkerShmem->enable_checksums)
+ {
+ LWLockRelease(DatachecksumsWorkerLock);
+ return;
+ }
+
+ /*
+ * Disabling checksums is likely to be a very quick
operation in
+ * many cases so trying to abort it to save the
checksums would
+ * run the risk of race conditions.
+ */
+ else
+ {
+ ereport(NOTICE,
+ (errmsg("data checksums are
concurrently being disabled, please retry")));
+
+ LWLockRelease(DatachecksumsWorkerLock);
+ return;
+ }
+
+ /* This should be unreachable */
+ Assert(false);
+ }
+ else if (!enable_checksums)
+ {
+ /*
+ * Data checksums are already being disabled, exit
silently.
+ */
+ if (DataChecksumsOffInProgress())
+ {
+ LWLockRelease(DatachecksumsWorkerLock);
+ return;
+ }
+
+ DatachecksumsWorkerShmem->abort = true;
+ LWLockRelease(DatachecksumsWorkerLock);
+ return;
+ }
The Assert seems unnecessary. The "if (!enable_checkums)" also seems
unnecessary, could be just "else".
+ /*
+ * The launcher is currently not running, so we need to query the system
+ * data checksum state to determine how to proceed based on the
requested
+ * target state.
+ */
"query the system" makes me think "checking the catalogs" or similar,
but this just looks at a few variables in shared memory.
+/*
+ * ShutdownDatachecksumsWorkerIfRunning
+ * Request shutdown of the datachecksumsworker
+ *
+ * This does not turn off processing immediately, it signals the checksum
+ * process to end when done with the current block.
+ */
+void
+ShutdownDatachecksumsWorkerIfRunning(void)
+{
+ LWLockAcquire(DatachecksumsWorkerLock, LW_EXCLUSIVE);
+
+ /* If the launcher isn't started, there is nothing to shut down */
+ if (DatachecksumsWorkerShmem->launcher_started)
+ DatachecksumsWorkerShmem->abort = true;
+
+ LWLockRelease(DatachecksumsWorkerLock);
+}
This function is unused.
+/*
+ * launcher_cancel_handler
+ *
+ * Internal routine for reacting to SIGINT and flagging the worker to abort.
+ * The worker won't be interrupted immediately but will check for abort flag
+ * between each block in a relation.
+ */
+static void
+launcher_cancel_handler(SIGNAL_ARGS)
+{
+ LWLockAcquire(DatachecksumsWorkerLock, LW_EXCLUSIVE);
+ DatachecksumsWorkerShmem->abort = true;
+ LWLockRelease(DatachecksumsWorkerLock);
+}
Acquiring an lwlock in signal handler is not safe.
Since this is a signal handler, it might still get called after the
process has finished processing, and has already set
DatachecksumsWorkerShmem->launcher_started = false. That seems like an
unexpected state.
+/*
+ * WaitForAllTransactionsToFinish
+ * Blocks awaiting all current transactions to finish
+ *
+ * Returns when all transactions which are active at the call of the function
+ * have ended, or if the postmaster dies while waiting. If the postmaster dies
+ * the abort flag will be set to indicate that the caller of this shouldn't
+ * proceed.
+ */
The caller of this function doesn't check the 'abort' flag AFAICS. I
think you could just use use WL_EXIT_ON_PM_DEATH to error out on
postmaster death.
+ while (true)
+ {
+ int processed_databases = 0;
+
+ /*
+ * Get a list of all databases to process. This may include
databases
+ * that were created during our runtime.
+ *
+ * Since a database can be created as a copy of any other
database
+ * (which may not have existed in our last run), we have to
repeat
+ * this loop until no new databases show up in the list. Since
we wait
+ * for all pre-existing transactions finish, this way we can be
+ * certain that there are no databases left without checksums.
+ */
+ DatabaseList = BuildDatabaseList();
I think it's unnecessary to wait out the transactions on the first
iteration of this loop. There must be at least one database, so there
will always be at least two iterations.
I think it would be more clear to move the
WaitForAllTransactionsToFinish() from BuildDatabaseList() to the callers.
@@ -3567,6 +3571,27 @@ RelationBuildLocalRelation(const char *relname,
relkind == RELKIND_MATVIEW)
RelationInitTableAccessMethod(rel);
+ /*
+ * Set the data checksum state. Since the data checksum state can
change at
+ * any time, the fetched value might be out of date by the time the
+ * relation is built. DataChecksumsNeedWrite returns true when data
+ * checksums are: enabled; are in the process of being enabled (state:
+ * "inprogress-on"); are in the process of being disabled (state:
+ * "inprogress-off"). Since relhaschecksums is only used to track
progress
+ * when data checksums are being enabled, and going from disabled to
+ * enabled will clear relhaschecksums before starting, it is safe to use
+ * this value for a concurrent state transition to off.
+ *
+ * If DataChecksumsNeedWrite returns false, and is concurrently changed
to
+ * true then that implies that checksums are being enabled. Worst case,
+ * this will lead to the relation being processed for checksums even
though
+ * each page written will have them already. Performing this last
shortens
+ * the window, but doesn't avoid it.
+ */
+ HOLD_INTERRUPTS();
+ rel->rd_rel->relhaschecksums = DataChecksumsNeedWrite();
+ RESUME_INTERRUPTS();
+
/*
* Okay to insert into the relcache hash table.
*
I grepped for relhashcheckums, and concluded that the value in the
relcache isn't actually used for anything. Not so! In
heap_create_with_catalog(), the actual pg_class row is constructed from
the relcache entry, so the value set in RelationBuildLocalRelation()
finds its way to pg_class. Perhaps it would be more clear to pass
relhachecksums directly as an argument to AddNewRelationTuple(). That
way, the value in the relcache would be truly never used.
- Heikki
