From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Andres Freund <andres@anarazel.de>
Cc: Jakub Wartak <Jakub.Wartak@tomtom.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: David Steele <david@pgmasters.net>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: WAL prefetch (another approach)
Date: Fri, 19 Mar 2021 02:29:04 +0100
Message-ID: <8dd2dcc2-0b6b-a4ab-c1b3-032c0a5ab1c3@enterprisedb.com> (raw)
In-Reply-To: <CA+hUKGLHTjbBPFX6E1ZCGVyQD30-vWtH=N6EcHEzW13q+jh7FQ@mail.gmail.com>
References: <CA+hUKGJ1=pOiNjSgXYJnjE3OyRtp8tjMRcON256e5EFpzPpgtA@mail.gmail.com>
<VI1PR0701MB69608CBCE44D80857E59572EF6CA0@VI1PR0701MB6960.eurprd07.prod.outlook.com>
<CA+hUKGKFeYPL9K+SRixcsx1+6HsHhqK+POZyrnnZjw1jERpGcQ@mail.gmail.com>
<20210210215033.GA27507@tamriel.snowman.net>
<103ec76d-8bbc-09e1-460c-8785751431b5@enterprisedb.com>
<20210212044614.5wqjkjbcmbwzever@alap3.anarazel.de>
<20210213213930.GM27507@tamriel.snowman.net>
<eed83eaa-91e1-91fe-32d0-ece36c0fa810@enterprisedb.com>
<20210214231815.GO27507@tamriel.snowman.net>
<17b3f450-048c-f3a6-7086-259523f33cbc@enterprisedb.com>
<20210317214331.GF20766@tamriel.snowman.net>
<28c3352e-5388-6410-aae7-05907079e6c9@enterprisedb.com>
<CA+hUKGLHTjbBPFX6E1ZCGVyQD30-vWtH=N6EcHEzW13q+jh7FQ@mail.gmail.com>
On 3/18/21 1:54 AM, Thomas Munro wrote:
> On Thu, Mar 18, 2021 at 12:00 PM Tomas Vondra
> <tomas.vondra@enterprisedb.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
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: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, thomas.munro@gmail.com, sfrost@snowman.net, andres@anarazel.de, Jakub.Wartak@tomtom.com, alvherre@2ndquadrant.com, tomas.vondra@2ndquadrant.com, 9erthalion6@gmail.com, david@pgmasters.net
Subject: Re: WIP: WAL prefetch (another approach)
In-Reply-To: <8dd2dcc2-0b6b-a4ab-c1b3-032c0a5ab1c3@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox