Re: Allow escape in application_name

On 2021/11/08 22:40, kuroda(dot)hayato(at)fujitsu(dot)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