Обсуждение: [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
On 2026-Jan-06, Mario González Troncoso wrote: > lgtm to the changes, thanks mate! OK, I have pushed this one, thanks. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Por suerte hoy explotó el califont porque si no me habría muerto de aburrido" (Papelucho)
Pushed the rest as a single commit. Thanks! BTW to show the impact this has on the tree, I rebuilt the inclusion tables I described previously[1] before and after this commit. This table shows how many files include each header before and after, and the difference between those two. I'm really surprised that files like storage/dsm.h and storage/dsa.h are now used by several hundred files less than before (I didn't even realize they were so heavily included). I'm not at all surprised that gin_tuple.h which was previously being included by 730 files is now being include by only 2 (and brin_tuple also now used only by 15 files instead of 743) -- this is exactly what I set out to fix here. [1] https://postgr.es/m/202510021240.ptc2zl5cvwen@alvherre.pgsql include │ num includes original │ num includes patched │ delta ──────────────────────────────────────┼───────────────────────┼──────────────────────┼─────── storage/dsm.h │ 3997 │ 2359 │ 1638 utils/dsa.h │ 2547 │ 1209 │ 1338 port/atomics.h │ 3354 │ 2097 │ 1257 nodes/tidbitmap.h │ 1803 │ 1041 │ 762 lib/pairingheap.h │ 2571 │ 1814 │ 757 utils/snapshot.h │ 2142 │ 1394 │ 748 access/gin_tuple.h │ 730 │ 2 │ 728 access/brin_internal.h │ 749 │ 21 │ 728 access/ginblock.h │ 748 │ 20 │ 728 access/brin_tuple.h │ 743 │ 15 │ 728 access/amapi.h │ 870 │ 143 │ 727 access/genam.h │ 1121 │ 397 │ 724 utils/typcache.h │ 810 │ 87 │ 723 storage/off.h │ 3964 │ 3259 │ 705 storage/itemptr.h │ 3639 │ 2934 │ 705 access/htup.h │ 3288 │ 2592 │ 696 utils/sortsupport.h │ 1828 │ 1234 │ 594 storage/bufpage.h │ 2068 │ 1494 │ 574 utils/relcache.h │ 3196 │ 2694 │ 502 nodes/pg_list.h │ 4989 │ 4488 │ 501 nodes/nodes.h │ 6044 │ 5543 │ 501 access/tupdesc.h │ 4029 │ 3530 │ 499 catalog/pg_attribute.h │ 4031 │ 3532 │ 499 access/transam.h │ 2675 │ 2214 │ 461 common/relpath.h │ 4292 │ 3866 │ 426 nodes/bitmapset.h │ 4386 │ 4089 │ 297 storage/fd.h │ 2583 │ 2422 │ 161 access/skey.h │ 1165 │ 1018 │ 147 storage/fileset.h │ 1616 │ 1524 │ 92 storage/sharedfileset.h │ 1276 │ 1184 │ 92 storage/spin.h │ 2736 │ 2647 │ 89 executor/instrument.h │ 744 │ 691 │ 53 nodes/memnodes.h │ 1177 │ 1124 │ 53 utils/memutils.h │ 1175 │ 1122 │ 53 nodes/miscnodes.h │ 745 │ 692 │ 53 utils/logtape.h │ 730 │ 677 │ 53 utils/tuplesort.h │ 727 │ 674 │ 53 lib/simplehash.h │ 724 │ 671 │ 53 access/tupconvert.h │ 717 │ 664 │ 53 utils/sharedtuplestore.h │ 715 │ 662 │ 53 nodes/execnodes.h │ 712 │ 659 │ 53 nodes/plannodes.h │ 836 │ 785 │ 51 access/attmap.h │ 859 │ 810 │ 49 storage/condition_variable.h │ 949 │ 901 │ 48 utils/tuplestore.h │ 908 │ 860 │ 48 access/itup.h │ 930 │ 883 │ 47 access/tupmacs.h │ 1347 │ 1301 │ 46 storage/proclist_types.h │ 1723 │ 1693 │ 30 utils/queryenvironment.h │ 1056 │ 1034 │ 22 nodes/primnodes.h │ 2289 │ 2281 │ 8 -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No renuncies a nada. No te aferres a nada."
On Tue, 6 Jan 2026 at 03:39, Álvaro Herrera <alvherre@kurilemu.de> wrote: > I also happened to notice an old typo "its" which should be "it's" in > tuplesort.c while reading your patch. There's a bit of conflicting opinion going on here with the changes made in 128897b101e0. "false when it's value for in-memory space" I think the differing opinions depend on what you fill in the missing words with in the badly written English. If you read "value" as "the value", then "it's" is correct. Whereas if you read it as "value is", then "its" is correct. I assume you didn't read it the same way as John did. Maybe worth fixing this up to prevent this from being continually changed and changed back. How about: - bool isMaxSpaceDisk; /* true when maxSpace is value for on-disk - * space, false when it's value for in-memory - * space */ + bool isMaxSpaceDisk; /* true when the maxSpace value tracking is + * on-disk space, false means it's tracking + * memory space */ David
On 2026-Jan-13, David Rowley wrote:
> There's a bit of conflicting opinion going on here with the changes
> made in 128897b101e0.
>
> "false when it's value for in-memory space"
>
> I think the differing opinions depend on what you fill in the missing
> words with in the badly written English. If you read "value" as "the
> value", then "it's" is correct. Whereas if you read it as "value is",
> then "its" is correct. I assume you didn't read it the same way as
> John did.
Yeah, if the "is" had been there I wouldn't have made the change. I
mean, this would have been correct:
true when maxSpace is value for on-disk
space, false when its value is for in-memory space
I debated whether to add the "is" or change the "its" to "it's" and
decided that the smaller change was better. But I agree that a bigger
rewording may be a better solution.
> + bool isMaxSpaceDisk; /* true when the maxSpace value tracking is
> + * on-disk space, false means it's tracking
> + * memory space */
Sounds about right, though I think we can shave some words off that.
How about
int64 maxSpace; /* maximum amount of space occupied among sort
* of groups, either in-memory or on-disk */
bool isMaxSpaceDisk; /* true when maxSpace tracks on-disk space, false
* means in-memory */
?
(CCed Jacob as author and John as committer of that change, in case they
care to comment.)
FWIW the original wording comes from d2d8a229bc58.
Incidentally, I notice that tuplesort_updatemax() has this comment:
/*
* Note: it might seem we should provide both memory and disk usage for a
* disk-based sort. However, the current code doesn't track memory space
* accurately once we have begun to return tuples to the caller (since we
* don't account for pfree's the caller is expected to do), so we cannot
* rely on availMem in a disk sort. This does not seem worth the overhead
* to fix. Is it worth creating an API for the memory context code to
* tell us how much is actually used in sortcontext?
*/
... and I think we already have the API that this comment hypothesizes
about.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)
On Tue, Jan 13, 2026 at 6:38 PM Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2026-Jan-13, David Rowley wrote: > > > There's a bit of conflicting opinion going on here with the changes > > made in 128897b101e0. > Sounds about right, though I think we can shave some words off that. > How about > > int64 maxSpace; /* maximum amount of space occupied among sort > * of groups, either in-memory or on-disk */ > bool isMaxSpaceDisk; /* true when maxSpace tracks on-disk space, false > * means in-memory */ > ? > > (CCed Jacob as author and John as committer of that change, in case they > care to comment.) Hmm, yeah, that commit wasn't quite enough to make that phrase clear, but I don't remember the interpretation. -- John Naylor Amazon Web Services
On Wed, 14 Jan 2026, 12:38 am Álvaro Herrera, <alvherre@kurilemu.de> wrote:
Sounds about right, though I think we can shave some words off that.
How about
int64 maxSpace; /* maximum amount of space occupied among sort
* of groups, either in-memory or on-disk */
bool isMaxSpaceDisk; /* true when maxSpace tracks on-disk space, false
* means in-memory */
?
I'm happy with that suggestion.
David
On 2026-Jan-14, David Rowley wrote: > I'm happy with that suggestion. Thanks, pushed. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/