Re: Weird failure with latches in curculio on v15

Hi,

On 2023-02-05 14:19:38 -0800, Nathan Bossart wrote:
> On Sun, Feb 05, 2023 at 09:49:57AM +0900, Michael Paquier wrote:
> > - Should we include archive_cleanup_command into the recovery modules
> > at all? We've discussed offloading that from the checkpointer, and it
> > makes the failure handling trickier when it comes to unexpected GUC
> > configurations, for one. The same may actually apply to
> > restore_end_command. Though it is done in the startup process now,
> > there may be an argument to offload that somewhere else based on the
> > timing of the end-of-recovery checkpoint. My opinion on this stuff is
> > that only including restore_command in the modules would make most
> > users I know of happy enough as it removes the overhead of the command
> > invocation from the startup process, if able to replay things fast
> > enough so as the restore command is the bottleneck.
> > restore_end_command would be simple enough, but if there is a wish to
> > redesign the startup process to offload it somewhere else, then the
> > recovery module makes backward-compatibility concerns harder to think
> > about in the long-term.
>
> I agree. I think we ought to first focus on getting the recovery modules
> interface and restore_command functionality in place before we take on more
> difficult things like archive_cleanup_command. But I still think the
> archive_cleanup_command/recovery_end_command functionality should
> eventually be added to recovery modules.

I tend not to agree. If you make the API that small, you're IME likely
to end up with something that looks somewhat incoherent once extended.

The more I think about it, the less I am convinced that
one-callback-per-segment, invoked just before needing the file, is the
right approach to address the performance issues of restore_commmand.

The main performance issue isn't the shell invocation overhead, it's
synchronously needing to restore the archive, before replay can
continue. It's also gonna be slow if a restore module copies the segment
from a remote system - the latency is the problem.

The only way the restore module approach can do better, is to
asynchronously restore ahead of the current segment. But for that the
API really isn't suited well. The signature of the relevant callback is:

> +typedef bool (*RecoveryRestoreCB) (const char *file, const char *path,
> + const char *lastRestartPointFileName);

That's not very suited to restoring "ahead of time". You need to parse
file to figure out whether a segment or something else is restored, turn
"file" back into an LSN, figure out where to to store further segments,
somehow hand off to some background worker, etc.

That doesn't strike me as something we want to happen inside multiple
restore libraries.

I think at the very least you'd want to have a separate callback for
restoring segments than for restoring other files. But more likely a
separate callback for each type of file to be restored.

For the timeline history case an parameter indicating that we don't want
to restore the file, just to see if there's a conflict, would make
sense.

For the segment files, we'd likely need a parameter to indicate whether
the restore is random or not.

Greetings,

Andres Freund