Date: Fri, 13 Mar 2020 18:15:09 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: WAL prefetch (another approach)
Message-ID: <20200313211509.GA14359@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+hUKG+5-C+EEaFFEYqqDrHjesdkX537H2_j9qL1i8Dq7yjoSA@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

I tried my luck at a quick read of this patchset.
I didn't manage to go over 0005 though, but I agree with Tomas that
having this be configurable in terms of bytes of WAL is not very
user-friendly.

First of all, let me join the crowd chanting that this is badly needed;
I don't need to repeat what Chittenden's talk showed.  "WAL recovery is
now 10x-20x times faster" would be a good item for pg13 press release, 
I think.

> From a61b4e00c42ace5db1608e02165f89094bf86391 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas.munro@gmail.com>
> Date: Tue, 3 Dec 2019 17:13:40 +1300
> Subject: [PATCH 1/5] Allow PrefetchBuffer() to be called with a SMgrRelation.
> 
> Previously a Relation was required, but it's annoying to have
> to create a "fake" one in recovery.

LGTM.

It's a pity to have to include smgr.h in bufmgr.h.  Maybe it'd be sane
to use a forward struct declaration and "struct SMgrRelation *" instead.


> From acbff1444d0acce71b0218ce083df03992af1581 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <tmunro@postgresql.org>
> Date: Mon, 9 Dec 2019 17:10:17 +1300
> Subject: [PATCH 2/5] Rename GetWalRcvWriteRecPtr() to GetWalRcvFlushRecPtr().
> 
> The new name better reflects the fact that the value it returns
> is updated only when received data has been flushed to disk.
> 
> An upcoming patch will make use of the latest data that was
> written without waiting for it to be flushed, so use more
> precise function names.

Ugh.  (Not for your patch -- I mean for the existing naming convention).
It would make sense to rename WalRcvData->receivedUpto in this commit,
maybe to flushedUpto.


> From d7fa7d82c5f68d0cccf441ce9e8dfa40f64d3e0d Mon Sep 17 00:00:00 2001
> From: Thomas Munro <tmunro@postgresql.org>
> Date: Mon, 9 Dec 2019 17:22:07 +1300
> Subject: [PATCH 3/5] Add WalRcvGetWriteRecPtr() (new definition).
> 
> A later patch will read received WAL to prefetch referenced blocks,
> without waiting for the data to be flushed to disk.  To do that,
> it needs to be able to see the write pointer advancing in shared
> memory.
> 
> The function formerly bearing name was recently renamed to
> WalRcvGetFlushRecPtr(), which better described what it does.

> +	pg_atomic_init_u64(&WalRcv->writtenUpto, 0);

Umm, how come you're using WalRcv here instead of walrcv?  I would flag
this patch for sneaky nastiness if this weren't mostly harmless.  (I
think we should do away with local walrcv pointers altogether.  But that
should be a separate patch, I think.)

> +	pg_atomic_uint64 writtenUpto;

Are we already using uint64s for XLogRecPtrs anywhere?  This seems
novel.  Given this, I wonder if the comment near "mutex" needs an
update ("except where atomics are used"), or perhaps just move the
member to after the line with mutex.


I didn't understand the purpose of inc_counter() as written.  Why not
just pg_atomic_fetch_add_u64(..., 1)?

>  /*
>   *	smgrprefetch() -- Initiate asynchronous read of the specified block of a relation.
> + *
> + *		In recovery only, this can return false to indicate that a file
> + *		doesn't	exist (presumably it has been dropped by a later WAL
> + *		record).
>   */
> -void
> +bool
>  smgrprefetch(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum)

I think this API, where the behavior of a low-level module changes
depending on InRecovery, is confusingly crazy.  I'd rather have the
callers specifying whether they're OK with a file that doesn't exist.

> +extern PrefetchBufferResult SharedPrefetchBuffer(SMgrRelation smgr_reln,
> +												 ForkNumber forkNum,
> +												 BlockNumber blockNum);
>  extern void PrefetchBuffer(Relation reln, ForkNumber forkNum,
>  						   BlockNumber blockNum);

Umm, I would keep the return values of both these functions in sync.
It's really strange that PrefetchBuffer does not return
PrefetchBufferResult, don't you think?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services