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
Attachments:
v1-0001-Include-necessary-header-files-in-radixtree.h.patchapplication/octet-stream; name=v1-0001-Include-necessary-header-files-in-radixtree.h.patchDownload
From 4fb26c805a76e0496f4c4e847c011f32e1fe3739 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 18 Nov 2024 15:11:08 -0800
Subject: [PATCH v1] Include necessary header files in radixtree.h.
When #include'ing radix_tree.h with RT_SHMEM, it could happen to raise
copiler errors due to missing the declarations of types and
functions.
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/include/lib/radixtree.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 88bf695e3f3..d6443c84a41 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -153,11 +153,13 @@
#include "postgres.h"
+#include "miscadmin.h"
#include "nodes/bitmapset.h"
#include "port/pg_bitutils.h"
#include "port/simd.h"
#include "utils/dsa.h"
#include "utils/memutils.h"
+#include "storage/lwlock.h"
/* helpers */
#define RT_MAKE_PREFIX(a) CppConcat(a,_)
--
2.43.5
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@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
Attachments:
v2-0001-Include-necessary-header-files-in-radixtree.h.patchapplication/octet-stream; name=v2-0001-Include-necessary-header-files-in-radixtree.h.patchDownload
From 291ecfb546a99a33c2bd09107c67e4a10d6e3719 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 18 Nov 2024 15:11:08 -0800
Subject: [PATCH v2] Include necessary header files in radixtree.h.
When #include'ing radixtree.h with RT_SHMEM, it could happen to raise
compiler errors due to missing some declarations of types and
functions.
This commit also removes the inclusion of postgres.h since it's
against our usual convention.
Reviewed-by: Heikki Linnakangas
Discussion: https://postgr.es/m/CAD21AoCU9YH%2Bb9Rr8YRw7UjmB%3D1zh8GKQkWNiuN9mVhMvkyrRg%40mail.gmail.com
---
src/include/lib/radixtree.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 88bf695e3f3..6bb0983a3bf 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -151,11 +151,11 @@
*-------------------------------------------------------------------------
*/
-#include "postgres.h"
-
+#include "miscadmin.h"
#include "nodes/bitmapset.h"
#include "port/pg_bitutils.h"
#include "port/simd.h"
+#include "storage/lwlock.h"
#include "utils/dsa.h"
#include "utils/memutils.h"
--
2.43.5
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
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@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
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
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
Attachments:
v3-0001-Remove-inclusion-of-fundamental-header-files-from.patchapplication/octet-stream; name=v3-0001-Remove-inclusion-of-fundamental-header-files-from.patchDownload
From abc993eb92e5839589652d42d979a3318bfc0c08 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 25 Nov 2024 21:29:07 -0800
Subject: [PATCH v3 1/2] Remove inclusion of fundamental header files from some
header files.
This commit removes fundamental header files such as "c.h" and
"postgres.h" from some header files since it's against our usual
convention.
Reported-by: Tom Lane
Reviewed-by: Heikki Linnakangas
Discussion: https://postgr.es/m/CAD21AoCU9YH%2Bb9Rr8YRw7UjmB%3D1zh8GKQkWNiuN9mVhMvkyrRg%40mail.gmail.com
---
src/bin/pg_combinebackup/copy_file.h | 1 -
src/include/fe_utils/option_utils.h | 2 --
src/include/fe_utils/query_utils.h | 2 --
3 files changed, 5 deletions(-)
diff --git a/src/bin/pg_combinebackup/copy_file.h b/src/bin/pg_combinebackup/copy_file.h
index d5ddc7e53d7..ecc1f3fc7ef 100644
--- a/src/bin/pg_combinebackup/copy_file.h
+++ b/src/bin/pg_combinebackup/copy_file.h
@@ -11,7 +11,6 @@
#ifndef COPY_FILE_H
#define COPY_FILE_H
-#include "c.h"
#include "common/checksum_helper.h"
/*
diff --git a/src/include/fe_utils/option_utils.h b/src/include/fe_utils/option_utils.h
index afd108fdabf..ff1d22459d3 100644
--- a/src/include/fe_utils/option_utils.h
+++ b/src/include/fe_utils/option_utils.h
@@ -12,8 +12,6 @@
#ifndef OPTION_UTILS_H
#define OPTION_UTILS_H
-#include "postgres_fe.h"
-
#include "common/file_utils.h"
typedef void (*help_handler) (const char *progname);
diff --git a/src/include/fe_utils/query_utils.h b/src/include/fe_utils/query_utils.h
index 9a680d5bffe..e7403d9cbd4 100644
--- a/src/include/fe_utils/query_utils.h
+++ b/src/include/fe_utils/query_utils.h
@@ -12,8 +12,6 @@
#ifndef QUERY_UTILS_H
#define QUERY_UTILS_H
-#include "postgres_fe.h"
-
#include "libpq-fe.h"
extern PGresult *executeQuery(PGconn *conn, const char *query, bool echo);
--
2.43.5
v3-0002-Include-necessary-header-files-in-radixtree.h.patchapplication/octet-stream; name=v3-0002-Include-necessary-header-files-in-radixtree.h.patchDownload
From c48346425ada1fa16a50b847d8c8b337f02f8fe5 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 25 Nov 2024 21:32:09 -0800
Subject: [PATCH v3 2/2] Include necessary header files in radixtree.h.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When #include'ing radixtree.h with RT_SHMEM, it could happen to raise
compiler errors due to missing some declarations of types and
functions.
This commit also removes the inclusion of postgres.h since it's
against our usual convention.
Backpatch to v17, where radixtree.h was introduced.
Reviewed-by: Heikki Linnakangas, Álvaro Herrera
Discussion: https://postgr.es/m/CAD21AoCU9YH%2Bb9Rr8YRw7UjmB%3D1zh8GKQkWNiuN9mVhMvkyrRg%40mail.gmail.com
Backpatch-through: 17
---
src/include/lib/radixtree.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 88bf695e3f3..1301f3fee44 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -151,13 +151,15 @@
*-------------------------------------------------------------------------
*/
-#include "postgres.h"
-
#include "nodes/bitmapset.h"
#include "port/pg_bitutils.h"
#include "port/simd.h"
#include "utils/dsa.h"
#include "utils/memutils.h"
+#ifdef RT_SHMEM
+#include "miscadmin.h"
+#include "storage/lwlock.h"
+#endif
/* helpers */
#define RT_MAKE_PREFIX(a) CppConcat(a,_)
--
2.43.5