On 18.10.24 21:21, Dmitry Dolgov wrote:
v1-0001-Allow-to-use-multiple-shared-memory-mappings.patch
Preparation, introduces the possibility to work with many shmem mappings. To
make it less invasive, I've duplicated the shmem API to extend it with the
shmem_slot argument, while redirecting the original API to it. There are
probably better ways of doing that, I'm open for suggestions.
After studying this a bit, I tend to think you should just change the
existing APIs in place. So for example,
void *ShmemAlloc(Size size);
becomes
void *ShmemAlloc(int shmem_slot, Size size);
There aren't that many callers, and all these duplicated interfaces
almost add more new code than they save.
It might be worth making exceptions for interfaces that are likely to be
used by extensions. For example, I see pg_stat_statements using
ShmemInitStruct() and ShmemInitHash(). But that seems to be it. Are
there any other examples out there? Maybe there are many more that I
don't see right now. But at least for the initialization functions, it
doesn't seem worth it to preserve the existing interfaces exactly.
In any case, I think the slot number should be the first argument. This
matches how MemoryContextAlloc() or also talloc() work.
(Now here is an idea: Could these just be memory contexts? Instead of
making six shared memory slots, could you make six memory contexts with
a special shared memory type. And ShmemAlloc becomes the allocation
function, etc.?)
I noticed the existing code made inconsistent use of PGShmemHeader * vs.
void *, which also bled into your patch. I made the attached little
patch to clean that up a bit.
I suggest splitting the struct ShmemSegment into one struct for the
three memory addresses and a separate array just for the slock_t's. The
former struct can then stay private in storage/ipc/shmem.c, only the
locks need to be exported.
Maybe rename ANON_MAPPINGS to something like NUM_ANON_MAPPINGS.
Also, maybe some of this should be declared in storage/shmem.h rather
than in storage/pg_shmem.h. We have the existing ShmemLock in there, so
it would be a bit confusing to have the per-segment locks elsewhere.
v1-0003-Introduce-multiple-shmem-slots-for-shared-buffers.patch
Splits shared_buffers into multiple slots, moving out structures that depend on
NBuffers into separate mappings. There are two large gaps here:
* Shmem size calculation for those mappings is not correct yet, it includes too
many other things (no particular issues here, just haven't had time).
* It makes hardcoded assumptions about what is the upper limit for resizing,
which is currently low purely for experiments. Ideally there should be a new
configuration option to specify the total available memory, which would be a
base for subsequent calculations.
Yes, I imagine a shared_buffers_hard_limit setting. We could maybe
default that to the total available memory, but it would also be good to
be able to specify it directly, for testing.
v1-0005-Use-anonymous-files-to-back-shared-memory-segment.patch
Allows an anonyous file to back a shared mapping. This makes certain things
easier, e.g. mappings visual representation, and gives an fd for possible
future customizations.
I think this could be a useful patch just by itself, without the rest of
the series, because of
> * By default, Linux will not add file-backed shared mappings into a
> core dump, making it more convenient to work with them in PostgreSQL:
> no more huge dumps to process.
This could be significant operational benefit.
When you say "by default", is this adjustable? Does someone actually
want the whole shared memory in their core file? (If it's adjustable,
is it also adjustable for anonymous mappings?)
I'm wondering about this change:
-#define PG_MMAP_FLAGS
(MAP_SHARED|MAP_ANONYMOUS|MAP_HASSEMAPHORE)
+#define PG_MMAP_FLAGS (MAP_SHARED|MAP_HASSEMAPHORE)
It looks like this would affect all mmap() calls, not only the one
you're changing. But that's the only one that uses this macro! I don't
understand why we need this; I don't see anything in the commit log
about this ever being used for any portability. I think we should just
get rid of it and have mmap() use the right flags directly.
I see that FreeBSD has a memfd_create() function. Might be worth a try.
Obviously, this whole thing needs a configure test for memfd_create()
anyway.
I see that memfd_create() has a MFD_HUGETLB flag. It's not very clear
how that interacts with the MAP_HUGETLB flag for mmap(). Do you need to
specify both of them if you want huge pages?
From 78562cb315da1cc5b35c07aba5a3fd7faacdad48 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 19 Nov 2024 13:15:15 +0100
Subject: [PATCH] More thorough use of PGShmemHeader * instead of void *
---
src/backend/port/sysv_shmem.c | 4 ++--
src/backend/port/win32_shmem.c | 4 ++--
src/backend/postmaster/launch_backend.c | 2 +-
src/backend/storage/ipc/shmem.c | 13 ++++---------
src/include/storage/pg_shmem.h | 2 +-
src/include/storage/shmem.h | 3 ++-
6 files changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 362a37d3b3a..fa6ee15ce56 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -92,7 +92,7 @@ typedef enum
unsigned long UsedShmemSegID = 0;
-void *UsedShmemSegAddr = NULL;
+PGShmemHeader *UsedShmemSegAddr = NULL;
static Size AnonymousShmemSize;
static void *AnonymousShmem = NULL;
@@ -892,7 +892,7 @@ PGSharedMemoryReAttach(void)
IpcMemoryId shmid;
PGShmemHeader *hdr;
IpcMemoryState state;
- void *origUsedShmemSegAddr = UsedShmemSegAddr;
+ PGShmemHeader *origUsedShmemSegAddr = UsedShmemSegAddr;
Assert(UsedShmemSegAddr != NULL);
Assert(IsUnderPostmaster);
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 3bcce9d3b63..827f9cd79b4 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -42,7 +42,7 @@
void *ShmemProtectiveRegion = NULL;
HANDLE UsedShmemSegID = INVALID_HANDLE_VALUE;
-void *UsedShmemSegAddr = NULL;
+PGShmemHeader *UsedShmemSegAddr = NULL;
static Size UsedShmemSegSize = 0;
static bool EnableLockPagesPrivilege(int elevel);
@@ -424,7 +424,7 @@ void
PGSharedMemoryReAttach(void)
{
PGShmemHeader *hdr;
- void *origUsedShmemSegAddr = UsedShmemSegAddr;
+ PGShmemHeader *origUsedShmemSegAddr = UsedShmemSegAddr;
Assert(ShmemProtectiveRegion != NULL);
Assert(UsedShmemSegAddr != NULL);
diff --git a/src/backend/postmaster/launch_backend.c
b/src/backend/postmaster/launch_backend.c
index 1f2d829ec5a..8f48c938968 100644
--- a/src/backend/postmaster/launch_backend.c
+++ b/src/backend/postmaster/launch_backend.c
@@ -94,7 +94,7 @@ typedef struct
void *ShmemProtectiveRegion;
HANDLE UsedShmemSegID;
#endif
- void *UsedShmemSegAddr;
+ PGShmemHeader *UsedShmemSegAddr;
slock_t *ShmemLock;
#ifdef USE_INJECTION_POINTS
struct InjectionPointsCtl *ActiveInjectionPoints;
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index 6d5f0839864..50f987ae240 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -92,18 +92,13 @@ static HTAB *ShmemIndex = NULL; /* primary index hashtable
for shmem */
/*
* InitShmemAccess() --- set up basic pointers to shared memory.
- *
- * Note: the argument should be declared "PGShmemHeader *seghdr",
- * but we use void to avoid having to include ipc.h in shmem.h.
*/
void
-InitShmemAccess(void *seghdr)
+InitShmemAccess(PGShmemHeader *seghdr)
{
- PGShmemHeader *shmhdr = (PGShmemHeader *) seghdr;
-
- ShmemSegHdr = shmhdr;
- ShmemBase = (void *) shmhdr;
- ShmemEnd = (char *) ShmemBase + shmhdr->totalsize;
+ ShmemSegHdr = seghdr;
+ ShmemBase = seghdr;
+ ShmemEnd = (char *) ShmemBase + seghdr->totalsize;
}
/*
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 3065ff5be71..7a07c5807ac 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -69,7 +69,7 @@ extern PGDLLIMPORT unsigned long UsedShmemSegID;
extern PGDLLIMPORT HANDLE UsedShmemSegID;
extern PGDLLIMPORT void *ShmemProtectiveRegion;
#endif
-extern PGDLLIMPORT void *UsedShmemSegAddr;
+extern PGDLLIMPORT PGShmemHeader *UsedShmemSegAddr;
#if !defined(WIN32) && !defined(EXEC_BACKEND)
#define DEFAULT_SHARED_MEMORY_TYPE SHMEM_TYPE_MMAP
diff --git a/src/include/storage/shmem.h b/src/include/storage/shmem.h
index 842989111c3..8cdbe7a89c8 100644
--- a/src/include/storage/shmem.h
+++ b/src/include/storage/shmem.h
@@ -27,7 +27,8 @@
/* shmem.c */
extern PGDLLIMPORT slock_t *ShmemLock;
-extern void InitShmemAccess(void *seghdr);
+struct PGShmemHeader; /* avoid including storage/pg_shmem.h
here */
+extern void InitShmemAccess(struct PGShmemHeader *seghdr);
extern void InitShmemAllocation(void);
extern void *ShmemAlloc(Size size);
extern void *ShmemAllocNoError(Size size);
--
2.47.0
