Re: Changing the state of data checksums in a running cluster

Tomas Vondra <tomas@vondra.me>

From: Tomas Vondra <tomas@vondra.me>

To: Daniel Gustafsson <daniel@yesql.se>

Cc: Michael Paquier <michael@paquier.xyz>, Michael Banck <mbanck@gmx.net>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Date: 2025-03-10T15:01:35Z

Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Use correct datatype for PID
- 0ca1b3010597 19 (unreleased) landed
Improve comments in online checksums code
- cd857dec0e0a 19 (unreleased) landed
Fix checksum state transition during promotion
- 5fee7cab1b87 19 (unreleased) landed
Fix regex searching for page verification failures in tests
- 486b9a9b9eb4 19 (unreleased) landed
Apply data-checksum worker throttling parameters
- 9a39056c418c 19 (unreleased) landed
Skip WAL for unlogged main fork during online checksum enable
- 2018bd616790 19 (unreleased) landed
Revert "Get rid of WALBufMappingLock"
- c13070a27b63 19 (unreleased) cited
Get rid of WALBufMappingLock
- bc22dc0e0ddc 18.0 cited
Improve grammar of options for command arrays in TAP tests
- ce1b0f9da03e 18.0 cited

Attachments

v20250310e-0001-Online-enabling-and-disabling-of-data-che.patch (text/x-patch) patch 0001
v20250310e-0002-review-fixes.patch (text/x-patch) patch 0002
v20250310e-0003-pgindent.patch (text/x-patch) patch 0003
v20250310e-0004-perltidy.patch (text/x-patch) patch 0004

On 3/10/25 14:27, Daniel Gustafsson wrote:
>> On 10 Mar 2025, at 12:17, Tomas Vondra <tomas@vondra.me> wrote:
>>
>> On 3/10/25 10:46, Tomas Vondra wrote:
>>> On 3/10/25 01:18, Tomas Vondra wrote:
> 
> Thank you so much for picking up and fixing the blockers, it's highly appreciated!
> 
>>> For me, this passes all CI tests, hopefully cfbot will be happy too.
> 
> Confirmed, it compiles clean, builds docs and passes all tests for me as well.
> 
> A few comments from reading over your changes:
> 
> +   launcher worker has this value set, the other worker processes
> +   have this <literal>NULL</literal>.
> There seems to be a word or two missing (same in a few places), should this be
> "have this set to NULL"?
> 

done

> 
> +   The command is currently waiting for a checkpoint to update the checksum
> +   state at the end.
> s/at the end/before finishing/?
> 

done

> 
> + * XXX aren't PG_DATA_ and DATA_ constants the same? why do we need both?
> They aren't mapping 1:1 as PG_DATA_ has the version numbers, and if checksums
> aren't enabled there is no version and thus there is no PG_DATA_CHECKSUMS_OFF.
> This could of course be remedied.  IIRC one reason for adding the enum was to
> get compiler warnings on missing cases when switch()ing over the value, but I
> don't think the current code has any switch.
> 

I haven't done anything about this. I'm not convinced it's an issue we
need to fix, and I haven't tried how much work would it be.

> 
> +   /* XXX isn't it weird there's no wait between the phase updates? */
> It is, I think we should skip PROGRESS_DATACHECKSUMS_PHASE_WAITING_BACKENDS in
> favor of PROGRESS_DATACHECKSUMS_PHASE_ENABLING.
> 

Removed the WAITING_BACKENDS phase.

> 
> +   * When enabling checksums, we have to wait for a checkpoint for the
> +   * checksums to e.
> Seems to be missing the punchline, "for the checksum state to be moved from
> in-progress to on" perhaps?
> 

done

> 
> It also needs a pgindent and pgperltidy but there were only small trivial
> changes there.
> 

done

Attached is an updated version.


-- 
Tomas Vondra