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")