Horton

Thread

Commits

  1. Allow walreceiver configuration to change on reload

  1. allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-11-25T21:25:43Z

    Hello all
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.

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.

Thank you for advance!

regards, Sergei

  2. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2018-11-25T21:43:13Z

    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

  3. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2018-11-26T00:13:29Z

    On Sun, Nov 25, 2018 at 01:43:13PM -0800, Andres Freund wrote:
> On 2018-11-26 00:25:43 +0300, Sergei Kornilov wrote:
>> 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.

No, that's unwise.  One of the reasons why conninfo is around is that we
know to which server a receiver is connected when specifying multiple
host and/or port targets in the configuration.  Please see 9a895462 for
the details.

Removing the slot does not look like an improvement to me either,
following Andres' argument.
--
Michael

  4. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-11-26T09:30:06Z

    Hi

>>  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.
Hmm... I considered SIGHUP processing was in fast loop and therefore shutdown should be fast. But i recheck code and found a possible long loop without processing SIGHUP (in case we receive new data faster than writes to disk). Ok, i will revert back.
How about write to WalRcvData only clobbered conninfo?

> Should probably note something like
>
> "This parameter can only be set in the <filename>postgresql.conf</filename>
> file or on the server command line."
Seems other PGC_SIGHUP settings have such note, fixed, thank you.

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

regards, Sergei

  5. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2018-11-26T17:46:36Z

    Hi,

On 2018-11-26 12:30:06 +0300, Sergei Kornilov wrote:
> Hmm... I considered SIGHUP processing was in fast loop and therefore shutdown should be fast. But i recheck code and found a possible long loop without processing SIGHUP (in case we receive new data faster than writes to disk). Ok, i will revert back.
> How about write to WalRcvData only clobbered conninfo?

I'm not sure I understand what you mean?  The way I'd solve this is that
that only walreceiver, at startup, writes out its conninfo/slot_name,
sourcing the values from the GUCs. That way there's no issue with values
being overwritten early.

- Andres

  6. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-11-26T18:21:41Z

    Hi

>>  Hmm... I considered SIGHUP processing was in fast loop and therefore shutdown should be fast. But i recheck code and found a possible long loop without processing SIGHUP (in case we receive new data faster than writes to disk). Ok, i will revert back.
>>  How about write to WalRcvData only clobbered conninfo?
>
> I'm not sure I understand what you mean?
I am about my initial proposal with remove conninfo wrom WalRcvData - walreceiver may run some time with old conninfo and 
> without this information that seems hard to debug.
Earlier i thought walreceiver will shutdown fast on SIGHUP.

> The way I'd solve this is that
> that only walreceiver, at startup, writes out its conninfo/slot_name,
> sourcing the values from the GUCs. That way there's no issue with values
> being overwritten early.
In second patch i follow exactly this logic.

regards, Sergei

  7. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2018-12-11T05:22:49Z

    On Mon, Nov 26, 2018 at 09:46:36AM -0800, Andres Freund wrote:
> I'm not sure I understand what you mean?  The way I'd solve this is that
> that only walreceiver, at startup, writes out its conninfo/slot_name,
> sourcing the values from the GUCs. That way there's no issue with values
> being overwritten early.

WalRcv->conninfo, as stored in the WAL receiver, clobbers some of its
fields and includes a set of default parameters with its values after
establishing the connection so as no sensible data shows up in
pg_stat_wal_receiver so you cannot simply compare what is stored in the
WAL receiver with the GUCs to do the decision-making.  For the password,
you'd want to do again a connection anyway even if the cloberred string
is the same.

What's proposed in the patch to issue an ERROR if the parameter has
changed does not look that bad to me as it depends on the process
context, but I think that this patch should document clearly that if
primary_conninfo or primary_slot_name are changed then the WAL receiver
is stopped immediately.

    /*
-    * replication slot name; is also used for walreceiver to connect with the
-    * primary
+    * replication slot name used by runned walreceiver
     */
Not sure that there is much point in updating those comments, because
it still has the same meaning in the new context.

-RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr, const char *conninfo,
-                    const char *slotname)
+RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr)
That's perhaps a matter of taste, but keeping the current API as-is
looks cleaner to me, particularly because those fields get copied into
WalRcv, so I'd rather not change what is done in
WaitForWALToBecomeAvailable and keep the patch at its simplest shape.

+$node_standby_2->reload; # should have effect without restart
This does not wait for the change to be effective, so I think that you
introduce a race condition for slow machines with this test, making it
unstable.
--
Michael

  8. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-12-11T08:59:08Z

    Hello

thank you for review!

> but I think that this patch should document clearly that if
> primary_conninfo or primary_slot_name are changed then the WAL receiver
> is stopped immediately.
Good idea, will change.

>     /*
> - * replication slot name; is also used for walreceiver to connect with the
> - * primary
> + * replication slot name used by runned walreceiver
>      */
> Not sure that there is much point in updating those comments, because
> it still has the same meaning in the new context.
"is also used" seems outdated for me. With proposed patch this field means only currently active slot, not "also".
Well, depends on RequestXLogStreaming discussion

> -RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr, const char *conninfo,
> - const char *slotname)
> +RequestXLogStreaming(TimeLineID tli, XLogRecPtr recptr)
> That's perhaps a matter of taste, but keeping the current API as-is
> looks cleaner to me, particularly because those fields get copied into
> WalRcv, so I'd rather not change what is done in
> WaitForWALToBecomeAvailable and keep the patch at its simplest shape.
Unfortunally here is race condition if i leave RequestXLogStreaming infrastructure as-is and walreceiver will use this struct for startup.
This case:
- startup process calls RequestXLogStreaming and fill WalRcv conninfo/slotname
- reload primary_conninnfo by SIGHUP
- walreceiver start with new GUC but trying conninfo/slotname from WalRcv struct. If walreceiver is able to connect - it will not restart till another error or primary_conninfo/slotname will be changed again (SIGHUP without change is not enough).

I already had such failures while testing this patch.

If walreceiver will use primary_conninfo GUC at startup - i see no reason to leave *conninfo in RequestXLogStreaming. These parameters will be misleading, we request them, but not using for anything.

> +$node_standby_2->reload; # should have effect without restart
> This does not wait for the change to be effective, so I think that you
> introduce a race condition for slow machines with this test, making it
> unstable.
No, here is no race condition because just after this line we wait this replication slot in upstream pg_catalog.pg_replication_slots (get_slot_xmins routine).
Before reload we have no replication slot and should reconnect here with replication slot. I can add some comment here.

regards, Sergei

  9. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-12-11T10:13:44Z

    Hello

After some thinking i can rewrite this patch in another way.

This is better or worse?

regards, Sergei

  10. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-12-11T10:16:23Z

    oops, forgot attach patch

11.12.2018, 13:14, "Sergei Kornilov" <sk@zsrv.org>:
> Hello
>
> After some thinking i can rewrite this patch in another way.
>
> This is better or worse?
>
> regards, Sergei

  11. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2018-12-12T04:32:08Z

    On Tue, Dec 11, 2018 at 11:59:08AM +0300, Sergei Kornilov wrote:
>> but I think that this patch should document clearly that if
>> primary_conninfo or primary_slot_name are changed then the WAL receiver
>> is stopped immediately.
>
> Good idea, will change.

+         <para>
+          <application>walreceiver</application> will be restarted after
+          <varname>primary_conninfo</varname> was changed.
+         </para>
Hm.  It may be cleaner to use "WAL receiver" here for clarity.  Perhaps
that's a matter of state but that seems cleaner when referring to the
actual process.  The docs share both grammar, depending on the context.

> If walreceiver will use primary_conninfo GUC at startup - i see no
> reason to leave *conninfo in RequestXLogStreaming. These parameters
> will be misleading, we request them, but not using for anything.

Oh, indeed.  My apologies for being confused here, I can see now your
point.  It seems to me that this is an extra cleanup caused by the
recovery parameters switched to be GUCs, and not something that we
should be changed as an effect to SIGHUP for primary_conninfo and
primary_slot_name.  So let's do this cleanup first.  For this purpose, I
am going to post a new thread with a proper patch, with in CC the folks
who moved the recovery parameters to be GUCs.

>> +$node_standby_2->reload; # should have effect without restart
>> This does not wait for the change to be effective, so I think that you
>> introduce a race condition for slow machines with this test, making it
>> unstable.
>
> No, here is no race condition because just after this line we wait
> this replication slot in upstream pg_catalog.pg_replication_slots
> (get_slot_xmins routine). 
> Before reload we have no replication slot and should reconnect here
> with replication slot. I can add some comment here.

Good point, I have forgotten the call to get_slot_xmins() below.
--
Michael

  12. Re: allow online change primary_conninfo

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-12-13T09:42:33Z

    At Tue, 11 Dec 2018 13:16:23 +0300, Sergei Kornilov <sk@zsrv.org> wrote in <9653601544523383@iva8-37fc2ad204cd.qloud-c.yandex.net>
