Date: Fri, 11 Mar 2022 17:21:37 +0900 (JST)
Message-Id: <20220311.172137.603201487885714707.horikyota.ntt@gmail.com>
To: y.sokolov@postgrespro.ru
Cc: pgsql-hackers@lists.postgresql.org, michail.nikolaev@gmail.com,
 x4mmm@yandex-team.ru, andres@anarazel.de, simon.riggs@enterprisedb.com
Subject: Re: BufferAlloc: don't take two simultaneous locks
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <20220311.154949.1643256093439212518.horikyota.ntt@gmail.com>
References: <e70ea8843dcc94cdb8e7892c5140d683f75e4baf.camel@postgrespro.ru>
	<20220311.153030.1881495127262295773.horikyota.ntt@gmail.com>
	<20220311.154949.1643256093439212518.horikyota.ntt@gmail.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20220311.172137.603201487885714707.horikyota.ntt%40gmail.com>
Precedence: bulk

At Fri, 11 Mar 2022 15:49:49 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Fri, 11 Mar 2022 15:30:30 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.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