MIME-Version: 1.0
References: <202404041745.jzrrw3ffyac7@alvherre.pgsql> <5e3a2a809388bc01b66bf7fa692eaac5622acc0a.camel@j-davis.com>
In-Reply-To: <5e3a2a809388bc01b66bf7fa692eaac5622acc0a.camel@j-davis.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 5 Apr 2024 09:59:09 +0530
Message-ID: <CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg@mail.gmail.com>
Subject: Re: LogwrtResult contended spinlock
To: Jeff Davis <pgsql@j-davis.com>
Cc: 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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg%40mail.gmail.com>
Precedence: bulk

On Fri, Apr 5, 2024 at 4:21=E2=80=AFAM Jeff Davis <pgsql@j-davis.com> 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/CALj2ACWmW+1ZdYE0BD5-4KVLzYGn3=3Dpudo=
mwPHuHmi4YQuOFSg@mail.gmail.com
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 =3D XLogCtl->logWriteResult; \
-        _target.Flush =3D XLogCtl->logFlushResult; \
+        _target.Flush =3D pg_atomic_read_u64(&XLogCtl->logFlushResult); \
+        pg_read_barrier(); \
+        _target.Write =3D 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=3D"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