Remove PG_MMAP_FLAGS

Started by Ashutosh Bapat3 months ago9 messageshackers
Jump to latest
#1Ashutosh Bapat
ashutosh.bapat@enterprisedb.com

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+4-5
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Ashutosh Bapat (#1)
Re: Remove PG_MMAP_FLAGS

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

#3Michael Paquier
michael@paquier.xyz
In reply to: Tom Lane (#2)
Re: Remove PG_MMAP_FLAGS

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

#4Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Michael Paquier (#3)
Re: Remove PG_MMAP_FLAGS

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+6-7
#5Michael Paquier
michael@paquier.xyz
In reply to: Ashutosh Bapat (#4)
Re: Remove PG_MMAP_FLAGS

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

#6Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Michael Paquier (#5)
Re: Remove PG_MMAP_FLAGS

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+6-7
#7Michael Paquier
michael@paquier.xyz
In reply to: Ashutosh Bapat (#6)
Re: Remove PG_MMAP_FLAGS

On Fri, Jan 23, 2026 at 03:33:33PM +0530, Ashutosh Bapat wrote:

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.

WFM.
--
Michael

#8Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Michael Paquier (#7)
Re: Remove PG_MMAP_FLAGS

On Sat, Jan 24, 2026 at 8:16 AM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Jan 23, 2026 at 03:33:33PM +0530, Ashutosh Bapat wrote:

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.

WFM.

Thanks a lot for taking care of this.

--
Best Wishes,
Ashutosh Bapat

#9Michael Paquier
michael@paquier.xyz
In reply to: Ashutosh Bapat (#8)
Re: Remove PG_MMAP_FLAGS

On Tue, Jan 27, 2026 at 08:42:35AM +0530, Ashutosh Bapat wrote:

Thanks a lot for taking care of this.

Sure. For the sake of the archives: c100340729b6.
--
Michael