> On Nov 10, 2025, at 21:48, Mario González Troncoso <[email protected]>
> 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 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. However this patch breaks the layering, 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 name to SharedAggInfo.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
