Horton

Thread

Commits

  1. Include necessary header files in radixtree.h.

  1. Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-11-18T23:20:52Z

    Hi,

I realized that building test_radixtree.c with TEST_SHARED_RT fails
because it eventually sets RT_SHMEM when #include'ing radixtree.h but
it's missing some header files to include. I've attached a patch to
include necessary header files in radixtree.h to make it
self-contained.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  2. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-11-18T23:41:30Z

    On 19/11/2024 01:20, Masahiko Sawada wrote:
> I realized that building test_radixtree.c with TEST_SHARED_RT fails
> because it eventually sets RT_SHMEM when #include'ing radixtree.h but
> it's missing some header files to include. I've attached a patch to
> include necessary header files in radixtree.h to make it
> self-contained.

+1. Please make sure the #includes are in alphabetical order.

While we're at it, I noticed that lib/radixtree.h includes "postgres.h". 
That's against our usual convention.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  3. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-11-19T00:02:15Z

    On Mon, Nov 18, 2024 at 3:41 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 19/11/2024 01:20, Masahiko Sawada wrote:
> > I realized that building test_radixtree.c with TEST_SHARED_RT fails
> > because it eventually sets RT_SHMEM when #include'ing radixtree.h but
> > it's missing some header files to include. I've attached a patch to
> > include necessary header files in radixtree.h to make it
> > self-contained.
>
> +1. Please make sure the #includes are in alphabetical order.

Fixed.

>
> While we're at it, I noticed that lib/radixtree.h includes "postgres.h".
> That's against our usual convention.

Good catch. I've updated the patch accordingly.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  4. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-19T00:50:09Z

    Masahiko Sawada <sawada.mshk@gmail.com> writes:
> On Mon, Nov 18, 2024 at 3:41 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> While we're at it, I noticed that lib/radixtree.h includes "postgres.h".
>> That's against our usual convention.

> Good catch. I've updated the patch accordingly.

Probably out of scope for this particular patch, but it occurred to me
to grep for other similar violations, and I found three:

src/bin/pg_combinebackup/copy_file.h:#include "c.h"
src/include/fe_utils/option_utils.h:#include "postgres_fe.h"
src/include/fe_utils/query_utils.h:#include "postgres_fe.h"

			regards, tom lane

  5. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-19T09:14:18Z

    On 19/11/2024 01:20, Masahiko Sawada wrote:
> I realized that building test_radixtree.c with TEST_SHARED_RT fails
> because it eventually sets RT_SHMEM when #include'ing radixtree.h but
> it's missing some header files to include. I've attached a patch to
> include necessary header files in radixtree.h to make it
> self-contained.

If those includes are only needed when RT_SHMEM is defined, I suggest
they should be guarded by #ifdef RT_SHMEM, per Peter E's IWYU efforts
lately.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

  6. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-11-19T22:37:25Z

    On Mon, Nov 18, 2024 at 4:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
> > On Mon, Nov 18, 2024 at 3:41 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >> While we're at it, I noticed that lib/radixtree.h includes "postgres.h".
> >> That's against our usual convention.
>
> > Good catch. I've updated the patch accordingly.
>
> Probably out of scope for this particular patch, but it occurred to me
> to grep for other similar violations, and I found three:
>
> src/bin/pg_combinebackup/copy_file.h:#include "c.h"
> src/include/fe_utils/option_utils.h:#include "postgres_fe.h"
> src/include/fe_utils/query_utils.h:#include "postgres_fe.h"

Good catch, make sense to fix them too. Probably we can fix them
including the removal of inclusion of postgres.h from radixtree.h in
one commit, and fix the inclusion issue in radixtree.h in another
commit.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  7. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-11-19T22:37:56Z

    On Tue, Nov 19, 2024 at 1:14 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 19/11/2024 01:20, Masahiko Sawada wrote:
> > I realized that building test_radixtree.c with TEST_SHARED_RT fails
> > because it eventually sets RT_SHMEM when #include'ing radixtree.h but
> > it's missing some header files to include. I've attached a patch to
> > include necessary header files in radixtree.h to make it
> > self-contained.
>
> If those includes are only needed when RT_SHMEM is defined, I suggest
> they should be guarded by #ifdef RT_SHMEM, per Peter E's IWYU efforts
> lately.

Indeed. I'll incorporate it in the next version.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

  8. Re: Fix an error while building test_radixtree.c with TEST_SHARED_RT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2024-11-26T06:12:29Z

    On Tue, Nov 19, 2024 at 2:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Nov 19, 2024 at 1:14 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > On 19/11/2024 01:20, Masahiko Sawada wrote:
> > > I realized that building test_radixtree.c with TEST_SHARED_RT fails
> > > because it eventually sets RT_SHMEM when #include'ing radixtree.h but
> > > it's missing some header files to include. I've attached a patch to
> > > include necessary header files in radixtree.h to make it
> > > self-contained.
> >
> > If those includes are only needed when RT_SHMEM is defined, I suggest
> > they should be guarded by #ifdef RT_SHMEM, per Peter E's IWYU efforts
> > lately.
>
> Indeed. I'll incorporate it in the next version.
>

I've attached the updated patches. Please review them.

I think we should backpatch the fix for radixtree.h so I kept these
changes separated from other changes.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com