Dear Fujii-san
Thank you for quick reviewing! I attached new ones.
> Regarding 0001 patch, IMO it's better to explain that
> postgres_fdw.application_name can be any string of any length and contain even
> non-ASCII characters, unlike application_name. So how about using the following
> description, instead?
>
> -----------------
> <varname>postgres_fdw.application_name</varname> can be any string of any
> length and contain even non-ASCII characters. However when it's passed to and
> used as <varname>application_name</varname> in a foreign server, note that it
> will be truncated to less than <symbol>NAMEDATALEN</symbol> characters
> and any characters other than printable ASCII ones in it will be replaced with
> question marks (<literal>?</literal>).
> -----------------
+1, Fixed.
> + int i;
> + for (i = n - 1; i >= 0; i--)
>
> As I told before, it's better to simplify this to "for (int i = n - 1; i >= 0; i--)".
Sorry, I missed. Fixed.
> Seems you removed the calls to pfree() from the patch. That's because the
> memory context used for the replaced application_name string will be released
> soon? Or it's better to call pfree()?
Because I used escape_param_str() and get_connect_string() as reference,
they did not release the memory. I reconsidered here, however, and I agreed
it is confusing that only keywords and values are pfree()'d.
I exported char* data and execute pfree() if it is used.
> + Same as <xref linkend="guc-log-line-prefix"/>, this is a
> + <function>printf</function>-style string. Unlike <xref
> linkend="guc-log-line-prefix"/>,
> + paddings are not allowed. Accepted escapes are as follows:
>
> Isn't it better to explain escape sequences in postgres_fdw.application_name
> more explicitly, as follows?
>
> -----------------
> <literal>%</literal> characters begin <quote>escape sequences</quote> that
> are replaced with status information as outlined below. Unrecognized escapes are
> ignored. Other characters are copied straight to the application name. Note that
> it's not allowed to specify a plus/minus sign or a numeric literal after the
> <literal>%</literal> and before the option, for alignment and padding.
> -----------------
Fixed.
> + <entry><literal>%u</literal></entry>
> + <entry>Local user name</entry>
>
> Average users can understand what "Local" here means?
Maybe not. I added descriptions and an example.
> + postgres_fdw.application_name is set to application_name of a pgfdw
> + connection after placeholder conversion, thus the resulting string is
> + subject to the same restrictions alreadby mentioned above.
>
> This description doesn't need to be added if 0001 patch is applied, is it? Because
> 0001 patch adds very similar description into the docs.
+1, removed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED