Horton

Re: [PATCH} Move instrumentation structs

Mario Gonzalez <gonzalemario@gmail.com>

View thread
From: Mario González Troncoso <gonzalemario@gmail.com>
To: Álvaro Herrera <alvherre@kurilemu.de>
Cc: Chao Li <li.evan.chao@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-12-15T14:45:45Z
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 →

  1. Move instrumentation-related structs to instrument_node.h

Attachments

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