Обсуждение: [PATCH} Move instrumentation structs

Hey there,

Based on the Alvaro's idea [1] about moving different instrumentation
related C structures and enums into one single header file, I'm
sending the following patches.
That single file is named `executor/instrument_node.h`

Local tests and CI tests are passing
https://cirrus-ci.com/build/6171192257675264

Thanks for reviewing them,
cheers.

[1] https://www.postgresql.org/message-id/202510051642.wwmn4mj77wch%40alvherre.pgsql
-- 
Mario Gonzalez
EDB: https://www.enterprisedb.com

> On Nov 10, 2025, at 21:48, Mario González Troncoso <gonzalemario@gmail.com> wrote:
>
> Hey there,
>
> Based on the Alvaro's idea [1] about moving different instrumentation
> related C structures and enums into one single header file, I'm
> sending the following patches.
> That single file is named `executor/instrument_node.h`
>
> Local tests and CI tests are passing
> https://cirrus-ci.com/build/6171192257675264
>
> Thanks for reviewing them,
> cheers.
>
> [1] https://www.postgresql.org/message-id/202510051642.wwmn4mj77wch%40alvherre.pgsql
> --
> Mario Gonzalez
> EDB: https://www.enterprisedb.com
>
<0002-Remove-brin-gin_tuple.h-from-tuplesort.h.patch><0003-Remove-storage-buf.h-from-access-relscan.h.patch><0004-Remove-unused-headers-from-execReplication.c.patch><0001-Move-instrumentation-related-structures-into-instrum.patch>

I quickly went through this patch and got a big concern. From what I have learned, “executor" depends on “access”. To
provethat, I quickly browse through    a bunch of files under executor and access, which showed me that files under
executorcan include access headers, but none of files under access includes executor headers. However this patch breaks
thelayering, for example: 

```
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index 26081693383..1c9a8cb4df8 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -16,6 +16,7 @@

 #include "access/gin_private.h"
 #include "access/relscan.h"
+#include "executor/instrument_node.h"
 #include "pgstat.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
```

I would suggest moving instrument_node.h to a layer above both access and executor.

The other small comment is:
```
+typedef struct SharedAgginfo
+{
+    int    num_workers;
+    AggregateInstrumentation sinstrument[FLEXIBLE_ARRAY_MEMBER];
+} SharedAggInfo;
```

The structure name is different from the alias name: “info” vs “Info”, which is unusual. Let’s change the structure
nameto SharedAggInfo. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 2025-Nov-11, Chao Li wrote:

> I quickly went through this patch and got a big concern. From what I
> have learned, “executor" depends on “access”. To prove that, I quickly
> browse through a bunch of files under executor and access, which
> showed me that files under executor can include access headers, but
> none of files under access includes executor headers.

Well, this isn't strictly true as a rule.  See for example this
conversation,
https://postgr.es/m/202509291356.o5t6ny2hoa3q@alvherre.pgsql

What I'm saying there is precisely that forbidding headers in
src/include/access from including any headers from src/include/executor
is not really workable; but we do intend to create a hierarchy, so that
there are few headers in src/include/executor that are allowed to be
included, and those that are should be as lean as possible.  This new
instrument_node.h header is in that category (notice it barely
#include's anything itself), so it's not a problem that files in
src/include/access include it.

Really, 0001 is just an enabler which doesn't have a huge impact by
itself.  But it does enable us to do 0002, where we remove inclusions of
brin_tuple.h and gin_tuple.h from utils/tuplesort.h, which is a large
gain.


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:

+/* ----------------
+ *   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")

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

On Mon, 15 Dec 2025 at 11:45, Mario González Troncoso
<gonzalemario@gmail.com> wrote:
>
>
> 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
>

Hey there. I'm updating the patch. BTW, only 0002 has changed where,
because of `213a1b89527 Move attribute statistics functions to
stat_utils.c`, we're also adding "typcache.h"
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -35,6 +35,7 @@
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
 #include "utils/syscache.h"
+#include "utils/typcache.h"

https://cirrus-ci.com/build/6354305906638848

>
> > +/* ----------------
> > + *   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.
> >
> >

--
Mario Gonzalez
EDB: https://www.enterprisedb.com

On 2026-Jan-05, Mario González Troncoso wrote:

> Hey there. I'm updating the patch.

Thanks.  Coincidentally, while I rode the train a few hundred kilometers
from your place yesterday, I was editing the comments in 0001 as I had
mentioned.  The only non-comment change, I think, is that you had left
#define NUM_TUPLESORTMETHODS in two places, which was quite odd.  I
removed the one in tuplesort.h and the comment that accompanied it.
Here's what I ended up with.  What do you think?

I also happened to notice an old typo "its" which should be "it's" in
tuplesort.c while reading your patch.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Y una voz del caos me habló y me dijo
"Sonríe y sé feliz, podría ser peor".
Y sonreí. Y fui feliz.
Y fue peor.

On Mon, 5 Jan 2026 at 11:39, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2026-Jan-05, Mario González Troncoso wrote:
>
> > Hey there. I'm updating the patch.
>
> Thanks.  Coincidentally, while I rode the train a few hundred kilometers
> from your place yesterday, I was editing the comments in 0001 as I had
> mentioned.  The only non-comment change, I think, is that you had left
> #define NUM_TUPLESORTMETHODS in two places, which was quite odd.  I
> removed the one in tuplesort.h and the comment that accompanied it.
> Here's what I ended up with.  What do you think?
>

Awesome. Yesterday I heard the train and I thought, it might be Alvaro
that's passing by.

lgtm to the changes, thanks mate!

> I also happened to notice an old typo "its" which should be "it's" in
> tuplesort.c while reading your patch.
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
> Y una voz del caos me habló y me dijo
> "Sonríe y sé feliz, podría ser peor".
> Y sonreí. Y fui feliz.
> Y fue peor.



--
Mario Gonzalez
EDB: https://www.enterprisedb.com