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
