Re: BufferAlloc: don't take two simultaneous locks

At Fri, 11 Mar 2022 15:49:49 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> At Fri, 11 Mar 2022 15:30:30 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> > Thanks! I looked into dynahash part.

Then I looked into bufmgr part. It looks fine to me but I have some
comments on code comments.

> * To change the association of a valid buffer, we'll need to have
> * exclusive lock on both the old and new mapping partitions.
> if (oldFlags & BM_TAG_VALID)

We don't take lock on the new mapping partition here.

+ * Clear out the buffer's tag and flags. We must do this to ensure that
+ * linear scans of the buffer array don't think the buffer is valid. We
+ * also reset the usage_count since any recency of use of the old content
+ * is no longer relevant.
+ *
+ * We are single pinner, we hold buffer header lock and exclusive
+ * partition lock (if tag is valid). Given these statements it is safe to
+ * clear tag since no other process can inspect it to the moment.

This comment is a merger of the comments from InvalidateBuffer and
BufferAlloc. But I think what we need to explain here is why we
invalidate the buffer here despite of we are going to reuse it soon.
And I think we need to state that the old buffer is now safe to use
for the new tag here. I'm not sure the statement is really correct
but clearing-out actually looks like safer.

> Now it is safe to use victim buffer for new tag. Invalidate the
> buffer before releasing header lock to ensure that linear scans of
> the buffer array don't think the buffer is valid. It is safe
> because it is guaranteed that we're the single pinner of the buffer.
> That pin also prevents the buffer from being stolen by others until
> we reuse it or return it to freelist.

So I want to revise the following comment.

- * Now it is safe to use victim buffer for new tag.
+ * Now reuse victim buffer for new tag.
> * Make sure BM_PERMANENT is set for buffers that must be written at every
> * checkpoint. Unlogged buffers only need to be written at shutdown
> * checkpoints, except for their "init" forks, which need to be treated
> * just like permanent relations.
> *
> * The usage_count starts out at 1 so that the buffer can survive one
> * clock-sweep pass.

But if you think the current commet is fine, I don't insist on the
comment chagnes.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center