On Wed, Feb 01, 2023 at 09:34:44PM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Hmm. Isn't that something that we should also document in startup.c
>> where both routines are defined? If we begin to use
>> PreRestoreCommand() and PostRestoreCommand() in more code paths in the
>> future, that could be again an issue.
>
> I was vaguely wondering about removing both of those functions
> in favor of an integrated function that does a system() call
> with those things before and after it.
It seems to me that this is pretty much the same as storing
in_restore_command in shell_restore.c, and that for recovery modules
this comes down to the addition of an extra callback called in
startup.c to check if the flag is up or not. Now the patch is doing
things the opposite way: like on HEAD, store the flag in startup.c but
switch it at will with the routines in startup.c. I find the approach
of the patch a bit more intuitive, TBH, as that makes the interface
simpler for other recovery modules that may want to switch the flag
back-and-forth, and I suspect that there may be cases in recovery
modules where we'd still want to switch the flag, but not necessarily
link it to system().
--
Michael