At Tue, 7 Sep 2021 11:30:27 +0000, "kuroda.hay...@fujitsu.com" <kuroda.hay...@fujitsu.com> wrote in > I attached the rebased version. Tests and descriptions were added. > In my understanding Ikeda-san's indication is included.
I have some comments by a quick look. + * one have higher priority. See also the previous comment. Is "the previous comment" "the comment above"? + for (i = n -1; i >= 0; i--) You might want a space between - and 1. +parse_application_name(StringInfo buf, const char *name) The name seems a bit too generic as it is a function only for postgres_fdw. + /* must be a '%', so skip to the next char */ + p++; + if (*p == '\0') + break; /* format error - ignore it */ I'm surprised by finding that undefined %-escapes and stray % behave differently between archive_command and log_line_prefix. I understand this behaves like the latter. + const char *username = MyProcPort->user_name; I'm not sure but even if user_name doesn't seem to be NULL, don't we want to do the same thing with %u of log_line_prefix for safety? Namely, setting [unknown] if user_name is NULL or "". The same can be said for %d. + * process_padding --- helper function for processing the format + * string in log_line_prefix Since this is no longer a static helper function for a specific function, the name and the comment should be more descriptive. That being said, in the first place the function seems reducible almost to a call to atol. By a quick measurement the function is about 38% faster (0.024us/call(the function) vs 0.039us/call(strtol) so I'm not sure we want to replace process_log_prefix_padding(), but we don't need to reuse the function in parse_application_name since it is called only once per connection. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
