Re: WIP: WAL prefetch (another approach)

On Thu, Mar 18, 2021 at 12:00 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> On 3/17/21 10:43 PM, Stephen Frost wrote:
> > Guess I'm just not a fan of pushing out a change that will impact
> > everyone by default, in a possibly negative way (or positive, though
> > that doesn't seem terribly likely, but who knows), without actually
> > measuring what that impact will look like in those more common cases.
> > Showing that it's a great win when you're on ZFS or running with FPWs
> > disabled is good and the expected best case, but we should be
> > considering the worst case too when it comes to performance
> > improvements.
> >
>
> Well, maybe it'll behave differently on systems with ZFS. I don't know,
> and I have no such machine to test that at the moment. My argument
> however remains the same - if if happens to be a problem, just don't
> enable (or disable) the prefetching, and you get the current behavior.

I see the road map for this feature being to get it working on every
OS via the AIO patchset, in later work, hopefully not very far in the
future (in the most portable mode, you get I/O worker processes doing
pread() or preadv() calls on behalf of recovery). So I'll be glad to
get this infrastructure in, even though it's maybe only useful for
some people in the first release.

> FWIW I'm not sure there was a discussion or argument about what should
> be the default setting (enabled or disabled). I'm fine with not enabling
> this by default, so that people have to enable it explicitly.
>
> In a way that'd be consistent with effective_io_concurrency being 1 by
> default, which almost disables regular prefetching.

Yeah, I'm not sure but I'd be fine with disabling it by default in the
initial release. The current patch set has it enabled, but that's
mostly for testing, it's not an opinion on how it should ship.

I've attached a rebased patch set with a couple of small changes:

1. I abandoned the patch that proposed
pg_atomic_unlocked_add_fetch_u{32,64}() and went for a simple function
local to xlogprefetch.c that just does pg_atomic_write_u64(counter,
pg_atomic_read_u64(counter) + 1), in response to complaints from
Andres[1].

2. I fixed a bug in ReadRecentBuffer(), and moved it into its own
patch for separate review.

I'm now looking at Horiguchi-san and Heikki's patch[2] to remove
XLogReader's callbacks, to try to understand how these two patch sets
are related. I don't really like the way those callbacks work, and
I'm afraid had to make them more complicated. But I don't yet know
very much about that other patch set. More soon.

[1] https://www.postgresql.org/message-id/20201230035736.qmyrtrpeewqbidfi%40alap3.anarazel.de
[2] https://www.postgresql.org/message-id/flat/20190418(dot)210257(dot)43726183(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp