MIME-Version: 1.0
References: <zljergweqti7x67lg5ije2rzjusie37nslsnkjkkby4laqqbfw@3p3zu522yykv>
In-Reply-To: <zljergweqti7x67lg5ije2rzjusie37nslsnkjkkby4laqqbfw@3p3zu522yykv>
From: Thomas Munro <thomas.munro@gmail.com>
Date: Mon, 10 Nov 2025 11:20:38 +1300
Message-ID: 
 <CA+hUKGKwzPJNcxCFUswe0K=0e7rG79dvqt9o17-E3soxLPxF+Q@mail.gmail.com>
Subject: Re: Don't synchronously wait for already-in-progress IO in read
 stream
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie>,
 Tomas Vondra <tv@fuzzy.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BhUKGKwzPJNcxCFUswe0K%3D0e7rG79dvqt9o17-E3soxLPxF%2BQ%40mail.gmail.com>
Precedence: bulk

On Fri, Sep 12, 2025 at 9:46=E2=80=AFAM Andres Freund <andres@anarazel.de> =
wrote:
+     * It's possible that another backend starts IO on the buffer between =
this
+     * check and the ReadBuffersCanStartIO(nowait =3D false) below. In tha=
t case
+     * we will synchronously wait for the IO below, but the window for tha=
t is
+     * small enough that it won't happen often enough to have a significan=
t
+     * performance impact.
+     */
+    if (ReadBuffersIOAlreadyInProgress(operation, buffers[nblocks_done]))
...
     /*
      * Check if we can start IO on the first to-be-read buffer.
      *
-     * If an I/O is already in progress in another backend, we want to wai=
t
-     * for the outcome: either done, or something went wrong and we will
-     * retry.
+     * If a synchronous I/O is in progress in another backend (it can't be
+     * this backend), we want to wait for the outcome: either done, or
+     * something went wrong and we will retry.
      */
     if (!ReadBuffersCanStartIO(buffers[nblocks_done], false))

"..., or an asynchronous IO was started after
ReadBuffersIOAlreadyInProgress() (unlikely), ..."?

I suppose (or perhaps vaguely recall from an off-list discussion?)
that you must have considered merging the new
is-it-already-in-progress check into ReadBuffersCanStartIO().  I
suppose the nowait argument would become a tri-state argument with a
value that means "don't wait for an in-progress read, just give me the
IO handle so I can 'join' it as a foreign waiter", with a new output
argument to receive the handle, or something along those lines, and I
guess you'd need a tri-state result, and perhaps s/Can/Try/ in the
name.  That'd remove the double-check (extra header lock-unlock cycle)
and associated race that can cause that rare synchronous wait (which
must still happen sometimes in the duelling concurrent scan use
case?), at the slight extra cost of having to allocate and free a
handle in the case of repeated blocks (eg the index->heap scan use
case), but at least that's just backend-local list pushups and doesn't
do extra work otherwise.  Is there some logical problem with that
approach?  Is the code just too clumsy?