public inbox for [email protected]  

help / color / mirror / Atom feed

From: Thomas Munro <[email protected]>
To: Andres Freund <[email protected]>
Cc: [email protected]
Cc: Peter Geoghegan <[email protected]>
Cc: Tomas Vondra <[email protected]>
Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
Date: Mon, 10 Nov 2025 11:20:38 +1300
Message-ID: <CA+hUKGKwzPJNcxCFUswe0K=0e7rG79dvqt9o17-E3soxLPxF+Q@mail.gmail.com> (raw)
In-Reply-To: <zljergweqti7x67lg5ije2rzjusie37nslsnkjkkby4laqqbfw@3p3zu522yykv>
References: <zljergweqti7x67lg5ije2rzjusie37nslsnkjkkby4laqqbfw@3p3zu522yykv>

On Fri, Sep 12, 2025 at 9:46 AM Andres Freund <[email protected]> wrote:
+     * It's possible that another backend starts IO on the buffer between this
+     * check and the ReadBuffersCanStartIO(nowait = false) below. In that case
+     * we will synchronously wait for the IO below, but the window for that is
+     * small enough that it won't happen often enough to have a significant
+     * 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 wait
-     * 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?

view thread (28+ 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]
  Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
  In-Reply-To: <CA+hUKGKwzPJNcxCFUswe0K=0e7rG79dvqt9o17-E3soxLPxF+Q@mail.gmail.com>

* 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