Re: Online enabling of checksums

Hi,

The patch looks good to me at a high level. Some notes below. I didn't
read through the whole thread, so sorry if some of these have been
discussed already.

> +void
> +ShutdownChecksumHelperIfRunning(void)
> +{
> + if (pg_atomic_unlocked_test_flag(&ChecksumHelperShmem->launcher_started))
> + {
> + /* Launcher not started, so nothing to shut down */
> + return;
> + }
> +
> + ereport(ERROR,
> + (errmsg("checksum helper is currently running, cannot disable checksums"),
> + errhint("Restart the cluster or wait for the worker to finish.")));
> +}

Is there no way to stop the checksum helper once it's started? That
seems rather user-unfriendly. I can imagine it being a pretty common
mistake to call pg_enable_data_checksums() on a 10 TB cluster, only to
realize that you forgot to set the cost limit, and that it's hurting
queries too much. At that point, you want to abort.

> + * This is intended to create the worklist for the workers to go through, and
> + * as we are only concerned with already existing databases we need to ever
> + * rebuild this list, which simplifies the coding.

I can't parse this sentence.

> +extern bool DataChecksumsEnabledOrInProgress(void);
> +extern bool DataChecksumsInProgress(void);
> +extern bool DataChecksumsDisabled(void);

I find the name of the DataChecksumsEnabledOrInProgress() function a bit
long. And doing this in PageIsVerified looks a bit weird:

> if (DataChecksumsEnabledOrInProgress() && !DataChecksumsInProgress())

I think I'd prefer functions like:

/* checksums should be computed on write? */
bool DataChecksumNeedWrite()
/* checksum should be verified on read? */
bool DataChecksumNeedVerify()

> + <literal>template0</literal> is by default not accepting connections, to
> + enable checksums you'll need to temporarily make it accept connections.

This was already discussed, and I agree with the other comments that
it's bad.

> +CREATE OR REPLACE FUNCTION pg_enable_data_checksums (
> + cost_delay int DEFAULT 0, cost_limit int DEFAULT 100)
> + RETURNS boolean STRICT VOLATILE LANGUAGE internal AS 'enable_data_checksums'
> + PARALLEL RESTRICTED;

pg_[enable|disable]_checksums() functions return a bool. It's not clear
to me when they would return what. I'd suggest marking them as 'void'
instead.

> --- a/src/include/storage/bufpage.h
> +++ b/src/include/storage/bufpage.h
> @@ -194,6 +194,7 @@ typedef PageHeaderData *PageHeader;
> */
> #define PG_PAGE_LAYOUT_VERSION 4
> #define PG_DATA_CHECKSUM_VERSION 1
> +#define PG_DATA_CHECKSUM_INPROGRESS_VERSION 2
>

This seems like a weird place for these PG_DATA_CHECKSUM_* constants.
They're not actually stored in the page header, as you might think. I
think the idea was to keep PG_DATA_CHECKSUM_VERSION close to
PG_PAGE_LAYOUT_VERSION, because the checksums affected the on-disk
format. I think it was a bit weird even before this patch, but now it's
worse. At least a better comment would be in order, or maybe move these
elsewhere.

Feature request: it'd be nice to update the 'ps status' with
set_ps_display, to show a rudimentary progress indicator. Like the name
and block number of the relation being processed. It won't tell you how
much is left to go, but at least it will give a warm fuzzy feeling to
the DBA that something is happening.

I didn't review the offline tool, pg_verify_checksums.

- Heikki