Re: Allow escape in application_name

At Sat, 4 Dec 2021 00:07:05 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> 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'"?

FWIW, I agree to these.

> + * 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 .

It is consistent to how we have described the similar features.

> -----------------------------
> 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?

It probably comes from my request, just for safety for uncertain
future.  They are actually not needed under the current usage of the
function, so *I* would not object to removing them if you are strongly
feel them out of place.  In that case, since NULL's leads to SEGV
outright, we would even not need assertions instead.

> +      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.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center