Message-ID: <c43c3edf3c9272fecf54880cd282dbc4f70c3f7e.camel@j-davis.com>
Subject: Re: LogwrtResult contended spinlock
From: Jeff Davis <pgsql@j-davis.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Alvaro
 Herrera <alvherre@alvh.no-ip.org>, Tom Lane <tgl@sss.pgh.pa.us>, Andres
 Freund <andres@anarazel.de>,  pgsql-hackers@lists.postgresql.org, Jaime
 Casanova <jcasanov@systemguards.com.ec>
Date: Thu, 22 Feb 2024 11:48:22 -0800
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/c43c3edf3c9272fecf54880cd282dbc4f70c3f7e.camel%40j-davis.com>
Precedence: bulk

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 >=3D recptr)
         return true;
     writeptr =3D GetXLogWriteRecPtr();
     return (writeptr >=3D 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