Subject: Re: WIP: WAL prefetch (another approach)
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>, Andres Freund <andres@anarazel.de>,
 Jakub Wartak <Jakub.Wartak@tomtom.com>,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Dmitry Dolgov <9erthalion6@gmail.com>, David Steele <david@pgmasters.net>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
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>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <8dd2dcc2-0b6b-a4ab-c1b3-032c0a5ab1c3@enterprisedb.com>
Date: Fri, 19 Mar 2021 02:29:04 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: 
 <CA+hUKGLHTjbBPFX6E1ZCGVyQD30-vWtH=N6EcHEzW13q+jh7FQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/8dd2dcc2-0b6b-a4ab-c1b3-032c0a5ab1c3%40enterprisedb.com>
Precedence: bulk

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