Re: allow online change primary_conninfo

Hi,

On 2019-02-16 14:50:35 +0300, Sergei Kornilov wrote:
> > I don't quite think this is the right design. IMO the startup process
> > should signal the walreceiver to shut down, and the wal receiver should
> > never look at the config.
>
> Ok, i can rewrite such way. Please check attached version.
>
> > Otherwise there's chances for knowledge of
> > pg.conf to differ between the processes.
>
> I still not understood why this:
> - is not issue for startup process with all primary_conninfo logic
> - but issue for walreceiver with Michael Paquier patch; therefore without any primary_conninfo change logic in startup.
> In both cases primary_conninfo change logic is only in one process.
>
> regards, Sergei

For one, it's not ok to just let the startup process think that the
walreceiver failed - that'll make it change source of WAL to the next
available method. Something we definitely do not want, as
restore_command is very commonly slower.

But just as importantly, I think, is that we ought to automate
cluster-wide tasks like failing over more inside postgres. And that's
much harder if different parts of PG, say the startup process and
walreceiver, have different assumptions about the current configuration.

> +void
> +ProcessStartupSigHup(void)
> +{
> + char *conninfo = pstrdup(PrimaryConnInfo);
> + char *slotname = pstrdup(PrimarySlotName);
> +
> + ProcessConfigFile(PGC_SIGHUP);
> +
> + /*
> + * we need shutdown running walreceiver if replication settings was
> + * changed. walreceiver will be started with new settings in next
> + * WaitForWALToBecomeAvailable iteration
> + */
> + if ((strcmp(conninfo, PrimaryConnInfo) != 0 ||
> + strcmp(slotname, PrimarySlotName) != 0) &&
> + WalRcvRunning())
> + {
> + ereport(LOG,
> + (errmsg("terminating walreceiver process due to change of %s",
> + strcmp(conninfo, PrimaryConnInfo) != 0 ? "primary_conninfo" : "primary_slot_name"),
> + errdetail("In a moment starts streaming WAL with new configuration.")));
> +
> + ShutdownWalRcv();
> + lastSourceFailed = true;
> + }
> +
> + pfree(conninfo);
> + pfree(slotname);
> +}

That'd still switch to a different method, something we do not want...

Greetings,

Andres Freund