Re: Allow escape in application_name

On 2021/11/08 22:40, kuroda.hayato@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