On 2015-11-17 14:14:50 +0300, Ildus Kurbangaliev wrote:
> 1) We can avoid constants, and use a standard steps for tranches
> creation.
The constants are actually a bit useful, to easily determine
builtin/non-builtin stuff.
> 2) We have only one global variable (BufferCtl)
Don't see the advantage. This adds another, albeit predictable,
indirection in frequent callsites. There's no architectural advantage in
avoiding these.
> 3) Tranches moved to shared memory, so we won't need to do
> an additional work here.
I can see some benefit, but it also doesn't seem like a huge benefit.
> 4) Also we can kept buffer locks from MainLWLockArray there (that was done
> in attached patch).
That's the 'blockLocks' thing? That's a pretty bad name. These are
buffer mapping locks. And this seems like a separate patch, separately
debated.
> + if (!foundCtl)
> {
> - int i;
> + /* Align descriptors to a cacheline boundary. */
> + ctl->base.bufferDescriptors = (void *)
> CACHELINEALIGN(ShmemAlloc(
> + NBuffers * sizeof(BufferDescPadded) +
> PG_CACHE_LINE_SIZE));
> +
> + ctl->base.bufferBlocks = (char *) ShmemAlloc(NBuffers * (Size)
> BLCKSZ);
I'm going to entirely veto this.
> + strncpy(ctl->IOLWLockTrancheName, "buffer_io",
> + BUFMGR_MAX_NAME_LENGTH);
> + strncpy(ctl->contentLWLockTrancheName, "buffer_content",
> + BUFMGR_MAX_NAME_LENGTH);
> + strncpy(ctl->blockLWLockTrancheName, "buffer_blocks",
> + BUFMGR_MAX_NAME_LENGTH);
> +
> + ctl->IOLocks = (LWLockMinimallyPadded *) ShmemAlloc(
> + sizeof(LWLockMinimallyPadded) * NBuffers);
This should be cacheline aligned.
> - buf->io_in_progress_lock = LWLockAssign();
> - buf->content_lock = LWLockAssign();
> + LWLockInitialize(BufferDescriptorGetContentLock(buf),
> + ctl->contentLWLockTrancheId);
> + LWLockInitialize(&ctl->IOLocks[i].lock,
> + ctl->IOLWLockTrancheId);
Seems weird to use the macro accessing content locks, but not IO locks.
> /* Note: these two macros only work on shared buffers, not local ones! */
> -#define BufHdrGetBlock(bufHdr) ((Block) (BufferBlocks + ((Size)
> (bufHdr)->buf_id) * BLCKSZ))
> +#define BufHdrGetBlock(bufHdr) ((Block) (BufferCtl->bufferBlocks +
> ((Size) (bufHdr)->buf_id) * BLCKSZ))
That's the additional indirection I'm talking about.
> @@ -353,9 +353,6 @@ NumLWLocks(void)
> /* Predefined LWLocks */
> numLocks = NUM_FIXED_LWLOCKS;
>
> - /* bufmgr.c needs two for each shared buffer */
> - numLocks += 2 * NBuffers;
> -
> /* proc.c needs one for each backend or auxiliary process */
> numLocks += MaxBackends + NUM_AUXILIARY_PROCS;
Didn't you also move the buffer mapping locks... ?
> diff --git a/src/include/storage/buf_internals.h
> b/src/include/storage/buf_internals.h
> index 521ee1c..e1795dc 100644
> --- a/src/include/storage/buf_internals.h
> +++ b/src/include/storage/buf_internals.h
> @@ -95,6 +95,7 @@ typedef struct buftag
> (a).forkNum == (b).forkNum \
> )
>
> +
> /*
unrelated change.
> * The shared buffer mapping table is partitioned to reduce contention.
> * To determine which partition lock a given tag requires, compute the tag's
> @@ -104,10 +105,9 @@ typedef struct buftag
> #define BufTableHashPartition(hashcode) \
> ((hashcode) % NUM_BUFFER_PARTITIONS)
> #define BufMappingPartitionLock(hashcode) \
> - (&MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + \
> - BufTableHashPartition(hashcode)].lock)
> + (&((BufferCtlData
> *)BufferCtl)->blockLocks[BufTableHashPartition(hashcode)].lock)
> #define BufMappingPartitionLockByIndex(i) \
> - (&MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + (i)].lock)
> + (&((BufferCtlData *)BufferCtl)->blockLocks[i].lock)
Again, unnecessary indirections.
> +/* Maximum length of a bufmgr lock name */
> +#define BUFMGR_MAX_NAME_LENGTH 32
If we were to do this I'd just use NAMEDATALEN.
> +/*
> + * Base control struct for the buffer manager data
> + * Located in shared memory.
> + *
> + * BufferCtl variable points to BufferCtlBase because of only
> + * the backend code knows about BufferDescPadded, LWLock and
> + * others structures and the same time it must be usable in
> + * the frontend code.
> + */
> +typedef struct BufferCtlData
> +{
> + BufferCtlBase base;
Eeek. What's the point here?
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