sk> oops, forgot attach patch
sk> 
sk> 11.12.2018, 13:14, "Sergei Kornilov" <sk@zsrv.org>:
sk> > Hello
sk> >
sk> > After some thinking i can rewrite this patch in another way.
sk> >
sk> > This is better or worse?

As the whole the new version looks better for me.

===
Do we no longer need static version of conninfo/slotname in
walreceiver.c? We can detect a change of the variables without
them (as you did in the previous version.).


===
I don't think it is acceptable that raw conninfo is exposed into
log file.

>  LOG:  parameter "primary_conninfo" changed to "host=/tmp port=5432 password=hoge"


===
> errmsg("closing replication connection because primary_conninfo was changed")));

Maybe it is better being as follows according to similar messages:

> errmsg("terminating walreceiver process due to change of primary_conninfo")));

And it *might* be good to have detail.

> errdetail("In a moment starts streaming WAL with new configuration.");


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-12-13T10:51:48Z

    Hello

> Do we no longer need static version of conninfo/slotname in
> walreceiver.c? We can detect a change of the variables without
> them (as you did in the previous version.).

Depends on this discussion: https://www.postgresql.org/message-id/20181212053042.GK17695@paquier.xyz
If walreceiver will use GUC conninfo for startup - then yes, we can remove such static variables.
If walreceiver will still use conninfo from WalRcv - we have race condition between RequestXLogStreaming from startup, config reload, and walreceiver start. In this case i need save conninfo from WalRcv and compare new GUC value with them.

> I don't think it is acceptable that raw conninfo is exposed into
> log file.
>
>>   LOG: parameter "primary_conninfo" changed to "host=/tmp port=5432 password=hoge"

Hmm. We have infrastructure to hide such values?
I need implement something like flag GUC_HIDE_VALUE_FROM_LOG near GUC_SUPERUSER_ONLY in separate thread? Log message will be changed to "LOG: parameter "primary_conninfo" changed" with this flag.

I also think that this is not a big problem. We may already have secret data in the logs. For example, in query text from application.

>>  errmsg("closing replication connection because primary_conninfo was changed")));
>
> Maybe it is better being as follows according to similar messages:
>
>>  errmsg("terminating walreceiver process due to change of primary_conninfo")));
>
> And it *might* be good to have detail.
>
>>  errdetail("In a moment starts streaming WAL with new configuration.");

Agreed, will change.

regards, Sergei

  14. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2018-12-14T07:55:42Z

    On Thu, Dec 13, 2018 at 01:51:48PM +0300, Sergei Kornilov wrote:
> Depends on this discussion: https://www.postgresql.org/message-id/20181212053042.GK17695@paquier.xyz
> If walreceiver will use GUC conninfo for startup - then yes, we can
> remove such static variables.  If walreceiver will still use conninfo
> from WalRcv - we have race condition between RequestXLogStreaming from
> startup, config reload, and walreceiver start. In this case I need
> save conninfo from WalRcv and compare new GUC value with them.

I would recommend waiting for the conclusion of other thread before
moving on with this one.  There are arguments for letting the startup
process deciding which connection string the WAL receiver should use as
well as letting the WAL receiver use directly the connection string from
the GUC.

> Hmm. We have infrastructure to hide such values?
> I need implement something like flag GUC_HIDE_VALUE_FROM_LOG near
> GUC_SUPERUSER_ONLY in separate thread? Log message will be changed to
> "LOG: parameter "primary_conninfo" changed" with this flag.

Good point.  Passwords in logs can be considered as a security issue.
Having such an option may be interesting for other things, though I am
not sure if just having an option to hide logs related to a given
parameter are the correct way to shape it.  We could also consider using
the show hook function of a parameter to print its value in such logs.
--
Michael

  15. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2018-12-14T09:00:39Z

    Hi

> I would recommend waiting for the conclusion of other thread before
> moving on with this one.
Agreed.
I mark this patch as Waiting on Author. Not quite correct for waiting another discussion, but most suitable.

> We could also consider using
> the show hook function of a parameter to print its value in such logs.
But show hook also affects "show primary_conninfo;" command and pg_settings.value
I already marked primary_conninfo as GUC_SUPERUSER_ONLY. I doubt if we need hide some connection info even from superuser.
Also this hook would be a bit complicated, i found suitable code only in libpqrcv_get_conninfo after establishing connect

regards, Sergei

  16. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-01-31T12:58:37Z

    Hi,

On 2018-12-14 16:55:42 +0900, Michael Paquier wrote:
> On Thu, Dec 13, 2018 at 01:51:48PM +0300, Sergei Kornilov wrote:
> > Depends on this discussion: https://www.postgresql.org/message-id/20181212053042.GK17695@paquier.xyz
> > If walreceiver will use GUC conninfo for startup - then yes, we can
> > remove such static variables.  If walreceiver will still use conninfo
> > from WalRcv - we have race condition between RequestXLogStreaming from
> > startup, config reload, and walreceiver start. In this case I need
> > save conninfo from WalRcv and compare new GUC value with them.
> 
> I would recommend waiting for the conclusion of other thread before
> moving on with this one.  There are arguments for letting the startup
> process deciding which connection string the WAL receiver should use as
> well as letting the WAL receiver use directly the connection string from
> the GUC.

I suggest continuing the development of the patch without relying on
that refactoring.

Greetings,

Andres Freund

  17. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-01-31T13:13:22Z

    Hello

Yeah, we have no consensus.

Another open question is about logging new primary_conninfo:

> LOG: parameter "primary_conninfo" changed to "host=/tmp port=5432 password=hoge"

I my opinion this is not issue, database logs can have sensitive data. User queries, for example.
If we not want expose such info - it is ok just hide new value from logs with new GUC flag? Or i need implement masked conninfo for this purpose?

regards, Sergei

  18. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-02-01T02:00:52Z

    On Thu, Jan 31, 2019 at 04:13:22PM +0300, Sergei Kornilov wrote:
> I my opinion this is not issue, database logs can have sensitive
> data. User queries, for example.  If we not want expose such info -
> it is ok just hide new value from logs with new GUC flag? Or i need
> implement masked conninfo for this purpose?

You have problems with things in this area for any commands logged and
able to show a connection string or a password, which can go down as
well to CREATE/ALTER ROLE or FDWs.  So for the purpose of what's
discussed on this thread it does not sound like a requirement to be
able to hide that.  Role DDLs can take an already-hashed input to
avoid that, still knowing the MD5 hash is sufficient for connection
(not for SCRAM!).  Now for FDWs..
--
Michael

  19. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-02-03T10:52:41Z

    Hi,

On 2019-01-31 16:13:22 +0300, Sergei Kornilov wrote:
> Hello
> 
> Yeah, we have no consensus.
> 

Are you planning to update the patch?  Given there's not been much
progress here, I think we ough tot mark the CF entry as returned with
feedback for now.


> Another open question is about logging new primary_conninfo:

> > LOG: parameter "primary_conninfo" changed to "host=/tmp port=5432 password=hoge"
> 
> I my opinion this is not issue, database logs can have sensitive data. User queries, for example.
> If we not want expose such info - it is ok just hide new value from logs with new GUC flag? Or i need implement masked conninfo for this purpose?

I agree that this doesn't need to be solved as part of this patch. Given
the config is in the conf file, I don't think it's meaningful to hide
this from the log. If necessary one can use client certs, service files,
etc.

Greetings,

Andres Freund

  20. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-02-03T12:33:38Z

    Hi

> I agree that this doesn't need to be solved as part of this patch.

Thank you!

> Are you planning to update the patch?

Sorry, i was busy last month. But, well, i already did such update: https://www.postgresql.org/message-id/9653601544523383%40iva8-37fc2ad204cd.qloud-c.yandex.net
v003 version does not change RequestXLogStreaming, not require "Making WAL receiver startup rely on GUC context" patch and does not hide new value from logs.

Here is updated version (from v003 patch) with few wording changes suggested by Kyotaro HORIGUCHI and Michael Paquier

regards, Sergei

  21. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-02-16T03:53:16Z

    Hi,

On 2019-02-03 15:33:38 +0300, Sergei Kornilov wrote:
> +/*
> + * Actual processing SIGHUP signal
> + */
> +static void
> +ProcessWalRcvSigHup(void)
> +{
> +	ProcessConfigFile(PGC_SIGHUP);
> +
> +	/*
> +	 * 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(current_conninfo, PrimaryConnInfo) != 0)
> +		ereport(FATAL,
> +				(errcode(ERRCODE_ADMIN_SHUTDOWN),
> +				 errmsg("terminating walreceiver process due to change of primary_conninfo"),
> +				 errdetail("In a moment starts streaming WAL with new configuration.")));
> +
> +	/*
> +	 * And the same for primary_slot_name.
> +	 */
> +	if (strcmp(current_slotname, PrimarySlotName) != 0)
> +		ereport(FATAL,
> +				(errcode(ERRCODE_ADMIN_SHUTDOWN),
> +				 errmsg("terminating walreceiver process due to change of primary_slot_name"),
> +				 errdetail("In a moment starts streaming WAL with new configuration.")));
> +
> +	XLogWalRcvSendHSFeedback(true);
> +}

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. Otherwise there's chances for knowledge of
pg.conf to differ between the processes.

