Re: allow online change primary_conninfo

Hi,

On 2019-09-19 17:46:06 +0300, Sergei Kornilov wrote:

> <para>
> - This parameter can only be set at server start.
> + This parameter can only be set in the <filename>postgresql.conf</filename>
> + file or on the server command line.
> This setting has no effect if the server is not in standby mode.
> </para>
> + <para>
> + If <varname>primary_conninfo</varname> is changed while WAL receiver is running,
> + the WAL receiver shuts down and then restarts with new setting,
> + except when primary_conninfo is an empty string.
> + </para>

From the sentence structure it's not clear that "except when
primary_conninfo is an empty string" only applies to "and then restarts
with new setting".

> +/*
> + * Need for restart running WalReceiver due the configuration change.
> + * Suitable only for XLOG_FROM_STREAM source
> + */
> +static bool pendingWalRcvRestart = false;

s/due the/due to a/ (or even "due to the").

> @@ -11862,6 +11869,17 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
> if (WalRcvStreaming())
> ShutdownWalRcv();
>
> + /*
> + * If wal receiver is requested to restart, we skip the
> + * next XLOG_FROM_ARCHIVE to immediately starting it.
> + */
> + if (pendingWalRcvRestart)
> + {
> + lastSourceFailed = true;
> + currentSource = XLOG_FROM_ARCHIVE;
> + continue;
> + }

I can't parse that comment. What does "skipping to starting" mean? I
assume it's just about avoiding wal_retrieve_retry_interval, but I think
the comment ought to be rephrased.

Also, should we really check this before scanning for new timelines?

Why is it the right thing to change to XLOG_FROM_ARCHIVE when we're just
restarting walreceiver? The server might unnecessarily get stuck in
archive based recovery for a long time this way? It seems to me that
we'd need to actually trigger RequestXLogStreaming() in this case.

Greetings,

Andres Freund