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

On Fri, Nov 3, 2023 at 10:59 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> On Mon, Oct 30, 2023 at 11:50 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
> > [...]
> [1] 0001-Make-all-SLRU-buffer-sizes-configurable: This is the same
> patch as the previous patch set
> [2] 0002-Add-a-buffer-mapping-table-for-SLRUs: Patch to introduce
> buffer mapping hash table
> [3] 0003-Partition-wise-slru-locks: Partition the hash table and also
> introduce partition-wise locks: this is a merge of 0003 and 0004 from
> the previous patch set but instead of bank-wise locks it has
> partition-wise locks and LRU counter.
> [4] 0004-Merge-partition-locks-array-with-buffer-locks-array: merging
> buffer locks and bank locks in the same array so that the bank-wise
> LRU counter does not fetch the next cache line in a hot function
> SlruRecentlyUsed()(same as 0005 from the previous patch set)
> [5] 0005-Ensure-slru-buffer-slots-are-in-multiple-of-number-of: Ensure
> that the number of slots is in multiple of the number of banks
> [...]

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

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

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

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

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

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.

Regards,
Amul