Date: Thu, 1 Oct 2020 14:07:17 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Online checksums verification in the backend
Message-ID: <20201001050717.GK8130@paquier.xyz>
References: 
 <CAOBaU_bJEYemqwzQ4p8+txuT64AXUg2L0p8qHgs5vNGuZ0_kVw@mail.gmail.com>
 <20200907085038.GA19261@paquier.xyz>
 <20200909092524.GC57691@nol>
 <20200909134130.GA29325@nol>
 <20200910074723.GA91494@nol>
 <20200910180610.GA62228@nol>
 <20200911073412.GJ2743@paquier.xyz>
 <CAOBaU_a-=xi6mPF5imNKMFRAoBEpkYkkAoW9Ss+XA4qApZ6WqQ@mail.gmail.com>
 <20200916034558.GB11110@paquier.xyz>
 <CAOBaU_Y_A0m5x2QwQ=z6aNf=M-uT1aFFVLeMcHfMDS8z+KrTTw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="pN9MePJoZbRKbUk1"
Content-Disposition: inline
In-Reply-To: 
 <CAOBaU_Y_A0m5x2QwQ=z6aNf=M-uT1aFFVLeMcHfMDS8z+KrTTw@mail.gmail.com>
Precedence: bulk


--pN9MePJoZbRKbUk1
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Sep 25, 2020 at 06:11:47PM +0800, Julien Rouhaud wrote:
> Thanks a lot for the tests!  I'm not surprised that forcing the lock
> will slow down the pg_check_relation() execution, but I'm a bit
> surprised that holding the buffer mapping lock longer in a workload
> that has a lot of evictions actually makes things faster.  Do you have
> any idea why that's the case?

That's still a bit unclear to me, but I have not spent much time
thinking about this particular point either.

> I'm assuming that you prefer to remove both the optimization and the
> throttling part?  I'll do that with the next version unless there's
> objections.

Yeah, any tests I have done tends to show that.  It would be good to
also check some perf profiles here, at least for the process running
the relation check in a loop.

> I agree that putting the code nearby ReadBuffer_common() would be a
> good idea.  However, that means that I can't move all the code to
> contrib/  I'm wondering what you'd like to see going there.  I can see
> some values in also having the SQL functions available in core rather
> than contrib, e.g. if you need to quickly check a relation on a
> standby, so without requiring to create the extension on the primary
> node first.

Good point.  This could make the user experience worse.

>  Then, I'm a bit worried about adding this code in ReadBuffer_common.
> What this code does is quite different, and I'm afraid that it'll make
> ReadBuffer_common more complex than needed, which  is maybe not a good
> idea for something as critical as this function.
>=20
> What do you think?

Yeah, I have been looking at ReadBuffer_common() and it is true that
it is complicated enough so we may not really need an extra mode or
more options, for a final logic that is actually different than what a
buffer read does: we just want to know if a page has a valid checksum
or not.  An idea that I got here would be to add a new, separate
function to do the page check directly in bufmgr.c, but that's what
you mean.  Now only the prefetch routine and ReadBuffer_common use
partition locks, but getting that done in the same file looks like a
good compromise to me.  It would be also possible to keep the BLCKSZ
buffer used to check the page directly in this routine, so as any
caller willing to do a check don't need to worry about any
allocation.
--
Michael

--pN9MePJoZbRKbUk1
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl91ZAUACgkQnvQgOdby
QH2xhw//QMkNdJsMRCE2ODREZRMRC8m+O0nmfq7wgRXM39j4NTGKBpPL6QjuLh3N
I9mmwUC2vUzUnklu2PANjiwzWBoZH7zwy3myEs5oUQwqhWFYqG+avTYSpmywdS/+
qkHIj121GK5pE35ZyociSe03bx6eHDdHCu6DFh4pvDi+kcJt8ZEXBl4Qqgl9reGb
Hrd7NjUlshVfaDVZiS5F+basSoKy7orXn0Sko/dtX8+pT735mOV/qFak6HvZx6BM
lDgy/1YDTzV5KNHG3bBsfLn6LDtEkK7iZZ1Ib/kl5bOxoDQls6OvHY6fnOPduubf
/5tjsoktvtua7m+ZNODQj7vZb4G5D9z+978jEnggjxrF0vmZYpXFo+r/d0uup9JO
IBNKMMUXjkLNM+HVGGQV8U9tx/KAXY9PxZZpCeUGzS3tmlVzjzWMnIE0gTR/kMs5
vFH0huofMH0TnaxLlDl9j5SqvmY0xuf73eYBnVHl6QhuznjPLTu/i+XHR0d0fL2Q
AoXQ9xanKZwJY2IvgtFlj5z7zEy195E9jyNghaUWEwEDzMnW5KVsXG9fhU76sTrm
DLzWmWQORYXtbWusAJNkrnjBYzTiXIh6lICiIVDr2HCYwJpRHjnEuSpKjJenDb31
OpzWrj3FwVdsous1lRLhSQHge3IrT1fMTYJspngqRIzKLeUa1HM=
=XWBx
-----END PGP SIGNATURE-----

--pN9MePJoZbRKbUk1--