Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl - Mailing list pgsql-hackers

From Tender Wang
Subject Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl
Date
Msg-id CAHewXNmVogiXuUj-XB+yCOdLk=UyKgUJVdD80t3nBZT_8WQc3Q@mail.gmail.com
Whole thread Raw
In response to Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl  (Xuneng Zhou <xunengzhou@gmail.com>)
Responses Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl
List pgsql-hackers


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.


The enum comment has inconsistent indentation:

+  /*
+ * States of the ExecMergeJoin state machine
+ */

Yeah, copy-paste issue. Fixed. 


--
Thanks,
Tender Wang
Attachment

pgsql-hackers by date:

Previous
From: shveta malik
Date:
Subject: Re: POC: enable logical decoding when wal_level = 'replica' without a server restart
Next
From: Amit Kapila
Date:
Subject: Re: Logical Replication of sequences