Allow some recovery parameters to be changed with reload

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

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

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

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

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

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

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

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

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

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
/messages/by-id/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

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
/messages/by-id/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]
/messages/by-id/601EE1F5-0B78-47E1-9AAE-C15F74A1C21D@postgrespro.ru

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

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
/messages/by-id/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]
/messages/by-id/601EE1F5-0B78-47E1-9AAE-C15F74A1C21D@postgrespro.ru

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

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 some script:

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 separate own 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 being changed? 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

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 some script:

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 separate own 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

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 the startup 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 standby mode in appropriate configuration state. Not sure about the rest checks in validateRecoveryParameters, maybe it's a wrong idea to recheck them here and I need to separate these checks into another function.

regards, Sergei

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 only the startup process knows StandbyModeRequested. I think it would be appropriate to call validateRecoveryParameters from StartupRereadConfig.

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

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 this 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_command correctly, just like /bin/false. Did not notice anything around StandbyMode conditions.

regards, Sergei

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. But this 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_command correctly, 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

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 empty restore_command. So we want to protect the user from such misconfiguration? I am fine if we decide that no additional handling 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 not enabled" check is only intended to protect the user from misconfiguration and the rest code will treat empty restore_command correctly, 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