Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock - Mailing list pgsql-hackers
| From | Amul Sul |
|---|---|
| Subject | Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock |
| Date | |
| Msg-id | CAAJ_b95TY8P-N6hFU4ACnBv7WQGTj9ANdDRQuF3Vc_WAxOX0dw@mail.gmail.com Whole thread Raw |
| In response to | Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock (Dilip Kumar <dilipbalaut@gmail.com>) |
| Responses |
Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock
|
| List | pgsql-hackers |
On Fri, Nov 3, 2023 at 10:59 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, Oct 30, 2023 at 11:50 AM Dilip Kumar <dilipbalaut@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));
+ * 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.
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.
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().
+ */
+ * 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[] */
- 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?
making it hard to understand the code. Not sure what we were getting out of
this?
--
partitions
I think the 0005 patch can be merged to 0001.
Regards,
Amul
pgsql-hackers by date: