> 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/
