From: Jeff Davis <pgsql@j-davis.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Date: Thu, 22 Feb 2024 11:48:22 -0800
Message-ID: <c43c3edf3c9272fecf54880cd282dbc4f70c3f7e.camel@j-davis.com> (raw)
In-Reply-To: <CA+TgmoZ84r83AxuTmdw7O0J0LnVn6FHc1wBe35GgD7u8KC8RfA@mail.gmail.com>
References: <20220923084941.g3756xqz6lwnqwso@alvherre.pgsql>
<ceae1334bcc0a94d1425956ec678d535714a6f0e.camel@j-davis.com>
<727449f3151c6b9ab76ba706fa4d30bf7b03ad4f.camel@j-davis.com>
<CALj2ACXrePj4E6ocKr-+b=rjT-8yeMmHnEeWQP1bc-WXETfTVw@mail.gmail.com>
<b43615437ac7d7fdef86a36e5d5bf3fc049bc11b.camel@j-davis.com>
<CA+TgmoZ84r83AxuTmdw7O0J0LnVn6FHc1wBe35GgD7u8KC8RfA@mail.gmail.com>
On Thu, 2024-02-22 at 10:17 +0530, Robert Haas wrote:
> I think the problems tend to be worst when you have some bit of data
> that's being frequently modified by multiple backends. Every backend
> that wants to modify the value needs to steal the cache line, and
> eventually you spend most of your CPU time stealing cache lines from
> other sockets and not much of it doing any actual work. If you have a
> value that's just being read by a lot of backends without
> modification, I think the cache line can be shared in read only mode
> by all the CPUs and it's not too bad.
That makes sense. I guess they'd be on the same cache line as well,
which means a write to either will invalidate both.
Some places (XLogWrite, XLogInsertRecord, XLogSetAsyncXactLSN,
GetFlushRecPtr, GetXLogWriteRecPtr) already update it from shared
memory anyway, so those are non-issues.
The potential problem areas (unless I missed something) are:
* AdvanceXLInsertBuffer reads it as an early check to see if a buffer
is already written out.
* XLogFlush / XLogNeedsFlush use it for an early return
* WALReadFromBuffers reads it for an error check (currently an
Assert, but we might want to make that an elog).
* We are discussing adding a Copy pointer, which would be advanced by
WaitXLogInsertionsToFinish(), and if we do replication-before-flush, we
may want to be more eager about advancing it. That could cause an
issue, as well.
I don't see the global non-shared variable as a huge problem, so if it
serves a purpose then I'm fine keeping it. Perhaps we could make it a
bit safer by using some wrapper functions. Something like:
bool
IsWriteRecPtrAtLeast(XLogRecPtr recptr)
{
XLogRecPtr writeptr;
if (LogwrtResult.Write >= recptr)
return true;
writeptr = GetXLogWriteRecPtr();
return (writeptr >= recptr);
}
That would reduce the number of direct references to LogwrtResult,
callers would never see a stale value, and would avoid the cache miss
problem that you're concerned about. Not sure if they'd need to be
inline or not.
Regards,
Jeff Davis
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: pgsql-hackers@postgresql.org
Cc: pgsql@j-davis.com, robertmhaas@gmail.com, bharath.rupireddyforpostgres@gmail.com, alvherre@alvh.no-ip.org, tgl@sss.pgh.pa.us, andres@anarazel.de, jcasanov@systemguards.com.ec
Subject: Re: LogwrtResult contended spinlock
In-Reply-To: <c43c3edf3c9272fecf54880cd282dbc4f70c3f7e.camel@j-davis.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox