Dear Horiguchi-san,
Thank you for giving many comments! I attached new patches.
I'm sorry for the late reply.
> I think we don't have a predecessor of the case like this where a
> behavior is decided from object option and GUC.
>
> I'm a bit uncomfortable with .conf configuration overrides server
> options, but I also think in-session-set GUC should override server
> options. So, it's slightly against POLA but from the standpoint of
> usability +0.5 to that prioritization since I cannot come up with a
> better idea.
OK, I keep the current spec. Please tell me if you come up with something.
> I thought it is nice to share process_format_string but the function
> is too tightly coupled with ErrorData detail as you pointed off-list.
> So I came to think we cannot use the function from outside. It could
> be a possibility to make the function be just a skeleton that takes a
> list of pairs of an escaped character and the associated callback
> function but that is apparently too-much complex. (It would be worth
> doing if we share the same backbone processing with archive_command,
> restore_command, recover_end_command and so on, but that is
> necessarily accompanied by the need to change the behavior either
> log_line_prefix or others.)
I agree that combining them makes source too complex.
And we have an another problem about native language support.
Sometimes espaces becomes "[unkown]" string in log_line_prefix(),
and they are gettext()'s target. So if we combine log_line_prefix() and parse_pgfdw_appname(),
some non-ascii characters may be set to postgres_fdw.applicaiton_name.
Of cause we can implement callback function, but I think it is the next step.
> I personally don't care even if this feature implements
> padding-reading logic differently from process_format_string, but if
> we don't like that, I would return to suggest using strtol in both
> functions.
>
> As Fujii-san pointed upthread, strtol behaves a bit different way than
> we expect here, but that can be handled by checking the starting
> characters.
> if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
> {
> char *endptr;
> padding = strtol(p, &endptr, 10);
> if (p == endptr)
> break;
> p = endptr;
> }
> else
> padding = 0;
I removed the support function based on your suggestion,
but added some if-statement in order to treats the special case: '%-p'.
And I found and fixed another bug. If users set application_name
both in server option and GUC, concatenated string was set as appname.
This is caused because we reused same StringInfo and parsing all appname string
even if we sucsess it. Now the array search will stop if sucseed,
and in failure case do resetStringInfo() and re-search.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED