On 2021/12/07 18:48, kuroda.hay...@fujitsu.com wrote:
Yeah, I followed your suggestion. But we deiced to keep codes clean, hence I removed the if-statements.
+1 because neither application_name, user_name nor database_name should be NULL for current usage. But if it's better to check whether they are NULL or not for some reasons or future usage, e.g., background worker not logging into any specified database may set application_name to '%d' and connect to a foreign server in the future, let's change the code later with comments. Thanks for updating the patch! 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>). ----------------- + 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--)". 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()? + 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. ----------------- + <entry><literal>%u</literal></entry> + <entry>Local user name</entry> Average users can understand what "Local" here means? + 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. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
