On Thu, 2023-12-07 at 15:59 +0530, Bharath Rupireddy wrote:
> In the attached v17 patch
0001 could impact performance could be impacted in a few ways:
* There's one additional write barrier inside
AdvanceXLInsertBuffer()
* AdvanceXLInsertBuffer() already holds WALBufMappingLock, so
the atomic access inside of it is somewhat redundant
* On some platforms, the XLogCtlData structure size will change
The patch has been out for a while and nobody seems concerned about
those things, and they look fine to me, so I assume these are not real
problems. I just wanted to highlight them.
Also, the description and the comments seem off. The patch does two
things: (a) make it possible to read a page without a lock, which means
we need to mark with InvalidXLogRecPtr while it's being initialized;
and (b) use 64-bit atomics to make it safer (or at least more
readable).
(a) feels like the most important thing, and it's a hard requirement
for the rest of the work, right?
(b) seems like an implementation choice, and I agree with it on
readability grounds.
Also:
+ * But it means that when we do this
+ * unlocked read, we might see a value that appears to be ahead of
the
+ * page we're looking for. Don't PANIC on that, until we've verified
the
+ * value while holding the lock.
Is that still true even without a torn read?
The code for 0001 itself looks good. These are minor concerns and I am
inclined to commit something like it fairly soon.
Regards,
Jeff Davis