> On 28 May 2026, at 21:04, Tomas Vondra <[email protected]> wrote: > > On 5/28/26 19:19, Daniel Gustafsson wrote: >>> On 28 May 2026, at 05:16, Kyotaro Horiguchi <[email protected]> wrote: >>> >>> ... >>> >>> 0004: 0004-Use-singular-datachecksum-consistently-in-process-na.patch >>> >>> In datachecksum_state.c, the launcher process is referred to in two >>> different ways: "datachecksum launcher" and "datachecksums >>> launcher". Considering the worker process name, I think the former is >>> probably the intended one, so this patch makes the naming consistent >>> accordingly. >>> >>> That said, I can also imagine an interpretation where "datachecksums" >>> was chosen intentionally to refer to the checksum feature or checksum >>> set as a whole, so I'm not entirely sure whether this should be >>> considered a real issue or just a stylistic inconsistency. >>> >>> Still, having both forms coexist seems somewhat error-prone in >>> practice, especially when typing or searching symbol names. >> >> I am clearly biased, or Stockholm syndromed, but DataChecksumsXXX was chosen >> deliberately since it affects the feature which is exposed with the GUC >> data_checksums. Renaming it does not improve clarity IMHO. The singular >> form >> "checksum" is used where it refers to a single entity, like the cluster state >> which can only be a single value. >> >> It would however be an improvement to rename the "datachecksum >> launcher|worker" >> cases you found to "datachecksums" since they are user facing. >> >> - * This creates the list of databases for the datachecksumsworker workers to >> + * This creates the list of databases for the datachecksum workers to >> >> This comment refers to a worker process of the type datachecksumsworker, the >> proposed change makes that less clear IMO. That said, the original wording >> isn't great so maybe "datachecksumsworker process" is better? > > My vote would be to use the plural "data checksums" almost everywhere, > except for the places that actually manipulates a single data checksum. > Because the feature is "data checksums" with GUC "data_checksums", we > manipulate all checksums in the cluster, and so on. > > So it'd be "datachecksums_state.c" and "datachecksums launcher" (which > would make it more consistent with the existing injection points, named > "datachecksumsworker-launcher-..." etc.).
I've pushed the worker/launcher rename now. I held off on renaming the file for now, I'm not entirely convinced that's an improvement. -- Daniel Gustafsson
