On Tue, 11 Nov 2025 at 09:22, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
>
> There are other issues in the 0001 patch though, apart from the weird
> typo you noticed. One is that macro definitions such as
> NUM_TUPLESORTMETHODS should move to the new file together with the enum
> which gives it life, together with the comment that explains it; also,
> some of the structs that are being moved have comments that make sense
> in their current location (because they are surrounded by related
> structs), but not so much in the new location. For instance, I would
> say this needs to be different:
>
Thanks guys for the feedback. I rebased from master as well and
applied the suggestions.
Regarding changing the comments, I reckon we can do it in the next
iteration if you still consider it worth it. But the code looks good
to me, I hope it does for you too:
https://cirrus-ci.com/build/6530088079982592
> +/* ----------------
> + * Values displayed by EXPLAIN ANALYZE
> + * ----------------
> + */
> +typedef struct HashInstrumentation
>
> This should say something like "Instrumentation for Hash nodes" or
> something like that. Less critically, the comment styling (those lines
> of dashes, vertical spacing, and so on) should be made consistent across
> the whole instrument_node.h file instead of using whatever was in the
> original file, which is an eclectic mix of various different styles.
>
> Another thing I'd do here is make 0001 as minimal as possible. I see
> that some files get a new #include "utils/typcache.h" line (or amapi.h
> or genam.h), for instance, but that change makes no sense in that patch.
> These additions should be in the 0002 patch. The only new #include line
> in the 0001 patch should be instrument_node.h itself, because we're
> explicitly not removing any other #include line anywhere.
>
> --
> Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
> "El que vive para el futuro es un iluso, y el que vive para el pasado,
> un imbécil" (Luis Adler, "Los tripulantes de la noche")
--
Mario Gonzalez
EDB: https://www.enterprisedb.com
