Thread: Allow some recovery parameters to be changed with reload

Allow some recovery parameters to be changed with reload

From
Peter Eisentraut
Date:
I think the recovery parameters

    archive_cleanup_command
    promote_trigger_file
    recovery_end_command
    recovery_min_apply_delay

can be changed from PGC_POSTMASTER to PGC_SIGHUP without any further
complications (unlike for example primary_conninfo, which is being
discussed elsewhere).

See attached patch.

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

Attachment

Re: Allow some recovery parameters to be changed with reload

From
Michael Paquier
Date:
On Mon, Feb 04, 2019 at 11:58:28AM +0100, Peter Eisentraut wrote:
> I think the recovery parameters
>
>     archive_cleanup_command

Only triggered by the checkpointer.

>     promote_trigger_file
>     recovery_end_command
>     recovery_min_apply_delay

Only looked at by the startup process.

> can be changed from PGC_POSTMASTER to PGC_SIGHUP without any further
> complications (unlike for example primary_conninfo, which is being
> discussed elsewhere).

I agree that this subset is straight-forward and safe to switch.  The
documentation changes look right.
--
Michael

Attachment

Re: Allow some recovery parameters to be changed with reload

From
Peter Eisentraut
Date:
On 05/02/2019 04:35, Michael Paquier wrote:
> On Mon, Feb 04, 2019 at 11:58:28AM +0100, Peter Eisentraut wrote:
>> I think the recovery parameters
>>
>>     archive_cleanup_command
> 
> Only triggered by the checkpointer.
> 
>>     promote_trigger_file
>>     recovery_end_command
>>     recovery_min_apply_delay
> 
> Only looked at by the startup process.
> 
>> can be changed from PGC_POSTMASTER to PGC_SIGHUP without any further
>> complications (unlike for example primary_conninfo, which is being
>> discussed elsewhere).
> 
> I agree that this subset is straight-forward and safe to switch.  The
> documentation changes look right.

Committed, thanks.

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


Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

>>  I think the recovery parameters
>>
>>      archive_cleanup_command
>
> Only triggered by the checkpointer.
>
>>      promote_trigger_file
>>      recovery_end_command
>>      recovery_min_apply_delay
>
> Only looked at by the startup process.

We have some possible trouble with restore_command? As far i know it also only looked at by the startup process.

regards, Sergei


Re: Allow some recovery parameters to be changed with reload

From
Peter Eisentraut
Date:
On 07/02/2019 09:14, Sergei Kornilov wrote:
> We have some possible trouble with restore_command? As far i know it also only looked at by the startup process.

Probably right.  I figured it would be useful to see what the outcome is
with primary_conninfo, so they can be treated similarly.

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


Re: Allow some recovery parameters to be changed with reload

From
Michael Paquier
Date:
On Thu, Feb 07, 2019 at 11:06:27PM +0100, Peter Eisentraut wrote:
> Probably right.  I figured it would be useful to see what the outcome is
> with primary_conninfo, so they can be treated similarly.

The interactions with waiting for WAL to be available and the WAL
receiver stresses me a bit for restore_command, as you could finish
with the startup process switching to use restore_command with a WAL
receiver still working behind and overwriting partially the recovered
segment, which could lead to corruption.  We should be *very* careful
about that.
--
Michael

Attachment

Re: Allow some recovery parameters to be changed with reload

From
Andres Freund
Date:
Hi,

On 2019-02-08 09:19:31 +0900, Michael Paquier wrote:
> On Thu, Feb 07, 2019 at 11:06:27PM +0100, Peter Eisentraut wrote:
> > Probably right.  I figured it would be useful to see what the outcome is
> > with primary_conninfo, so they can be treated similarly.
> 
> The interactions with waiting for WAL to be available and the WAL
> receiver stresses me a bit for restore_command, as you could finish
> with the startup process switching to use restore_command with a WAL
> receiver still working behind and overwriting partially the recovered
> segment, which could lead to corruption.  We should be *very* careful
> about that.

I'm not clear on the precise mechanics you're imagining here, could you
expand a bit? We kill the walreceiver when switching from receiver to
restore command, and wait for it to acknowledge that, no?
C.F. ShutdownWalRcv() call in the lastSourceFailed branch of
WaitForWALToBecomeAvailable().

Greetings,

Andres Freund


Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

I want to return to this discussion, since primary_conninfo is now PGC_SIGHUP (and I hope will not be reverted)

