On Fri, Mar 06, 2020 at 05:22:16PM +0900, Michael Paquier wrote:
> Thanks for the suggestion. Avoiding dead code makes sense as well
> here. I'll think about this stuff a bit more first.
Okay, after pondering more on that, here is a first cut with a patch
refactoring restore_command build to src/common/. One thing I have
changed compared to the other versions is that BuildRestoreCommand()
now returns a boolean to tell if the command was successfully built or
not.
A second thing. As of now the interface of BuildRestoreCommand()
assumes that the resulting buffer has an allocation of MAXPGPATH.
This should be fine because that's an assumption we rely on a lot in
the code, be it frontend or backend. However, it could also be a trap
for any caller of this routine if they allocate something smaller.
Wouldn't it be cleaner to pass down the size of the result buffer
directly to BuildRestoreCommand() and use the length given by the
caller of the routine as a sanity check?
Any thoughts?
--
Michael