Thread: Allow some recovery parameters to be changed with reload
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
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
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
Attachment
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
Attachment
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
Attachment
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Hello > OK, so I pushed the patch. Thanks! Thank you! regards, Sergei