Re: [PATCH] Refactoring of LWLock tranches - Mailing list pgsql-hackers

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