Re: allow online change primary_conninfo

Hi,

On 2018-11-26 00:25:43 +0300, Sergei Kornilov wrote:
> Now we integrate recovery.conf into GUC system. So i would like to start discussion to make primary_conninfo and
primary_slot_namePGC_SIGHUP.
 
> My work-in-progress patch attached.

Cool!


> I think we have no futher reason to have a copy of conninfo and slot name in WalRcvData, so i propose remove these
fieldsfrom pg_stat_get_wal_receiver() (and pg_stat_wal_receiver view). This data can be accesible now via regular GUC
commands.

Is that wise? It seems possible that wal receivers run for a while with
the old connection information, and without this information that seems
hard to debug.


> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index db1a2d4..faa8e17 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -3797,7 +3797,6 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
>            <varname>primary_conninfo</varname> string.
>           </para>
>           <para>
> -          This parameter can only be set at server start.
>            This setting has no effect if the server is not in standby mode.
>           </para>
>          </listitem>

Should probably note something like

"This parameter can only be set in the <filename>postgresql.conf</filename>
file or on the server command line."


> @@ -435,9 +420,33 @@ WalReceiverMain(void)
>  
>                  if (got_SIGHUP)
>                  {
> +                    char    *conninfo = pstrdup(PrimaryConnInfo);
> +                    char    *slotname = pstrdup(PrimarySlotName);
> +
>                      got_SIGHUP = false;
>                      ProcessConfigFile(PGC_SIGHUP);
>                      XLogWalRcvSendHSFeedback(true);
> +
> +                    /*
> +                     * If primary_conninfo has been changed while walreceiver is running,
> +                     * shut down walreceiver so that a new walreceiver is started and
> +                     * initiates replication with the new connection information.
> +                     */
> +                    if (strcmp(conninfo, PrimaryConnInfo) != 0)
> +                        ereport(FATAL,
> +                                (errcode(ERRCODE_ADMIN_SHUTDOWN),
> +                                 errmsg("closing replication connection because primary_conninfo was changed")));
> +
> +                    /*
> +                     * And the same for primary_slot_name.
> +                     */
> +                    if (strcmp(slotname, PrimarySlotName) != 0)
> +                        ereport(FATAL,
> +                                (errcode(ERRCODE_ADMIN_SHUTDOWN),
> +                                 errmsg("closing replication connection because primary_slot_name was changed")));
> +
> +                    pfree(conninfo);
> +                    pfree(slotname);

I'd strongly advocate moving this to a separate function.


Greetings,

Andres Freund