Subject: Re: LogwrtResult contended spinlock

From: Jeff Davis <[email-masked]>
Date: Thu, 04 Apr 2024 15:50:58 -0700

On Thu, 2024-04-04 at 19:45 +0200, Alvaro Herrera wrote:
> 1. Using pg_atomic_write_membarrier_u64 is useless and it imposes
> mora
> barriers than we actually need.  So I switched back to
> pg_atomic_write_u64 and add one barrier between the two writes.  Same
> for reads.

+1.

This looks correct to me. Just before the writes there's a spinlock,
which acts as a full barrier; and just afterwards, the function returns
and the WALWriteLock is released, again acting as a full barrier. The
write barrier in between enforces the Write >= Flush invariant.

> 2. Using monotonic_advance for Write and Flush is useless.

+1.

> 3. Testing the invariant that the Copy pointer cannot be 0 is
> useless,
> because we initialize that pointer to EndOfLog during StartupXLOG.
> So, removed.

+1.

> 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.

>   Please argue why we need this patch.

I'm not sure what you mean by "this patch"?

> 5. The existing weird useless-looking block at the end of XLogWrite
> is
> there because we used to have it to declare a volatile pointer to
> XLogCtl (cf.  commit 6ba4ecbf477e).  That's no longer needed, so we
> could remove it.  Or we could leave it alone (just because it's
> ancient
> and it doesn't hurt anything), but there's no reason to have the new
> invariant-testing block inside the same block.  So I added another
> weird
> useless-looking block, except that this one does have two variable
> declaration at its top.

That didn't bother me, but it could be cleaned up a bit in a later
patch.

> 6. In a few places, we read both Write and Flush to only use one of
> them.  This is wasteful and we could dial this back to reading only
> the
> one we need.  Andres suggested as much in [1].  I didn't touch this
> in
> the current patch, and I don't necessarily think we need to address
> it
> right now.  Addressing this should probably done similar to what I
> posted in [2]'s 0002.

I agree that it should be a separate patch. I haven't thought about the
consequences of making them fully independent -- I think that means we
give up the invariant that Copy >= Write >= Flush?

Regarding the patches themselves, 0001 looks good to me.

For 0002, did you consider having pg_atomic_monotonic_advance_u64()
return the currval?
	
Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/CALj2ACV6rS%2B7iZx5%2BoAvyXJaN4AG-djAQeM1mrM%3DYSDkVrUs7g%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/20230125211540.zylu74dj2uuh3k7w%40awork3.anarazel.de
[3]
https://www.postgresql.org/message-id/CALj2ACW65mqn6Ukv57SqDTMzAJgd1N_AdQtDgy%2BgMDqu6v618Q%40mail.gmail.com

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 5 Apr 2024 09:59:09 +0530

> 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=[email-masked]
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()

+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