Horton

Thread

  1. Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Tender Wang <tndrwang@gmail.com> — 2025-08-30T06:09:27Z

    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

  2. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Chao Li <li.evan.chao@gmail.com> — 2025-09-01T01:25:51Z

    
> 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/

  3. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-16T07:07:21Z

    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

  4. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Richard Guo <guofenglinux@gmail.com> — 2025-10-16T09:53:07Z

    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

  5. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    wenhui qiu <qiuwenhuifx@gmail.com> — 2025-10-16T10:04:03Z

    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:

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

  6. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Tender Wang <tndrwang@gmail.com> — 2025-10-16T12:24:31Z

    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.


-- 
Thanks,
Tender Wang

  7. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-10-17T02:31:38Z

    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

  8. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Tender Wang <tndrwang@gmail.com> — 2025-10-17T04:20:37Z

    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

  9. Re: Fix an unnecessary cast calling elog in ExecHashJoinImpl

    Chao Li <li.evan.chao@gmail.com> — 2025-10-17T06:20:07Z

    
> 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/