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