Re: Allow escape in application_name

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