Feedback-ID: ia2694551:Fastmail
Date: Mon, 8 Apr 2024 10:24:15 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
	pgsql-hackers@lists.postgresql.org,
 Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Message-ID: <202404080824.zwfgjynpnvif@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <51111525fb84ecdadaf99c13a2fa63e4be08041e.camel@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202404080824.zwfgjynpnvif%40alvherre.pgsql>
Precedence: bulk

On 2024-Apr-07, Jeff Davis wrote:

> On Sun, 2024-04-07 at 14:19 +0200, Alvaro Herrera wrote:
> > I pushed the "copy" pointer now, except that I renamed it to
> > "insert",
> > which is what we call the operation being tracked.  I also added some
> > comments.
> 
> The "copy" pointer, as I called it, is not the same as the "insert"
> pointer (as returned by GetXLogInsertRecPtr()).
> 
> LSNs before the "insert" pointer are reserved for WAL inserters (and
> may or may not have made it to WAL buffers yet). LSNs before the "copy"
> pointer are written to WAL buffers with CopyXLogRecordToWAL() (and may
> or may not have been evicted to the OS file yet).

It seems a matter of interpretation.  WAL insertion starts with
reserving the space (in ReserveXLogInsertLocation) and moving the
CurrBytePos point forward; the same WAL insertion completes when the
actual data has been copied to the buffers.  It is this process as a
whole that we can an insertion.  CurrBytePos (what GetXLogInsertRecPtr
returns) is the point where the next insertions are going to occur; the
new logInsertResult is the point where previous insertions are known to
have completed.

We could think about insertions as something that's happening to a range
of bytes.  CurrBytePos is the high end of that range, logInsertResult is
its low end.  (Although in reality we don't know the true low end,
because the process writing the oldest WAL doesn't advertise as soon as
it finishes its insertion, because it doesn't know that it is writing
the oldest.  We only know that X is this "oldest known" after we have
waited for all those that were inserting earlier than X to have
finished.)

My trouble with the "copy" term is that we don't use that term anywhere
in relation to WAL.  It's a term we don't need.  This "copy" is in
reality just the insertion, after it's finished.  The "Result" suffix
is intended to convey that it's the point where the operation has
successfully finished.

Maybe we can add some commentary to make this clearer.

Now, if you're set on renaming the variable back to Copy, I won't
object.

Lastly, I just noticed that I forgot credits and discussion link in that
commit message.  I would have attributed authorship to Bharath though,
because I had not realized that this patch had actually been written by
you initially[1].  My apologies.

[1] https://postgr.es/m/727449f3151c6b9ab76ba706fa4d30bf7b03ad4f.camel@j-davis.com

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/