On Fri, Nov 13, 2015 at 11:37 AM, Ildus Kurbangaliev <i.kurbangal...@postgrespro.ru> wrote: > Thanks! That's very inspiring.
Cool. It was great work; I am very happy with how it turned out. > I have some questions about next steps on other tranches. > First of all, I think we can have two API calls, something like: > > 1) LWLockRequestTranche(char *tranche_name, int locks_count) > 2) LWLockGetTranche(char *tranche_name) > > LWLockRequestTranche reserve an item in main tranches array in shared memory, > and > allocates space for name, LWLockTranche and LWLocks. There is first question. > It is > ok if this call will be from *ShmemSize functions? We keep those requests, > calculate a required size in LWLockShmemSize (by moving this call to the end) > and create all these tranches in CreateLWLocks. I think what we should do about the buffer locks is polish up this patch and get it applied: http://www.postgresql.org/message-id/20150907175909.gd5...@alap3.anarazel.de I think it needs to be adapted to use predefined constants for the tranche IDs instead of hardcoded values, maybe based on the attached tranche-constants.patch. This approach could be extended for the other stuff in the main tranche, and I think that would be cleaner than inventing LWLockRequestTranche as you are proposing. Just make the tranche IDs constants, and then the rest fits nicely into the framework we've already got. Also, I think we should rip all the volatile qualifiers out of bufmgr.c, using the second attached patch, devolatileize-bufmgr.patch. The comment claiming that we need this because spinlocks aren't compiler barriers is obsolete. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
commit bbf99acca380543c2943c79454ac07b97fd6f2f2 Author: Robert Haas <rh...@postgresql.org> Date: Sun Nov 15 10:37:19 2015 -0500 tranche-ify buffer locks diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 6bca02c..43534d6 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -445,7 +445,7 @@ CreateLWLocks(void) /* Initialize all LWLocks in main array */ for (id = 0, lock = MainLWLockArray; id < numLocks; id++, lock++) - LWLockInitialize(&lock->lock, 0); + LWLockInitialize(&lock->lock, LWTRANCHE_MAIN); /* * Initialize the dynamic-allocation counters, which are stored just @@ -457,7 +457,7 @@ CreateLWLocks(void) LWLockCounter = (int *) ((char *) MainLWLockArray - 3 * sizeof(int)); LWLockCounter[0] = NUM_FIXED_LWLOCKS; LWLockCounter[1] = numLocks; - LWLockCounter[2] = 1; /* 0 is the main array */ + LWLockCounter[2] = LWTRANCHE_LAST_BUILTIN_ID + 1; } if (LWLockTrancheArray == NULL) @@ -466,12 +466,13 @@ CreateLWLocks(void) LWLockTrancheArray = (LWLockTranche **) MemoryContextAlloc(TopMemoryContext, LWLockTranchesAllocated * sizeof(LWLockTranche *)); + Assert(LWLockTranchesAllocated > LWTRANCHE_LAST_BUILTIN_ID); } MainLWLockTranche.name = "main"; MainLWLockTranche.array_base = MainLWLockArray; MainLWLockTranche.array_stride = sizeof(LWLockPadded); - LWLockRegisterTranche(0, &MainLWLockTranche); + LWLockRegisterTranche(LWTRANCHE_MAIN, &MainLWLockTranche); } /* diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index 4653e09..a5fbd20 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -177,6 +177,15 @@ extern void LWLockRegisterTranche(int tranche_id, LWLockTranche *tranche); extern void LWLockInitialize(LWLock *lock, int tranche_id); /* + * We reserve a few predefined tranche IDs. These values will never be + * returned by LWLockNewTrancheId. + */ +#define LWTRANCHE_MAIN 0 +#define LWTRANCHE_BUFFER_CONTENT 1 +#define LWTRANCHE_BUFFER_IO_IN_PROGRESS 2 +#define LWTRANCHE_LAST_BUILTIN_ID LWTRANCHE_BUFFER_IO_IN_PROGRESS + +/* * Prior to PostgreSQL 9.4, we used an enum type called LWLockId to refer * to LWLocks. New code should instead use LWLock *. However, for the * convenience of third-party code, we include the following typedef.
From 9e7f9219b5e752da46be0e26a0be074191ae8f62 Mon Sep 17 00:00:00 2001 From: Robert Haas <rh...@postgresql.org> Date: Sun, 15 Nov 2015 10:24:03 -0500 Subject: [PATCH 1/3] De-volatile-ize. --- src/backend/storage/buffer/bufmgr.c | 95 +++++++++++++++++------------------ src/backend/storage/buffer/freelist.c | 18 +++---- src/include/storage/buf_internals.h | 11 ++-- 3 files changed, 59 insertions(+), 65 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 8c0358e..63188a3 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -92,11 +92,11 @@ int effective_io_concurrency = 0; int target_prefetch_pages = 0; /* local state for StartBufferIO and related functions */ -static volatile BufferDesc *InProgressBuf = NULL; +static BufferDesc *InProgressBuf = NULL; static bool IsForInput; /* local state for LockBufferForCleanup */ -static volatile BufferDesc *PinCountWaitBuf = NULL; +static BufferDesc *PinCountWaitBuf = NULL; /* * Backend-Private refcount management: @@ -395,24 +395,24 @@ static Buffer ReadBuffer_common(SMgrRelation reln, char relpersistence, ForkNumber forkNum, BlockNumber blockNum, ReadBufferMode mode, BufferAccessStrategy strategy, bool *hit); -static bool PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy); -static void PinBuffer_Locked(volatile BufferDesc *buf); -static void UnpinBuffer(volatile BufferDesc *buf, bool fixOwner); +static bool PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy); +static void PinBuffer_Locked(BufferDesc *buf); +static void UnpinBuffer(BufferDesc *buf, bool fixOwner); static void BufferSync(int flags); static int SyncOneBuffer(int buf_id, bool skip_recently_used); -static void WaitIO(volatile BufferDesc *buf); -static bool StartBufferIO(volatile BufferDesc *buf, bool forInput); -static void TerminateBufferIO(volatile BufferDesc *buf, bool clear_dirty, +static void WaitIO(BufferDesc *buf); +static bool StartBufferIO(BufferDesc *buf, bool forInput); +static void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, int set_flag_bits); static void shared_buffer_write_error_callback(void *arg); static void local_buffer_write_error_callback(void *arg); -static volatile BufferDesc *BufferAlloc(SMgrRelation smgr, +static BufferDesc *BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, BlockNumber blockNum, BufferAccessStrategy strategy, bool *foundPtr); -static void FlushBuffer(volatile BufferDesc *buf, SMgrRelation reln); +static void FlushBuffer(BufferDesc *buf, SMgrRelation reln); static void AtProcExit_Buffers(int code, Datum arg); static void CheckForBufferLeaks(void); static int rnode_comparator(const void *p1, const void *p2); @@ -663,7 +663,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, BlockNumber blockNum, ReadBufferMode mode, BufferAccessStrategy strategy, bool *hit) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; Block bufBlock; bool found; bool isExtend; @@ -927,7 +927,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, * * No locks are held either at entry or exit. */ -static volatile BufferDesc * +static BufferDesc * BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, BlockNumber blockNum, BufferAccessStrategy strategy, @@ -941,7 +941,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, LWLock *oldPartitionLock; /* buffer partition lock for it */ BufFlags oldFlags; int buf_id; - volatile BufferDesc *buf; + BufferDesc *buf; bool valid; /* create a tag so we can lookup the buffer */ @@ -1281,7 +1281,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, * to acquire the necessary locks; if so, don't mess it up. */ static void -InvalidateBuffer(volatile BufferDesc *buf) +InvalidateBuffer(BufferDesc *buf) { BufferTag oldTag; uint32 oldHash; /* hash value for oldTag */ @@ -1380,7 +1380,7 @@ retry: void MarkBufferDirty(Buffer buffer) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; if (!BufferIsValid(buffer)) elog(ERROR, "bad buffer ID: %d", buffer); @@ -1436,7 +1436,7 @@ ReleaseAndReadBuffer(Buffer buffer, BlockNumber blockNum) { ForkNumber forkNum = MAIN_FORKNUM; - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; if (BufferIsValid(buffer)) { @@ -1485,7 +1485,7 @@ ReleaseAndReadBuffer(Buffer buffer, * some callers to avoid an extra spinlock cycle. */ static bool -PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy) +PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy) { Buffer b = BufferDescriptorGetBuffer(buf); bool result; @@ -1547,7 +1547,7 @@ PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy) * its state can change under us. */ static void -PinBuffer_Locked(volatile BufferDesc *buf) +PinBuffer_Locked(BufferDesc *buf) { Buffer b; PrivateRefCountEntry *ref; @@ -1578,7 +1578,7 @@ PinBuffer_Locked(volatile BufferDesc *buf) * Those that don't should pass fixOwner = FALSE. */ static void -UnpinBuffer(volatile BufferDesc *buf, bool fixOwner) +UnpinBuffer(BufferDesc *buf, bool fixOwner) { PrivateRefCountEntry *ref; Buffer b = BufferDescriptorGetBuffer(buf); @@ -1672,7 +1672,7 @@ BufferSync(int flags) num_to_write = 0; for (buf_id = 0; buf_id < NBuffers; buf_id++) { - volatile BufferDesc *bufHdr = GetBufferDescriptor(buf_id); + BufferDesc *bufHdr = GetBufferDescriptor(buf_id); /* * Header spinlock is enough to examine BM_DIRTY, see comment in @@ -1707,7 +1707,7 @@ BufferSync(int flags) num_written = 0; while (num_to_scan-- > 0) { - volatile BufferDesc *bufHdr = GetBufferDescriptor(buf_id); + BufferDesc *bufHdr = GetBufferDescriptor(buf_id); /* * We don't need to acquire the lock here, because we're only looking @@ -2079,7 +2079,7 @@ BgBufferSync(void) static int SyncOneBuffer(int buf_id, bool skip_recently_used) { - volatile BufferDesc *bufHdr = GetBufferDescriptor(buf_id); + BufferDesc *bufHdr = GetBufferDescriptor(buf_id); int result = 0; ReservePrivateRefCountEntry(); @@ -2252,7 +2252,7 @@ CheckForBufferLeaks(void) void PrintBufferLeakWarning(Buffer buffer) { - volatile BufferDesc *buf; + BufferDesc *buf; int32 loccount; char *path; BackendId backend; @@ -2324,7 +2324,7 @@ BufmgrCommit(void) BlockNumber BufferGetBlockNumber(Buffer buffer) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; Assert(BufferIsPinned(buffer)); @@ -2346,7 +2346,7 @@ void BufferGetTag(Buffer buffer, RelFileNode *rnode, ForkNumber *forknum, BlockNumber *blknum) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; /* Do the same checks as BufferGetBlockNumber. */ Assert(BufferIsPinned(buffer)); @@ -2382,7 +2382,7 @@ BufferGetTag(Buffer buffer, RelFileNode *rnode, ForkNumber *forknum, * as the second parameter. If not, pass NULL. */ static void -FlushBuffer(volatile BufferDesc *buf, SMgrRelation reln) +FlushBuffer(BufferDesc *buf, SMgrRelation reln) { XLogRecPtr recptr; ErrorContextCallback errcallback; @@ -2520,7 +2520,7 @@ RelationGetNumberOfBlocksInFork(Relation relation, ForkNumber forkNum) bool BufferIsPermanent(Buffer buffer) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; /* Local buffers are used only for temp relations. */ if (BufferIsLocal(buffer)) @@ -2550,7 +2550,7 @@ BufferIsPermanent(Buffer buffer) XLogRecPtr BufferGetLSNAtomic(Buffer buffer) { - volatile BufferDesc *bufHdr = GetBufferDescriptor(buffer - 1); + BufferDesc *bufHdr = GetBufferDescriptor(buffer - 1); char *page = BufferGetPage(buffer); XLogRecPtr lsn; @@ -2613,7 +2613,7 @@ DropRelFileNodeBuffers(RelFileNodeBackend rnode, ForkNumber forkNum, for (i = 0; i < NBuffers; i++) { - volatile BufferDesc *bufHdr = GetBufferDescriptor(i); + BufferDesc *bufHdr = GetBufferDescriptor(i); /* * We can make this a tad faster by prechecking the buffer tag before @@ -2703,7 +2703,7 @@ DropRelFileNodesAllBuffers(RelFileNodeBackend *rnodes, int nnodes) for (i = 0; i < NBuffers; i++) { RelFileNode *rnode = NULL; - volatile BufferDesc *bufHdr = GetBufferDescriptor(i); + BufferDesc *bufHdr = GetBufferDescriptor(i); /* * As in DropRelFileNodeBuffers, an unlocked precheck should be safe @@ -2767,7 +2767,7 @@ DropDatabaseBuffers(Oid dbid) for (i = 0; i < NBuffers; i++) { - volatile BufferDesc *bufHdr = GetBufferDescriptor(i); + BufferDesc *bufHdr = GetBufferDescriptor(i); /* * As in DropRelFileNodeBuffers, an unlocked precheck should be safe @@ -2799,7 +2799,7 @@ PrintBufferDescs(void) for (i = 0; i < NBuffers; ++i) { - volatile BufferDesc *buf = GetBufferDescriptor(i); + BufferDesc *buf = GetBufferDescriptor(i); Buffer b = BufferDescriptorGetBuffer(buf); /* theoretically we should lock the bufhdr here */ @@ -2822,7 +2822,7 @@ PrintPinnedBufs(void) for (i = 0; i < NBuffers; ++i) { - volatile BufferDesc *buf = GetBufferDescriptor(i); + BufferDesc *buf = GetBufferDescriptor(i); Buffer b = BufferDescriptorGetBuffer(buf); if (GetPrivateRefCount(b) > 0) @@ -2863,7 +2863,7 @@ void FlushRelationBuffers(Relation rel) { int i; - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; /* Open rel at the smgr level if not already done */ RelationOpenSmgr(rel); @@ -2955,7 +2955,7 @@ void FlushDatabaseBuffers(Oid dbid) { int i; - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; /* Make sure we can handle the pin inside the loop */ ResourceOwnerEnlargeBuffers(CurrentResourceOwner); @@ -3064,7 +3064,7 @@ IncrBufferRefCount(Buffer buffer) void MarkBufferDirtyHint(Buffer buffer, bool buffer_std) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; Page page = BufferGetPage(buffer); if (!BufferIsValid(buffer)) @@ -3198,7 +3198,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std) void UnlockBuffers(void) { - volatile BufferDesc *buf = PinCountWaitBuf; + BufferDesc *buf = PinCountWaitBuf; if (buf) { @@ -3224,7 +3224,7 @@ UnlockBuffers(void) void LockBuffer(Buffer buffer, int mode) { - volatile BufferDesc *buf; + BufferDesc *buf; Assert(BufferIsValid(buffer)); if (BufferIsLocal(buffer)) @@ -3250,7 +3250,7 @@ LockBuffer(Buffer buffer, int mode) bool ConditionalLockBuffer(Buffer buffer) { - volatile BufferDesc *buf; + BufferDesc *buf; Assert(BufferIsValid(buffer)); if (BufferIsLocal(buffer)) @@ -3280,7 +3280,7 @@ ConditionalLockBuffer(Buffer buffer) void LockBufferForCleanup(Buffer buffer) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; Assert(BufferIsValid(buffer)); Assert(PinCountWaitBuf == NULL); @@ -3392,7 +3392,7 @@ HoldingBufferPinThatDelaysRecovery(void) bool ConditionalLockBufferForCleanup(Buffer buffer) { - volatile BufferDesc *bufHdr; + BufferDesc *bufHdr; Assert(BufferIsValid(buffer)); @@ -3445,7 +3445,7 @@ ConditionalLockBufferForCleanup(Buffer buffer) * WaitIO -- Block until the IO_IN_PROGRESS flag on 'buf' is cleared. */ static void -WaitIO(volatile BufferDesc *buf) +WaitIO(BufferDesc *buf) { /* * Changed to wait until there's no IO - Inoue 01/13/2000 @@ -3492,7 +3492,7 @@ WaitIO(volatile BufferDesc *buf) * FALSE if someone else already did the work. */ static bool -StartBufferIO(volatile BufferDesc *buf, bool forInput) +StartBufferIO(BufferDesc *buf, bool forInput) { Assert(!InProgressBuf); @@ -3558,8 +3558,7 @@ StartBufferIO(volatile BufferDesc *buf, bool forInput) * be 0, or BM_VALID if we just finished reading in the page. */ static void -TerminateBufferIO(volatile BufferDesc *buf, bool clear_dirty, - int set_flag_bits) +TerminateBufferIO(BufferDesc *buf, bool clear_dirty, int set_flag_bits) { Assert(buf == InProgressBuf); @@ -3590,7 +3589,7 @@ TerminateBufferIO(volatile BufferDesc *buf, bool clear_dirty, void AbortBufferIO(void) { - volatile BufferDesc *buf = InProgressBuf; + BufferDesc *buf = InProgressBuf; if (buf) { @@ -3643,7 +3642,7 @@ AbortBufferIO(void) static void shared_buffer_write_error_callback(void *arg) { - volatile BufferDesc *bufHdr = (volatile BufferDesc *) arg; + BufferDesc *bufHdr = (BufferDesc *) arg; /* Buffer is pinned, so we can read the tag without locking the spinlock */ if (bufHdr != NULL) @@ -3662,7 +3661,7 @@ shared_buffer_write_error_callback(void *arg) static void local_buffer_write_error_callback(void *arg) { - volatile BufferDesc *bufHdr = (volatile BufferDesc *) arg; + BufferDesc *bufHdr = (BufferDesc *) arg; if (bufHdr != NULL) { diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index bc2c773..0234572 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -98,9 +98,9 @@ typedef struct BufferAccessStrategyData /* Prototypes for internal functions */ -static volatile BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy); +static BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy); static void AddBufferToRing(BufferAccessStrategy strategy, - volatile BufferDesc *buf); + BufferDesc *buf); /* * ClockSweepTick - Helper routine for StrategyGetBuffer() @@ -179,10 +179,10 @@ ClockSweepTick(void) * To ensure that no one else can pin the buffer before we do, we must * return the buffer with the buffer header spinlock still held. */ -volatile BufferDesc * +BufferDesc * StrategyGetBuffer(BufferAccessStrategy strategy) { - volatile BufferDesc *buf; + BufferDesc *buf; int bgwprocno; int trycounter; @@ -338,7 +338,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy) * StrategyFreeBuffer: put a buffer on the freelist */ void -StrategyFreeBuffer(volatile BufferDesc *buf) +StrategyFreeBuffer(BufferDesc *buf) { SpinLockAcquire(&StrategyControl->buffer_strategy_lock); @@ -584,10 +584,10 @@ FreeAccessStrategy(BufferAccessStrategy strategy) * * The bufhdr spin lock is held on the returned buffer. */ -static volatile BufferDesc * +static BufferDesc * GetBufferFromRing(BufferAccessStrategy strategy) { - volatile BufferDesc *buf; + BufferDesc *buf; Buffer bufnum; /* Advance to next ring slot */ @@ -639,7 +639,7 @@ GetBufferFromRing(BufferAccessStrategy strategy) * is called with the spinlock held, it had better be quite cheap. */ static void -AddBufferToRing(BufferAccessStrategy strategy, volatile BufferDesc *buf) +AddBufferToRing(BufferAccessStrategy strategy, BufferDesc *buf) { strategy->buffers[strategy->current] = BufferDescriptorGetBuffer(buf); } @@ -656,7 +656,7 @@ AddBufferToRing(BufferAccessStrategy strategy, volatile BufferDesc *buf) * if this buffer should be written and re-used. */ bool -StrategyRejectBuffer(BufferAccessStrategy strategy, volatile BufferDesc *buf) +StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf) { /* We only do this in bulkread mode */ if (strategy->btype != BAS_BULKREAD) diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 521ee1c..19247c4 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -194,11 +194,6 @@ typedef union BufferDescPadded /* * Macros for acquiring/releasing a shared buffer header's spinlock. * Do not apply these to local buffers! - * - * Note: as a general coding rule, if you are using these then you probably - * need to be using a volatile-qualified pointer to the buffer header, to - * ensure that the compiler doesn't rearrange accesses to the header to - * occur before or after the spinlock is acquired/released. */ #define LockBufHdr(bufHdr) SpinLockAcquire(&(bufHdr)->buf_hdr_lock) #define UnlockBufHdr(bufHdr) SpinLockRelease(&(bufHdr)->buf_hdr_lock) @@ -216,10 +211,10 @@ extern BufferDesc *LocalBufferDescriptors; */ /* freelist.c */ -extern volatile BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy); -extern void StrategyFreeBuffer(volatile BufferDesc *buf); +extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy); +extern void StrategyFreeBuffer(BufferDesc *buf); extern bool StrategyRejectBuffer(BufferAccessStrategy strategy, - volatile BufferDesc *buf); + BufferDesc *buf); extern int StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc); extern void StrategyNotifyBgWriter(int bgwprocno); -- 1.7.12.4 (Apple Git-37)
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers