Re: Common function for percent placeholder replacement

On 04.01.23 01:37, Nathan Bossart wrote:
> In general, +1.
> 
> On Tue, Dec 20, 2022 at 06:30:40AM +0100, Peter Eisentraut wrote:
>> (Still need to think about Robert's comment about lack of error context.)
> 
> Would adding the name of the GUC be sufficient?
> 
>     ereport(ERROR,
>             (errmsg("could not build %s", guc_name),
>              errdetail("string ends unexpectedly after escape character \"%%\"")));

done

The errors now basically look like an invalid GUC value.

>> + * 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.)