Dear Fujii-san,
Thanks for comments!
> >> Thanks for the new version. I don't object for reusing
> >> process_log_prefix_padding, but I still find it strange that the
> >> function with the name 'process_padding' is in string.c. If we move
> >> it to string.c, I'd like to name it "pg_fast_strtoi" or something and
> >> change the interface to int pg_fast_strtoi(const char *p, char
> >> **endptr) that is (roughly) compatible to strtol. What do (both) you
> >> think about this?
> >
> > I agree that this interface might be confused.
> > I changed its name and interface. How do you think?
> > Actually I cannot distinguish the name is good or not,
> > but I could not think of anything else...
>
> The name using the word "strtoint" sounds confusing to me
> because the behavior of the function is different from strtoint() or
> pg_strtoint32(), etc. Otherwise we can easily misunderstand that
> pg_fast_strtoint() can be used as alternative of strtoint() or
> pg_strtoint32(). I have no better idea for the name, though..
you mean that this is not strtoXXX, right? If so we should
go back to process_padding().... Horiguchi-san, do you have any idea?
And I added pg_restrict keywords for compiler optimizations.
> >> I didn't fully checked in what case parse_pgfdw_appname gives "" as
> >> result, I feel that we should use the original value in that
> >> case. That is,
> >>
> >>> parse_pgfdw_appname(&buf, vaues[i]);
> >>>
> >>> /* use the result if any, otherwise use the original string */
> >>> if (buf.data[0] != 0)
> >>> values[i] = buf.data;
> >>>
> >>> /* break if it results in non-empty string */
> >>> if (values[0][0] != 0)
> >>> break;
>
> Agreed. It's strange to use the application_name of the server
> object in that case. There seems to be four options:
>
> (1) Use the original postgres_fdw.application_name like "%b"
> (2) Use the application_name of the server object (if set)
> (3) Use fallback_application_name
> (4) Use empty string as application_name
>
> (2) and (3) look strange to me because we expect that
> postgres_fdw.application_name should override application_name
> of the server object and fallback_application_mame.
>
> Also reporting invalid escape sequence in application_name as it is,
> i.e., (1), looks strange to me.
>
> So (4) looks most intuitive and similar behavior to log_line_prefix.
> Thought?
I agreed that (2) and (3) breaks the rule which should override server option.
Hence I chose (4), values[i] substituted to buf.data in any case.
Attached is the latest version.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED