On 2021/11/08 22:40, kuroda.hay...@fujitsu.com wrote:
Attached patches are the latest version.
Thanks for updating the patch! + buf = makeStringInfo(); This is necessary only when application_name is set. So it's better to avoid this if appname is not set. Currently StringInfo variable is defined in connect_pg_server() and passed to parse_pgfdw_appname(). But there is no strong reason why the variable needs to be defined connect_pg_server(). Right? If so how about refactoring parse_pgfdw_appname() so that it defines StringInfoData variable and returns the processed character string? You can see such coding at, for example, escape_param_str() in dblink.c. If this refactoring is done, probably we can get rid of "#include "lib/stringinfo.h"" line from connection.c because connect_pg_server() no longer needs to use StringInfo. + int i; <snip> + for (i = n - 1; i >= 0; i--) I'm tempted to simplify these into "for (int i = n - 1; i >= 0; i--)". + /* + * Search application_name and replace it if found. + * + * We search paramters from the end because the later + * one have higher priority. See also the above comment. + */ How about updating these comments into the following? ----------------------- Search the parameter arrays to find application_name setting, and replace escape sequences in it with status information if found. The arrays are searched backwards because the last value is used if application_name is repeatedly set. ----------------------- + if (strcmp(buf->data, "") != 0) Is this condition the same as "data[0] == '\0'"? + * parse postgres_fdw.application_name and set escaped string. + * This function is almost same as log_line_prefix(), but + * accepted escape sequence is different and this cannot understand + * padding integer. + * + * Note that argument buf must be initialized. How about updating these comments into the following? I thought that using the term "parse" was a bit confusing because what the function actually does is to replace escape seequences in application_name. Probably it's also better to rename the function, e.g., to process_pgfdw_appname . ----------------------------- Replace escape sequences beginning with % character in the given application_name with status information, and return it. ----------------------------- + if (appname == NULL) + appname = "[unknown]"; + if (username == NULL || *username == '\0') + username = "[unknown]"; + if (dbname == NULL || *dbname == '\0') + dbname = "[unknown]"; I'm still not sure why these are necessary. Could you clarify that? + Same as <xref linkend="guc-log-line-prefix"/>, this is a + <function>printf</function>-style string. Accepted escapes are + bit different from <xref linkend="guc-log-line-prefix"/>, + but padding can be used like as it. This description needs to be updated. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
