From 677a089ac8a8e0f9e700d60bbe59fc91f5b5276b Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Mon, 6 Jan 2025 14:40:51 +0530 Subject: [PATCH 08/11] Add TODOs and questions about previous commits The commit just marks the places which need more work or whether the code raises some questions. This is not an exhaustive list of TODOs. More TODOs may come up as I work with these patches further. Ashutosh Bapat --- src/backend/port/sysv_shmem.c | 8 +++++- src/backend/storage/buffer/buf_init.c | 12 +++++++++ src/backend/storage/buffer/bufmgr.c | 5 ++++ src/backend/storage/buffer/freelist.c | 3 +++ src/backend/storage/ipc/ipc.c | 4 +++ src/backend/storage/ipc/ipci.c | 10 +++++++ src/backend/storage/ipc/shmem.c | 26 ++++++++++++++++--- src/backend/storage/lmgr/lwlock.c | 5 ++++ src/backend/tcop/postgres.c | 6 +++++ .../utils/activity/wait_event_names.txt | 1 + src/include/storage/buf_internals.h | 5 ++++ src/include/storage/pg_shmem.h | 4 +++ 12 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c index 992ed849dc0..2b144d45cf0 100644 --- a/src/backend/port/sysv_shmem.c +++ b/src/backend/port/sysv_shmem.c @@ -1011,7 +1011,13 @@ AnonymousShmemResize(void) LWLockRelease(ShmemResizeLock); } - } + + /* + * TODO: Shouldn't we call ResizeBufferPool() here as well? Or those + * backend who can not lock the LWLock conditionally won't resize the + * buffers. + */ + } return true; } diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c index b7de0ab6b0d..248fbf1633b 100644 --- a/src/backend/storage/buffer/buf_init.c +++ b/src/backend/storage/buffer/buf_init.c @@ -211,6 +211,12 @@ BufferManagerShmemInit(void) * initNew flag indicates that the caller wants new buffers to be initialized. * No locks are taking in this function, it is the caller responsibility to * make sure only one backend can work with new buffers. + * + * TODO: Avoid code duplication with BufferManagerShmemInit() and also assess + * which functionality in the latter is required in this function. + * similar to BufferManagerShmemInit, but applied only to the buffers in the + * range between NBuffersOld and NBuffers. + * */ void ResizeBufferPool(int NBuffersOld, bool initNew) @@ -293,6 +299,12 @@ ResizeBufferPool(int NBuffersOld, bool initNew) } /* Correct last entry of linked list */ + /* + * TODO: I think this needs to be done only when expanding the buffers. + * + * TODO: We should also fix the freelist to not point to a shrunk + * buffer and to append the new buffers to the existing free list. + */ GetBufferDescriptor(NBuffers - 1)->freeNext = FREENEXT_END_OF_LIST; /* Init other shared buffer-management stuff */ diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 80b0d0c5ded..b6bec73e6b7 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2974,6 +2974,11 @@ BufferSync(int flags) UnlockBufHdr(bufHdr, buf_state); /* Check for barrier events in case NBuffers is large. */ + /* + * TODO: If we allow buffer resizing while this loop is being executed, + * the loop will need to consider the new value of NBuffers. Hence avoid + * resizing if this loop is being executed. + */ if (ProcSignalBarrierPending) ProcessProcSignalBarrier(); } diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 4919a92f2be..45a6e768332 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -484,6 +484,9 @@ StrategyInitialize(bool init) * a new entry before deleting the old. In principle this could be * happening in each partition concurrently, so we could need as many as * NBuffers + NUM_BUFFER_PARTITIONS entries. + * + * TODO: If we are resizing, we need to preserve the earlier entries, don't + * we? */ InitBufTable(NBuffers + NUM_BUFFER_PARTITIONS); diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c index 9d526eb43fd..38bd6f41130 100644 --- a/src/backend/storage/ipc/ipc.c +++ b/src/backend/storage/ipc/ipc.c @@ -70,6 +70,10 @@ static void proc_exit_prepare(int code); * ---------------------------------------------------------------- */ +/* + * TODO: Why do we need to increase this by 20? I didn't notice any new calls to + * on_shmem_exit or on_proc_exit or before_shmem_exit. + */ #define MAX_ON_EXITS 40 struct ONEXIT diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index a2c635f288e..029ad28fe0b 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -204,6 +204,12 @@ AttachSharedMemoryStructs(void) /* * CreateSharedMemoryAndSemaphores * Creates and initializes shared memory and semaphores. + * + * TODO: IMO this function should be rewritten to calculate the size of each + * shared memory slot or mapping. Instead of passing slot number to + * CalculateShmemSize, we should instead let each shared memory module use their + * own slot number and update the required sizes in the corresponding mapping. + * Then allocate shared memory in each of the mappings. */ void CreateSharedMemoryAndSemaphores(void) @@ -225,6 +231,10 @@ CreateSharedMemoryAndSemaphores(void) * Create the shmem segment. * * XXX: Do multiple shims are needed, one per segment? + * + * TODO: while each slot will return a different shim, only the last one + * is passed to dsm_postmaster_startup(). Is that right? Shouldn't we + * pass all of them or none. */ seghdr = PGSharedMemoryCreate(size, &shim); diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index 226b38ba979..b569d125507 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -86,6 +86,11 @@ ShmemSegment Segments[ANON_MAPPINGS]; * Primary index hashtable for shmem, for simplicity we use a single for all * shared memory segments. There can be performance consequences of that, and * an alternative option would be to have one index per shared memory segments. + * + * TODO: shouldn't this be part of the ShmemSegment structure? Some shared + * memory segments that hold only one structure do not need their pointers to be + * stored in the shared hash table, instead they could be part of the segments + * itself. */ static HTAB *ShmemIndex = NULL; @@ -493,9 +498,18 @@ ShmemInitStructInSegment(const char *name, Size size, bool *foundPtr, { /* * Structure is in the shmem index so someone else has allocated it - * already. Verify the structure's size: - * - If it's the same, we've found the expected structure. - * - If it's different, we're resizing the expected structure. + * already. Verify the structure's size: - If it's the same, we've found + * the expected structure. - If it's different, we're resizing the + * expected structure. + * + * TODO: This works because every structure that needs to be resized + * resides in a shmem slot by itself. But it won't work if a slot + * contains more structures, that need to be resized, placed in adjacent + * memory. Also we are not updating the Shmem stats like freeoffset. I + * think we will keep all resizable structures in a slot for themselves, + * and not have a hash table in such slots since resizing the hash table + * itself might cause memory to be allocated next to the resizable + * structure making it difficult to resize it. */ if (result->size != size) result->size = size; @@ -587,6 +601,12 @@ pg_get_shmem_allocations(PG_FUNCTION_ARGS) hash_seq_init(&hstat, ShmemIndex); + /* + * TODO: For the sake of completeness we should rotate through all the slots + * (after saving slotwise ShmemIndex, if any). Do we want to also output + * shmem slot name, but that would expose the slotified structure of shared + * memory. + */ /* output all allocated entries */ memset(nulls, 0, sizeof(nulls)); /* XXX: take all shared memory segments into account. */ diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 40aa4014b5f..8b6fe9b24f7 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -608,6 +608,11 @@ LWLockNewTrancheId(void) LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int)); /* We use the ShmemLock spinlock to protect LWLockCounter */ + /* + * TODO: We have retained ShmemLock global variable, should we use it here + * instead of main segment lock? We will need spinlock init on the global + * one if yes. + */ SpinLockAcquire(Segments[MAIN_SHMEM_SEGMENT].ShmemLock); result = (*LWLockCounter)++; SpinLockRelease(Segments[MAIN_SHMEM_SEGMENT].ShmemLock); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 04cdd0d24d8..e88448f2846 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4671,6 +4671,12 @@ PostgresMain(const char *dbname, const char *username) /* * (6) check for any other interesting events that happened while we * slept. + * TODO: When a backend is waiting for a command, it won't reload + * configuration and hence wouldn't notice change in shared_buffers. The + * change is only noticed after the command is received and the control + * comes here. We may need to improve this in case we want to resize + * shared buffers or perform of part of that operation in assign_hook + * implementation (e.g. AnonymousShmemResize()). */ if (ConfigReloadPending) { diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt index 947f13cb1fa..4203c987edc 100644 --- a/src/backend/utils/activity/wait_event_names.txt +++ b/src/backend/utils/activity/wait_event_names.txt @@ -348,6 +348,7 @@ WALSummarizer "Waiting to read or update WAL summarization state." DSMRegistry "Waiting to read or update the dynamic shared memory registry." InjectionPoint "Waiting to read or update information related to injection points." SerialControl "Waiting to read or update shared pg_serial state." +# TODO, not used anywhere, do we need it? ShmemResize "Waiting to resize shared memory." # diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 4595f5a9676..416c405fe4e 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -22,6 +22,11 @@ #include "storage/condition_variable.h" #include "storage/lwlock.h" #include "storage/shmem.h" +/* + * TODO: this header files doesn't use anything in pg_shmem.h but the files which + * include this file may. We should include pg_shmem.h in those files rather than + * here. + */ #include "storage/pg_shmem.h" #include "storage/smgr.h" #include "storage/spin.h" diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h index b597df0d3a3..3f103d708a5 100644 --- a/src/include/storage/pg_shmem.h +++ b/src/include/storage/pg_shmem.h @@ -43,6 +43,10 @@ typedef struct PGShmemHeader /* standard header for all Postgres shmem */ #endif } PGShmemHeader; +/* + * TODO: should we define it in shmem.c where the previous global variables were + * declared? Do we need this structure outside shmem.c? + */ typedef struct ShmemSegment { PGShmemHeader *ShmemSegHdr; /* shared mem segment header */ -- 2.34.1