Fix an unnecessary cast calling elog in ExecHashJoinImpl
Hi,
While debugging the HashJoin codes, I noticed below codes
in ExecHashJoinImpl():
elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);
The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.
--
Thanks,
Tender Wang
Attachments:
0001-Fix-an-unnecessary-cast-calling-elog-in-ExecHashJoin.patchtext/plain; charset=US-ASCII; name=0001-Fix-an-unnecessary-cast-calling-elog-in-ExecHashJoin.patchDownload
From e19c237a147c909ff0338a84f968674d67cb3861 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Sat, 30 Aug 2025 14:00:18 +0800
Subject: [PATCH] Fix an unnecessary cast calling elog in ExecHashJoinImpl()
---
src/backend/executor/nodeHashjoin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 5661ad76830..30b02eb9f0d 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -669,7 +669,7 @@ ExecHashJoinImpl(PlanState *pstate, bool parallel)
default:
elog(ERROR, "unrecognized hashjoin state: %d",
- (int) node->hj_JoinState);
+ node->hj_JoinState);
}
}
}
--
2.34.1
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
Hi,
While debugging the HashJoin codes, I noticed below codes in ExecHashJoinImpl():
elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.--
Thanks,
Tender Wang
<0001-Fix-an-unnecessary-cast-calling-elog-in-ExecHashJoin.patch>
Yes, hj_JoinState is of type int, so the type cast to int is not needed. The change looks good to me.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi,
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
Hi,
While debugging the HashJoin codes, I noticed below codes in ExecHashJoinImpl():
elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.--
Thanks,
Tender Wang
<0001-Fix-an-unnecessary-cast-calling-elog-in-ExecHashJoin.patch>Yes, hj_JoinState is of type int, so the type cast to int is not needed. The change looks good to me.
LGTM.
Best,
Xuneng
On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
While debugging the HashJoin codes, I noticed below codes in ExecHashJoinImpl():elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.
Yes, hj_JoinState is of type int, so the type cast to int is not needed. The change looks good to me.
LGTM.
I think we can remove this cast, although it's so trivial that it
doesn't seem to have any real impact. A similar cast also exists for
mj_JoinState in ExecMergeJoin().
(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)
- Richard
Hi Richard
(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)
Agree +1 ,enum is safer because it has a fixed set of predefined values,
whereas int has a much larger range of possible values.
On Thu, Oct 16, 2025 at 5:53 PM Richard Guo <guofenglinux@gmail.com> wrote:
Show quoted text
On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
While debugging the HashJoin codes, I noticed below codes inExecHashJoinImpl():
elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.Yes, hj_JoinState is of type int, so the type cast to int is not
needed. The change looks good to me.
LGTM.
I think we can remove this cast, although it's so trivial that it
doesn't seem to have any real impact. A similar cast also exists for
mj_JoinState in ExecMergeJoin().(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)- Richard
Richard Guo <guofenglinux@gmail.com> 于2025年10月16日周四 17:53写道：
On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
While debugging the HashJoin codes, I noticed below codes inExecHashJoinImpl():
elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.Yes, hj_JoinState is of type int, so the type cast to int is not
needed. The change looks good to me.
LGTM.
I think we can remove this cast, although it's so trivial that it
doesn't seem to have any real impact. A similar cast also exists for
mj_JoinState in ExecMergeJoin().(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)
Make sense.
Please check the v2 patch.
--
Thanks,
Tender Wang
Attachments:
v2-Use-enum-to-define-the-state-machine-for-HashJoin-an.patchtext/plain; charset=US-ASCII; name=v2-Use-enum-to-define-the-state-machine-for-HashJoin-an.patchDownload
From eb0237190ffc84d7fe3ff38e6d4499761193f505 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Thu, 16 Oct 2025 20:10:18 +0800
Subject: [PATCH] Use enum to define the state machine for HashJoin and
MergeJoin.
---
src/backend/executor/nodeHashjoin.c | 11 +--------
src/backend/executor/nodeMergejoin.c | 16 +------------
src/include/nodes/execnodes.h | 34 ++++++++++++++++++++++++++--
src/tools/pgindent/typedefs.list | 2 ++
4 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 5661ad76830..a5908892381 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -174,15 +174,6 @@
#include "utils/wait_event.h"
-/*
- * States of the ExecHashJoin state machine
- */
-#define HJ_BUILD_HASHTABLE 1
-#define HJ_NEED_NEW_OUTER 2
-#define HJ_SCAN_BUCKET 3
-#define HJ_FILL_OUTER_TUPLE 4
-#define HJ_FILL_INNER_TUPLES 5
-#define HJ_NEED_NEW_BATCH 6
/* Returns true if doing null-fill on outer relation */
#define HJ_FILL_OUTER(hjstate) ((hjstate)->hj_NullInnerTupleSlot != NULL)
@@ -669,7 +660,7 @@ ExecHashJoinImpl(PlanState *pstate, bool parallel)
default:
elog(ERROR, "unrecognized hashjoin state: %d",
- (int) node->hj_JoinState);
+ node->hj_JoinState);
}
}
}
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index a233313128a..2aee0502758 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -99,20 +99,6 @@
#include "utils/lsyscache.h"
-/*
- * States of the ExecMergeJoin state machine
- */
-#define EXEC_MJ_INITIALIZE_OUTER 1
-#define EXEC_MJ_INITIALIZE_INNER 2
-#define EXEC_MJ_JOINTUPLES 3
-#define EXEC_MJ_NEXTOUTER 4
-#define EXEC_MJ_TESTOUTER 5
-#define EXEC_MJ_NEXTINNER 6
-#define EXEC_MJ_SKIP_TEST 7
-#define EXEC_MJ_SKIPOUTER_ADVANCE 8
-#define EXEC_MJ_SKIPINNER_ADVANCE 9
-#define EXEC_MJ_ENDOUTER 10
-#define EXEC_MJ_ENDINNER 11
/*
* Runtime data for each mergejoin clause
@@ -1426,7 +1412,7 @@ ExecMergeJoin(PlanState *pstate)
*/
default:
elog(ERROR, "unrecognized mergejoin state: %d",
- (int) node->mj_JoinState);
+ node->mj_JoinState);
}
}
}
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index a36653c37f9..3e1365dd466 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2195,6 +2195,24 @@ typedef struct NestLoopState
* InnerEContext workspace for computing inner tuple's join values
* ----------------
*/
+
+ /*
+ * States of the ExecMergeJoin state machine
+ */
+typedef enum MJ_ExecState {
+ EXEC_MJ_INITIALIZE_OUTER = 1,
+ EXEC_MJ_INITIALIZE_INNER,
+ EXEC_MJ_JOINTUPLES,
+ EXEC_MJ_NEXTOUTER,
+ EXEC_MJ_TESTOUTER,
+ EXEC_MJ_NEXTINNER,
+ EXEC_MJ_SKIP_TEST,
+ EXEC_MJ_SKIPOUTER_ADVANCE,
+ EXEC_MJ_SKIPINNER_ADVANCE,
+ EXEC_MJ_ENDOUTER,
+ EXEC_MJ_ENDINNER
+} MJ_ExecState;
+
/* private in nodeMergejoin.c: */
typedef struct MergeJoinClauseData *MergeJoinClause;
@@ -2203,7 +2221,7 @@ typedef struct MergeJoinState
JoinState js; /* its first field is NodeTag */
int mj_NumClauses;
MergeJoinClause mj_Clauses; /* array of length mj_NumClauses */
- int mj_JoinState;
+ MJ_ExecState mj_JoinState;
bool mj_SkipMarkRestore;
bool mj_ExtraMarks;
bool mj_ConstFalseJoin;
@@ -2246,6 +2264,18 @@ typedef struct MergeJoinState
* ----------------
*/
+ /*
+ * States of the ExecHashJoin state machine
+ */
+typedef enum HJ_ExecState {
+ HJ_BUILD_HASHTABLE = 1,
+ HJ_NEED_NEW_OUTER,
+ HJ_SCAN_BUCKET,
+ HJ_FILL_OUTER_TUPLE,
+ HJ_FILL_INNER_TUPLES,
+ HJ_NEED_NEW_BATCH
+} HJ_ExecState;
+
/* these structs are defined in executor/hashjoin.h: */
typedef struct HashJoinTupleData *HashJoinTuple;
typedef struct HashJoinTableData *HashJoinTable;
@@ -2265,7 +2295,7 @@ typedef struct HashJoinState
TupleTableSlot *hj_NullOuterTupleSlot;
TupleTableSlot *hj_NullInnerTupleSlot;
TupleTableSlot *hj_FirstOuterTupleSlot;
- int hj_JoinState;
+ HJ_ExecState hj_JoinState;
bool hj_MatchedOuter;
bool hj_OuterNotEmpty;
} HashJoinState;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ee1cab6190f..8fe926cbffb 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1145,6 +1145,7 @@ HASH_SEQ_STATUS
HE
HEntry
HIST_ENTRY
+HJ_ExecState
HKEY
HLOCAL
HMAC_CTX
@@ -1654,6 +1655,7 @@ MGVTBL
MINIDUMPWRITEDUMP
MINIDUMP_TYPE
MJEvalResult
+MJ_ExecState
MTTargetRelLookup
MVDependencies
MVDependency
--
2.34.1
Hi Tender,
On Thu, Oct 16, 2025 at 8:24 PM Tender Wang <tndrwang@gmail.com> wrote:
Richard Guo <guofenglinux@gmail.com> 于2025年10月16日周四 17:53写道：
On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
While debugging the HashJoin codes, I noticed below codes in ExecHashJoinImpl():elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.Yes, hj_JoinState is of type int, so the type cast to int is not needed. The change looks good to me.
LGTM.
I think we can remove this cast, although it's so trivial that it
doesn't seem to have any real impact. A similar cast also exists for
mj_JoinState in ExecMergeJoin().(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)Make sense.
Please check the v2 patch.
If we decide to converts HashJoin and MergeJoin state machine
definitions from #define macros to enum types, we might need to keep
the (int) casts from the elog() error messages:
elog(ERROR, "unrecognized hashjoin state: %d", (int) node->hj_JoinState);
elog(ERROR, "unrecognized mergejoin state: %d", (int) node->mj_JoinState);
The enum comment has inconsistent indentation:
+ /*
+ * States of the ExecMergeJoin state machine
+ */Best,
Xuneng
Xuneng Zhou <xunengzhou@gmail.com> 于2025年10月17日周五 10:31写道：
Hi Tender,
On Thu, Oct 16, 2025 at 8:24 PM Tender Wang <tndrwang@gmail.com> wrote:
Richard Guo <guofenglinux@gmail.com> 于2025年10月16日周四 17:53写道：
On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com>
wrote:
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com>
wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
While debugging the HashJoin codes, I noticed below codes inExecHashJoinImpl():
elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems
unnecessary.
So I remove it in the attached patch.
Yes, hj_JoinState is of type int, so the type cast to int is not
needed. The change looks good to me.
LGTM.
I think we can remove this cast, although it's so trivial that it
doesn't seem to have any real impact. A similar cast also exists for
mj_JoinState in ExecMergeJoin().(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)Make sense.
Please check the v2 patch.
If we decide to converts HashJoin and MergeJoin state machine
definitions from #define macros to enum types, we might need to keep
the (int) casts from the elog() error messages:elog(ERROR, "unrecognized hashjoin state: %d", (int) node->hj_JoinState);
elog(ERROR, "unrecognized mergejoin state: %d", (int) node->mj_JoinState);
I searched the entire codebase and found that the accumStats() function is
in pgbench.c,
we have
pg_fatal("unexpected error status: %d", estatus);
and the estatus is enum type.
So we can no need to use cast, too.
The enum comment has inconsistent indentation:
+ /* + * States of the ExecMergeJoin state machine + */
Yeah, copy-paste issue. Fixed.
--
Thanks,
Tender Wang
Attachments:
v3-Use-enum-to-define-the-state-machine-for-HashJoin-an.patchapplication/octet-stream; name=v3-Use-enum-to-define-the-state-machine-for-HashJoin-an.patchDownload
From 2c5a4e40d405f07dff29c7cf07a93491d1bb2f41 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Thu, 16 Oct 2025 20:10:18 +0800
Subject: [PATCH] Use enum to define the state machine for HashJoin and
MergeJoin.
---
src/backend/executor/nodeHashjoin.c | 11 +--------
src/backend/executor/nodeMergejoin.c | 16 +------------
src/include/nodes/execnodes.h | 34 ++++++++++++++++++++++++++--
src/tools/pgindent/typedefs.list | 2 ++
4 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 5661ad76830..a5908892381 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -174,15 +174,6 @@
#include "utils/wait_event.h"
-/*
- * States of the ExecHashJoin state machine
- */
-#define HJ_BUILD_HASHTABLE 1
-#define HJ_NEED_NEW_OUTER 2
-#define HJ_SCAN_BUCKET 3
-#define HJ_FILL_OUTER_TUPLE 4
-#define HJ_FILL_INNER_TUPLES 5
-#define HJ_NEED_NEW_BATCH 6
/* Returns true if doing null-fill on outer relation */
#define HJ_FILL_OUTER(hjstate) ((hjstate)->hj_NullInnerTupleSlot != NULL)
@@ -669,7 +660,7 @@ ExecHashJoinImpl(PlanState *pstate, bool parallel)
default:
elog(ERROR, "unrecognized hashjoin state: %d",
- (int) node->hj_JoinState);
+ node->hj_JoinState);
}
}
}
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index a233313128a..2aee0502758 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -99,20 +99,6 @@
#include "utils/lsyscache.h"
-/*
- * States of the ExecMergeJoin state machine
- */
-#define EXEC_MJ_INITIALIZE_OUTER 1
-#define EXEC_MJ_INITIALIZE_INNER 2
-#define EXEC_MJ_JOINTUPLES 3
-#define EXEC_MJ_NEXTOUTER 4
-#define EXEC_MJ_TESTOUTER 5
-#define EXEC_MJ_NEXTINNER 6
-#define EXEC_MJ_SKIP_TEST 7
-#define EXEC_MJ_SKIPOUTER_ADVANCE 8
-#define EXEC_MJ_SKIPINNER_ADVANCE 9
-#define EXEC_MJ_ENDOUTER 10
-#define EXEC_MJ_ENDINNER 11
/*
* Runtime data for each mergejoin clause
@@ -1426,7 +1412,7 @@ ExecMergeJoin(PlanState *pstate)
*/
default:
elog(ERROR, "unrecognized mergejoin state: %d",
- (int) node->mj_JoinState);
+ node->mj_JoinState);
}
}
}
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index a36653c37f9..679b59cdfc0 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2195,6 +2195,24 @@ typedef struct NestLoopState
* InnerEContext workspace for computing inner tuple's join values
* ----------------
*/
+
+/*
+ * States of the ExecMergeJoin state machine
+ */
+typedef enum MJ_ExecState {
+ EXEC_MJ_INITIALIZE_OUTER = 1,
+ EXEC_MJ_INITIALIZE_INNER,
+ EXEC_MJ_JOINTUPLES,
+ EXEC_MJ_NEXTOUTER,
+ EXEC_MJ_TESTOUTER,
+ EXEC_MJ_NEXTINNER,
+ EXEC_MJ_SKIP_TEST,
+ EXEC_MJ_SKIPOUTER_ADVANCE,
+ EXEC_MJ_SKIPINNER_ADVANCE,
+ EXEC_MJ_ENDOUTER,
+ EXEC_MJ_ENDINNER
+} MJ_ExecState;
+
/* private in nodeMergejoin.c: */
typedef struct MergeJoinClauseData *MergeJoinClause;
@@ -2203,7 +2221,7 @@ typedef struct MergeJoinState
JoinState js; /* its first field is NodeTag */
int mj_NumClauses;
MergeJoinClause mj_Clauses; /* array of length mj_NumClauses */
- int mj_JoinState;
+ MJ_ExecState mj_JoinState;
bool mj_SkipMarkRestore;
bool mj_ExtraMarks;
bool mj_ConstFalseJoin;
@@ -2246,6 +2264,18 @@ typedef struct MergeJoinState
* ----------------
*/
+/*
+ * States of the ExecHashJoin state machine
+ */
+typedef enum HJ_ExecState {
+ HJ_BUILD_HASHTABLE = 1,
+ HJ_NEED_NEW_OUTER,
+ HJ_SCAN_BUCKET,
+ HJ_FILL_OUTER_TUPLE,
+ HJ_FILL_INNER_TUPLES,
+ HJ_NEED_NEW_BATCH
+} HJ_ExecState;
+
/* these structs are defined in executor/hashjoin.h: */
typedef struct HashJoinTupleData *HashJoinTuple;
typedef struct HashJoinTableData *HashJoinTable;
@@ -2265,7 +2295,7 @@ typedef struct HashJoinState
TupleTableSlot *hj_NullOuterTupleSlot;
TupleTableSlot *hj_NullInnerTupleSlot;
TupleTableSlot *hj_FirstOuterTupleSlot;
- int hj_JoinState;
+ HJ_ExecState hj_JoinState;
bool hj_MatchedOuter;
bool hj_OuterNotEmpty;
} HashJoinState;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 377a7946585..fcb6337f15a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1145,6 +1145,7 @@ HASH_SEQ_STATUS
HE
HEntry
HIST_ENTRY
+HJ_ExecState
HKEY
HLOCAL
HMAC_CTX
@@ -1654,6 +1655,7 @@ MGVTBL
MINIDUMPWRITEDUMP
MINIDUMP_TYPE
MJEvalResult
+MJ_ExecState
MTTargetRelLookup
MVDependencies
MVDependency
--
2.34.1
On Oct 17, 2025, at 12:20, Tender Wang <tndrwang@gmail.com> wrote:
Xuneng Zhou <xunengzhou@gmail.com> 于2025年10月17日周五 10:31写道：
Hi Tender,On Thu, Oct 16, 2025 at 8:24 PM Tender Wang <tndrwang@gmail.com> wrote:
Richard Guo <guofenglinux@gmail.com> 于2025年10月16日周四 17:53写道：
On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
While debugging the HashJoin codes, I noticed below codes in ExecHashJoinImpl():elog(ERROR, "unrecognized hashjoin state: %d",
(int) node->hj_JoinState);The type of hj_JoinState is already int, so the cast seems unnecessary.
So I remove it in the attached patch.Yes, hj_JoinState is of type int, so the type cast to int is not needed. The change looks good to me.
LGTM.
I think we can remove this cast, although it's so trivial that it
doesn't seem to have any real impact. A similar cast also exists for
mj_JoinState in ExecMergeJoin().(These values represent the state of the Join state machine for
HashJoin and MergeJoin respectively, so I kind of wonder if it might
be better to define them as enum rather than using int.)Make sense.
Please check the v2 patch.
If we decide to converts HashJoin and MergeJoin state machine
definitions from #define macros to enum types, we might need to keep
the (int) casts from the elog() error messages:elog(ERROR, "unrecognized hashjoin state: %d", (int) node->hj_JoinState);
elog(ERROR, "unrecognized mergejoin state: %d", (int) node->mj_JoinState);I searched the entire codebase and found that the accumStats() function is in pgbench.c,
we have
pg_fatal("unexpected error status: %d", estatus);
and the estatus is enum type.
So we can no need to use cast, too.
Passing enums directly to “%d” is acceptable, but it’s a good practice to do so explicitly.
As long as build doesn’t raise a warning, and Coverity doesn’t complain about it, it should be fine. Given the existing usage that Tender found, looks like Coverity won’t complain about it. So I think it’s fine.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/