public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: Jeff Davis <[email protected]>
Cc: Alvaro Herrera <[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 09:59:09 +0530
Message-ID: <CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>

On Fri, Apr 5, 2024 at 4:21 AM Jeff Davis <[email protected]> wrote:
>
> > 4. If we're not modifying any callers of WALReadFromBuffers(), then
> > AFAICS the added check that the request is not past the Copy pointer
> > is
> > useless.  In a quick look at that code, I think we only try to read
> > data
> > that's been flushed, not written, so the stricter check that we don't
> > read data that hasn't been Copied does nothing.
>
> Bharath has indicated that he may call WALReadFromBuffers() in an
> extension, so I believe some error checking is appropriate there.
>
> >   (Honestly I'm not sure
> > that we want XLogSendPhysical to be reading data that has not been
> > written, or do we?)
>
> Not yet, but there has been some discussion[1][2] about future work to
> allow replicating data before it's been flushed locally.

Right. Although callers of WALReadFromBuffers() in core postgres
doesn't need it now (in future, they will), but it allows one to write
something up externally - for example, see 0004 patch in
https://www.postgresql.org/message-id/[email protected]...
where I implemented a xlogreader page_read callback that just keeps
reading the WAL that's fully copied to WAL buffers.

> Regarding the patches themselves, 0001 looks good to me.

A few comments on 0001:

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?

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?

> For 0002, did you consider having pg_atomic_monotonic_advance_u64()
> return the currval?

+1 for returning currval here.

Except for the above comments, the patches look good to me. I've also
run the test loop for any assertion failure - for i in {1..100}; do
make check PROVE_TESTS="t/027_stream_regress.pl"; if [ $? -ne 0 ];
then echo "The command failed on iteration $i"; break; fi; done.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

view thread (3+ 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: <CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg@mail.gmail.com>

* 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