Re: [PATCH] Refactoring of LWLock tranches

Thank you for the review. I've made changes according to your comments.
I don't stick on current names in the patch.

I've removed all unnecerrary indirections, and added cache aligning
to LWLock arrays.

On Tue, 17 Nov 2015 19:36:13 +0100
"andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de> wrote:

> 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.

Maybe I'm missing something here, but I don't see much difference with
other tranches, created in Postgres startup. In my opinion they are also
pretty much builtin.

>
> > 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.

The moving base tranches to shared memory has been discussed many times.
The point is using them later in pg_stat_activity and other monitoring
views.

>
>
> > 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.

Changed to BufMappingPartitionLWLocks. If this patch is all about
separating LWLocks of the buffer manager, why not use one patch for this
task?

>
> > + 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.

Fixed

>
> > - 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.

Fixed

>
> > /* 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.

Fixed

>
> > +/* Maximum length of a bufmgr lock name */
> > +#define BUFMGR_MAX_NAME_LENGTH 32
>
> If we were to do this I'd just use NAMEDATALEN.

Fixed

>
> > +/*
> > + * 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?

The point was using BufferCtlBase in the backend and frontend code.
The frontend code doesn't know about LWLock and other structures.
Anyway I've removed this code.

--
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company