> On 2019-02-08 09:19:31 +0900, Michael Paquier wrote:
>>  On Thu, Feb 07, 2019 at 11:06:27PM +0100, Peter Eisentraut wrote:
>>  > Probably right. I figured it would be useful to see what the outcome is
>>  > with primary_conninfo, so they can be treated similarly.
>>
>>  The interactions with waiting for WAL to be available and the WAL
>>  receiver stresses me a bit for restore_command, as you could finish
>>  with the startup process switching to use restore_command with a WAL
>>  receiver still working behind and overwriting partially the recovered
>>  segment, which could lead to corruption. We should be *very* careful
>>  about that.
>
> I'm not clear on the precise mechanics you're imagining here, could you
> expand a bit? We kill the walreceiver when switching from receiver to
> restore command, and wait for it to acknowledge that, no?
> C.F. ShutdownWalRcv() call in the lastSourceFailed branch of
> WaitForWALToBecomeAvailable().

So...
We call restore_command only when walreceiver is stopped.
We use restore_command only in startup process - so we have no race condition between processes.
We have some issues here? Or we can just make restore_command reloadable as attached?

regards, Sergei
Attachment

Re: Allow some recovery parameters to be changed with reload

From
Robert Haas
Date:
On Sat, Mar 28, 2020 at 7:21 AM Sergei Kornilov <sk@zsrv.org> wrote:
> So...
> We call restore_command only when walreceiver is stopped.
> We use restore_command only in startup process - so we have no race condition between processes.
> We have some issues here? Or we can just make restore_command reloadable as attached?

I don't see the problem here, either. Does anyone else see a problem,
or some reason not to press forward with this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: Allow some recovery parameters to be changed with reload

From
Michael Paquier
Date:
On Wed, Aug 05, 2020 at 11:41:49AM -0400, Robert Haas wrote:
> On Sat, Mar 28, 2020 at 7:21 AM Sergei Kornilov <sk@zsrv.org> wrote:
>> So...
>> We call restore_command only when walreceiver is stopped.
>> We use restore_command only in startup process - so we have no race condition between processes.
>> We have some issues here? Or we can just make restore_command reloadable as attached?
>
> I don't see the problem here, either. Does anyone else see a problem,
> or some reason not to press forward with this?

Sorry for the late reply.  I have been looking at that stuff again,
and restore_command can be called in the context of a WAL sender
process within the page_read callback of logical decoding via
XLogReadDetermineTimeline(), as readTimeLineHistory() could look for a
timeline history file.  So restore_command is not used only in the
startup process.
--
Michael

Attachment

Re: Allow some recovery parameters to be changed with reload

From
Robert Haas
Date:
On Sun, Aug 9, 2020 at 1:21 AM Michael Paquier <michael@paquier.xyz> wrote:
> Sorry for the late reply.  I have been looking at that stuff again,
> and restore_command can be called in the context of a WAL sender
> process within the page_read callback of logical decoding via
> XLogReadDetermineTimeline(), as readTimeLineHistory() could look for a
> timeline history file.  So restore_command is not used only in the
> startup process.

Hmm, interesting. But, does that make this change wrong, apart from
the comments? Like, in the case of primary_conninfo, maybe some
confusion could result if the startup process decided whether to ask
for a WAL receiver based on thinking primary_conninfo being set, while
that process thought that it wasn't actually set after all, as
previously discussed in
http://postgr.es/m/CA+TgmoZVmJX1+QTWw2tSnPHrnkwKZxC3ZsRynFB-Fpzm1Oxuhw@mail.gmail.com
... but what's the corresponding hazard here, exactly? It doesn't seem
that there's any way in which the decision one process makes affects
the decision the other process makes. There's still a race condition:
it's possible for a walsender to use the old restore_command after the
startup process had already used the new one, or the other way around.
However, it doesn't seem like that should confuse anything inside the
server, and therefore I'm not sure we need to code around it.

If you or someone else thinks we do, then it'd be nice to hear why,
and what guarantees you think we should be aiming to achieve.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: Allow some recovery parameters to be changed with reload

From
Anastasia Lubennikova
Date:
On 10.08.2020 23:20, Robert Haas wrote:
> On Sun, Aug 9, 2020 at 1:21 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Sorry for the late reply.  I have been looking at that stuff again,
>> and restore_command can be called in the context of a WAL sender
>> process within the page_read callback of logical decoding via
>> XLogReadDetermineTimeline(), as readTimeLineHistory() could look for a
>> timeline history file.  So restore_command is not used only in the
>> startup process.
> Hmm, interesting. But, does that make this change wrong, apart from
> the comments? Like, in the case of primary_conninfo, maybe some
> confusion could result if the startup process decided whether to ask
> for a WAL receiver based on thinking primary_conninfo being set, while
> that process thought that it wasn't actually set after all, as
> previously discussed in
> http://postgr.es/m/CA+TgmoZVmJX1+QTWw2tSnPHrnkwKZxC3ZsRynFB-Fpzm1Oxuhw@mail.gmail.com
> ... but what's the corresponding hazard here, exactly? It doesn't seem
> that there's any way in which the decision one process makes affects
> the decision the other process makes. There's still a race condition:
> it's possible for a walsender
Did you mean walreceiver here?
>   to use the old restore_command after the
> startup process had already used the new one, or the other way around.
> However, it doesn't seem like that should confuse anything inside the
> server, and therefore I'm not sure we need to code around it.
I came up with following scenario. Let's say we have xlog files 1,2,3 in 
dir1 and files 4,5 in dir2. If startup process had only handled files 1 
and 2, before we switched restore_command from reading dir1 to reading 
dir2, it will fail to find next file. IIUC, it will assume that recovery 
is done, start server and walreceiver. The walreceiver will fail as 
well. I don't know, how realistic is this case, though.

