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