At Tue, 7 Sep 2021 11:30:27 +0000, "kuroda.hayato@fujitsu.com" <kuroda.hayato@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