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