From: Sergei Kornilov <sk@zsrv.org>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: "masao.fujii@oss.nttdata.com" <masao.fujii@oss.nttdata.com>,
	"a.lubennikova@postgrespro.ru" <a.lubennikova@postgrespro.ru>,
	"robertmhaas@gmail.com" <robertmhaas@gmail.com>,
	"michael@paquier.xyz" <michael@paquier.xyz>,
	"andres@anarazel.de" <andres@anarazel.de>,
	"peter.eisentraut@2ndquadrant.com" <peter.eisentraut@2ndquadrant.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
In-Reply-To: <20201110.135420.826057682358861616.horikyota.ntt@gmail.com>
References: <56311604665173@mail.yandex.ru>
	<a136a397-1401-9531-20c1-1b83575fd68d@oss.nttdata.com>
	<108021604695421@mail.yandex.ru>
 <20201110.135420.826057682358861616.horikyota.ntt@gmail.com>
Subject: Re: Allow some recovery parameters to be changed with reload
MIME-Version: 1.0
Date: Tue, 10 Nov 2020 14:13:17 +0300
Message-Id: <4363101605003835@mail.yandex.ru>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=utf-8
Precedence: bulk

Hello

> If someone changes restore_command to '' then reload while crash
> recovery is running, the server stops for no valid reason.

While *crash* recovery is running? It's possible only during Point-in-Time Recovery, no?
At the beginning of validateRecoveryParameters we check ArchiveRecoveryRequested, which can be set in two cases:

* if recovery.signal found - same check on recovery start. Otherwise it is possible to early end recovery because of empty restore_command. So we want to protect the user from such misconfiguration? I am fine if we decide that no additional handling is needed.
* if standby.signal found - this FATAL is not reachable because StandbyModeRequested is also set.

During crash recovery validateRecoveryParameters does nothing.

> If restore_command is set to 'hoge' (literally:p, that is, anything
> unexecutable) and send SIGHUP while archive recovery is running, the
> server stops. I think we need to handle these cases more gracefully,

I think we can not perform such check reliable. As in my example earlier:

> restore_command = '. /etc/wal-g/WALG_AWS_ENV; wal-g wal-fetch "%f" "%p"'

How do we find the commands first? For any shell? And even: we learned that the binary is unexecutable. But what to do next?

> If someone changes restore_command by mistake to something executable
> but fails to offer the specfied file even if it exists, the running
> archive recovery finishes then switches timeline unexpectedly.

Or executable file was just removed. Which is clearly a pilot error. Is this differs from changing restore_command?

>>  I do not know the history of this fatal ereport. It looks like "must specify restore_command when standby mode is not enabled" check is only intended to protect the user from misconfiguration and the rest code will treat empty restore_command correctly, just like /bin/false. Did not notice anything around StandbyMode conditions.
>
> If restore_command is not changable after server-start, it would be
> valid for startup to stop for inexecutable content for the variable
> since there's no way to proceed recovery.

Why not use local pg_wal? There may be already enough WAL.

regards, Sergei