From 553a6b185ee7270bf206387421e50b48c7a8b97b Mon Sep 17 00:00:00 2001 From: Dilip Kumar Date: Tue, 12 Jul 2022 17:10:04 +0530 Subject: [PATCH v1] Convert buf_internal.h macros to static inline functions Readability wise inline functions are better compared to macros and this will also help to write cleaner and readable code for 64-bit relfilenode because as part of that patch we will have to do some complex bitwise operation so doing that inside the inline function will be cleaner. --- src/backend/storage/buffer/buf_init.c | 2 +- src/backend/storage/buffer/bufmgr.c | 16 ++-- src/backend/storage/buffer/localbuf.c | 12 +-- src/include/storage/buf_internals.h | 156 +++++++++++++++++++++------------- 4 files changed, 111 insertions(+), 75 deletions(-) diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c index 2862e9e..55f646d 100644 --- a/src/backend/storage/buffer/buf_init.c +++ b/src/backend/storage/buffer/buf_init.c @@ -116,7 +116,7 @@ InitBufferPool(void) { BufferDesc *buf = GetBufferDescriptor(i); - CLEAR_BUFFERTAG(buf->tag); + CLEAR_BUFFERTAG(&buf->tag); pg_atomic_init_u32(&buf->state, 0); buf->wait_backend_pgprocno = INVALID_PGPROCNO; diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index e257ae2..0ec5891 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -515,7 +515,7 @@ PrefetchSharedBuffer(SMgrRelation smgr_reln, Assert(BlockNumberIsValid(blockNum)); /* create a tag so we can lookup the buffer */ - INIT_BUFFERTAG(newTag, smgr_reln->smgr_rlocator.locator, + INIT_BUFFERTAG(&newTag, smgr_reln->smgr_rlocator.locator, forkNum, blockNum); /* determine its hash code and partition lock ID */ @@ -632,7 +632,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN ResourceOwnerEnlargeBuffers(CurrentResourceOwner); ReservePrivateRefCountEntry(); - INIT_BUFFERTAG(tag, rlocator, forkNum, blockNum); + INIT_BUFFERTAG(&tag, rlocator, forkNum, blockNum); if (BufferIsLocal(recent_buffer)) { @@ -640,7 +640,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN buf_state = pg_atomic_read_u32(&bufHdr->state); /* Is it still valid and holding the right tag? */ - if ((buf_state & BM_VALID) && BUFFERTAGS_EQUAL(tag, bufHdr->tag)) + if ((buf_state & BM_VALID) && BUFFERTAGS_EQUAL(&tag, &bufHdr->tag)) { /* Bump local buffer's ref and usage counts. */ ResourceOwnerRememberBuffer(CurrentResourceOwner, recent_buffer); @@ -669,7 +669,7 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN else buf_state = LockBufHdr(bufHdr); - if ((buf_state & BM_VALID) && BUFFERTAGS_EQUAL(tag, bufHdr->tag)) + if ((buf_state & BM_VALID) && BUFFERTAGS_EQUAL(&tag, &bufHdr->tag)) { /* * It's now safe to pin the buffer. We can't pin first and ask @@ -1124,7 +1124,7 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, uint32 buf_state; /* create a tag so we can lookup the buffer */ - INIT_BUFFERTAG(newTag, smgr->smgr_rlocator.locator, forkNum, blockNum); + INIT_BUFFERTAG(&newTag, smgr->smgr_rlocator.locator, forkNum, blockNum); /* determine its hash code and partition lock ID */ newHash = BufTableHashCode(&newTag); @@ -1507,7 +1507,7 @@ retry: buf_state = LockBufHdr(buf); /* If it's changed while we were waiting for lock, do nothing */ - if (!BUFFERTAGS_EQUAL(buf->tag, oldTag)) + if (!BUFFERTAGS_EQUAL(&buf->tag, &oldTag)) { UnlockBufHdr(buf, buf_state); LWLockRelease(oldPartitionLock); @@ -1539,7 +1539,7 @@ retry: * linear scans of the buffer array don't think the buffer is valid. */ oldFlags = buf_state & BUF_FLAG_MASK; - CLEAR_BUFFERTAG(buf->tag); + CLEAR_BUFFERTAG(&buf->tag); buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK); UnlockBufHdr(buf, buf_state); @@ -3356,7 +3356,7 @@ FindAndDropRelFileLocatorBuffers(RelFileLocator rlocator, ForkNumber forkNum, uint32 buf_state; /* create a tag so we can lookup the buffer */ - INIT_BUFFERTAG(bufTag, rlocator, forkNum, curBlock); + INIT_BUFFERTAG(&bufTag, rlocator, forkNum, curBlock); /* determine its hash code and partition lock ID */ bufHash = BufTableHashCode(&bufTag); diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 41a0807..6618ef1 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -68,7 +68,7 @@ PrefetchLocalBuffer(SMgrRelation smgr, ForkNumber forkNum, BufferTag newTag; /* identity of requested block */ LocalBufferLookupEnt *hresult; - INIT_BUFFERTAG(newTag, smgr->smgr_rlocator.locator, forkNum, blockNum); + INIT_BUFFERTAG(&newTag, smgr->smgr_rlocator.locator, forkNum, blockNum); /* Initialize local buffers if first request in this session */ if (LocalBufHash == NULL) @@ -117,7 +117,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, bool found; uint32 buf_state; - INIT_BUFFERTAG(newTag, smgr->smgr_rlocator.locator, forkNum, blockNum); + INIT_BUFFERTAG(&newTag, smgr->smgr_rlocator.locator, forkNum, blockNum); /* Initialize local buffers if first request in this session */ if (LocalBufHash == NULL) @@ -131,7 +131,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, { b = hresult->id; bufHdr = GetLocalBufferDescriptor(b); - Assert(BUFFERTAGS_EQUAL(bufHdr->tag, newTag)); + Assert(BUFFERTAGS_EQUAL(&bufHdr->tag, &newTag)); #ifdef LBDEBUG fprintf(stderr, "LB ALLOC (%u,%d,%d) %d\n", smgr->smgr_rlocator.locator.relNumber, forkNum, blockNum, -b - 1); @@ -253,7 +253,7 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, if (!hresult) /* shouldn't happen */ elog(ERROR, "local buffer hash table corrupted"); /* mark buffer invalid just in case hash insert fails */ - CLEAR_BUFFERTAG(bufHdr->tag); + CLEAR_BUFFERTAG(&bufHdr->tag); buf_state &= ~(BM_VALID | BM_TAG_VALID); pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); } @@ -354,7 +354,7 @@ DropRelFileLocatorLocalBuffers(RelFileLocator rlocator, ForkNumber forkNum, if (!hresult) /* shouldn't happen */ elog(ERROR, "local buffer hash table corrupted"); /* Mark buffer invalid */ - CLEAR_BUFFERTAG(bufHdr->tag); + CLEAR_BUFFERTAG(&bufHdr->tag); buf_state &= ~BUF_FLAG_MASK; buf_state &= ~BUF_USAGECOUNT_MASK; pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); @@ -398,7 +398,7 @@ DropRelFileLocatorAllLocalBuffers(RelFileLocator rlocator) if (!hresult) /* shouldn't happen */ elog(ERROR, "local buffer hash table corrupted"); /* Mark buffer invalid */ - CLEAR_BUFFERTAG(bufHdr->tag); + CLEAR_BUFFERTAG(&bufHdr->tag); buf_state &= ~BUF_FLAG_MASK; buf_state &= ~BUF_USAGECOUNT_MASK; pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state); diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index aded5e8..d8ddb54 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -95,28 +95,32 @@ typedef struct buftag BlockNumber blockNum; /* blknum relative to begin of reln */ } BufferTag; -#define CLEAR_BUFFERTAG(a) \ -( \ - (a).rlocator.spcOid = InvalidOid, \ - (a).rlocator.dbOid = InvalidOid, \ - (a).rlocator.relNumber = InvalidRelFileNumber, \ - (a).forkNum = InvalidForkNumber, \ - (a).blockNum = InvalidBlockNumber \ -) - -#define INIT_BUFFERTAG(a,xx_rlocator,xx_forkNum,xx_blockNum) \ -( \ - (a).rlocator = (xx_rlocator), \ - (a).forkNum = (xx_forkNum), \ - (a).blockNum = (xx_blockNum) \ -) - -#define BUFFERTAGS_EQUAL(a,b) \ -( \ - RelFileLocatorEquals((a).rlocator, (b).rlocator) && \ - (a).blockNum == (b).blockNum && \ - (a).forkNum == (b).forkNum \ -) +static inline void +CLEAR_BUFFERTAG(BufferTag *tag) +{ + tag->rlocator.spcOid = InvalidOid; + tag->rlocator.dbOid = InvalidOid; + tag->rlocator.relNumber = InvalidRelFileNumber; + tag->forkNum = InvalidForkNumber; + tag->blockNum = InvalidBlockNumber; +} + +static inline void +INIT_BUFFERTAG(BufferTag *tag, RelFileLocator rlocator, + ForkNumber forkNum, BlockNumber blockNum) +{ + tag->rlocator = rlocator; + tag->forkNum = forkNum; + tag->blockNum = blockNum; +} + +static inline bool +BUFFERTAGS_EQUAL(BufferTag *tag1, BufferTag *tag2) +{ + return RelFileLocatorEquals(tag1->rlocator, tag2->rlocator) && + (tag1->blockNum == tag2->blockNum) && + (tag1->forkNum == tag2->forkNum); +} /* * The shared buffer mapping table is partitioned to reduce contention. @@ -124,13 +128,24 @@ typedef struct buftag * hash code with BufTableHashCode(), then apply BufMappingPartitionLock(). * NB: NUM_BUFFER_PARTITIONS must be a power of 2! */ -#define BufTableHashPartition(hashcode) \ - ((hashcode) % NUM_BUFFER_PARTITIONS) -#define BufMappingPartitionLock(hashcode) \ - (&MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + \ - BufTableHashPartition(hashcode)].lock) -#define BufMappingPartitionLockByIndex(i) \ - (&MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + (i)].lock) +static inline uint32 +BufTableHashPartition(uint32 hashcode) +{ + return hashcode % NUM_BUFFER_PARTITIONS; +} + +static inline LWLock * +BufMappingPartitionLock(uint32 hashcode) +{ + return &MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + + BufTableHashPartition(hashcode)].lock; +} + +static inline LWLock * +BufMappingPartitionLockByIndex(uint32 index) +{ + return &MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + index].lock; +} /* * BufferDesc -- shared descriptor/state data for a single shared buffer. @@ -220,37 +235,6 @@ typedef union BufferDescPadded char pad[BUFFERDESC_PAD_TO_SIZE]; } BufferDescPadded; -#define GetBufferDescriptor(id) (&BufferDescriptors[(id)].bufferdesc) -#define GetLocalBufferDescriptor(id) (&LocalBufferDescriptors[(id)]) - -#define BufferDescriptorGetBuffer(bdesc) ((bdesc)->buf_id + 1) - -#define BufferDescriptorGetIOCV(bdesc) \ - (&(BufferIOCVArray[(bdesc)->buf_id]).cv) -#define BufferDescriptorGetContentLock(bdesc) \ - ((LWLock*) (&(bdesc)->content_lock)) - -extern PGDLLIMPORT ConditionVariableMinimallyPadded *BufferIOCVArray; - -/* - * The freeNext field is either the index of the next freelist entry, - * or one of these special values: - */ -#define FREENEXT_END_OF_LIST (-1) -#define FREENEXT_NOT_IN_LIST (-2) - -/* - * Functions for acquiring/releasing a shared buffer header's spinlock. Do - * not apply these to local buffers! - */ -extern uint32 LockBufHdr(BufferDesc *desc); -#define UnlockBufHdr(desc, s) \ - do { \ - pg_write_barrier(); \ - pg_atomic_write_u32(&(desc)->state, (s) & (~BM_LOCKED)); \ - } while (0) - - /* * The PendingWriteback & WritebackContext structure are used to keep * information about pending flush requests to be issued to the OS. @@ -276,11 +260,63 @@ typedef struct WritebackContext /* in buf_init.c */ extern PGDLLIMPORT BufferDescPadded *BufferDescriptors; +extern PGDLLIMPORT ConditionVariableMinimallyPadded *BufferIOCVArray; extern PGDLLIMPORT WritebackContext BackendWritebackContext; /* in localbuf.c */ extern PGDLLIMPORT BufferDesc *LocalBufferDescriptors; + +static inline BufferDesc * +GetBufferDescriptor(uint32 id) +{ + return &(BufferDescriptors[id]).bufferdesc; +} + +static inline BufferDesc * +GetLocalBufferDescriptor(uint32 id) +{ + return &LocalBufferDescriptors[id]; +} + +static inline Buffer +BufferDescriptorGetBuffer(BufferDesc *bdesc) +{ + return (Buffer) (bdesc->buf_id + 1); +} + +static inline ConditionVariable * +BufferDescriptorGetIOCV(BufferDesc *bdesc) +{ + return &(BufferIOCVArray[bdesc->buf_id]).cv; +} + +static inline LWLock * +BufferDescriptorGetContentLock(BufferDesc *bdesc) +{ + return (LWLock *) (&bdesc->content_lock); +} + +/* + * The freeNext field is either the index of the next freelist entry, + * or one of these special values: + */ +#define FREENEXT_END_OF_LIST (-1) +#define FREENEXT_NOT_IN_LIST (-2) + +/* + * Functions for acquiring/releasing a shared buffer header's spinlock. Do + * not apply these to local buffers! + */ +extern uint32 LockBufHdr(BufferDesc *desc); + +static inline void +UnlockBufHdr(BufferDesc *desc, uint32 buf_state) +{ + pg_write_barrier(); + pg_atomic_write_u32(&desc->state, buf_state & (~BM_LOCKED)); +} + /* in bufmgr.c */ /* -- 1.8.3.1