On Mon, Mar 9, 2020 at 1:46 PM Michael Paquier <mich...@paquier.xyz> wrote: > > 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. > Yeah. If we're returning only -1 and 0, it's better to use a boolean. If we're planning to provide some information about which parameter is missing, a few negative integer values might be useful. But, I feel that's unnecessary in this case.
> 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? > That's a good suggestion. But, it's unlikely that a caller would pass something longer than MAXPGPATH and we indeed use that value a lot in the code. IMHO, it looks okay to me to have that assumption here as well. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