Greetings,

Andres Freund

  22. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-02-16T11:50:35Z

    Hi

> 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

  23. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-02-16T15:10:25Z

    On Sat, Feb 16, 2019 at 02:50:35PM +0300, Sergei Kornilov wrote:
> +	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;
> +	}

If you do that, the startup process would try to jump to a different
source to fetch WAL if archiving is enabled.  Is that really what we
want?  It would be nice to have one message for primary_conninfo being
updated, and one for primary_slot_name so as if both are changed at
the same time the user gets the correct information.

"In a moment starts streaming WAL with new configuration." sounds
strange.  It would be more natural to have something like "The WAL
receiver is going to be restarted with the new configuration", just a
suggestion.
--
Michael

  24. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-02-16T19:18:37Z

    Hi

> If you do that, the startup process would try to jump to a different
> source to fetch WAL if archiving is enabled. Is that really what we
> want?

Yes, similar behavior with exit walreceiver by any reason. Same as in all previous patch versions.
(not sure this can be changed in some small and elegant way)

> It would be nice to have one message for primary_conninfo being
> updated, and one for primary_slot_name so as if both are changed at
> the same time the user gets the correct information.

Hm, maybe even better would be separate message if both settings are changed? Doing this in attached patch.

> "In a moment starts streaming WAL with new configuration." sounds
> strange. It would be more natural to have something like "The WAL
> receiver is going to be restarted with the new configuration", just a
> suggestion.

This phrase was proposed here: https://www.postgresql.org/message-id/20181213.184233.215171075.horiguchi.kyotaro%40lab.ntt.co.jp
My english is poor, so i added message just as proposed. I replaced to your variant.

regards, Sergei

  25. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-02-17T19:27:20Z

    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

  26. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-02-17T21:06:05Z

    Hi

> 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.

