Remove PG_MMAP_FLAGS
Hi,
Over [1] Peter mentioned that PG_MMAP_FLAGS is not used for
portability even though it's placed in portability/mem.h. That might
have been the intention when it was added in
b0fc0df9364d2d2d17c0162cf3b8b59f6cb09f67. But history does not show it
being used that way at any point in time. Per suggestion removing that
macro and instead using the flags directly in CreateAnonymousSegment()
which is the only place where it's used.
PFA patch for the same.
--
Best Wishes,
Ashutosh Bapat
Attachments:
v20260112-0001-Fix-PGShmemType-and-HugePagesType-typedefs.patchtext/x-patch; charset=US-ASCII; name=v20260112-0001-Fix-PGShmemType-and-HugePagesType-typedefs.patchDownload
From 8558a47360a31f9d9b10133702b40b1398bdce99 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Mon, 12 Jan 2026 14:34:01 +0530
Subject: [PATCH v20260112] Fix PGShmemType and HugePagesType typedefs
The automatic process for collecting typedefs in the buildfarm only picks up
typedef names that are used to declare objects (variables, struct fields,
function parameters or results). The typedef names mentioned in the subject are
not referenced at all, anywhere. Hence they are not picked up by the buildfarm
process causing their declarations to be wrongly indented. This commit changes
them to plain enums thus avoiding the wrong indentation.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Fix suggested by: Tom Lane <tgl@sss.pgh.pa.us>
---
src/include/storage/pg_shmem.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 3aeada554b2..b1803278542 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -48,21 +48,21 @@ extern PGDLLIMPORT int huge_page_size;
extern PGDLLIMPORT int huge_pages_status;
/* Possible values for huge_pages and huge_pages_status */
-typedef enum
+enum HugePagesType
{
HUGE_PAGES_OFF,
HUGE_PAGES_ON,
HUGE_PAGES_TRY, /* only for huge_pages */
HUGE_PAGES_UNKNOWN, /* only for huge_pages_status */
-} HugePagesType;
+};
/* Possible values for shared_memory_type */
-typedef enum
+enum PGShmemType
{
SHMEM_TYPE_WINDOWS,
SHMEM_TYPE_SYSV,
SHMEM_TYPE_MMAP,
-} PGShmemType;
+};
#ifndef WIN32
extern PGDLLIMPORT unsigned long UsedShmemSegID;
base-commit: 707f905399b4e47c295fe247f76fbbe53c737984
--
2.34.1
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
Over [1] Peter mentioned that PG_MMAP_FLAGS is not used for
portability even though it's placed in portability/mem.h. That might
have been the intention when it was added in
b0fc0df9364d2d2d17c0162cf3b8b59f6cb09f67. But history does not show it
being used that way at any point in time. Per suggestion removing that
macro and instead using the flags directly in CreateAnonymousSegment()
which is the only place where it's used.
I think you attached the wrong patch? This one doesn't touch
PG_MMAP_FLAGS.
regards, tom lane
On Thu, Jan 22, 2026 at 02:17:08PM -0500, Tom Lane wrote:
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
Over [1] Peter mentioned that PG_MMAP_FLAGS is not used for
portability even though it's placed in portability/mem.h. That might
have been the intention when it was added in
b0fc0df9364d2d2d17c0162cf3b8b59f6cb09f67. But history does not show it
being used that way at any point in time. Per suggestion removing that
macro and instead using the flags directly in CreateAnonymousSegment()
which is the only place where it's used.I think you attached the wrong patch? This one doesn't touch
PG_MMAP_FLAGS.
PG_MMAP_FLAGS is still used in two places in sysv_shmem.c, where I
guess the intention of Robert back in b0fc0df9364d was to not
copy-paste the same flag values multiple times. I can still get the
intention even today, so, if we were to do something, why don't you
just make PG_MMAP_FLAGS local to sysv_shmem.c and call it a day?
Honestly, I don't think that we should change this code at all: I also
like the current idea of PG_MMAP_FLAGS being defined in the same place
where we check for HASSEMAPHORE and ANONYMOUS, so it comes down to
this being a matter of taste.
--
Michael
On Fri, Jan 23, 2026 at 4:53 AM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Jan 22, 2026 at 02:17:08PM -0500, Tom Lane wrote:
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
Over [1] Peter mentioned that PG_MMAP_FLAGS is not used for
portability even though it's placed in portability/mem.h. That might
have been the intention when it was added in
b0fc0df9364d2d2d17c0162cf3b8b59f6cb09f67. But history does not show it
being used that way at any point in time. Per suggestion removing that
macro and instead using the flags directly in CreateAnonymousSegment()
which is the only place where it's used.I think you attached the wrong patch? This one doesn't touch
PG_MMAP_FLAGS.
I didn't do a good job writing that email: attached wrong patch and
also didn't provide the required reference. Sorry for that. Corrected
in this email.
PG_MMAP_FLAGS is still used in two places in sysv_shmem.c, where I
guess the intention of Robert back in b0fc0df9364d was to not
copy-paste the same flag values multiple times. I can still get the
intention even today, so, if we were to do something, why don't you
just make PG_MMAP_FLAGS local to sysv_shmem.c and call it a day?Honestly, I don't think that we should change this code at all: I also
like the current idea of PG_MMAP_FLAGS being defined in the same place
where we check for HASSEMAPHORE and ANONYMOUS, so it comes down to
this being a matter of taste.
As Peter mentioned in the shared buffers resizing thread [1]/messages/by-id/12add41a-7625-4639-a394-a5563e349322@eisentraut.org it's
placement and name in mem.h led us to think that it affects all mmap
calls or that all mmap calls should use those flags. Neither of which
is true. We have different mmap calls using different set of flags.
Attached patch proposes changes similar (not exact) as you suggest
above. Please take a look.
[1]: /messages/by-id/12add41a-7625-4639-a394-a5563e349322@eisentraut.org
--
Best Wishes,
Ashutosh Bapat
Attachments:
v20260123-0001-Remove-PG_MMAP_FLAGS.patchtext/x-patch; charset=US-ASCII; name=v20260123-0001-Remove-PG_MMAP_FLAGS.patchDownload
From 7e8a4457c8640fd62cf562e98c0b0df9ae80157f Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Thu, 22 Jan 2026 20:48:36 +0530
Subject: [PATCH v20260123] Remove PG_MMAP_FLAGS
Though the macro seems to be something meant to be used for all mmap calls, it
is actually used only in CreateAnonymousSegment(). Other calls to mmap() use
different set of flags. The macros was introduced in commit
b0fc0df9364d2d2d17c0162cf3b8b59f6cb09f67, but was never used for portability.
Remove it and use the flags directly in CreateAnonymousSegment().
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Suggested by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://www.postgresql.org/message-id/12add41a-7625-4639-a394-a5563e349322@eisentraut.org
---
src/backend/port/sysv_shmem.c | 10 ++++++----
src/include/portability/mem.h | 2 --
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index de491897118..0f4ecac16e8 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -601,6 +601,8 @@ CreateAnonymousSegment(Size *size)
Size allocsize = *size;
void *ptr = MAP_FAILED;
int mmap_errno = 0;
+ int mmap_flags = (MAP_SHARED | MAP_ANONYMOUS | MAP_HASSEMAPHORE);
+
#ifndef MAP_HUGETLB
/* PGSharedMemoryCreate should have dealt with this case */
@@ -612,15 +614,15 @@ CreateAnonymousSegment(Size *size)
* Round up the request size to a suitable large value.
*/
Size hugepagesize;
- int mmap_flags;
+ int huge_mmap_flags;
- GetHugePageSize(&hugepagesize, &mmap_flags);
+ GetHugePageSize(&hugepagesize, &huge_mmap_flags);
if (allocsize % hugepagesize != 0)
allocsize += hugepagesize - (allocsize % hugepagesize);
ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
- PG_MMAP_FLAGS | mmap_flags, -1, 0);
+ mmap_flags | huge_mmap_flags, -1, 0);
mmap_errno = errno;
if (huge_pages == HUGE_PAGES_TRY && ptr == MAP_FAILED)
elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
@@ -644,7 +646,7 @@ CreateAnonymousSegment(Size *size)
*/
allocsize = *size;
ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
- PG_MMAP_FLAGS, -1, 0);
+ mmap_flags, -1, 0);
mmap_errno = errno;
}
diff --git a/src/include/portability/mem.h b/src/include/portability/mem.h
index 091328f680d..c048e8836c5 100644
--- a/src/include/portability/mem.h
+++ b/src/include/portability/mem.h
@@ -38,8 +38,6 @@
#define MAP_NOSYNC 0
#endif
-#define PG_MMAP_FLAGS (MAP_SHARED|MAP_ANONYMOUS|MAP_HASSEMAPHORE)
-
/* Some really old systems don't define MAP_FAILED. */
#ifndef MAP_FAILED
#define MAP_FAILED ((void *) -1)
base-commit: f9a468c664a4605e3080383ffaa302057d56feb1
--
2.34.1
On Fri, Jan 23, 2026 at 08:04:18AM +0530, Ashutosh Bapat wrote:
As Peter mentioned in the shared buffers resizing thread [1] it's
placement and name in mem.h led us to think that it affects all mmap
calls or that all mmap calls should use those flags. Neither of which
is true. We have different mmap calls using different set of flags.
Attached patch proposes changes similar (not exact) as you suggest
above. Please take a look.[1] /messages/by-id/12add41a-7625-4639-a394-a5563e349322@eisentraut.org
Okay, point taken. I can live with your patch suggestion based on
what you have attached.
--
Michael
On Fri, Jan 23, 2026 at 10:12 AM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jan 23, 2026 at 08:04:18AM +0530, Ashutosh Bapat wrote:
As Peter mentioned in the shared buffers resizing thread [1] it's
placement and name in mem.h led us to think that it affects all mmap
calls or that all mmap calls should use those flags. Neither of which
is true. We have different mmap calls using different set of flags.
Attached patch proposes changes similar (not exact) as you suggest
above. Please take a look.[1] /messages/by-id/12add41a-7625-4639-a394-a5563e349322@eisentraut.org
Okay, point taken. I can live with your patch suggestion based on
what you have attached.
Thanks.
I revised the patch a bit. The macro had parenthesis around the macro
definition which are not required in the assignment. Removed them.
Also revised the commit message a bit.
--
Best Wishes,
Ashutosh Bapat
Attachments:
v20260123_2-0001-Remove-PG_MMAP_FLAGS.patchtext/x-patch; charset=US-ASCII; name=v20260123_2-0001-Remove-PG_MMAP_FLAGS.patchDownload
From 5299c2f228f1ff2110511faca661aa2f63adca26 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Thu, 22 Jan 2026 20:48:36 +0530
Subject: [PATCH v20260123] Remove PG_MMAP_FLAGS
Based on the file and name of the macro, it seems that the macro was meant to be
used for all mmap() calls. But it is actually used only in
CreateAnonymousSegment(). Each of the other calls to mmap() use flags according
to the requirement of the caller. The macro was introduced in commit
b0fc0df9364d2d2d17c0162cf3b8b59f6cb09f67, but was never used for portability.
Remove it and use the flags directly in CreateAnonymousSegment().
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Suggested by: Peter Eisentraut <peter@eisentraut.org>
Discussion: https://www.postgresql.org/message-id/12add41a-7625-4639-a394-a5563e349322@eisentraut.org
---
src/backend/port/sysv_shmem.c | 10 ++++++----
src/include/portability/mem.h | 2 --
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index de491897118..eaa28c9c1e6 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -601,6 +601,8 @@ CreateAnonymousSegment(Size *size)
Size allocsize = *size;
void *ptr = MAP_FAILED;
int mmap_errno = 0;
+ int mmap_flags = MAP_SHARED | MAP_ANONYMOUS | MAP_HASSEMAPHORE;
+
#ifndef MAP_HUGETLB
/* PGSharedMemoryCreate should have dealt with this case */
@@ -612,15 +614,15 @@ CreateAnonymousSegment(Size *size)
* Round up the request size to a suitable large value.
*/
Size hugepagesize;
- int mmap_flags;
+ int huge_mmap_flags;
- GetHugePageSize(&hugepagesize, &mmap_flags);
+ GetHugePageSize(&hugepagesize, &huge_mmap_flags);
if (allocsize % hugepagesize != 0)
allocsize += hugepagesize - (allocsize % hugepagesize);
ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
- PG_MMAP_FLAGS | mmap_flags, -1, 0);
+ mmap_flags | huge_mmap_flags, -1, 0);
mmap_errno = errno;
if (huge_pages == HUGE_PAGES_TRY && ptr == MAP_FAILED)
elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
@@ -644,7 +646,7 @@ CreateAnonymousSegment(Size *size)
*/
allocsize = *size;
ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
- PG_MMAP_FLAGS, -1, 0);
+ mmap_flags, -1, 0);
mmap_errno = errno;
}
diff --git a/src/include/portability/mem.h b/src/include/portability/mem.h
index 091328f680d..c048e8836c5 100644
--- a/src/include/portability/mem.h
+++ b/src/include/portability/mem.h
@@ -38,8 +38,6 @@
#define MAP_NOSYNC 0
#endif
-#define PG_MMAP_FLAGS (MAP_SHARED|MAP_ANONYMOUS|MAP_HASSEMAPHORE)
-
/* Some really old systems don't define MAP_FAILED. */
#ifndef MAP_FAILED
#define MAP_FAILED ((void *) -1)
base-commit: a36164e7465fd1e10e94569e9c1c07656e38a9de
--
2.34.1