public inbox for [email protected]  

help / color / mirror / Atom feed

From: Kyotaro Horiguchi <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: BufferAlloc: don't take two simultaneous locks
Date: Fri, 11 Mar 2022 17:21:37 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

At Fri, 11 Mar 2022 15:49:49 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in 
> At Fri, 11 Mar 2022 15:30:30 +0900 (JST), Kyotaro Horiguchi <[email protected]> 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

view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: BufferAlloc: don't take two simultaneous locks
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox