On Thu, Jan 12, 2023 at 10:17:21AM -0800, Nathan Bossart wrote: > On Thu, Jan 12, 2023 at 03:30:40PM +0900, Michael Paquier wrote: > IMHO I don't think there's an urgent need to rename it, but if there's a > better name that people like, I'm happy to do so.
Okay. >> Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with >> the duplication for the segment name build), so I would be tempted to >> get this one done. My gut tells me that we'd better remove the >> duplication and just pass down the two fields to >> shell_archive_cleanup() and shell_recovery_end(), with the segment >> name given to ExecuteRecoveryCommand().. > > I moved the duplicated logic to its own function in v6. While looking at 0001, I have noticed one issue as of the following block in shell_restore(): + if (wait_result_is_signal(rc, SIGTERM)) + proc_exit(1); + + ereport(wait_result_is_any_signal(rc, true) ? FATAL : DEBUG2, + (errmsg("could not restore file \"%s\" from archive: %s", + file, wait_result_to_str(rc)))); This block of code would have been executed even if rc == 0, which is incorrect because the command would have succeeded. HEAD would not have done that, actually, as RestoreArchivedFile() would return before. I guess that you have not noticed it because this basically just generated incorrect DEBUG2 messages when rc == 0? One part that this slightly influences is the order of the reports when the command succeeds the follow-up stat() fails to check the size, where we reverse these two logs: DEBUG2, "could not restore" LOG/FATAL, "could not stat file" However, that does not really change the value of the information reported: if the stat() failed, the failure mode is the same except that we don't get the extra DEBUG2/"could not restore" message, which does not really matter except if your elevel is high enough for that and there is always the LOG for that.. Once this issue was fixed, nothing else stood out, so applied this part. -- Michael
signature.asc
Description: PGP signature