Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock

On Wed, Nov 8, 2023 at 10:52 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:

Thanks for review Amul,

> Here are some minor comments:
>
> + * By default, we'll use 1MB of for every 1GB of shared buffers, up to the
> + * maximum value that slru.c will allow, but always at least 16 buffers.
> */
> Size
> CommitTsShmemBuffers(void)
> {
> - return Min(256, Max(4, NBuffers / 256));
> + /* Use configured value if provided. */
> + if (commit_ts_buffers > 0)
> + return Max(16, commit_ts_buffers);
> + return Min(256, Max(16, NBuffers / 256));
>
> Do you mean "4MB of for every 1GB" in the comment?

You are right

> --
>
> diff --git a/src/include/access/commit_ts.h b/src/include/access/commit_ts.h
> index 5087cdce51..78d017ad85 100644
> --- a/src/include/access/commit_ts.h
> +++ b/src/include/access/commit_ts.h
> @@ -16,7 +16,6 @@
> #include "replication/origin.h"
> #include "storage/sync.h"
>
> -
> extern PGDLLIMPORT bool track_commit_timestamp;
>
> A spurious change.

Will fix

> --
>
> @@ -168,10 +180,19 @@ SimpleLruShmemSize(int nslots, int nlsns)
>
> if (nlsns > 0)
> sz += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr)); /* group_lsn[] */
> -
> return BUFFERALIGN(sz) + BLCKSZ * nslots;
> }
>
> Another spurious change in 0002 patch.

Will fix

> --
>
> +/*
> + * The slru buffer mapping table is partitioned to reduce contention. To
> + * determine which partition lock a given pageno requires, compute the pageno's
> + * hash code with SlruBufTableHashCode(), then apply SlruPartitionLock().
> + */
>
> I didn't see SlruBufTableHashCode() & SlruPartitionLock() functions anywhere in
> your patches, is that outdated comment?

Yes will fix it, actually, there are some major design changes to this.

> --
>
> - sz += MAXALIGN(nslots * sizeof(LWLockPadded)); /* buffer_locks[] */
> - sz += MAXALIGN(SLRU_NUM_PARTITIONS * sizeof(LWLockPadded)); /* part_locks[] */
> + sz += MAXALIGN((nslots + SLRU_NUM_PARTITIONS) * sizeof(LWLockPadded)); /* locks[] */
>
> I am a bit uncomfortable with these changes, merging parts and buffer locks
> making it hard to understand the code. Not sure what we were getting out of
> this?

Yes, even I do not like this much because it is confusing. But the
advantage of this is that we are using a single pointer for the lock
which means the next variable for the LRU counter will come in the
same cacheline and frequent updates of lru counter will be benefitted
from this. Although I don't have any number which proves this.
Currently, I want to focus on all the base patches and keep this patch
as add on and later if we find its useful and want to pursue this then
we will see how to make it better readable.

>
> Subject: [PATCH v4 5/5] Ensure slru buffer slots are in multiple of numbe of
> partitions
>
> I think the 0005 patch can be merged to 0001.

Yeah in the next version, it is done that way. Planning to post end of the day.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com