On 09/02/2026 03:52, Andres Freund wrote:
On 2026-02-07 14:59:34 +0200, Heikki Linnakangas wrote:
+/*
+ * Try to set a single hint bit in a buffer.
+ *
+ * This is a bit faster than BufferBeginSetHintBits() /
+ * BufferFinishSetHintBits() when setting a single hint bit, but slower than
+ * the former when setting several hint bits.
+ */
+bool
+BufferSetHintBits16(uint16 *ptr, uint16 val, Buffer buffer)
This could use some more explanation. The point is that this does "*ptr =
val", if it's allowed to set hint bits. That's not obvious. And "single hint
bit" isn't really accurate, as you could update multiple bits in *ptr with
one call.
Agreed. I updated it to
* Try to set hint bits on a single 16bit value in a buffer.
*
* If hint bits are allowed to be set, set *ptr = val, try mark the buffer
* dirty and return true. Otherwise false is returned.
*
* *ptr needs to be a pointer to memory within the buffer.
*
* This is a bit faster than BufferBeginSetHintBits() /
* BufferFinishSetHintBits() when setting hints once in a buffer, but slower
* than the former when setting hint bits multiple times in the same buffer.
+1. Instead of "try mark the buffer dirty", I'd say just "mark the
buffer dirty". The only reason it might not to mark the buffer dirty is
that it was already marked dirty, right? I wouldn't call that a failure.
/*
* If the buffer was dirty, try to write it out. There is a race
* condition here, in that someone might dirty it after we released the
* buffer header lock above. We will recheck the dirty bit after
* re-locking the buffer header.
*/
It's not clear what "above" means in that paragraph. Where do we release the
buffer header lock? In StrategyGetBuffer?
(This is not actually new with this patch; it goes back to commit
5e89985928. Before that, there was a call to PinBuffer_Locked() which
released the spinlock.)
Yea, looks like I should have edited the comment in that commit. Updated to:
* If the buffer was dirty, try to write it out. There is a race
* condition here, another backend could dirty the buffer between
* StrategyGetBuffer() checking that it is not in use and invalidating
the
* buffer below. That's addressed by InvalidateVictimBuffer() verifying
* that the buffer is not dirty.
+1
+/*
+ * Helper for BufferBeginSetHintBits() and BufferSetHintBits16().
+ *
+ * This checks if the current lock mode already suffices to allow hint bits
+ * being set and, if not, whether the current lock can be upgraded.
+ *
+ * Updates *lockstate when returning true.
+ */
+static inline bool
+SharedBufferBeginSetHintBits(Buffer buffer, BufferDesc *buf_hdr, uint64
*lockstate)
Would be good to be more explicit what returning true/false here means.
Hm, ISTM that'd just end up restating the comments for
BufferBeginSetHintBits(). Given SharedBufferBeginSetHintBits() is just an
implementation detail for BufferBeginSetHintBits/BufferSetHintBits16, I don't
think it's worth restating the details here.
Ok fair.
- Heikki
