Hi,
On 2022-09-20 12:45:24 +0300, Aleksander Alekseev wrote:
> > I'm not sure how to avoid any undefined behaviour without locks though.
> > Even with locks, performance is much better. But is it good enough for
> > production?
>
> Potentially you could avoid taking locks by utilizing atomic
> operations and lock-free algorithms. But these algorithms are
> typically error-prone and not always produce a faster code than the
> lock-based ones. I'm pretty confident this is out of scope of this
> particular patch.
Why would you need lockfree operations? All you need to do is to read
BufferDesc->state into a local variable and then make decisions based on that?
> + for (int i = 0; i < NBuffers; i++)
> + {
> + BufferDesc *bufHdr;
> + uint32 buf_state;
> +
> + bufHdr = GetBufferDescriptor(i);
> +
> + /* Lock each buffer header before inspecting. */
> + buf_state = LockBufHdr(bufHdr);
> +
> + /* Invalid RelFileNumber means the buffer is unused */
> + if (RelFileNumberIsValid(BufTagGetRelNumber(&bufHdr->tag)))
> + {
> + buffers_used++;
> + usagecount_avg += BUF_STATE_GET_USAGECOUNT(buf_state);
> +
> + if (buf_state & BM_DIRTY)
> + buffers_dirty++;
> + }
> + else
> + buffers_unused++;
> +
> + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
> + buffers_pinned++;
> +
> + UnlockBufHdr(bufHdr, buf_state);
> + }
I.e. instead of locking the buffer header as done above, this could just do
something along these lines:
BufferDesc *bufHdr;
uint32 buf_state;
bufHdr = GetBufferDescriptor(i);
buf_state = pg_atomic_read_u32(&bufHdr->state);
if (buf_state & BM_VALID)
{
buffers_used++;
usagecount_avg += BUF_STATE_GET_USAGECOUNT(buf_state);
if (buf_state & BM_DIRTY)
buffers_dirty++;
}
else
buffers_unused++;
if (BUF_STATE_GET_REFCOUNT(buf_state) > 0)
buffers_pinned++;
Without a memory barrier you can get very slightly "out-of-date" values of the
state, but that's fine in this case.
Greetings,
Andres Freund
