Re: LogwrtResult contended spinlock

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