Re: WIP: WAL prefetch (another approach)

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(dot)munro(at)gmail(dot)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(at)postgresql(dot)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(at)postgresql(dot)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