public inbox for [email protected]  

help / color / mirror / Atom feed

From: Alvaro Herrera <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Jeff Davis <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected]
Cc: Jaime Casanova <[email protected]>
Subject: Re: LogwrtResult contended spinlock
Date: Fri, 5 Apr 2024 10:41:01 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg@mail.gmail.com>

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)

view thread (2+ messages)

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], [email protected], [email protected]
  Subject: Re: LogwrtResult contended spinlock
  In-Reply-To: <[email protected]>

* 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