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
v11_0002_allow_escapes.patch
Description: v11_0002_allow_escapes.patch
