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

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

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)

On Mon, Nov 18, 2024 at 3:41 PM Heikki Linnakangas <hlinnaka(at)iki(dot)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

Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
> On Mon, Nov 18, 2024 at 3:41 PM Heikki Linnakangas <hlinnaka(at)iki(dot)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

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)

On Mon, Nov 18, 2024 at 4:50 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
> > On Mon, Nov 18, 2024 at 3:41 PM Heikki Linnakangas <hlinnaka(at)iki(dot)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

On Tue, Nov 19, 2024 at 1:14 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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

On Tue, Nov 19, 2024 at 2:37 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Nov 19, 2024 at 1:14 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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