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