I've noticed a few things here, v16 attached with some rather largish
changes.
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.
2. Using monotonic_advance for Write and Flush is useless. We can use a
simple atomic_write with a write barrier in between. The reason is
that, as Andres said[1], we only write those with WALWriteLock held, so
it's not possible for them to move forward while we aren't looking. All
callers of XLogWrite do RefreshXLogWriteResult() with the WALWriteLock
held. Therefore we can just use pg_atomic_write_u64. Consequently I
moved the addition of the monotonic advance function to the patch that
adds Copy.
3. Testing the invariant that the Copy pointer cannot be 0 is useless,
because we initialize that pointer to EndOfLog during StartupXLOG.
So, removed.
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. (Honestly I'm not sure
that we want XLogSendPhysical to be reading data that has not been
written, or do we?) Please argue why we need 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.
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.
[1] https://postgr.es/m/20210130023011.n545o54j65t4kgxn@alap3.anarazel.de
[2] https://postgr.es/m/202203221611.hqbjdinzsbu2@alvherre.pgsql
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/