Re: Online checksums verification in the backend - Mailing list pgsql-hackers

On Sun, Nov 01, 2020 at 05:39:40PM -0800, Andres Freund wrote:
> I'm a bit limited writing - one handed for a while following an injury
> on Friday...

Oops.  Take care.

> On 2020-11-02 10:05:25 +0900, Michael Paquier wrote:
> > There is no place doing that on HEAD.
>
> Err?
>
>     /* see if the block is in the buffer pool or not */
>     LWLockAcquire(partLock, LW_SHARED);
>     buf_id = BufTableLookup(&buf_tag, buf_hash);
>
> [...]
>
> How is this not doing IO while holding a buffer mapping lock?

Well, other than the one we are discussing of course :)

>
>
> > This specific point was mentioned in the first message of this thread,
> > 7th paragraph.  That was a long thread, so it is easy to miss:
> > https://www.postgresql.org/message-id/CAOBaU_aVvMjQn=ge5qPiJOPMmOj5=ii3st5Q0Y+WuLML5sR17w@mail.gmail.com
>
> The code clearly doesnt implement it that way.
>
>
> > I am wondering what you have in mind regarding the use of
> > BM_IO_IN_PROGRESS or a similar flag.  Wouldn't that imply some
> > consequences for other existing buffers in the table, like a possible
> > eviction?
>
> You'd need exactly one empty buffer for that - it can be reused for the
> next to-be-checked buffer.
>
>
> > I'd like to think that we should not do any manipulation of
> > the buffer tables in this case.
>
> Why? Its the way we lock buffers - why is this so special that we need
> to do differently?
>
>
> > Hence, in order to prevent a
> > concurrent activity to load in shared buffers the page currently
> > checked on disk, I got to think that we would need something new here,
> > like a filtering hash table that would be checked each time a backend
> > tries to insert an entry into the buffer tables.
>
> Thats going to slow down everything a bit - the mapping already is a
> bottleneck.
>
>
> > 1) If the buffer is in shared buffers, we have the APIs to solve that
> > by using a pin, unlock the partition, and then do the I/O.  (Still
> > that's unsafe with the smgrwrite() argument?)
>
> Thats why you need an appropriate relation lock... Something CheckBuffer
> didnt bother to document. Its a restriction, but one we probably can
> live with.
>
>
> > 2) If the buffer is not in shared buffers, we don't have what it takes
> > to solve the problem yet.
>
> We do. Set up enough state for the case to be otherwise the same as the
> in s_b case.
>
> > But even if we solve this problem, we will
> > never really be sure that this is entirely safe, as per the argument
> > with concurrent smgrwrite() calls.  Current in-core code assumes that
> > this can happen only for init forks of unlogged relations which would
> > not be visible yet in the backend doing a page check, still it can be
> > really easy to break this assumption with any new code added by a new
> > feature.
>
> It also happens in a few other cases than just init forks. But
> visibility & relation locking can take care of that. But you need to
> document that. If the locking allows concurent readers - and especially
> concurrent writers, then you cant really use smgrwite for anything but
> relation extension.

--
Michael