Re: recovery modules

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