From: | "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Fujii Masao' <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, 'Kyotaro Horiguchi' <horikyota(dot)ntt(at)gmail(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "ikedamsh(at)oss(dot)nttdata(dot)com" <ikedamsh(at)oss(dot)nttdata(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | RE: Allow escape in application_name |
Date: | 2021-12-07 08:55:59 |
Message-ID: | TYAPR01MB58665204ACC2870BC7364EC9F56E9@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Fujii-san,
Thank you for reviewing! I'll post the latest version.
> 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.
I refactored that. Thanks!
> + 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.
Prefer yours. Fixed.
> + if (strcmp(buf->data, "") != 0)
>
> Is this condition the same as "data[0] == '\0'"?
Maybe "data[0] != '\0'", but fixed.
> + * 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.
> -----------------------------
Thank you for suggestions. I confused the word "parse."
Fixed and renamed.
> + 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.
I missed there. Fixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | vignesh C | 2021-12-07 09:05:05 | Re: Fix a bug in DecodeAbort() and improve input data check on subscriber. |
Previous Message | Bharath Rupireddy | 2021-12-07 08:27:56 | Re: Pre-allocating WAL files |