Dear Horiguchi-san,
Thank you for reviewing! I attached the fixed version.
> Is "the previous comment" "the comment above"?
Yeah, fixed.
> + for (i = n -1; i >= 0; i--)
>
> You might want a space between - and 1.
Fixed.
> +parse_application_name(StringInfo buf, const char *name)
>
> The name seems a bit too generic as it is a function only for
> postgres_fdw.
Indeed. I renamed to parse_pgfdw_appname().
> + /* 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.
Indeed. pgarch_archiveXlog() treats undefined escapes as nothing special,
but log_line_prefix() stop parsing immediately.
They have no description about it in docs.
I will not treat it in this thread and follow log_line_prefix(),
but I agree it is strange.
> + 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.
I think they will be never NULL in current implementation,
but your suggestion is better. Checks were added in %a, %u and %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.
My first impression is that they use the function
because they want to know the end of sequence and do error-handling,
but I agree this can be replaced by strtol().
I changed the function to strtol() and restored process_log_prefix_padding().
How do you think? Does it follow your suggestion?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED