Horton
Menu

Re: WIP: WAL prefetch (another approach)

Tomas Vondra <tomas.vondra@enterprisedb.com>

View thread
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
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>
Date: 2021-03-19T01:29:04Z
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.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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix recovery_prefetch docs.

  2. Prefetch data referenced by the WAL, take II.

  3. Add circular WAL decoding buffer, take II.

  4. Fix generation of ./INSTALL for the distribution tarball

  5. Revert recovery prefetching feature.

  6. Sync guc.c and postgresql.conf.sample with the SGML docs.

  7. Add information of total data processed to replication slot stats.

  8. Doc: Review for "Optionally prefetch referenced data in recovery."

  9. Add circular WAL decoding buffer.

  10. Optionally prefetch referenced data in recovery.

  11. Remove read_page callback from XLogReader.

  12. Provide ReadRecentBuffer() to re-pin buffers by ID.

  13. Provide recovery_init_sync_method=syncfs.

  14. Mark factorial operator, and postfix operators in general, as deprecated.

  15. Rationalize GetWalRcv{Write,Flush}RecPtr().

  16. Support PrefetchBuffer() in recovery.

  17. Prevent hard failures of standbys caused by recycled WAL segments