Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Move instrumentation-related structs to instrument_node.h
- 2defd0006255 19 (unreleased) landed
-
[PATCH} Move instrumentation structs
Mario Gonzalez <gonzalemario@gmail.com> — 2025-11-10T13:48:18Z
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
-
Re: [PATCH} Move instrumentation structs
Chao Li <li.evan.chao@gmail.com> — 2025-11-11T06:57:07Z
> 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 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/ -
Re: [PATCH} Move instrumentation structs
Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-11T12:22:23Z
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")
-
Re: [PATCH} Move instrumentation structs
Mario Gonzalez <gonzalemario@gmail.com> — 2025-12-15T14:45:45Z
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