In general,. this feature looks useful and consistent with previous 
changes, so I am interested in pushing it forward.
Sergey, could you please attach this thread to the upcoming CF, if 
you're going to continue working on it.

  A few more questions:
- RestoreArchivedFile() is also used by pg_rewind. I don't see any 
particular problem with it, just want to remind that we should test it too.
- How will it interact with possible future optimizations of archive 
restore? For example, WAL prefetch [1].

  [1] 
https://www.postgresql.org/message-id/flat/601EE1F5-0B78-47E1-9AAE-C15F74A1C21D@postgrespro.ru

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




Re: Allow some recovery parameters to be changed with reload

From
Kyotaro Horiguchi
Date:
At Thu, 22 Oct 2020 01:59:07 +0300, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote in
> On 10.08.2020 23:20, Robert Haas wrote:
> > On Sun, Aug 9, 2020 at 1:21 AM Michael Paquier <michael@paquier.xyz>
> > wrote:
> >> Sorry for the late reply.  I have been looking at that stuff again,
> >> and restore_command can be called in the context of a WAL sender
> >> process within the page_read callback of logical decoding via
> >> XLogReadDetermineTimeline(), as readTimeLineHistory() could look for a
> >> timeline history file.  So restore_command is not used only in the
> >> startup process.
> > Hmm, interesting. But, does that make this change wrong, apart from
> > the comments? Like, in the case of primary_conninfo, maybe some
> > confusion could result if the startup process decided whether to ask
> > for a WAL receiver based on thinking primary_conninfo being set, while
> > that process thought that it wasn't actually set after all, as
> > previously discussed in
> > http://postgr.es/m/CA+TgmoZVmJX1+QTWw2tSnPHrnkwKZxC3ZsRynFB-Fpzm1Oxuhw@mail.gmail.com
> > ... but what's the corresponding hazard here, exactly? It doesn't seem
> > that there's any way in which the decision one process makes affects
> > the decision the other process makes. There's still a race condition:
> > it's possible for a walsender
> Did you mean walreceiver here?

It's logical walsender. restore_command is used within
logical_read_xlog_page() via XLogReadDetermineTimeline().

> >   to use the old restore_command after the
> > startup process had already used the new one, or the other way around.
> > However, it doesn't seem like that should confuse anything inside the
> > server, and therefore I'm not sure we need to code around it.
> I came up with following scenario. Let's say we have xlog files 1,2,3
> in dir1 and files 4,5 in dir2. If startup process had only handled
> files 1 and 2, before we switched restore_command from reading dir1 to
> reading dir2, it will fail to find next file. IIUC, it will assume
> that recovery is done, start server and walreceiver. The walreceiver
> will fail as well. I don't know, how realistic is this case, though.

That operation is somewhat bogus, if the server is not in standby
mode.  In standby mode, startup waits for the next segment safely.

> In general,. this feature looks useful and consistent with previous
> changes, so I am interested in pushing it forward.

Agreed. The feature seems to work fine as far as we don't make a
change of restore_command that moves to another history.  Otherwise
recovery doesn't work correctly regaredless whether it is PGC_SIGHUP
or not.

> Sergey, could you please attach this thread to the upcoming CF, if
> you're going to continue working on it.
>
>  A few more questions:
> - RestoreArchivedFile() is also used by pg_rewind. I don't see any
> - particular problem with it, just want to remind that we should test it
> - too.
> - How will it interact with possible future optimizations of archive
> - restore? For example, WAL prefetch [1].
>
>  [1]
> https://www.postgresql.org/message-id/flat/601EE1F5-0B78-47E1-9AAE-C15F74A1C21D@postgrespro.ru

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

Sorry for late response.

>>  > ... but what's the corresponding hazard here, exactly? It doesn't seem
>>  > that there's any way in which the decision one process makes affects
>>  > the decision the other process makes. There's still a race condition:
>>  > it's possible for a walsender
>>  Did you mean walreceiver here?
>
> It's logical walsender. restore_command is used within
> logical_read_xlog_page() via XLogReadDetermineTimeline().

Still have no idea what's the corresponding hazard here.

