On Fri, Nov 3, 2023 at 10:59 AM Dilip Kumar <dilipbal...@gmail.com> wrote:
> On Mon, Oct 30, 2023 at 11:50 AM Dilip Kumar <dilipbal...@gmail.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