Feedback-ID: ia2694551:Fastmail
Date: Fri, 5 Apr 2024 10:41:01 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.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: <202404050841.qajhfn4wkksi@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202404050841.qajhfn4wkksi%40alvherre.pgsql>
Precedence: bulk

On 2024-Apr-05, Bharath Rupireddy wrote:

> 1.
>  /*
>   * Update local copy of shared XLogCtl->log{Write,Flush}Result
> + *
> + * It's critical that Flush always trails Write, so the order of the reads is
> + * important, as is the barrier.
>   */
>  #define RefreshXLogWriteResult(_target) \
>      do { \
> -        _target.Write = XLogCtl->logWriteResult; \
> -        _target.Flush = XLogCtl->logFlushResult; \
> +        _target.Flush = pg_atomic_read_u64(&XLogCtl->logFlushResult); \
> +        pg_read_barrier(); \
> +        _target.Write = pg_atomic_read_u64(&XLogCtl->logWriteResult); \
>      } while (0)
> 
> Is it "Flush always trails Write" or "Flush always leades Write"? I
> guess the latter, no?

Well, Flush cannot lead Write.  We cannot Flush what hasn't been
Written!  To me, "Flush trails Write" means that flush is behind.  That
seems supported by Merriam-Webster[1], which says in defining it as a
transitive verb

3 a : to follow upon the scent or trace of : TRACK
  b : to follow in the footsteps of : PURSUE
  c : to follow along behind
  d : to lag behind (someone, such as a competitor)

Maybe it's not super clear as a term.  We could turn it around and say "Write
always leads Flush".

[1] https://www.merriam-webster.com/dictionary/trail

The reason we have the barrier, and the reason we write and read them in
this order, is that we must never read a Flush value that is newer than
the Write value we read.  That is: the values are written in pairs
(w1,f1) first, (w2,f2) next, and so on.  It's okay if we obtain (w2,f1)
(new write, old flush), but it's not okay if we obtain (w1,f2) (old
write, new flush).  If we did, we might end up with a Flush value that's
ahead of Write, violating the invariant.  

> 2.
> +
> +        pg_atomic_write_u64(&XLogCtl->logWriteResult, LogwrtResult.Write);
> +        pg_write_barrier();
> +        pg_atomic_write_u64(&XLogCtl->logFlushResult, LogwrtResult.Flush);
>      }
> 
> Maybe add the reason as to why we had to write logWriteResult first
> and then logFlushResult, similar to the comment atop
> RefreshXLogWriteResult?

Hmm, I had one there and removed it.  I'll put something back.

> > For 0002, did you consider having pg_atomic_monotonic_advance_u64()
> > return the currval?
> 
> +1 for returning currval here.

Oh yeah, I had that when the monotonic stuff was used by 0001 but lost
it when I moved to 0002.  I'll push 0001 now and send an updated 0002
with the return value added.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)