On 2021/12/09 19:52, kuroda.hay...@fujitsu.com wrote:
Dear Fujii-san
Thank you for quick reviewing! I attached new ones.
Thanks for updating the patches!
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.
Could you tell me why you added new paragraph into the middle of existing
paragraph? This change caused the following error when compiling the docs.
postgres-fdw.sgml:952: parser error : Opening and ending tag mismatch: listitem
line 934 and para
</para>
How about adding that new paragraph just after the existing one, instead?
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.
Understood.
+ /*
+ * The parsing result became an empty string,
+ * and that means other "application_name" will
be used
+ * for connecting to the remote server.
+ * So we must set values[i] to an empty string
+ * and search the array again.
+ */
+ values[i] = "";
Isn't pfree() necessary inside the loop also when data[0]=='\0' case? If so, probably
"data" would need to be set to NULL just after pfree(). Otherwise pfree()'d
"data" can be pfree()'d again at the end of connect_pg_server().
+ <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.
Thanks! But, since the term "local server" is already used in the docs, we can use
"the setting value of application_name in local server" etc?
I agree that it's good idea to add the example about how escape sequences are
replaced.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
