From: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
---|---|
To: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Cc: | Stephen Frost <sfrost(at)snowman(dot)net>, Andres Freund <andres(at)anarazel(dot)de>, Jakub Wartak <Jakub(dot)Wartak(at)tomtom(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, David Steele <david(at)pgmasters(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: WIP: WAL prefetch (another approach) |
Date: | 2021-03-19 01:29:04 |
Message-ID: | 8dd2dcc2-0b6b-a4ab-c1b3-032c0a5ab1c3@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 3/18/21 1:54 AM, Thomas Munro wrote:
> 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.
>
+1 to that
>> 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.
>
+1 to that too. Better to have it disabled by default than not at all.
> 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.
>
OK. Do you think we should get both of those patches in, or do we need
to commit them in a particular order? Or what is your concern?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2021-03-19 01:37:11 | Re: fdatasync performance problem with large number of DB files |
Previous Message | Tomas Vondra | 2021-03-19 01:21:29 | Re: cleanup temporary files after crash |