Replace buffer I/O locks with condition variables (reviving an old patch)

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Replace buffer I/O locks with condition variables (reviving an old patch)
Date: 2021-02-26 06:08:12
Message-ID: CA+hUKGJ8nBFrjLuCTuqKN0pd2PQOwj9b_jnsiGFFMDvUxahj_A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello hackers,

Back in 2016, Robert Haas proposed to replace I/O locks with condition
variables[1]. Condition variables went in and have found lots of
uses, but this patch to replace a bunch of LWLocks and some busy
looping did not. Since then, it has been tested quite a lot as part
of the AIO project[2], which currently depends on it. That's why I'm
interested in following up now. I asked Robert if he planned to
re-propose it and he said I should go for it, so... here I go.

At the time, Tom Lane said:

> Hmm. I fear the only reason you see an advantage there is that you don't
> (yet) have any general-purpose mechanism for an aborting transaction to
> satisfy its responsibilities vis-a-vis waiters on condition variables.
> Instead, this wins specifically because you stuck some bespoke logic into
> AbortBufferIO. OK ... but that sounds like we're going to end up with
> every single condition variable that ever exists in the system needing to
> be catered for separately and explicitly during transaction abort cleanup.
> Which does not sound promising from a reliability standpoint. On the
> other hand, I don't know what the equivalent rule to "release all LWLocks
> during abort" might look like for condition variables, so I don't know
> if it's even possible to avoid that.

It's true that cases like this one need bespoke logic, but that was
already the case: you have to make sure you call TerminateBufferIO()
as before, it's just that BM_IO_IN_PROGRESS-clearing is now a
CV-broadcastable event. That seems reasonable to me. As for the more
general point about the danger of waiting on CVs when potential
broadcasters might abort, and with the considerable benefit of a few
years of hindsight: I think the existing users of CVs mostly fall
into the category of waiters that will be shut down by a higher
authority if the expected broadcaster aborts. Examples: Parallel
query's interrupt-based error system will abort every back end waiting
at a parallel hash join barrier if any process involved in the query
aborts, and the whole cluster will be shut down if you're waiting for
a checkpoint when the checkpointer dies.

It looks like there may be a nearby opportunity to improve another
(rare?) busy loop, when InvalidateBuffer() encounters a pinned buffer,
based on this comment:

* ... Note that if the other guy has pinned the buffer but not
* yet done StartBufferIO, WaitIO will fall through and we'll effectively
* be busy-looping here.)

[1] https://www.postgresql.org/message-id/flat/CA%2BTgmoaj2aPti0yho7FeEf2qt-JgQPRWb0gci_o1Hfr%3DC56Xng%40mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/20210223100344.llw5an2aklengrmn%40alap3.anarazel.de

Attachment Content-Type Size
0001-Replace-buffer-I-O-locks-with-condition-variables.patch text/x-patch 12.7 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrey V. Lepikhov 2021-02-26 06:20:49 Re: Global snapshots
Previous Message Kyotaro Horiguchi 2021-02-26 05:37:50 Re: libpq debug log