Ok. This was not mentioned before Michael response yesterday. restore_command is much faster compared to database restart, also switch to a different method was proposed few years ago by Simon Riggs in original change recovery.conf proposal ( https://www.postgresql.org/message-id/flat/CANP8+jLO5fmfudbB1b1iw3pTdOK1HBM=xMTaRfOa5zpDVcqzew@mail.gmail.com/ ). I assumed we can start with this. Sorry for your wasted time.

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

Ok, do not want means we do not want. Will try change behavior.

regards, Sergei

  27. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-02-18T03:19:54Z

    On Mon, Feb 18, 2019 at 12:06:05AM +0300, Sergei Kornilov wrote:
> Ok. This was not mentioned before Michael response
> yesterday. restore_command is much faster compared to database
> restart [...]

The startup process would not switch back to streaming with archiving
enabled until it has consumed all the segments available.  I recall
David Steele mentioning me that a perl command invocation can take
easily 100ms.  On a system which generates more than 10 segments per
segment, you can guess what happens..  I think that this argument is a
non-starter if we want to make the WAL receiver a maximum available.

>> That'd still switch to a different method, something we do not want...
> 
> Ok, do not want means we do not want. Will try change behavior.

Thanks Sergei for considering it.  The code paths touched are
sensitive, so we had better be careful here.
--
Michael

  28. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-03-02T10:49:51Z

    Hello

This might be not the right way, but I can't think of a better way to not switch to a different method than split of lastSourceFailed processing and starting new source. Something like refactoring in first attached patch. I move RequestXLogStreaming logic from lastSourceFailed processing and add new pendingRestartSource indicator.
Second patch is mostly the same as previous version but uses new pendingRestartSource mechanism instead of lastSourceFailed.

Thanks in advance!

regards, Sergei

  29. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-03-04T03:19:18Z

    On Sat, Mar 02, 2019 at 01:49:51PM +0300, Sergei Kornilov wrote:
> This might be not the right way, but I can't think of a better way
> to not switch to a different method than split of lastSourceFailed
> processing and starting new source. Something like refactoring in
> first attached patch. I move RequestXLogStreaming logic from
> lastSourceFailed processing and add new pendingRestartSource
> indicator.

OK.  This needs a rather close lookup, and I am not actually sure that
you even need pendingRestartSource.  Please let me a couple of days
before coming back to you on 0001.

> Second patch is mostly the same as previous version but uses new
> pendingRestartSource mechanism instead of lastSourceFailed.

This, on the other hand, looks like the implementation we are looking
for based on the discussions which happened until now to have the
startup process handle the shutdown and restart of the WAL receiver.
--
Michael

  30. Re: Re: allow online change primary_conninfo

    David Steele <david@pgmasters.net> — 2019-03-25T08:32:21Z

    Hi Michael,

On 3/4/19 7:19 AM, Michael Paquier wrote:
> On Sat, Mar 02, 2019 at 01:49:51PM +0300, Sergei Kornilov wrote:
>> This might be not the right way, but I can't think of a better way
>> to not switch to a different method than split of lastSourceFailed
>> processing and starting new source. Something like refactoring in
>> first attached patch. I move RequestXLogStreaming logic from
>> lastSourceFailed processing and add new pendingRestartSource
>> indicator.
> 
> OK.  This needs a rather close lookup, and I am not actually sure that
> you even need pendingRestartSource.  Please let me a couple of days
> before coming back to you on 0001.
> 
>> Second patch is mostly the same as previous version but uses new
>> pendingRestartSource mechanism instead of lastSourceFailed.
> 
> This, on the other hand, looks like the implementation we are looking
> for based on the discussions which happened until now to have the
> startup process handle the shutdown and restart of the WAL receiver.

Do you know when you'll have a chance to revisit this?

Regards,
-- 
-David
david@pgmasters.net

  31. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-04-03T20:00:00Z

    Hi,

On 2019-03-25 12:32:21 +0400, David Steele wrote:
> On 3/4/19 7:19 AM, Michael Paquier wrote:
> > On Sat, Mar 02, 2019 at 01:49:51PM +0300, Sergei Kornilov wrote:
> > > This might be not the right way, but I can't think of a better way
> > > to not switch to a different method than split of lastSourceFailed
> > > processing and starting new source. Something like refactoring in
> > > first attached patch. I move RequestXLogStreaming logic from
> > > lastSourceFailed processing and add new pendingRestartSource
> > > indicator.
> > 
> > OK.  This needs a rather close lookup, and I am not actually sure that
> > you even need pendingRestartSource.  Please let me a couple of days
> > before coming back to you on 0001.
> > 
> > > Second patch is mostly the same as previous version but uses new
> > > pendingRestartSource mechanism instead of lastSourceFailed.
> > 
> > This, on the other hand, looks like the implementation we are looking
> > for based on the discussions which happened until now to have the
> > startup process handle the shutdown and restart of the WAL receiver.
> 
> Do you know when you'll have a chance to revisit this?

I think we unfortunately got to mark this as returned with
feedback. I've not done so, but just switched the entry to waiting on
author.

Greetings,

Andres Freund

  32. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-04-03T20:49:54Z

    Hi

> I think we unfortunately got to mark this as returned with
> feedback. I've not done so, but just switched the entry to waiting on
> author.

Why returned with feedback? Why waiting on author? I didn't receive a feedback for latest published patch version. What can I do as author? Patch still applied (thanks cf bot)
Obviously too late for pg12, but why can not be target pg13 and therefore be moved to next CF?

regards, Sergei

  33. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-04-03T20:53:01Z

    Hi,

On 2019-04-03 23:49:54 +0300, Sergei Kornilov wrote:
> > I think we unfortunately got to mark this as returned with
> > feedback. I've not done so, but just switched the entry to waiting on
> > author.
> 
> Why returned with feedback? Why waiting on author? I didn't receive a
> feedback for latest published patch version. What can I do as author?
> Patch still applied (thanks cf bot) Obviously too late for pg12, but
> why can not be target pg13 and therefore be moved to next CF?

Well, my impression was that the patch didn't yet really address the
feedback. And thus should have been marked as waiting on author for a
while.

Greetings,

Andres Freund

  34. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-04-03T21:27:55Z

    Hi 

>>  > I think we unfortunately got to mark this as returned with
>>  > feedback. I've not done so, but just switched the entry to waiting on
>>  > author.
>>
>>  Why returned with feedback? Why waiting on author? I didn't receive a
>>  feedback for latest published patch version. What can I do as author?
>>  Patch still applied (thanks cf bot) Obviously too late for pg12, but
>>  why can not be target pg13 and therefore be moved to next CF?
>
> Well, my impression was that the patch didn't yet really address the
> feedback. And thus should have been marked as waiting on author for a
> while.

Not agree. Latest patch version perform walreceiver restart without switch to a different method as discussed. Here is no race condition between startup process and walreceiver because conninfo passed via WalRcvData struct as currently. I miss something important?
Michael Paquier had no possibility to review latest implementation, but did not say this is totally wrong, just asked wait a rather close lookup.

Of cource we can close this cf entry. I would be happy if someone else post proper implementation. And I can rework my implementation again, but I don’t know how the correct implementation should look or why latest implementation is wrong.

regards, Sergei

  35. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-04-04T02:06:05Z

    On Thu, Apr 04, 2019 at 12:27:55AM +0300, Sergei Kornilov wrote:
> Not agree. Latest patch version perform walreceiver restart without
> switch to a different method as discussed. Here is no race condition
> between startup process and walreceiver because conninfo passed via
> WalRcvData struct as currently. I miss something important? 
> Michael Paquier had no possibility to review latest implementation,
> but did not say this is totally wrong, just asked wait a rather
> close lookup.

I agree with Sergei's statement here.  He has sent a patch for review,
which I have looked up a bit, but not enough to provide a full review
unfortunately, and this even if I was listed as a reviewer of it.  So
if somebody is to blame here, that's me.

> Of cource we can close this cf entry. I would be happy if someone
> else post proper implementation. And I can rework my implementation
> again, but I don’t know how the correct implementation should look
> or why latest implementation is wrong. 

Moving this patch to next CF is fine.
--
Michael

  36. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-04-04T02:13:30Z

    On 2019-04-04 11:06:05 +0900, Michael Paquier wrote:
> On Thu, Apr 04, 2019 at 12:27:55AM +0300, Sergei Kornilov wrote:
> > Not agree. Latest patch version perform walreceiver restart without
> > switch to a different method as discussed. Here is no race condition
> > between startup process and walreceiver because conninfo passed via
> > WalRcvData struct as currently. I miss something important? 
> > Michael Paquier had no possibility to review latest implementation,
> > but did not say this is totally wrong, just asked wait a rather
> > close lookup.
> 
> I agree with Sergei's statement here.  He has sent a patch for review,
> which I have looked up a bit, but not enough to provide a full review
> unfortunately, and this even if I was listed as a reviewer of it.  So
> if somebody is to blame here, that's me.

> > Of cource we can close this cf entry. I would be happy if someone
> > else post proper implementation. And I can rework my implementation
> > again, but I don’t know how the correct implementation should look
> > or why latest implementation is wrong. 
> 
> Moving this patch to next CF is fine.

Cool, sorry for the misunderstanding then.

  37. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-07-01T11:33:39Z

    Hello

Updated version attached. Merge conflict was about tests count in 001_stream_rep.pl. Nothing else was changed. My approach can be still incorrect, any redesign ideas are welcome. Thanks in advance!

regards, Sergei

  38. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-07-30T06:42:39Z

    On Mon, Jul 01, 2019 at 02:33:39PM +0300, Sergei Kornilov wrote:
> Updated version attached. Merge conflict was about tests count in
> 001_stream_rep.pl. Nothing else was changed. My approach can be
> still incorrect, any redesign ideas are welcome. Thanks in advance! 

It has been some time, and I am finally catching up with this patch.

+         <para>
+          WAL receiver will be restarted after <varname>primary_slot_name</varname>
+          was changed.
          </para>
The sentence sounds strange.  Here is a suggestion:
The WAL receiver is restarted after an update of primary_slot_name (or
primary_conninfo).

The comment at the top of the call of ProcessStartupSigHup() in
HandleStartupProcInterrupts() needs to be updated as it mentions a
configuration file re-read, but that's not the case anymore.

pendingRestartSource's name does not represent what it does, as it is
only associated with the restart of a WAL receiver when in streaming
state, and that's a no-op for the archive mode and the local mode.

So, the patch splits the steps taken when checking for a WAL source by
adding an extra step after the failure handling that you are calling
the restart step.  When a failure happens for the stream mode
(shutdown of WAL receiver, promotion. etc), there is a switch to the
archive mode, and nothing is changed in this case in your patch.  So
when shutting down the WAL receiver after a parameter change, what
happens is that the startup process waits for retrieve_retry_interval
before moving back to the archive mode.  Only after scanning again the
archives do we restart a new WAL receiver.  However, if the restart of
the WAL receiver is planned because of an update of primary_conninfo
(or slot), shouldn't the follow-up mode be XLOG_FROM_STREAM without
waiting for wal_retrieve_retry_interval ms for extra WAL to become
available?
--
Michael

  39. Re: allow online change primary_conninfo

    Thomas Munro <thomas.munro@gmail.com> — 2019-08-01T09:06:59Z

    On Tue, Jul 30, 2019 at 6:42 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Jul 01, 2019 at 02:33:39PM +0300, Sergei Kornilov wrote:
> > Updated version attached. Merge conflict was about tests count in
> > 001_stream_rep.pl. Nothing else was changed. My approach can be
> > still incorrect, any redesign ideas are welcome. Thanks in advance!
>
> [review]

Unfortunately this comes right that the end of the CF, but the good
news is that there is another one just around the corner.  Moved to
September.

-- 
Thomas Munro
https://enterprisedb.com

  40. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2019-08-01T09:13:58Z

    On Thu, Aug 01, 2019 at 09:06:59PM +1200, Thomas Munro wrote:
> Unfortunately this comes right that the end of the CF, but the good
> news is that there is another one just around the corner.  Moved to
> September.

Moving it to the next CF is fine, thanks.  The author had no time to
reply since my last lookup.
--
Michael

  41. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-08-01T10:09:51Z

    Hello

> It has been some time, and I am finally catching up with this patch.

Thank you!

> + <para>
> + WAL receiver will be restarted after <varname>primary_slot_name</varname>
> + was changed.
>           </para>
> The sentence sounds strange. Here is a suggestion:
> The WAL receiver is restarted after an update of primary_slot_name (or
> primary_conninfo).
>
> The comment at the top of the call of ProcessStartupSigHup() in
> HandleStartupProcInterrupts() needs to be updated as it mentions a
> configuration file re-read, but that's not the case anymore.

I agree. But I want to know if this is an acceptable idea in general, before spending a few more months polishing the text on something we don’t want.

> pendingRestartSource's name does not represent what it does, as it is
> only associated with the restart of a WAL receiver when in streaming
> state, and that's a no-op for the archive mode and the local mode.

Currently we have only one source with such operation, so I can name variable pendingRestartWalreceiver and replace corresponding switch to compact condition. If someday we need additional actions for another wal source - we can replace this code. Looks like some form of premature optimization from me

> However, if the restart of
> the WAL receiver is planned because of an update of primary_conninfo
> (or slot), shouldn't the follow-up mode be XLOG_FROM_STREAM without
> waiting for wal_retrieve_retry_interval ms for extra WAL to become
> available?

Hmm. Standby with log_min_messages = debug2 and default wal_retrieve_retry_interval = 5s gives me:

2019-08-01 12:38:31.255 MSK 15707 @ from  [vxid: txid:0] [] DEBUG:  sendtime 2019-08-01 12:38:31.254905+03 receipttime 2019-08-01 12:38:31.254986+03 replication apply delay 0 ms transfer latency 0 ms
2019-08-01 12:38:31.255 MSK 15707 @ from  [vxid: txid:0] [] DEBUG:  sending write 0/3023FA8 flush 0/3023F38 apply 0/3023F38
2019-08-01 12:38:31.262 MSK 15707 @ from  [vxid: txid:0] [] DEBUG:  sending write 0/3023FA8 flush 0/3023FA8 apply 0/3023F38
2019-08-01 12:38:31.262 MSK 15707 @ from  [vxid: txid:0] [] DEBUG:  sending write 0/3023FA8 flush 0/3023FA8 apply 0/3023FA8
2019-08-01 12:38:31.457 MSK 15699 @ from  [vxid: txid:0] [] LOG:  received SIGHUP, reloading configuration files
2019-08-01 12:38:31.458 MSK 15699 @ from  [vxid: txid:0] [] LOG:  parameter "primary_conninfo" changed to " host='/tmp' port=5555"
2019-08-01 12:38:31.459 MSK 15700 @ from  [vxid:1/0 txid:0] [] LOG:  The WAL receiver is going to be restarted due to change of primary_conninfo
2019-08-01 12:38:31.459 MSK 15703 @ from  [vxid: txid:0] [] DEBUG:  checkpointer updated shared memory configuration values
2019-08-01 12:38:31.459 MSK 15707 @ from  [vxid: txid:0] [] FATAL:  terminating walreceiver process due to administrator command
2019-08-01 12:38:31.463 MSK 15724 @ from  [vxid: txid:0] [] LOG:  started streaming WAL from primary at 0/3000000 on timeline 1
2019-08-01 12:38:31.464 MSK 15724 @ from  [vxid: txid:0] [] DEBUG:  sendtime 2019-08-01 12:38:31.463954+03 receipttime 2019-08-01 12:38:31.464228+03 replication apply delay 0 ms transfer latency 0 ms
2019-08-01 12:38:31.464 MSK 15724 @ from  [vxid: txid:0] [] DEBUG:  sendtime 2019-08-01 12:38:31.464068+03 receipttime 2019-08-01 12:38:31.464375+03 replication apply delay 0 ms transfer latency 0 ms
2019-08-01 12:38:31.464 MSK 15724 @ from  [vxid: txid:0] [] DEBUG:  sending write 0/3023FA8 flush 0/3023FA8 apply 0/3023FA8

No source switch, no wal_retrieve_retry_interval waiting. (wal writes on primary by \watch 0.3 query)
RequestXLogStreaming set walRcvState to WALRCV_STARTING - correct state for WalRcvStreaming and therefore we have no lastSourceFailed.

regards, Sergei

  42. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-08-28T09:49:46Z

    Hello

Updated patch attached. (also I merged into one file)

> + <para>
> + WAL receiver will be restarted after <varname>primary_slot_name</varname>
> + was changed.
>           </para>
> The sentence sounds strange. Here is a suggestion:
> The WAL receiver is restarted after an update of primary_slot_name (or
> primary_conninfo).

Changed.

> The comment at the top of the call of ProcessStartupSigHup() in
> HandleStartupProcInterrupts() needs to be updated as it mentions a
> configuration file re-read, but that's not the case anymore.

Changed to "Process any requests or signals received recently." like in other places, e.g. syslogger

> pendingRestartSource's name does not represent what it does, as it is
> only associated with the restart of a WAL receiver when in streaming
> state, and that's a no-op for the archive mode and the local mode.

I renamed to pendingWalRcvRestart and replaced switch with simple condition.

> So when shutting down the WAL receiver after a parameter change, what
> happens is that the startup process waits for retrieve_retry_interval
> before moving back to the archive mode. Only after scanning again the
> archives do we restart a new WAL receiver.

As I answered earlier, here is no switch to archive or wal_retrieve_retry_interval waiting in my patch. I recheck on current revision too:

2019-08-28 12:16:27.295 MSK 11180 @ from  [vxid: txid:0] [] DEBUG:  sending write 0/30346C8 flush 0/30346C8 apply 0/30346C8
2019-08-28 12:16:27.493 MSK 11172 @ from  [vxid: txid:0] [] LOG:  received SIGHUP, reloading configuration files
2019-08-28 12:16:27.494 MSK 11172 @ from  [vxid: txid:0] [] LOG:  parameter "primary_conninfo" changed to "host='/tmp' port=5555 sslmode=disable sslcompression=0 gssencmode=disable target_session_attrs=any"
2019-08-28 12:16:27.496 MSK 11173 @ from  [vxid:1/0 txid:0] [] LOG:  The WAL receiver is going to be restarted due to change of primary_conninfo
2019-08-28 12:16:27.496 MSK 11176 @ from  [vxid: txid:0] [] DEBUG:  checkpointer updated shared memory configuration values
2019-08-28 12:16:27.496 MSK 11180 @ from  [vxid: txid:0] [] FATAL:  terminating walreceiver process due to administrator command
2019-08-28 12:16:27.500 MSK 11335 @ from  [vxid: txid:0] [] LOG:  started streaming WAL from primary at 0/3000000 on timeline 1
2019-08-28 12:16:27.500 MSK 11335 @ from  [vxid: txid:0] [] DEBUG:  sendtime 2019-08-28 12:16:27.50037+03 receipttime 2019-08-28 12:16:27.500821+03 replication apply delay 0 ms transfer latency 0 ms

No "DEBUG:  switched WAL source from stream to archive after failure" messages, no time difference (wal_retrieve_retry_interval = 5s).

regards, Sergei

  43. Re: allow online change primary_conninfo

    Fujii Masao <masao.fujii@gmail.com> — 2019-09-18T14:34:17Z

    On Wed, Aug 28, 2019 at 6:50 PM Sergei Kornilov <sk@zsrv.org> wrote:
>
> Hello
>
> Updated patch attached. (also I merged into one file)

Thanks for updating the patch! Here are some comments from me.

    #primary_conninfo = '' # connection string to sending server
    # (change requires restart)
    #primary_slot_name = '' # replication slot on sending server
    # (change requires restart)

ISTM that you need to update the above parts in postgresql.conf.sample.


    /* we don't expect primary_conninfo to change */

ISTM that you need to update the above comment in walreceiver.c.


+         <para>
+          The WAL receiver is restarted after an update of
<varname>primary_conninfo</varname>.
+         </para>

If primary_conninfo is set to an empty string, walreceiver just shuts down,
not restarts. The above description in the doc is not always true.
So I'm thinking something like the following description is better.
Thought?

    If primary_conninfo 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.


There is the case where walreceiver doesn't shut down immediately
after the change of primary_conninfo. If the change happens while
the startup process in paused state (e.g., by pg_wal_replay_pause(),
recovery_min_apply_delay, recovery conflict, etc), the startup
process tries to terminate walreceiver after it gets out of such state.
Shouldn't this fact be documented as a note?

Regards,

-- 
Fujii Masao

  44. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-09-19T08:02:57Z

    Hello

Thank you for review!

> ISTM that you need to update the above parts in postgresql.conf.sample.

Good catch, I forgot about conf sample.

> ISTM that you need to update the above comment in walreceiver.c.

Changed

> If primary_conninfo is set to an empty string, walreceiver just shuts down,
> not restarts. The above description in the doc is not always true.
> So I'm thinking something like the following description is better.
> Thought?
>
>     If primary_conninfo 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.

Ok, changed.
I leave primary_slot_name description as before.

> There is the case where walreceiver doesn't shut down immediately
> after the change of primary_conninfo. If the change happens while
> the startup process in paused state (e.g., by pg_wal_replay_pause(),
> recovery_min_apply_delay, recovery conflict, etc), the startup
> process tries to terminate walreceiver after it gets out of such state.
> Shouldn't this fact be documented as a note?

Hmm. Is somewhere documented that walreceiver will receive WAL while the startup process in paused state?
(didn't add such note in current version)

regards, Sergei

  45. Re: allow online change primary_conninfo

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-09-19T09:11:33Z

    Hello. I looked this and have some comments.

At Wed, 28 Aug 2019 12:49:46 +0300, Sergei Kornilov <sk@zsrv.org> wrote in <34084371566985786@sas1-0a6c2e2b59d7.qloud-c.yandex.net>
sk> Hello
sk> 
sk> Updated patch attached. (also I merged into one file)
sk> 
sk> > + <para>
sk> > + WAL receiver will be restarted after <varname>primary_slot_name</varname>
sk> > + was changed.
sk> >           </para>
sk> > The sentence sounds strange. Here is a suggestion:
sk> > The WAL receiver is restarted after an update of primary_slot_name (or
sk> > primary_conninfo).
sk> 
sk> Changed.

-          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.

I'm not sure it's good to change the context of this
description. This was mentioning that changing of this parameter
requires server (re)start. So if we want to be on the same
context after rewriting, it would be like "This parameter can be
set any time and causes WAL receiver restart with the new setting
if the server is in standby mode."

And If I'm not missing something, I don't find an (explict)
paramter of postmaster for setting primary_conninfo.



sk> > The comment at the top of the call of ProcessStartupSigHup() in
sk> > HandleStartupProcInterrupts() needs to be updated as it mentions a
sk> > configuration file re-read, but that's not the case anymore.
sk> 
sk> Changed to "Process any requests or signals received recently." like in other places, e.g. syslogger
sk>
sk> > pendingRestartSource's name does not represent what it does, as it is
sk> > only associated with the restart of a WAL receiver when in streaming
sk> > state, and that's a no-op for the archive mode and the local mode.
sk> 
sk> I renamed to pendingWalRcvRestart and replaced switch with simple condition.
sk> 
sk> > So when shutting down the WAL receiver after a parameter change, what
sk> > happens is that the startup process waits for retrieve_retry_interval
sk> > before moving back to the archive mode. Only after scanning again the
sk> > archives do we restart a new WAL receiver.
sk> 
sk> As I answered earlier, here is no switch to archive or wal_retrieve_retry_interval waiting in my patch. I recheck on current revision too:

@@ -11787,48 +11793,6 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 					if (!StandbyMode)
 						return false;
 
-                    /*
-                     * If primary_conninfo is set, launch walreceiver to try
-                     * to stream the missing WAL.
-                     *
-                     * If fetching_ckpt is true, RecPtr points to the initial
-                     * checkpoint location. In that case, we use RedoStartLSN

Mentioning code, the movement of the code block makes the
surrounding swtich almost useless and the structure and the
result looks somewhat strange.

Couldn't we do the same thing by just skipping the wait and
setting lastSourceFailed to true in the case of intentional
walreceiver restart?

The attached is an incomplete (fraction) patch to sketch what is
in my mind. With something like this and making
ProcessStartupSigHup kill walreceiver would work.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  46. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-09-19T14:46:06Z

    Hello

Thank you for review!

> - 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.
>
> I'm not sure it's good to change the context of this
> description. This was mentioning that changing of this parameter
> requires server (re)start. So if we want to be on the same
> context after rewriting, it would be like "This parameter can be
> set any time and causes WAL receiver restart with the new setting
> if the server is in standby mode."

Was written such way after this review: https://www.postgresql.org/message-id/20181125214313.lydvmrraqjfrb3s2%40alap3.anarazel.de

> And If I'm not missing something, I don't find an (explict)
> paramter of postmaster for setting primary_conninfo.

Well, we have common -c option: -c name=value

> Couldn't we do the same thing by just skipping the wait and
> setting lastSourceFailed to true in the case of intentional
> walreceiver restart?

Yes, it's possible. Let's see... Done in attached variant.
We need check pendingWalRcvRestart before rescanLatestTimeLine lines.

regards, Sergei

  47. Re: allow online change primary_conninfo

    Andres Freund <andres@anarazel.de> — 2019-09-20T18:09:48Z

    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

  48. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-09-21T10:06:25Z

    Hello

Thank you for review! Can you please also check v4 version? v5 implements design suggested by Kyotaro Horiguchi-san, while v4 has another design. Which one do you prefer? Or are both wrong?

> 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.

Design idea is to rewrite current state from working XLOG_FROM_STREAM to failed XLOG_FROM_ARCHIVE (without actually try this method on this iteration) and immediately go to next iteration to advance the state machine. Next iteration after failed archive recovery is walreceiver. So walreceiver will be stopped just before this lines and started on next iteration. Walreceiver will be restarted, we do not call restore_command

> Also, should we really check this before scanning for new timelines?

Hmm, on the next day... No, this is not really necessary.

> 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.

I hope I clarified this in design idea description above.

Thank you!

regards, Sergei

  49. Re: allow online change primary_conninfo

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-09-25T05:19:17Z

    Hello.

At Sat, 21 Sep 2019 13:06:25 +0300, Sergei Kornilov <sk@zsrv.org> wrote in <41171569060385@myt5-b646bde4b8f3.qloud-c.yandex.net>
> Hello
> 
> Thank you for review! Can you please also check v4 version? v5 implements design suggested by Kyotaro Horiguchi-san, while v4 has another design. Which one do you prefer? Or are both wrong?
> 
> > 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.
> 
> Design idea is to rewrite current state from working XLOG_FROM_STREAM to failed XLOG_FROM_ARCHIVE (without actually try this method on this iteration) and immediately go to next iteration to advance the state machine. Next iteration after failed archive recovery is walreceiver. So walreceiver will be stopped just before this lines and started on next iteration. Walreceiver will be restarted, we do not call restore_command

Sorry, it's my bad. It meant "If wal receiver is requested to
restart, change state to XLOG_FROM_STREAM immediately skipping
the next XLOG_FROM_ARCHIVE state.".

> > Also, should we really check this before scanning for new timelines?
> 
> Hmm, on the next day... No, this is not really necessary.

No problem when timeline is not changed when explicit restart of
wal receiver.  But if it happened just after the standby received
new hisotry file, we suffer an extra restart of wal receiver. It
seems better that we check that in the case.

> > 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.
> 
> I hope I clarified this in design idea description above.

My suggestion was just to pretend that the next XLOG_FROM_STREAM
failed, but the outcome actually looks wierd as Andres commented.

I think that comes from the structure of the state machine. WAL
receiver is started not at the beginning of XLOG_FROM_STREAM
state but at the end of XLOG_FROM_ARCHIVE. So we need to switch
once to XLOG_FROM_ARCHIVE in order to start wal receiver.

So, I'd like to propose to move the stuff to the second switch().
(See the attached incomplete patch.) This is rather similar to
Sergei's previous proposal, but the structure of the state
machine is kept.

Some other comments are below.

In ProcessStartupSigHup, conninfo and slotname don't need to be
retained until the end of the function.

The log message in the function seems to be too detailed.  On the
other hand, if we changed primary_conninfo to '' (stop) or vise
versa (start), the message (restart) looks strange.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  50. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2019-10-31T10:02:36Z

    Hello

> So, I'd like to propose to move the stuff to the second switch().
> (See the attached incomplete patch.) This is rather similar to
> Sergei's previous proposal, but the structure of the state
> machine is kept.

Very similar to my v4 proposal (also move RequestXLogStreaming call), but closer to currentSource reading. No objections from me, attached patch is changed this way.
I renamed start_wal_receiver to startWalReceiver - this style looks more consistent to near code.

> +					/*
> +					 * Else, check if WAL receiver is still active.
> +					 */
> +					else if (!WalRcvStreaming())

I think we still need wait WalRcvStreaming after RequestXLogStreaming call. So I remove else branch and leave separate condition.

> In ProcessStartupSigHup, conninfo and slotname don't need to be
> retained until the end of the function.

Agreed, I move pfree

> The log message in the function seems to be too detailed. On the
> other hand, if we changed primary_conninfo to '' (stop) or vise
> versa (start), the message (restart) looks strange.

I have no strong opinion here. These messages was changed many times during this thread lifetime, can be changed anytime. I think this is not issue since we have no consensus about overall design.
Write detailed messages was proposed here: https://www.postgresql.org/message-id/20190216151025.GJ2240%40paquier.xyz

> or vise versa (start)

I explicitly check currentSource and WalRcvRunning, so we have no such messages if user had no walreceiver before.

regards, Sergei

  51. Re: allow online change primary_conninfo

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-01-21T14:30:06Z

    Hi,

I'm a bit confused by the status of this patch - it's marked as Waiting
on Author (since last week), but that status was set by Sergei Kornilov
who is also the patch author. And there have been no messages since the
beginning of 2019-11 CF.

So what's the current status of this patch?


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  52. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-01-21T15:03:18Z

    Hello

"Waiting on Author" is the right status. I found logical conflict with recently added wal_receiver_create_temp_slot GUC and my tests are currently broken. Will fix it and post new version.

PS: also, I surprised why it's ok for wal_receiver_create_temp_slot to be PGC_SIGHUP and ignore change of this setting until walreceiver will reconnect by unrelated reason. I means walreceiver does nothing special on SIGHUP. In common case change of wal_receiver_create_temp_slot setting will have effect only during restart of walreceiver process. And therefore we will switch to archive recovery. But such design was strongly rejected for my patch year ago.

regards, Sergei

  53. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2020-01-22T05:14:31Z

    On Tue, Jan 21, 2020 at 06:03:18PM +0300, Sergei Kornilov wrote:
> PS: also, I surprised why it's ok for wal_receiver_create_temp_slot
> to be PGC_SIGHUP and ignore change of this setting until walreceiver
> will reconnect by unrelated reason. I means walreceiver does nothing
> special on SIGHUP. In common case change of
> wal_receiver_create_temp_slot setting will have effect only during
> restart of walreceiver process. And therefore we will switch to
> archive recovery. But such design was strongly rejected for my patch
> year ago. 

[ Looks at 3297308... ]
Yeah, you are right.  I was not paying much attention but something
does not stick here.  My understanding is that we should have the WAL
receiver receive the value it needs to use from the startup process
(aka via RequestXLogStreaming from xlog.c), and that we ought to make
this new parameter PGC_POSTMASTER instead of PGC_SIGHUP.  HEAD is
inconsistent here.
--
Michael

  54. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-01-22T15:51:06Z

    Hello

> Yeah, you are right. I was not paying much attention but something
> does not stick here. My understanding is that we should have the WAL
> receiver receive the value it needs to use from the startup process
> (aka via RequestXLogStreaming from xlog.c), and that we ought to make
> this new parameter PGC_POSTMASTER instead of PGC_SIGHUP. HEAD is
> inconsistent here.

Thank you!

I attached two patches:
- first changes wal_receiver_create_temp_slot to PGC_POSTMASTER and moved the logic to RequestXLogStreaming
- second is based on last published v6 version of main patch. It changes wal_receiver_create_temp_slot back to PGC_SIGHUP along with primary_conninfo and primary_slot_name and will restart walreceiver if need.

Regarding the main patch: we have several possibilities for moving RequestXLogStreaming call. We need to choose one.
And, of course, changes in the text...

regards, Sergei

  55. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-01-31T11:26:24Z

    Hello
Small rebase due to merge conflict of the tests. No functional changes since v7.

PS: also it is end of current CF, I will mark patch entry as moved to the next CF.

  56. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-13T14:12:04Z

    On 2020-Jan-22, Michael Paquier wrote:

> On Tue, Jan 21, 2020 at 06:03:18PM +0300, Sergei Kornilov wrote:
> > PS: also, I surprised why it's ok for wal_receiver_create_temp_slot
> > to be PGC_SIGHUP and ignore change of this setting until walreceiver
> > will reconnect by unrelated reason. I means walreceiver does nothing
> > special on SIGHUP. In common case change of
> > wal_receiver_create_temp_slot setting will have effect only during
> > restart of walreceiver process. And therefore we will switch to
> > archive recovery. But such design was strongly rejected for my patch
> > year ago. 
> 
> [ Looks at 3297308... ]
> Yeah, you are right.  I was not paying much attention but something
> does not stick here.  My understanding is that we should have the WAL
> receiver receive the value it needs to use from the startup process
> (aka via RequestXLogStreaming from xlog.c), and that we ought to make
> this new parameter PGC_POSTMASTER instead of PGC_SIGHUP.  HEAD is
> inconsistent here.

I'm CCing Peter as committer of 329730827848.

What are the downsides of changing wal_receiver_create_temp_slot to
PGC_POSTMASTER?  It seems pretty nasty to requires a full server
restart.  Maybe we can signal all walreceivers at that point so that
they restart with the correct setting?  That's much less problematic, I
would think.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  57. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-13T14:17:38Z

    On 2020-Mar-13, Alvaro Herrera wrote:

> What are the downsides of changing wal_receiver_create_temp_slot to
> PGC_POSTMASTER?  It seems pretty nasty to requires a full server
> restart.  Maybe we can signal all walreceivers at that point so that
> they restart with the correct setting?  That's much less problematic, I
> would think.

... oh, that's exactly what 0002 does.  So patch 0001 is only about
making a temporary change to the create_temp_slot to be consistent with
existing policy, before changing the policy.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  58. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2020-03-16T04:16:05Z

    On Fri, Mar 13, 2020 at 11:17:38AM -0300, Alvaro Herrera wrote:
> ... oh, that's exactly what 0002 does.  So patch 0001 is only about
> making a temporary change to the create_temp_slot to be consistent with
> existing policy, before changing the policy.

Yes.  In my opinion, patch 0002 should not change the GUC mode of
wal_receiver_create_temp_slot as the discussion here is about 
primary_conninfo, even if both may share some logic regarding WAL
receiver shutdown and its restart triggered by the startup process.

Patch 0001 has actually been presented on this thread first:
https://www.postgresql.org/message-id/753391579708726@iva3-77ae5995f07f.qloud-c.yandex.net
And there is an independent patch registered in this CF:
https://commitfest.postgresql.org/27/2456/

Should we add patch 0001 as an open item for v13 as there is a risk of
forgetting this issue?  I have created a wiki blank page a couple of
weeks back:
https://wiki.postgresql.org/wiki/PostgreSQL_13_Open_Items
--
Michael

  59. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-17T16:53:15Z

    Hello

Sorry for late replies.

> Yes. In my opinion, patch 0002 should not change the GUC mode of
> wal_receiver_create_temp_slot as the discussion here is about
> primary_conninfo, even if both may share some logic regarding WAL
> receiver shutdown and its restart triggered by the startup process.

Ok, I removed related changes from main patch. Along with minor merge conflict.

> Patch 0001 has actually been presented on this thread first:
> https://www.postgresql.org/message-id/753391579708726@iva3-77ae5995f07f.qloud-c.yandex.net
> And there is an independent patch registered in this CF:
> https://commitfest.postgresql.org/27/2456/

Yep, 0001 is separate patch. I will post copy of this patch here to make cfbot works.  Main patch 0002 requires resetting of is_temp_slot in RequestXLogStreaming to works properly.

> Should we add patch 0001 as an open item for v13 as there is a risk of
> forgetting this issue?

I think yes.

Well, it seems better to move this patch to next commitfest?

regards, Sergei

  60. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-17T18:10:33Z

    On 2020-Mar-17, Sergei Kornilov wrote:

> Well, it seems better to move this patch to next commitfest?

What?  You want to make wal_receiver_create_temp_slot unchangeable and
default to off in pg13, and delay the patch that fixes those things to
pg14?  That makes no sense to me.  Please keep them both here so that we
can get things to a usable state.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  61. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-17T20:18:46Z

    Hello

>>  Well, it seems better to move this patch to next commitfest?
>
> What? You want to make wal_receiver_create_temp_slot unchangeable and
> default to off in pg13, and delay the patch that fixes those things to
> pg14? That makes no sense to me.

I want to handle similar things in a similar way.
wal_receiver_create_temp_slot has good design? I will change my patch in same way in this case. But something like that was strongly rejected a year ago.

> Please keep them both here so that we can get things to a usable state.

Yes, of course.

Here I attached 3 patches:
0001 is copy from https://commitfest.postgresql.org/27/2456/ It changes wal_receiver_create_temp_slot to PGC_POSTMASTER, changes the default value to off, and moves the logic to the startup process.
0002 changes primary_conninfo and primary_slot_name to be PGC_SIGHUP
0003 changes wal_receiver_create_temp_slot back to be PGC_SIGHUP. Michael Paquier asks to remove this from 0002, you ask to leave it in this thread. So, I made separate patch on top of 0002.

Thank you

regards, Sergei

  62. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-24T21:49:22Z

    I think the startup sighup handler should be in startup.c, not xlog.c,
which has enough random code already.  We can add an accessor in xlog.c
to let changing the walrcv status flag, to be called from the signal
handler.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  63. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-26T00:08:12Z

    On 2020-Mar-24, Alvaro Herrera wrote:

> I think the startup sighup handler should be in startup.c, not xlog.c,
> which has enough random code already.  We can add an accessor in xlog.c
> to let changing the walrcv status flag, to be called from the signal
> handler.

... as in the attached version.

Sergei included LOG messages to indicate which setting has been changed.
I put these in "#if 0" for now, but I'm thinking to remove these
altogether; we already have LOG messages when a setting changes value,
per ProcessConfigFileInternal(); the log sequence looks like this, taken
from tmp_check/log/001_stream_rep_standby_2.log after running the tests:

2020-03-25 20:54:19.413 -03 [17493] LOG:  received SIGHUP, reloading configuration files
2020-03-25 20:54:19.415 -03 [17493] LOG:  parameter "primary_slot_name" changed to "standby_2"
2020-03-25 20:54:19.415 -03 [17493] LOG:  parameter "wal_receiver_status_interval" changed to "1"
2020-03-25 20:54:19.422 -03 [17569] LOG:  started streaming WAL from primary at 0/3000000 on timeline 1
2020-03-25 20:54:19.426 -03 [17494] LOG:  wal receiver process shutdown requested
2020-03-25 20:54:19.426 -03 [17569] FATAL:  terminating walreceiver process due to administrator command
2020-03-25 20:54:19.433 -03 [17572] LOG:  started streaming WAL from primary at 0/3000000 on timeline 1

which looks sufficient.  Maybe we can reword that new message, say "wal
receiver process shutdown forced by parameter change".  Not sure if we
can or should adjust the FATAL line; probably not worth the trouble.

Thoughts welcome.  I'm thinking on getting this applied shortly.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  64. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2020-03-26T06:22:06Z

    On Wed, Mar 25, 2020 at 09:08:12PM -0300, Alvaro Herrera wrote:
> ... as in the attached version.

Patch 0001 is one that has already been discussed previously (thanks
for keeping the extra comments about GUCs and WAL receivers).  That
looks fine to me.

> Sergei included LOG messages to indicate which setting has been changed.
> I put these in "#if 0" for now, but I'm thinking to remove these
> altogether; we already have LOG messages when a setting changes value,
> per ProcessConfigFileInternal(); the log sequence looks like this, taken
> from tmp_check/log/001_stream_rep_standby_2.log after running the tests:

Yes, it makes sense to remove any knowledge of GUC updates from
StartupRequestWalReceiverRestart().  I would add a description at the
top of this routine by the way.

+extern void ProcessStartupSigHup(void);
This is declared, but used nowhere in patch 0002.

Wouldn't it be better to be more careful about the NULL-ness of the
string parameters in StartupRereadConfig()?

+   if (!slotnameChanged && strcmp(PrimarySlotName, "") == 0)
+       tempSlotChanged = tempSlot != wal_receiver_create_temp_slot;
I would add parens here for clarity.

> which looks sufficient.  Maybe we can reword that new message, say "wal
> receiver process shutdown forced by parameter change".  Not sure if we
> can or should adjust the FATAL line; probably not worth the trouble.

There is merit to keep the LOG in StartupRequestWalReceiverRestart()
unchanged, as the routine may get called in the future in some other
context.
--
Michael

  65. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-26T09:53:24Z

    Hello

Thank you! You were ahead of me. I wanted to double-check my changes this morning before posting.

> Sergei included LOG messages to indicate which setting has been changed.
> I put these in "#if 0" for now, but I'm thinking to remove these
> altogether;

No objections.

> Not sure if we can or should adjust the FATAL line; probably not worth the trouble.

I think it's ok. walreceiver is terminated exactly due to administrator command.

regards, Sergei

  66. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-27T00:39:17Z

    Now, would anyone be too upset if I push these in this other order?  I
realized that the reason the tests broke after Sergei's patch is that
recovery/t/001_stream_rep.pl's get_slot_xmins() is broken for temp
walreceiver slots, since it's using the non-temp name it tries to give
to the slot, rather than the temp name under which it is actually
created.  The workaround proposed by 0002 is to edit standby_1's config
to set walreceiver's slot to be non-temp.

Thanks to Justin Pryzby for offlist typo corrections.

The reason is that I think I would like to get Sergei's patch pushed
right away (0001+0002, as a single commit); but that I think there's
more to attack in the walreceiver temp slot code than 0003 here, and I
don't want to delay the new feature any longer while I figure out the
fix for that.


(The thing is: if I specify primary_slot_name in the config, why is the
temp walreceiver slot code not obeying that name?  I think walreceiver
should create a temp slot, sure, but using the given name rather than
coming up with a random name.)

(The other reason is that I would like to push one patch to fix
walreceiver tmp slot rather than two, setting the thing first this way
and then the opposite way.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  67. Re: allow online change primary_conninfo

    Michael Paquier <michael@paquier.xyz> — 2020-03-27T05:39:05Z

    On Thu, Mar 26, 2020 at 09:39:17PM -0300, Alvaro Herrera wrote:
> Now, would anyone be too upset if I push these in this other order?  I
> realized that the reason the tests broke after Sergei's patch is that
> recovery/t/001_stream_rep.pl's get_slot_xmins() is broken for temp
> walreceiver slots, since it's using the non-temp name it tries to give
> to the slot, rather than the temp name under which it is actually
> created.  The workaround proposed by 0002 is to edit standby_1's config
> to set walreceiver's slot to be non-temp.

FWIW, I find a bit strange that the change introduced in
001_stream_rep.pl as of patch 0002 is basically undone in 0003 by
changing the default value of wal_receiver_create_temp_slot.

> The reason is that I think I would like to get Sergei's patch pushed
> right away (0001+0002, as a single commit); but that I think there's
> more to attack in the walreceiver temp slot code than 0003 here, and I
> don't want to delay the new feature any longer while I figure out the
> fix for that.

Not sure I agree with this approach.  I'd rather fix all the existing
known problems first, and then introduce the new features on top of
what we consider to be a clean state.  If we lack of time between the
first and second patches, we may have a risk of keeping the code with
the new feature but without the fixes discussed for
wal_receiver_create_temp_slot.

> (The thing is: if I specify primary_slot_name in the config, why is the
> temp walreceiver slot code not obeying that name?  I think walreceiver
> should create a temp slot, sure, but using the given name rather than
> coming up with a random name.)

Good point.  I am not sure either why the current logic has been
chosen.  The discussion related to the original patch is in this area:
https://www.postgresql.org/message-id/4792e456-d75f-8e6a-2d47-34b8f78c266f@2ndquadrant.com

> (The other reason is that I would like to push one patch to fix
> walreceiver tmp slot rather than two, setting the thing first this way
> and then the opposite way.)

So your problem here is that by applying first 0003 and then 0001-0002
you would finish by switching wal_receiver_create_temp_slot to
PGC_POSTMASTER, and then back to PGC_SIGHUP again?
--
Michael

  68. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-27T11:18:50Z

    Hello

> I realized that the reason the tests broke after Sergei's patch is that
> recovery/t/001_stream_rep.pl's get_slot_xmins() is broken for temp
> walreceiver slots, since it's using the non-temp name it tries to give
> to the slot, rather than the temp name under which it is actually
> created. The workaround proposed by 0002 is to edit standby_1's config
> to set walreceiver's slot to be non-temp.

This is bug in behavior, not in tests.
We need walrcv->is_temp_slot = false; somewhere in RequestXLogStreaming to works correctly.

HEAD is not affected since primary_slot_name cannot be changed online.

> (The thing is: if I specify primary_slot_name in the config, why is the
> temp walreceiver slot code not obeying that name? I think walreceiver
> should create a temp slot, sure, but using the given name rather than
> coming up with a random name.)

Hm, interesting idea.

regards, Sergei

  69. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-27T18:22:20Z

    Hello

In other words, patches in reverse order:
0001 will change primary_conninfo and primary_slot_name to reloadable
0002 will move wal_receiver_create_temp_slot logic to startup process (without changing to PGC_POSTMASTER)
0003 is new draft patch: wal_receiver_create_temp_slot will use the given name or generate new one.

regards, Sergei

  70. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-27T19:27:45Z

    On 2020-Mar-27, Sergei Kornilov wrote:

> Hello
> 
> > I realized that the reason the tests broke after Sergei's patch is that
> > recovery/t/001_stream_rep.pl's get_slot_xmins() is broken for temp
> > walreceiver slots, since it's using the non-temp name it tries to give
> > to the slot, rather than the temp name under which it is actually
> > created. The workaround proposed by 0002 is to edit standby_1's config
> > to set walreceiver's slot to be non-temp.
> 
> This is bug in behavior, not in tests.
> We need walrcv->is_temp_slot = false; somewhere in RequestXLogStreaming to works correctly.
> 
> HEAD is not affected since primary_slot_name cannot be changed online.

I pushed the wal_receiver_create_temp_slot bugfix, because I realized
after looking for long enough at WalReceiverMain() that the code was
beyond saving.  I'll be pushing the rest of this later today.

Thanks,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  71. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-27T19:51:49Z

    On 2020-Mar-27, Alvaro Herrera wrote:

> I pushed the wal_receiver_create_temp_slot bugfix, because I realized
> after looking for long enough at WalReceiverMain() that the code was
> beyond saving.  I'll be pushing the rest of this later today.

So here's the next one.  I still have to go over the doc changes here,
but at least the tests are passing for me.

I think I should set aside your new draft for now, but I think we should
still get it in pg13 to avoid having the change the semantics of the
walreceiver temp slot in the next release.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  72. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-27T20:50:43Z

    Hello

Thank you!

> I think I should set aside your new draft for now

I agree, this patch definitely needs a bit more time to review. (currently it applies on top of v13 patch cleanly)

> but I think we should still get it in pg13 to avoid having the change the semantics of the
> walreceiver temp slot in the next release.

Good point.

regards, Sergei

  73. Re: allow online change primary_conninfo

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-27T23:39:11Z

    On 2020-Mar-27, Alvaro Herrera wrote:

> On 2020-Mar-27, Alvaro Herrera wrote:
> 
> > I pushed the wal_receiver_create_temp_slot bugfix, because I realized
> > after looking for long enough at WalReceiverMain() that the code was
> > beyond saving.  I'll be pushing the rest of this later today.
> 
> So here's the next one.  I still have to go over the doc changes here,
> but at least the tests are passing for me.

Pushed with some documentation tweaks.

Many thanks for working on this!

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  74. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-03-28T10:49:43Z

    Hello

Thank you very much!

I attached updated patch: walreceiver will use configured primary_slot_name as temporary slot name if wal_receiver_create_temp_slot is enabled.

regards, Sergei

  75. Re: allow online change primary_conninfo

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-04-01T14:03:05Z

    On 2020-03-28 11:49, Sergei Kornilov wrote:
> I attached updated patch: walreceiver will use configured primary_slot_name as temporary slot name if wal_receiver_create_temp_slot is enabled.

The original setup worked consistently with pg_basebackup.  In 
pg_basebackup, if you specify -S/--slot, then it uses a permanent slot 
with the given name.  This is the same behavior as primary_slot_name has 
now.  We should be careful that we don't create two sets of 
options/settings that look similar but combine in different ways.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  76. Re: allow online change primary_conninfo

    Maxim Orlov <m.orlov@postgrespro.ru> — 2020-10-16T16:50:01Z

    On 2020-03-28 02:39, Alvaro Herrera wrote:
> On 2020-Mar-27, Alvaro Herrera wrote:
> 
>> On 2020-Mar-27, Alvaro Herrera wrote:
>> 
>> > I pushed the wal_receiver_create_temp_slot bugfix, because I realized
>> > after looking for long enough at WalReceiverMain() that the code was
>> > beyond saving.  I'll be pushing the rest of this later today.
>> 
>> So here's the next one.  I still have to go over the doc changes here,
>> but at least the tests are passing for me.
> 
> Pushed with some documentation tweaks.
> 
> Many thanks for working on this!

Nice work! What do you think of possibility to add restore_command?

Is it a good idea to make a restore_command to be reloadable via SUGHUP 
too?

On the one hand it looks reasonable since primary_conninfo got such 
ability. On the other hand we don't exactly know whether the actual 
process after reload config would use "new" command or "old"  since it 
may take a long time to finish running old command (in case of scp or 
smth).

Also setting faulty restore_command lead to server termination, which is 
rather unexpectedly.
If anyone makes some minor typo in restore_command, say write 
restore_command='cp1 /mnt/srv/%f %p' instead of restore_command='cp 
/mnt/srv/%f %p' and do SELECT pg_reload_conf() then server will 
terminate.

Do you consider this feature useful? Any suggestions?

---
Best regards,
Maxim Orlov.

  77. Re: allow online change primary_conninfo

    Sergei Kornilov <sk@zsrv.org> — 2020-10-16T20:01:58Z

    Hello

Yep, I think it's useful and I already posted patch in this thread: https://www.postgresql.org/message-id/flat/3090621585393698%40myt5-1466095fe4e5.qloud-c.yandex.net#ee6574e93982b5628d140f15ffffcb44
Currently without consensus

regards, Sergei