Date: Tue, 18 Feb 2020 14:20:16 +0900 (JST)
Message-Id: <20200218.142016.2107516686265665330.horikyota.ntt@gmail.com>
To: masao.fujii@oss.nttdata.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: RecoveryWalAll and RecoveryWalStream wait events
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <124997ee-096a-5d09-d8da-2c7a57d0816e@oss.nttdata.com>
References: <124997ee-096a-5d09-d8da-2c7a57d0816e@oss.nttdata.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello.

At Tue, 18 Feb 2020 12:25:51 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> Hi,
> 
> RecoveryWalAll and RecoveryWalStream wait events are documented as
> follows.
> 
>     RecoveryWalAll
>     Waiting for WAL from any kind of source (local, archive or stream) at
>     recovery.
> 
>     RecoveryWalStream
>     Waiting for WAL from a stream at recovery.
> 
> But as far as I read the code, RecoveryWalAll is reported only when
> waiting
> for WAL from a stream. So the current description looks
> incorrect. What's
> described now for RecoveryWalStream seems rather fit to
> RecoveryWalAll.
> I'd like to change the description of RecoveryWalAll to "Waiting for
> WAL
>  from a stream at recovery".

Good catch!

> Regarding RecoveryWalStream, as far as I read the code, while this
> event is
> being reported, the startup process is waiting for next trial to
> retrieve
> WAL data when WAL data is not available from any sources, based on
> wal_retrieve_retry_interval. So this current description looks also
> incorrect. I'd like to change it to "Waiting when WAL data is not
> available
>  from any kind of sources (local, archive or stream) before trying
>  again
>  to retrieve WAL data".
> 
> Thought?

I agree that the corrected description sound correct in meaning. The
latter seems a bit lengthy, though.

> Also the current names of these wait events sound confusing. I think
> that RecoveryWalAll should be changed to RecoveryWalStream.
> RecoveryWalStream should be RecoveryRetrieveRetryInterval or
> something.

I agree to the former, I think RecoveryWalInterval works well enough.

> Another problem is that the current wait event types of them also look
> strange. Currently the type of them is Activity, but IMO it's better
> to
> use IPC for RecoveryWalAll because it's waiting for walreceiver to
> receive new WAL. Also it's better to use Timeout for RecoveryWalStream
> because it's waiting depending on wal_retrieve_retry_interval.

Do you mean condition variable by the "IPC"? But the WaitLatch waits
not only for new WAL but also for trigger, SIGHUP, shutdown and
walreceiver events other than new WAL. I'm not sure that condition
variable fits for the purpose.

> The changes of wait event types and names would break the
> compatibility
> of wait events in pg_stat_activity. So this change should not be
> applied
> to the back branches, but it's ok to apply in the master. Right?

FWIW, It seems right.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center