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

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