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
