Re: Common function for percent placeholder replacement

On Mon, Jan 09, 2023 at 09:36:12AM +0100, Peter Eisentraut wrote:
> On 04.01.23 01:37, Nathan Bossart wrote:
>> On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote:
>> > + * A value may be NULL.  If the corresponding placeholder is found in the
>> > + * input string, the whole function returns NULL.
>> 
>> This appears to be carried over from BuildRestoreCommand(), and AFAICT it
>> is only necessary because pg_rewind doesn't support %r in restore_command.
>> IMHO this behavior is counterintuitive and possibly error-prone and should
>> result in an ERROR instead.  Since pg_rewind is the only special case, it
>> could independently check for %r before building the command.
> 
> Yeah, this annoyed me, too.  I have now changed it so that a NULL "value" is
> the same as an unsupported placeholder.  This preserves the existing
> behavior.
> 
> (Having pg_rewind check for %r itself would probably require replicating
> most of the string processing logic (consider something like "%%r"), so it
> didn't seem appealing.)

Sounds good to me.

> +        nativePath = pstrdup(path);
> +        make_native_path(nativePath);

> +        nativePath = pstrdup(xlogpath);
> +        make_native_path(nativePath);

Should these be freed?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com