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_name PGC_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 fields from 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