>>  > to use the old restore_command after the
>>  > startup process had already used the new one, or the other way around.
>>  > However, it doesn't seem like that should confuse anything inside the
>>  > server, and therefore I'm not sure we need to code around it.
>>  I came up with following scenario. Let's say we have xlog files 1,2,3
>>  in dir1 and files 4,5 in dir2. If startup process had only handled
>>  files 1 and 2, before we switched restore_command from reading dir1 to
>>  reading dir2, it will fail to find next file. IIUC, it will assume
>>  that recovery is done, start server and walreceiver. The walreceiver
>>  will fail as well. I don't know, how realistic is this case, though.
>
> That operation is somewhat bogus, if the server is not in standby
> mode. In standby mode, startup waits for the next segment safely.

I think it's pilot error. It is already possible to change anything in restore_command by wrapping real command into
somescript:
 

> restore_command = '/bin/restore_wal.sh "%f" "%p"'

And one can simple replace this file with something else with different logic. Or even by using some command with
separateown settings. Real world example ( https://github.com/wal-g/wal-g ):
 

> restore_command = '. /etc/wal-g/WALG_AWS_ENV; wal-g wal-fetch "%f" "%p"'

And it is possible to change the real WAL source in ENV script without changing the restore_command. We can't track
this,so I not see new issues here.
 

>>  Sergey, could you please attach this thread to the upcoming CF, if
>>  you're going to continue working on it.

Sure, I created one: https://commitfest.postgresql.org/30/2802/

>>  - How will it interact with possible future optimizations of archive
>>  - restore? For example, WAL prefetch [1].

Shouldn't we ask the author of such a patch and not me? In particular, does this patch rely on the restore_command not
beingchanged? Probably some form of synchronisation would be neccesary in infrastructure for parallel executing restore
commands.On / off handling of restore_command will most likely be required. I did not review this patch.
 

regards, Sergei



Re: Allow some recovery parameters to be changed with reload

From
Fujii Masao
Date:

On 2020/10/28 21:02, Sergei Kornilov wrote:
> Hello
> 
> Sorry for late response.
> 
>>>   > ... but what's the corresponding hazard here, exactly? It doesn't seem
>>>   > that there's any way in which the decision one process makes affects
>>>   > the decision the other process makes. There's still a race condition:
>>>   > it's possible for a walsender
>>>   Did you mean walreceiver here?
>>
>> It's logical walsender. restore_command is used within
>> logical_read_xlog_page() via XLogReadDetermineTimeline().
> 
> Still have no idea what's the corresponding hazard here.
> 
>>>   > to use the old restore_command after the
>>>   > startup process had already used the new one, or the other way around.
>>>   > However, it doesn't seem like that should confuse anything inside the
>>>   > server, and therefore I'm not sure we need to code around it.
>>>   I came up with following scenario. Let's say we have xlog files 1,2,3
>>>   in dir1 and files 4,5 in dir2. If startup process had only handled
>>>   files 1 and 2, before we switched restore_command from reading dir1 to
>>>   reading dir2, it will fail to find next file. IIUC, it will assume
>>>   that recovery is done, start server and walreceiver. The walreceiver
>>>   will fail as well. I don't know, how realistic is this case, though.
>>
>> That operation is somewhat bogus, if the server is not in standby
>> mode. In standby mode, startup waits for the next segment safely.
> 
> I think it's pilot error. It is already possible to change anything in restore_command by wrapping real command into
somescript:
 
> 
>> restore_command = '/bin/restore_wal.sh "%f" "%p"'
> 
> And one can simple replace this file with something else with different logic. Or even by using some command with
separateown settings. Real world example ( https://github.com/wal-g/wal-g ):
 
> 
>> restore_command = '. /etc/wal-g/WALG_AWS_ENV; wal-g wal-fetch "%f" "%p"'
> 
> And it is possible to change the real WAL source in ENV script without changing the restore_command. We can't track
this,so I not see new issues here.
 
> 
>>>   Sergey, could you please attach this thread to the upcoming CF, if
>>>   you're going to continue working on it.
> 
> Sure, I created one: https://commitfest.postgresql.org/30/2802/

+1 to mark restore_command as PGC_SIGHUP.

Currently when restore_command is not set, archive recovery fails
at the beginning. With the patch, how should we treat the case where
retore_command is reset to empty during archive recovery? We should
reject that change of restore_command?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

> Currently when restore_command is not set, archive recovery fails
> at the beginning. With the patch, how should we treat the case where
> retore_command is reset to empty during archive recovery? We should
> reject that change of restore_command?

Good point. I think we should reject that change. But (AFAIC) I cannot use GUC check callback for this purpose, as only
thestartup process knows StandbyModeRequested. I think it would be appropriate to call validateRecoveryParameters from
StartupRereadConfig.As side effect this add warning/hint "specified neither primary_conninfo nor restore_command" in
standbymode in appropriate configuration state. Not sure about the rest checks in validateRecoveryParameters, maybe
it'sa wrong idea to recheck them here and I need to separate these checks into another function.
 

regards, Sergei
Attachment

Re: Allow some recovery parameters to be changed with reload

From
Fujii Masao
Date:

On 2020/11/06 21:36, Sergei Kornilov wrote:
> Hello
> 
>> Currently when restore_command is not set, archive recovery fails
>> at the beginning. With the patch, how should we treat the case where
>> retore_command is reset to empty during archive recovery? We should
>> reject that change of restore_command?
> 
> Good point. I think we should reject that change. But (AFAIC) I cannot use GUC check callback for this purpose, as
onlythe startup process knows StandbyModeRequested. I think it would be appropriate to call validateRecoveryParameters
fromStartupRereadConfig.
 

I don't think this idea is ok because emptying restore_command and the reload
of configuration file could cause the server doing archive recovery to
shut down with FATAL error.

I'm wondering if it's safe to allow restore_command to be emptied during
archive recovery. Even when it's emptied, archive recovery can proceed
by reading WAL files from pg_wal directory. This is the same behavior as
when restore_command is set to, e.g., /bin/false. So maybe we don't need
to treat the empty restore_command so special??

OTOH, we should not remove the check of restore_command in
validateRecoveryParameters(). Otherwise, when users forget to specify
restore_command when starting archive recovery, recovery could
wrongly proceed and the database could get corrupted.

Regards,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

> I'm wondering if it's safe to allow restore_command to be emptied during
> archive recovery. Even when it's emptied, archive recovery can proceed
> by reading WAL files from pg_wal directory. This is the same behavior as
> when restore_command is set to, e.g., /bin/false.

I am always confused by this implementation detail. restore_command fails? Fine, let's just read file from pg_wal. But
thisis different topic...
 

I do not know the history of this fatal ereport. It looks like "must specify restore_command when standby mode is not
enabled"check is only intended to protect the user from misconfiguration and the rest code will treat empty
restore_commandcorrectly, just like /bin/false. Did not notice anything around StandbyMode conditions.
 

regards, Sergei



Re: Allow some recovery parameters to be changed with reload

From
Kyotaro Horiguchi
Date:
At Sat, 07 Nov 2020 00:36:33 +0300, Sergei Kornilov <sk@zsrv.org> wrote in 
> Hello
> 
> > I'm wondering if it's safe to allow restore_command to be emptied during
> > archive recovery. Even when it's emptied, archive recovery can proceed
> > by reading WAL files from pg_wal directory. This is the same behavior as
> > when restore_command is set to, e.g., /bin/false.
> 
> I am always confused by this implementation detail. restore_command fails? Fine, let's just read file from pg_wal.
Butthis is different topic...
 

--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -114,6 +114,11 @@ StartupRereadConfig(void)
 
     if (conninfoChanged || slotnameChanged || tempSlotChanged)
         StartupRequestWalReceiverRestart();
+
+    /*
+     * Check the combination of new parameters
+     */
+    validateRecoveryParameters();

If someone changes restore_command to '' then reload while crash
recovery is running, the server stops for no valid reason. If
restore_command is set to 'hoge' (literally:p, that is, anything
unexecutable) and send SIGHUP while archive recovery is running, the
server stops.  I think we need to handle these cases more gracefully,
I think. That said, I think we should keep the current behavior that
the server stops if the same happens just after server start.

If someone changes restore_command by mistake to something executable
but fails to offer the specfied file even if it exists, the running
archive recovery finishes then switches timeline unexpectedly.  With
the same reasoning to the discussion abou inexecutable contents just
above, that behavior seems valid when the variable has not changed
since startup, but I'm not sure what to do if that happens by a reload
while (archive|crash) recovery is proceeding.

> I do not know the history of this fatal ereport. It looks like "must specify restore_command when standby mode is not
enabled"check is only intended to protect the user from misconfiguration and the rest code will treat empty
restore_commandcorrectly, just like /bin/false. Did not notice anything around StandbyMode conditions.
 

If restore_command is not changable after server-start, it would be
valid for startup to stop for inexecutable content for the variable
since there's no way to proceed recovery.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

> If someone changes restore_command to '' then reload while crash
> recovery is running, the server stops for no valid reason.

While *crash* recovery is running? It's possible only during Point-in-Time Recovery, no?
At the beginning of validateRecoveryParameters we check ArchiveRecoveryRequested, which can be set in two cases:

* if recovery.signal found - same check on recovery start. Otherwise it is possible to early end recovery because of
emptyrestore_command. So we want to protect the user from such misconfiguration? I am fine if we decide that no
additionalhandling is needed.
 
* if standby.signal found - this FATAL is not reachable because StandbyModeRequested is also set.

During crash recovery validateRecoveryParameters does nothing.

> If restore_command is set to 'hoge' (literally:p, that is, anything
> unexecutable) and send SIGHUP while archive recovery is running, the
> server stops. I think we need to handle these cases more gracefully,

I think we can not perform such check reliable. As in my example earlier:

> restore_command = '. /etc/wal-g/WALG_AWS_ENV; wal-g wal-fetch "%f" "%p"'

How do we find the commands first? For any shell? And even: we learned that the binary is unexecutable. But what to do
next?

> If someone changes restore_command by mistake to something executable
> but fails to offer the specfied file even if it exists, the running
> archive recovery finishes then switches timeline unexpectedly.

Or executable file was just removed. Which is clearly a pilot error. Is this differs from changing restore_command?

>>  I do not know the history of this fatal ereport. It looks like "must specify restore_command when standby mode is
notenabled" check is only intended to protect the user from misconfiguration and the rest code will treat empty
restore_commandcorrectly, just like /bin/false. Did not notice anything around StandbyMode conditions.
 
>
> If restore_command is not changable after server-start, it would be
> valid for startup to stop for inexecutable content for the variable
> since there's no way to proceed recovery.

Why not use local pg_wal? There may be already enough WAL.

regards, Sergei



Re: Allow some recovery parameters to be changed with reload

From
Kyotaro Horiguchi
Date:
Hello.

At Tue, 10 Nov 2020 14:13:17 +0300, Sergei Kornilov <sk@zsrv.org> wrote in
> Hello
>
> > If someone changes restore_command to '' then reload while crash
> > recovery is running, the server stops for no valid reason.
>
> While *crash* recovery is running? It's possible only during Point-in-Time Recovery, no?

Even if PITR is commanded, crash recovery can run before starting
archive recovery if the server was not gracefully shut down.
Parameter reload can happen while crash recovery. And
validateRecoveryParameters() calls "ereport(FATAL" in that case.

> At the beginning of validateRecoveryParameters we check ArchiveRecoveryRequested, which can be set in two cases:

That does not prevent crash recovery from running.

> * if recovery.signal found - same check on recovery start. Otherwise it is possible to early end recovery because of
emptyrestore_command. So we want to protect the user from such misconfiguration? I am fine if we decide that no
additionalhandling is needed. 
> * if standby.signal found - this FATAL is not reachable because StandbyModeRequested is also set.
>
> During crash recovery validateRecoveryParameters does nothing.



> > If restore_command is set to 'hoge' (literally:p, that is, anything
> > unexecutable) and send SIGHUP while archive recovery is running, the
> > server stops. I think we need to handle these cases more gracefully,
>
> I think we can not perform such check reliable. As in my example earlier:
>
> > restore_command = '. /etc/wal-g/WALG_AWS_ENV; wal-g wal-fetch "%f" "%p"'
>
> How do we find the commands first? For any shell? And even: we learned that the binary is unexecutable. But what to
donext? 

I don't suggest to check if the command actually works, I suggested to
avoid server stop even if the parameters failed to run after a
config-reload.

> > If someone changes restore_command by mistake to something executable
> > but fails to offer the specfied file even if it exists, the running
> > archive recovery finishes then switches timeline unexpectedly.
>
> Or executable file was just removed. Which is clearly a pilot error. Is this differs from changing restore_command?

I don't know. I just think that it is not proper that "ALTER SYSTEM" +
config-reload causes server stop.

> >>  I do not know the history of this fatal ereport. It looks like "must specify restore_command when standby mode is
notenabled" check is only intended to protect the user from misconfiguration and the rest code will treat empty
restore_commandcorrectly, just like /bin/false. Did not notice anything around StandbyMode conditions. 
> >
> > If restore_command is not changable after server-start, it would be
> > valid for startup to stop for inexecutable content for the variable
> > since there's no way to proceed recovery.
>
> Why not use local pg_wal? There may be already enough WAL.

Mmm. If the file to read is in pg_wal, restore_command won't be
executed in the first place?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

> Even if PITR is commanded, crash recovery can run before starting
> archive recovery if the server was not gracefully shut down.

Hmm... Still not sure how it's possible. Both readRecoverySignalFile and validateRecoveryParameters are called early in
StartupXLOG.If PITR was commanded - we follow PITR logic. If requested recovery stop point is before consistent
recoverypoint we shutdown the database with another FATAL.
 
I mean such place:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/transam/xlog.c;h=9d3f1c12fc56f61da4d2b9bf08c54d31b9757ef7;hb=29be9983a64c011eac0b9ee29895cce71e15ea77#l6891
If we start recovery by any reason and a archive recovery was requested - we start archive recovery instead of crash
recovery.

> I don't know. I just think that it is not proper that "ALTER SYSTEM" +
> config-reload causes server stop.

I got your point. How about pause the recovery process? Like proposed in https://commitfest.postgresql.org/30/2489/
For example,
* restore_command become empty on SIGHUP while PITR was requested
* we set recovery to pause
* if user call pg_wal_replay_resume and restore_command is still empty - we shutdown the database
* if user fix restore_command - we continue restore.

But it seems complicated if we just don't need special handling here. We still require restore_command to be set to
startrecovery. In case the user later wants to set the restore_command to empty - let's assume that's correct (FATAL if
PITRtarget is after the end of local pg_wal, promote otherwise).
 

>>  Why not use local pg_wal? There may be already enough WAL.
>
> Mmm. If the file to read is in pg_wal, restore_command won't be
> executed in the first place?

Startup process will call restore_command in any case regardless of pg_wal content. (xlogarchive.c,
RestoreArchivedFile)

> * When doing archive recovery, we always prefer an archived log file even
> * if a file of the same name exists in XLOGDIR.  The reason is that the
> * file in XLOGDIR could be an old, un-filled or partly-filled version
> * that was copied and restored as part of backing up $PGDATA.

regards, Sergei



Re: Allow some recovery parameters to be changed with reload

From
Fujii Masao
Date:

On 2020/11/07 6:36, Sergei Kornilov wrote:
> Hello
> 
>> I'm wondering if it's safe to allow restore_command to be emptied during
>> archive recovery. Even when it's emptied, archive recovery can proceed
>> by reading WAL files from pg_wal directory. This is the same behavior as
>> when restore_command is set to, e.g., /bin/false.
> 
> I am always confused by this implementation detail. restore_command fails? Fine, let's just read file from pg_wal.
Butthis is different topic...
 
> 
> I do not know the history of this fatal ereport. It looks like "must specify restore_command when standby mode is not
enabled"check is only intended to protect the user from misconfiguration and the rest code will treat empty
restore_commandcorrectly, just like /bin/false.
 

Maybe.

Anyway, for now I think that your first patch would be enough, i.e.,
just change the context of restore_command to PGC_SIGHUP.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

> Anyway, for now I think that your first patch would be enough, i.e.,
> just change the context of restore_command to PGC_SIGHUP.

Glad to hear. Attached a rebased version of the original proposal.

regards, Sergei
Attachment

Re: Allow some recovery parameters to be changed with reload

From
Fujii Masao
Date:

On 2020/11/12 4:38, Sergei Kornilov wrote:
> Hello
> 
>> Anyway, for now I think that your first patch would be enough, i.e.,
>> just change the context of restore_command to PGC_SIGHUP.
> 
> Glad to hear. Attached a rebased version of the original proposal.

Thanks for rebasing the patch!

     This parameter is required for archive recovery,

I found the above description in config.sgml. I was just wondering
if it should be updated so that the actual specification is described or not.
The actual spec is that restore_command is required to start archive
recovery, but optional (i.e., the parameter can be reset to an empty)
after archive recovery has started. But this updated version of
description would be rather confusing to users. So I'm now thinking
not to update that.

Does anyone object to the patch? If no, I'm thinking to commit the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: Allow some recovery parameters to be changed with reload

From
Kyotaro Horiguchi
Date:
At Thu, 26 Nov 2020 22:43:48 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2020/11/12 4:38, Sergei Kornilov wrote:
> > Hello
> > 
> >> Anyway, for now I think that your first patch would be enough, i.e.,
> >> just change the context of restore_command to PGC_SIGHUP.
> > Glad to hear. Attached a rebased version of the original proposal.
> 
> Thanks for rebasing the patch!
> 
>     This parameter is required for archive recovery,
> 
> I found the above description in config.sgml. I was just wondering
> if it should be updated so that the actual specification is described
> or not.
> The actual spec is that restore_command is required to start archive
> recovery, but optional (i.e., the parameter can be reset to an empty)
> after archive recovery has started. But this updated version of
> description would be rather confusing to users. So I'm now thinking
> not to update that.
> 
> Does anyone object to the patch? If no, I'm thinking to commit the
> patch.

Although I don't object to make the parameter reloadable, I think it
needs to be documented that server could stop after reloading if the
server failed to execute the new command line.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Allow some recovery parameters to be changed with reload

From
Fujii Masao
Date:

On 2020/11/27 9:30, Kyotaro Horiguchi wrote:
> At Thu, 26 Nov 2020 22:43:48 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>
>>
>> On 2020/11/12 4:38, Sergei Kornilov wrote:
>>> Hello
>>>
>>>> Anyway, for now I think that your first patch would be enough, i.e.,
>>>> just change the context of restore_command to PGC_SIGHUP.
>>> Glad to hear. Attached a rebased version of the original proposal.
>>
>> Thanks for rebasing the patch!
>>
>>      This parameter is required for archive recovery,
>>
>> I found the above description in config.sgml. I was just wondering
>> if it should be updated so that the actual specification is described
>> or not.
>> The actual spec is that restore_command is required to start archive
>> recovery, but optional (i.e., the parameter can be reset to an empty)
>> after archive recovery has started. But this updated version of
>> description would be rather confusing to users. So I'm now thinking
>> not to update that.
>>
>> Does anyone object to the patch? If no, I'm thinking to commit the
>> patch.
> 
> Although I don't object to make the parameter reloadable, I think it
> needs to be documented that server could stop after reloading if the
> server failed to execute the new command line.

You mean that we should document that if restore_command is set to improper command mistakenly, archive recovery may
failto restore some archived WAL files and finish without replaying those WAL? But isn't this true even without
applyingthe patch?
 

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: Allow some recovery parameters to be changed with reload

From
Kyotaro Horiguchi
Date:
At Fri, 27 Nov 2020 09:48:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2020/11/27 9:30, Kyotaro Horiguchi wrote:
> > At Thu, 26 Nov 2020 22:43:48 +0900, Fujii Masao
> > <masao.fujii@oss.nttdata.com> wrote in
> >>
> >>
> >> On 2020/11/12 4:38, Sergei Kornilov wrote:
> >>> Hello
> >>>
> >>>> Anyway, for now I think that your first patch would be enough, i.e.,
> >>>> just change the context of restore_command to PGC_SIGHUP.
> >>> Glad to hear. Attached a rebased version of the original proposal.
> >>
> >> Thanks for rebasing the patch!
> >>
> >>      This parameter is required for archive recovery,
> >>
> >> I found the above description in config.sgml. I was just wondering
> >> if it should be updated so that the actual specification is described
> >> or not.
> >> The actual spec is that restore_command is required to start archive
> >> recovery, but optional (i.e., the parameter can be reset to an empty)
> >> after archive recovery has started. But this updated version of
> >> description would be rather confusing to users. So I'm now thinking
> >> not to update that.
> >>
> >> Does anyone object to the patch? If no, I'm thinking to commit the
> >> patch.
> > Although I don't object to make the parameter reloadable, I think it
> > needs to be documented that server could stop after reloading if the
> > server failed to execute the new command line.
> 
> You mean that we should document that if restore_command is set to
> improper command mistakenly, archive recovery may fail to restore some
> archived WAL files and finish without replaying those WAL? But isn't
> this true even without applying the patch?

If we set a wrong command in .conf and start the server in recovery
mode, the server immediately stops. It's fine.  If we change
restore_command wrong way on a running server, "pg_ctl reload" stops
the server.  If it is a hot standby, the server stops unexpectedly.

However, after rechecking, I found that recovery_end_command with
wrong content causes server stop at the end of recovery, or at
promotion. And that variable is PGC_SIGHUP.

So I agree not to document that.  Sorry for the noise.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Allow some recovery parameters to be changed with reload

From
Fujii Masao
Date:

On 2020/11/27 12:05, Kyotaro Horiguchi wrote:
> At Fri, 27 Nov 2020 09:48:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>
>>
>> On 2020/11/27 9:30, Kyotaro Horiguchi wrote:
>>> At Thu, 26 Nov 2020 22:43:48 +0900, Fujii Masao
>>> <masao.fujii@oss.nttdata.com> wrote in
>>>>
>>>>
>>>> On 2020/11/12 4:38, Sergei Kornilov wrote:
>>>>> Hello
>>>>>
>>>>>> Anyway, for now I think that your first patch would be enough, i.e.,
>>>>>> just change the context of restore_command to PGC_SIGHUP.
>>>>> Glad to hear. Attached a rebased version of the original proposal.
>>>>
>>>> Thanks for rebasing the patch!
>>>>
>>>>       This parameter is required for archive recovery,
>>>>
>>>> I found the above description in config.sgml. I was just wondering
>>>> if it should be updated so that the actual specification is described
>>>> or not.
>>>> The actual spec is that restore_command is required to start archive
>>>> recovery, but optional (i.e., the parameter can be reset to an empty)
>>>> after archive recovery has started. But this updated version of
>>>> description would be rather confusing to users. So I'm now thinking
>>>> not to update that.
>>>>
>>>> Does anyone object to the patch? If no, I'm thinking to commit the
>>>> patch.
>>> Although I don't object to make the parameter reloadable, I think it
>>> needs to be documented that server could stop after reloading if the
>>> server failed to execute the new command line.
>>
>> You mean that we should document that if restore_command is set to
>> improper command mistakenly, archive recovery may fail to restore some
>> archived WAL files and finish without replaying those WAL? But isn't
>> this true even without applying the patch?
> 
> If we set a wrong command in .conf and start the server in recovery
> mode, the server immediately stops. It's fine.  If we change
> restore_command wrong way on a running server, "pg_ctl reload" stops
> the server.  If it is a hot standby, the server stops unexpectedly.
> 
> However, after rechecking, I found that recovery_end_command with
> wrong content causes server stop at the end of recovery, or at
> promotion. And that variable is PGC_SIGHUP.
> 
> So I agree not to document that.  Sorry for the noise.

OK, so I pushed the patch. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: Allow some recovery parameters to be changed with reload

From
Sergei Kornilov
Date:
Hello

> OK, so I pushed the patch. Thanks!

Thank you!

regards, Sergei