public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Andrey M. Borodin <[email protected]>
Cc: Anastasia Lubennikova <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: MultiXact\SLRU buffers configuration
Date: Mon, 26 Oct 2020 04:05:26 +0300
Message-ID: <CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi!
On Mon, Sep 28, 2020 at 5:41 PM Andrey M. Borodin <[email protected]> wrote:
> > 28 авг. 2020 г., в 23:08, Anastasia Lubennikova <[email protected]> написал(а):
> >
> > 1) The first patch is sensible and harmless, so I think it is ready for committer. I haven't tested the performance impact, though.
> >
> > 2) I like the initial proposal to make various SLRU buffers configurable, however, I doubt if it really solves the problem, or just moves it to another place?
> >
> > The previous patch you sent was based on some version that contained changes for other slru buffers numbers: 'multixact_offsets_slru_buffers' and 'multixact_members_slru_buffers'. Have you just forgot to attach them? The patch message "[PATCH v2 2/4]" hints that you had 4 patches)
> > Meanwhile, I attach the rebased patch to calm down the CFbot. The changes are trivial.
> >
> > 2.1) I think that both min and max values for this parameter are too extreme. Have you tested them?
> >
> > + &multixact_local_cache_entries,
> > + 256, 2, INT_MAX / 2,
> >
> > 2.2) MAX_CACHE_ENTRIES is not used anymore, so it can be deleted.
> >
> > 3) No changes for third patch. I just renamed it for consistency.
>
> Thank you for your review.
>
> Indeed, I had 4th patch with tests, but these tests didn't work well: I still did not manage to stress SLRUs to reproduce problem from production...
>
> You are absolutely correct in point 2: I did only tests with sane values. And observed extreme performance degradation with values ~ 64 megabytes. I do not know which highest values should we pick? 1Gb? Or highest possible functioning value?
>
> I greatly appreciate your review, sorry for so long delay. Thanks!
I took a look at this patchset.
The 1st and 3rd patches look good to me. I made just minor improvements.
1) There is still a case when SimpleLruReadPage_ReadOnly() relocks the
SLRU lock, which is already taken in exclusive mode. I've evaded this
by passing the lock mode as a parameter to
SimpleLruReadPage_ReadOnly().
3) CHECK_FOR_INTERRUPTS() is not needed anymore, because it's called
inside ConditionVariableSleep() if needed. Also, no current wait
events use slashes, and I don't think we should introduce slashes
here. Even if we should, then we should also rename existing wait
events to be consistent with a new one. So, I've renamed the new wait
event to remove the slash.
Regarding the patch 2. I see the current documentation in the patch
doesn't explain to the user how to set the new parameter. I think we
should give users an idea what workloads need high values of
multixact_local_cache_entries parameter and what doesn't. Also, we
should explain the negative aspects of high values
multixact_local_cache_entries. Ideally, we should get the advantage
without overloading users with new nontrivial parameters, but I don't
have a particular idea here.
I'd like to propose committing 1 and 3, but leave 2 for further review.
------
Regards,
Alexander Korotkov
Attachments:
[application/octet-stream] v4-0001-Use-shared-lock-in-GetMultiXactIdMembers-for-offsets.patch (12.2K, ../CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com/2-v4-0001-Use-shared-lock-in-GetMultiXactIdMembers-for-offsets.patch)
download | inline diff:
From 12bbf5dbd372a431d106e7e8c3ed50ffa5707a19 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Mon, 26 Oct 2020 01:50:08 +0300
Subject: [PATCH 1/3] Use shared lock in GetMultiXactIdMembers for offsets and
members
Previously the read of multixact required exclusive control locks for both
offsets and members SLRUs. This could lead to the excessive lock contention.
This commit we makes multixacts SLRU take advantage of SimpleLruReadPage_ReadOnly
similar to clog, commit_ts and subtrans.
In order to evade extra reacquiring of CLRU lock, we teach
SimpleLruReadPage_ReadOnly() to take into account the current lock mode and
report resulting lock mode back.
Discussion: https://postgr.es/m/a7f1c4e1-1015-92a4-2bd4-6736bd13d03e%40postgrespro.ru#c496c4e75fc0605094a0e1f763e6a6ec
Author: Andrey Borodin
Reviewed-by: Kyotaro Horiguchi, Daniel Gustafsson
Reviewed-by: Anastasia Lubennikova, Alexander Korotkov
---
src/backend/access/transam/clog.c | 4 +++-
src/backend/access/transam/commit_ts.c | 4 +++-
src/backend/access/transam/multixact.c | 22 +++++++++++++++-------
src/backend/access/transam/slru.c | 23 +++++++++++++++++------
src/backend/access/transam/subtrans.c | 4 +++-
src/backend/commands/async.c | 4 +++-
src/backend/storage/lmgr/predicate.c | 4 +++-
src/include/access/slru.h | 2 +-
src/include/storage/lwlock.h | 2 ++
9 files changed, 50 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 034349aa7b9..4c372065dee 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -640,10 +640,11 @@ TransactionIdGetStatus(TransactionId xid, XLogRecPtr *lsn)
int lsnindex;
char *byteptr;
XidStatus status;
+ LWLockMode lockmode = LW_NONE;
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(XactCtl, pageno, xid);
+ slotno = SimpleLruReadPage_ReadOnly(XactCtl, pageno, xid, &lockmode);
byteptr = XactCtl->shared->page_buffer[slotno] + byteno;
status = (*byteptr >> bshift) & CLOG_XACT_BITMASK;
@@ -651,6 +652,7 @@ TransactionIdGetStatus(TransactionId xid, XLogRecPtr *lsn)
lsnindex = GetLSNIndex(slotno, xid);
*lsn = XactCtl->shared->group_lsn[lsnindex];
+ Assert(lockmode != LW_NONE);
LWLockRelease(XactSLRULock);
return status;
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index cb8a9688018..c19b0b5399e 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -288,6 +288,7 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
CommitTimestampEntry entry;
TransactionId oldestCommitTsXid;
TransactionId newestCommitTsXid;
+ LWLockMode lockmode = LW_NONE;
if (!TransactionIdIsValid(xid))
ereport(ERROR,
@@ -342,7 +343,7 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
}
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(CommitTsCtl, pageno, xid);
+ slotno = SimpleLruReadPage_ReadOnly(CommitTsCtl, pageno, xid, &lockmode);
memcpy(&entry,
CommitTsCtl->shared->page_buffer[slotno] +
SizeOfCommitTimestampEntry * entryno,
@@ -352,6 +353,7 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
if (nodeid)
*nodeid = entry.nodeid;
+ Assert(lockmode != LW_NONE);
LWLockRelease(CommitTsSLRULock);
return *ts != 0;
}
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 43653fe5721..ccbce90f0ea 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1237,6 +1237,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
MultiXactId tmpMXact;
MultiXactOffset nextOffset;
MultiXactMember *ptr;
+ LWLockMode lockmode = LW_NONE;
debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
@@ -1340,12 +1341,13 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
* time on every multixact creation.
*/
retry:
- LWLockAcquire(MultiXactOffsetSLRULock, LW_EXCLUSIVE);
pageno = MultiXactIdToOffsetPage(multi);
entryno = MultiXactIdToOffsetEntry(multi);
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
+ /* lock is acquired by SimpleLruReadPage_ReadOnly */
+ slotno = SimpleLruReadPage_ReadOnly(MultiXactOffsetCtl, pageno,
+ multi, &lockmode);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
offset = *offptr;
@@ -1377,7 +1379,8 @@ retry:
entryno = MultiXactIdToOffsetEntry(tmpMXact);
if (pageno != prev_pageno)
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
+ slotno = SimpleLruReadPage_ReadOnly(MultiXactOffsetCtl, pageno,
+ tmpMXact, &lockmode);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
@@ -1395,14 +1398,14 @@ retry:
length = nextMXOffset - offset;
}
+ Assert(lockmode != LW_NONE);
LWLockRelease(MultiXactOffsetSLRULock);
ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
*members = ptr;
/* Now get the members themselves. */
- LWLockAcquire(MultiXactMemberSLRULock, LW_EXCLUSIVE);
-
+ lockmode = LW_NONE;
truelength = 0;
prev_pageno = -1;
for (i = 0; i < length; i++, offset++)
@@ -1418,7 +1421,8 @@ retry:
if (pageno != prev_pageno)
{
- slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
+ slotno = SimpleLruReadPage_ReadOnly(MultiXactMemberCtl, pageno,
+ multi, &lockmode);
prev_pageno = pageno;
}
@@ -1441,6 +1445,7 @@ retry:
truelength++;
}
+ Assert(lockmode != LW_NONE);
LWLockRelease(MultiXactMemberSLRULock);
/*
@@ -2733,6 +2738,7 @@ find_multixact_start(MultiXactId multi, MultiXactOffset *result)
int entryno;
int slotno;
MultiXactOffset *offptr;
+ LWLockMode lockmode = LW_NONE;
Assert(MultiXactState->finishedStartup);
@@ -2749,10 +2755,12 @@ find_multixact_start(MultiXactId multi, MultiXactOffset *result)
return false;
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(MultiXactOffsetCtl, pageno, multi);
+ slotno = SimpleLruReadPage_ReadOnly(MultiXactOffsetCtl, pageno,
+ multi, &lockmode);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
offset = *offptr;
+ Assert(lockmode != LW_NONE);
LWLockRelease(MultiXactOffsetSLRULock);
*result = offset;
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 16a78986971..ca0a23ab875 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -487,17 +487,23 @@ SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
* Return value is the shared-buffer slot number now holding the page.
* The buffer's LRU access info is updated.
*
- * Control lock must NOT be held at entry, but will be held at exit.
- * It is unspecified whether the lock will be shared or exclusive.
+ * Control lock must be held in *lock_mode mode, which may be LW_NONE. Control
+ * lock will be held at exit in at least shared mode. Resulting control lock
+ * mode is set to *lock_mode.
*/
int
-SimpleLruReadPage_ReadOnly(SlruCtl ctl, int pageno, TransactionId xid)
+SimpleLruReadPage_ReadOnly(SlruCtl ctl, int pageno, TransactionId xid,
+ LWLockMode *lock_mode)
{
SlruShared shared = ctl->shared;
int slotno;
/* Try to find the page while holding only shared lock */
- LWLockAcquire(shared->ControlLock, LW_SHARED);
+ if (*lock_mode == LW_NONE)
+ {
+ LWLockAcquire(shared->ControlLock, LW_SHARED);
+ *lock_mode = LW_SHARED;
+ }
/* See if page is already in a buffer */
for (slotno = 0; slotno < shared->num_slots; slotno++)
@@ -517,8 +523,13 @@ SimpleLruReadPage_ReadOnly(SlruCtl ctl, int pageno, TransactionId xid)
}
/* No luck, so switch to normal exclusive lock and do regular read */
- LWLockRelease(shared->ControlLock);
- LWLockAcquire(shared->ControlLock, LW_EXCLUSIVE);
+ if (*lock_mode != LW_EXCLUSIVE)
+ {
+ Assert(*lock_mode == LW_NONE);
+ LWLockRelease(shared->ControlLock);
+ LWLockAcquire(shared->ControlLock, LW_EXCLUSIVE);
+ *lock_mode = LW_EXCLUSIVE;
+ }
return SimpleLruReadPage(ctl, pageno, true, xid);
}
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index 0111e867c79..7bb15431893 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -113,6 +113,7 @@ SubTransGetParent(TransactionId xid)
int slotno;
TransactionId *ptr;
TransactionId parent;
+ LWLockMode lockmode = LW_NONE;
/* Can't ask about stuff that might not be around anymore */
Assert(TransactionIdFollowsOrEquals(xid, TransactionXmin));
@@ -123,12 +124,13 @@ SubTransGetParent(TransactionId xid)
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(SubTransCtl, pageno, xid);
+ slotno = SimpleLruReadPage_ReadOnly(SubTransCtl, pageno, xid, &lockmode);
ptr = (TransactionId *) SubTransCtl->shared->page_buffer[slotno];
ptr += entryno;
parent = *ptr;
+ Assert(lockmode != LW_NONE);
LWLockRelease(SubtransSLRULock);
return parent;
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 8dbcace3f93..8b419575590 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -2002,6 +2002,7 @@ asyncQueueReadAllNotifications(void)
int curoffset = QUEUE_POS_OFFSET(pos);
int slotno;
int copysize;
+ LWLockMode lockmode = LW_NONE;
/*
* We copy the data from SLRU into a local buffer, so as to avoid
@@ -2010,7 +2011,7 @@ asyncQueueReadAllNotifications(void)
* part of the page we will actually inspect.
*/
slotno = SimpleLruReadPage_ReadOnly(NotifyCtl, curpage,
- InvalidTransactionId);
+ InvalidTransactionId, &lockmode);
if (curpage == QUEUE_POS_PAGE(head))
{
/* we only want to read as far as head */
@@ -2027,6 +2028,7 @@ asyncQueueReadAllNotifications(void)
NotifyCtl->shared->page_buffer[slotno] + curoffset,
copysize);
/* Release lock that we got from SimpleLruReadPage_ReadOnly() */
+ Assert(lockmode != LW_NONE);
LWLockRelease(NotifySLRULock);
/*
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index 8a365b400c6..a4df90a8aeb 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -924,6 +924,7 @@ SerialGetMinConflictCommitSeqNo(TransactionId xid)
TransactionId tailXid;
SerCommitSeqNo val;
int slotno;
+ LWLockMode lockmode = LW_NONE;
Assert(TransactionIdIsValid(xid));
@@ -946,8 +947,9 @@ SerialGetMinConflictCommitSeqNo(TransactionId xid)
* but will return with that lock held, which must then be released.
*/
slotno = SimpleLruReadPage_ReadOnly(SerialSlruCtl,
- SerialPage(xid), xid);
+ SerialPage(xid), xid, &lockmode);
val = SerialValue(slotno, xid);
+ Assert(lockmode != LW_NONE);
LWLockRelease(SerialSLRULock);
return val;
}
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index b39b43504d8..4b66d3b592b 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -142,7 +142,7 @@ extern int SimpleLruZeroPage(SlruCtl ctl, int pageno);
extern int SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
TransactionId xid);
extern int SimpleLruReadPage_ReadOnly(SlruCtl ctl, int pageno,
- TransactionId xid);
+ TransactionId xid, LWLockMode *lock_mode);
extern void SimpleLruWritePage(SlruCtl ctl, int slotno);
extern void SimpleLruWriteAll(SlruCtl ctl, bool allow_redirtied);
extern void SimpleLruTruncate(SlruCtl ctl, int cutoffPage);
diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index af9b41795d2..2684c8e51c9 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -129,6 +129,8 @@ extern PGDLLIMPORT int NamedLWLockTrancheRequests;
typedef enum LWLockMode
{
+ LW_NONE, /* Not a lock mode. Indicates that there is
+ * no lock. */
LW_EXCLUSIVE,
LW_SHARED,
LW_WAIT_UNTIL_FREE /* A special mode used in PGPROC->lwWaitMode,
--
2.14.3
[application/octet-stream] v4-0002-Make-MultiXact-local-cache-size-configurable.patch (3.7K, ../CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com/3-v4-0002-Make-MultiXact-local-cache-size-configurable.patch)
download | inline diff:
From b1af887f4c4e5402dce1b6ea226ac8b81b92443f Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Mon, 26 Oct 2020 03:44:50 +0300
Subject: [PATCH 2/3] Make MultiXact local cache size configurable
---
doc/src/sgml/config.sgml | 16 ++++++++++++++++
src/backend/access/transam/multixact.c | 2 +-
src/backend/utils/init/globals.c | 2 ++
src/backend/utils/misc/guc.c | 10 ++++++++++
src/include/miscadmin.h | 2 ++
5 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f043433e318..fd4ca293476 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1823,6 +1823,22 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-multixact-local-cache-entries" xreflabel="multixact_local-cache-entries">
+ <term><varname>multixact_local_cache_entries</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>multixact_local_cache_entries</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specifies the number cached MultiXact by backend. Any SLRU lookup is preceeded
+ by cache lookup. Higher numbers of cache size help to deduplicate lock sets, while
+ lower values make cache lookup faster.
+ It defaults to 256.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-max-stack-depth" xreflabel="max_stack_depth">
<term><varname>max_stack_depth</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index ccbce90f0ea..57be24c0cc1 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1613,7 +1613,7 @@ mXactCachePut(MultiXactId multi, int nmembers, MultiXactMember *members)
qsort(entry->members, nmembers, sizeof(MultiXactMember), mxactMemberComparator);
dlist_push_head(&MXactCache, &entry->node);
- if (MXactCacheMembers++ >= MAX_CACHE_ENTRIES)
+ if (MXactCacheMembers++ >= multixact_local_cache_entries)
{
dlist_node *node;
mXactCacheEnt *entry;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 6ab82168398..9ca71933dcc 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -149,3 +149,5 @@ int VacuumCostBalance = 0; /* working state for vacuum */
bool VacuumCostActive = false;
double vacuum_cleanup_index_scale_factor;
+
+int multixact_local_cache_entries = 256;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a62d64eaa47..d667578cc33 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2257,6 +2257,16 @@ static struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"multixact_local_cache_entries", PGC_SUSET, RESOURCES_MEM,
+ gettext_noop("Sets the number of cached MultiXact by backend."),
+ NULL
+ },
+ &multixact_local_cache_entries,
+ 256, 2, INT_MAX / 2,
+ NULL, NULL, NULL
+ },
+
{
{"temp_buffers", PGC_USERSET, RESOURCES_MEM,
gettext_noop("Sets the maximum number of temporary buffers used by each session."),
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 72e33523984..01af61c963a 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -162,6 +162,8 @@ extern PGDLLIMPORT int MaxConnections;
extern PGDLLIMPORT int max_worker_processes;
extern PGDLLIMPORT int max_parallel_workers;
+extern PGDLLIMPORT int multixact_local_cache_entries;
+
extern PGDLLIMPORT int MyProcPid;
extern PGDLLIMPORT pg_time_t MyStartTime;
extern PGDLLIMPORT TimestampTz MyStartTimestamp;
--
2.14.3
[application/octet-stream] v4-0003-Add-conditional-variable-to-wait-for-next-MultXact-o.patch (4.2K, ../CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com/4-v4-0003-Add-conditional-variable-to-wait-for-next-MultXact-o.patch)
download | inline diff:
From b878ffe74a10d34d312a7be66df7a53b423aaaec Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Mon, 26 Oct 2020 01:55:31 +0300
Subject: [PATCH 3/3] Add conditional variable to wait for next MultXact offset
in corner case
GetMultiXactIdMembers() has a corner case, when the next multixact offset is
not yet set. In this case GetMultiXactIdMembers() has to sleep till this offset
is set. Currently the sleeping is implemented in naive way using pg_sleep()
and retry. This commit implements sleeping with conditional variable, which
provides more efficient way for waiting till the event.
Discussion: https://postgr.es/m/a7f1c4e1-1015-92a4-2bd4-6736bd13d03e%40postgrespro.ru#c496c4e75fc0605094a0e1f763e6a6ec
Author: Andrey Borodin
Reviewed-by: Kyotaro Horiguchi, Daniel Gustafsson
Reviewed-by: Anastasia Lubennikova, Alexander Korotkov
---
src/backend/access/transam/multixact.c | 29 +++++++++++++++++++++++++++--
src/backend/postmaster/pgstat.c | 2 ++
src/include/pgstat.h | 1 +
3 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 57be24c0cc1..09df91899ec 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -82,6 +82,7 @@
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pg_trace.h"
+#include "pgstat.h"
#include "postmaster/autovacuum.h"
#include "storage/lmgr.h"
#include "storage/pmsignal.h"
@@ -233,6 +234,7 @@ typedef struct MultiXactStateData
/* support for members anti-wraparound measures */
MultiXactOffset offsetStopLimit; /* known if oldestOffsetKnown */
+ ConditionVariable nextoff_cv;
/*
* Per-backend data starts here. We have two arrays stored in the area
* immediately following the MultiXactStateData struct. Each is indexed by
@@ -892,6 +894,14 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
/* Exchange our lock */
LWLockRelease(MultiXactOffsetSLRULock);
+ /*
+ * Let everybody know the offset of this mxid is recorded now. The waiters
+ * are waiting for the offset of the mxid next of the target to know the
+ * number of members of the target mxid, so we don't need to wait for
+ * members of this mxid are recorded.
+ */
+ ConditionVariableBroadcast(&MultiXactState->nextoff_cv);
+
LWLockAcquire(MultiXactMemberSLRULock, LW_EXCLUSIVE);
prev_pageno = -1;
@@ -1389,9 +1399,23 @@ retry:
if (nextMXOffset == 0)
{
/* Corner case 2: next multixact is still being filled in */
+
+ /*
+ * The recorder of the next mxid is just before writing the offset.
+ * Wait for the offset to be written.
+ */
+ ConditionVariablePrepareToSleep(&MultiXactState->nextoff_cv);
+
+ /*
+ * We don't have to recheck if multixact was filled in during
+ * ConditionVariablePrepareToSleep(), because we were holding
+ * MultiXactOffsetSLRULock.
+ */
LWLockRelease(MultiXactOffsetSLRULock);
- CHECK_FOR_INTERRUPTS();
- pg_usleep(1000L);
+
+ ConditionVariableSleep(&MultiXactState->nextoff_cv,
+ WAIT_EVENT_WAIT_NEXT_MXMEMBERS);
+ ConditionVariableCancelSleep();
goto retry;
}
@@ -1873,6 +1897,7 @@ MultiXactShmemInit(void)
/* Make sure we zero out the per-backend state */
MemSet(MultiXactState, 0, SHARED_MULTIXACT_STATE_SIZE);
+ ConditionVariableInit(&MultiXactState->nextoff_cv);
}
else
Assert(found);
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 822f0ebc628..b99398a97e9 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4020,6 +4020,8 @@ pgstat_get_wait_ipc(WaitEventIPC w)
break;
case WAIT_EVENT_XACT_GROUP_UPDATE:
event_name = "XactGroupUpdate";
+ case WAIT_EVENT_WAIT_NEXT_MXMEMBERS:
+ event_name = "MultiXactWaitNextMembers";
break;
/* no default case, so that compiler will warn */
}
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index a821ff4f158..75ede141ab7 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -952,6 +952,7 @@ typedef enum
WAIT_EVENT_REPLICATION_SLOT_DROP,
WAIT_EVENT_SAFE_SNAPSHOT,
WAIT_EVENT_SYNC_REP,
+ WAIT_EVENT_WAIT_NEXT_MXMEMBERS,
WAIT_EVENT_XACT_GROUP_UPDATE
} WaitEventIPC;
--
2.14.3
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: MultiXact\SLRU buffers configuration
In-Reply-To: <CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox