On 11/09/2015 10:32 PM, Ildus Kurbangaliev wrote:
On Nov 9, 2015, at 7:56 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
Ildus Kurbangaliev wrote:
Thanks for the review. I've attached a new version of SLRU patch. I've
removed add_postfix and fixed EXEC_BACKEND case.
Thanks.
Please do not use "committs" in commit_ts.c; people didn't like the
abbreviated name without the underscore. But then, why are we
abbreviating here? We could keep it complete and with a space instead
of underscore, so why not use just "commit timestamp", because it's just
a string, right?
In multixact.c, is there a reason to have underscore in the strings? We
could substitute it with a space and it'd look prettier; but really, we
could also keep those names parallel to subdirectory names by using the
already existing string parameter as name here, and not add another one.
I do not insist on concrete names or a case here, but I think that identifiers
are more
useful when they don't contain spaces. For example that name will be exposed
later
in other places and can be part of some longer string.
Why do we have two per-buffer loops in SimpleLruInit? I mean, why not
add the LWLockInitialize call to the second one?
Thanks. I didn't see that.
I'm up to speed on how the LWLockTranche API works -- does assigning to
tranche_name a pstrdup string work okay? Is the pstrdup really
necessary?
I think pstrdup can be removed here.
/* Initialize our shared state struct */
diff --git a/src/backend/access/transam/slru.c
b/src/backend/access/transam/slru.c
index 90c7cf5..868b35a 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -157,6 +157,8 @@ SimpleLruShmemSize(int nslots, int nlsns)
if (nlsns > 0)
sz += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr)); /*
group_lsn[] */
+ sz += MAXALIGN(nslots * sizeof(LWLockPadded)); /* lwlocks[] */
+
return BUFFERALIGN(sz) + BLCKSZ * nslots;
}
What is the "lwlocks[]" comment supposed to mean? I don't think there's
a struct member with that name, is there?
It just means that we are allocating memory for an array of lwlocks,
i'll change it.
Uhm, actually, why do we keep buffer_locks[] at all? This arrangement
seems pretty odd, where if I understand correctly we have one array
which is the tranche and another array which points to each item in the
tranche ...
Actually yes, that is a good idea.
Attached a new version of the patch that moves SLRU tranches and LWLocks
to SLRU control structs.
`buffer_locks` field now contains LWLocks itself, so we have some
economy of the memory here. `pstrdup` removed in SimpleLruInit. I didn't
change names from the previous patch yet, but I don't mind if they'll
be changed.
--
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 3a58f1e..ea83655 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -456,7 +456,7 @@ void
CLOGShmemInit(void)
{
ClogCtl->PagePrecedes = CLOGPagePrecedes;
- SimpleLruInit(ClogCtl, "CLOG Ctl", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE,
+ SimpleLruInit(ClogCtl, "clog", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE,
CLogControlLock, "pg_clog");
}
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index b21a313..349228d 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -478,7 +478,7 @@ CommitTsShmemInit(void)
bool found;
CommitTsCtl->PagePrecedes = CommitTsPagePrecedes;
- SimpleLruInit(CommitTsCtl, "CommitTs Ctl", CommitTsShmemBuffers(), 0,
+ SimpleLruInit(CommitTsCtl, "commit_timestamp", CommitTsShmemBuffers(), 0,
CommitTsControlLock, "pg_commit_ts");
commitTsShared = ShmemInitStruct("CommitTs shared",
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 7d97085..b66a2b6 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1838,10 +1838,10 @@ MultiXactShmemInit(void)
MultiXactMemberCtl->PagePrecedes = MultiXactMemberPagePrecedes;
SimpleLruInit(MultiXactOffsetCtl,
- "MultiXactOffset Ctl", NUM_MXACTOFFSET_BUFFERS, 0,
+ "multixact_offset", NUM_MXACTOFFSET_BUFFERS, 0,
MultiXactOffsetControlLock, "pg_multixact/offsets");
SimpleLruInit(MultiXactMemberCtl,
- "MultiXactMember Ctl", NUM_MXACTMEMBER_BUFFERS, 0,
+ "multixact_member", NUM_MXACTMEMBER_BUFFERS, 0,
MultiXactMemberControlLock, "pg_multixact/members");
/* Initialize our shared state struct */
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 90c7cf5..6a5c7c3 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -152,7 +152,7 @@ SimpleLruShmemSize(int nslots, int nlsns)
sz += MAXALIGN(nslots * sizeof(bool)); /* page_dirty[] */
sz += MAXALIGN(nslots * sizeof(int)); /* page_number[] */
sz += MAXALIGN(nslots * sizeof(int)); /* page_lru_count[] */
- sz += MAXALIGN(nslots * sizeof(LWLock *)); /* buffer_locks[] */
+ sz += MAXALIGN(nslots * sizeof(LWLockPadded)); /* buffer_locks[] */
if (nlsns > 0)
sz += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr)); /* group_lsn[] */
@@ -203,8 +203,6 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
offset += MAXALIGN(nslots * sizeof(int));
shared->page_lru_count = (int *) (ptr + offset);
offset += MAXALIGN(nslots * sizeof(int));
- shared->buffer_locks = (LWLock **) (ptr + offset);
- offset += MAXALIGN(nslots * sizeof(LWLock *));
if (nlsns > 0)
{
@@ -212,20 +210,33 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
offset += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));
}
+ /* Initialize LWLocks */
+ shared->buffer_locks = (LWLockPadded *) ShmemAlloc(sizeof(LWLockPadded) * nslots);
+
+ shared->locks_tranche_id = LWLockNewTrancheId();
+ shared->locks_tranche.name = name;
+ shared->locks_tranche.array_base = shared->buffer_locks;
+ shared->locks_tranche.array_stride = sizeof(LWLockPadded);
+
ptr += BUFFERALIGN(offset);
for (slotno = 0; slotno < nslots; slotno++)
{
+ LWLockInitialize(&shared->buffer_locks[slotno].lock,
+ shared->locks_tranche_id);
+
shared->page_buffer[slotno] = ptr;
shared->page_status[slotno] = SLRU_PAGE_EMPTY;
shared->page_dirty[slotno] = false;
shared->page_lru_count[slotno] = 0;
- shared->buffer_locks[slotno] = LWLockAssign();
ptr += BLCKSZ;
}
}
else
Assert(found);
+ /* Register SLRU tranche in the main tranches array */
+ LWLockRegisterTranche(shared->locks_tranche_id, &shared->locks_tranche);
+
/*
* Initialize the unshared control struct, including directory path. We
* assume caller set PagePrecedes.
@@ -308,8 +319,8 @@ SimpleLruWaitIO(SlruCtl ctl, int slotno)
/* See notes at top of file */
LWLockRelease(shared->ControlLock);
- LWLockAcquire(shared->buffer_locks[slotno], LW_SHARED);
- LWLockRelease(shared->buffer_locks[slotno]);
+ LWLockAcquire(&shared->buffer_locks[slotno].lock, LW_SHARED);
+ LWLockRelease(&shared->buffer_locks[slotno].lock);
LWLockAcquire(shared->ControlLock, LW_EXCLUSIVE);
/*
@@ -323,7 +334,7 @@ SimpleLruWaitIO(SlruCtl ctl, int slotno)
if (shared->page_status[slotno] == SLRU_PAGE_READ_IN_PROGRESS ||
shared->page_status[slotno] == SLRU_PAGE_WRITE_IN_PROGRESS)
{
- if (LWLockConditionalAcquire(shared->buffer_locks[slotno], LW_SHARED))
+ if (LWLockConditionalAcquire(&shared->buffer_locks[slotno].lock, LW_SHARED))
{
/* indeed, the I/O must have failed */
if (shared->page_status[slotno] == SLRU_PAGE_READ_IN_PROGRESS)
@@ -333,7 +344,7 @@ SimpleLruWaitIO(SlruCtl ctl, int slotno)
shared->page_status[slotno] = SLRU_PAGE_VALID;
shared->page_dirty[slotno] = true;
}
- LWLockRelease(shared->buffer_locks[slotno]);
+ LWLockRelease(&shared->buffer_locks[slotno].lock);
}
}
}
@@ -402,7 +413,7 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
shared->page_dirty[slotno] = false;
/* Acquire per-buffer lock (cannot deadlock, see notes at top) */
- LWLockAcquire(shared->buffer_locks[slotno], LW_EXCLUSIVE);
+ LWLockAcquire(&shared->buffer_locks[slotno].lock, LW_EXCLUSIVE);
/* Release control lock while doing I/O */
LWLockRelease(shared->ControlLock);
@@ -422,7 +433,7 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
shared->page_status[slotno] = ok ? SLRU_PAGE_VALID : SLRU_PAGE_EMPTY;
- LWLockRelease(shared->buffer_locks[slotno]);
+ LWLockRelease(&shared->buffer_locks[slotno].lock);
/* Now it's okay to ereport if we failed */
if (!ok)
@@ -518,7 +529,7 @@ SlruInternalWritePage(SlruCtl ctl, int slotno, SlruFlush fdata)
shared->page_dirty[slotno] = false;
/* Acquire per-buffer lock (cannot deadlock, see notes at top) */
- LWLockAcquire(shared->buffer_locks[slotno], LW_EXCLUSIVE);
+ LWLockAcquire(&shared->buffer_locks[slotno].lock, LW_EXCLUSIVE);
/* Release control lock while doing I/O */
LWLockRelease(shared->ControlLock);
@@ -547,7 +558,7 @@ SlruInternalWritePage(SlruCtl ctl, int slotno, SlruFlush fdata)
shared->page_status[slotno] = SLRU_PAGE_VALID;
- LWLockRelease(shared->buffer_locks[slotno]);
+ LWLockRelease(&shared->buffer_locks[slotno].lock);
/* Now it's okay to ereport if we failed */
if (!ok)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index 6b70982..36426c5 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -178,7 +178,7 @@ void
SUBTRANSShmemInit(void)
{
SubTransCtl->PagePrecedes = SubTransPagePrecedes;
- SimpleLruInit(SubTransCtl, "SUBTRANS Ctl", NUM_SUBTRANS_BUFFERS, 0,
+ SimpleLruInit(SubTransCtl, "subtrans", NUM_SUBTRANS_BUFFERS, 0,
SubtransControlLock, "pg_subtrans");
/* Override default assumption that writes should be fsync'd */
SubTransCtl->do_fsync = false;
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 5059e3d..38231c9 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -479,7 +479,7 @@ AsyncShmemInit(void)
* Set up SLRU management of the pg_notify data.
*/
AsyncCtl->PagePrecedes = asyncQueuePagePrecedes;
- SimpleLruInit(AsyncCtl, "Async Ctl", NUM_ASYNC_BUFFERS, 0,
+ SimpleLruInit(AsyncCtl, "async", NUM_ASYNC_BUFFERS, 0,
AsyncCtlLock, "pg_notify");
/* Override default assumption that writes should be fsync'd */
AsyncCtl->do_fsync = false;
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index fd4b479..6bca02c 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -76,11 +76,6 @@
*/
#include "postgres.h"
-#include "access/clog.h"
-#include "access/commit_ts.h"
-#include "access/multixact.h"
-#include "access/subtrans.h"
-#include "commands/async.h"
#include "miscadmin.h"
#include "pg_trace.h"
#include "postmaster/postmaster.h"
@@ -364,24 +359,6 @@ NumLWLocks(void)
/* proc.c needs one for each backend or auxiliary process */
numLocks += MaxBackends + NUM_AUXILIARY_PROCS;
- /* clog.c needs one per CLOG buffer */
- numLocks += CLOGShmemBuffers();
-
- /* commit_ts.c needs one per CommitTs buffer */
- numLocks += CommitTsShmemBuffers();
-
- /* subtrans.c needs one per SubTrans buffer */
- numLocks += NUM_SUBTRANS_BUFFERS;
-
- /* multixact.c needs two SLRU areas */
- numLocks += NUM_MXACTOFFSET_BUFFERS + NUM_MXACTMEMBER_BUFFERS;
-
- /* async.c needs one per Async buffer */
- numLocks += NUM_ASYNC_BUFFERS;
-
- /* predicate.c needs one per old serializable xid buffer */
- numLocks += NUM_OLDSERXID_BUFFERS;
-
/* slot.c needs one for each slot */
numLocks += max_replication_slots;
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 47b4ada..4b4ad81 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -794,7 +794,7 @@ OldSerXidInit(void)
* Set up SLRU management of the pg_serial data.
*/
OldSerXidSlruCtl->PagePrecedes = OldSerXidPagePrecedesLogically;
- SimpleLruInit(OldSerXidSlruCtl, "OldSerXid SLRU Ctl",
+ SimpleLruInit(OldSerXidSlruCtl, "oldserxid",
NUM_OLDSERXID_BUFFERS, 0, OldSerXidLock, "pg_serial");
/* Override default assumption that writes should be fsync'd */
OldSerXidSlruCtl->do_fsync = false;
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index f60e75b..0d70cb6 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -69,7 +69,6 @@ typedef struct SlruSharedData
bool *page_dirty;
int *page_number;
int *page_lru_count;
- LWLock **buffer_locks;
/*
* Optional array of WAL flush LSNs associated with entries in the SLRU
@@ -99,6 +98,11 @@ typedef struct SlruSharedData
* the latest page.
*/
int latest_page_number;
+
+ /* LWLocks */
+ int locks_tranche_id;
+ LWLockTranche locks_tranche;
+ LWLockPadded *buffer_locks;
} SlruSharedData;
typedef SlruSharedData *SlruShared;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
