RE: Allow escape in application_name

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-09 10:52:38
Message-ID: TYAPR01MB5866D6DF6BDC20B90253E522F5709@TYAPR01MB5866.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Fujii-san

Thank you for quick reviewing! I attached new ones.

> Regarding 0001 patch, IMO it's better to explain that
> postgres_fdw.application_name can be any string of any length and contain even
> non-ASCII characters, unlike application_name. So how about using the following
> description, instead?
>
> -----------------
> <varname>postgres_fdw.application_name</varname> can be any string of any
> length and contain even non-ASCII characters. However when it's passed to and
> used as <varname>application_name</varname> in a foreign server, note that it
> will be truncated to less than <symbol>NAMEDATALEN</symbol> characters
> and any characters other than printable ASCII ones in it will be replaced with
> question marks (<literal>?</literal>).
> -----------------

+1, Fixed.

> + int i;
> + for (i = n - 1; i >= 0; i--)
>
> As I told before, it's better to simplify this to "for (int i = n - 1; i >= 0; i--)".

Sorry, I missed. Fixed.

> Seems you removed the calls to pfree() from the patch. That's because the
> memory context used for the replaced application_name string will be released
> soon? Or it's better to call pfree()?

Because I used escape_param_str() and get_connect_string() as reference,
they did not release the memory. I reconsidered here, however, and I agreed
it is confusing that only keywords and values are pfree()'d.
I exported char* data and execute pfree() if it is used.

> + Same as <xref linkend="guc-log-line-prefix"/>, this is a
> + <function>printf</function>-style string. Unlike <xref
> linkend="guc-log-line-prefix"/>,
> + paddings are not allowed. Accepted escapes are as follows:
>
> Isn't it better to explain escape sequences in postgres_fdw.application_name
> more explicitly, as follows?
>
> -----------------
> <literal>%</literal> characters begin <quote>escape sequences</quote> that
> are replaced with status information as outlined below. Unrecognized escapes are
> ignored. Other characters are copied straight to the application name. Note that
> it's not allowed to specify a plus/minus sign or a numeric literal after the
> <literal>%</literal> and before the option, for alignment and padding.
> -----------------

Fixed.

> + <entry><literal>%u</literal></entry>
> + <entry>Local user name</entry>
>
> Average users can understand what "Local" here means?

Maybe not. I added descriptions and an example.

> + postgres_fdw.application_name is set to application_name of a pgfdw
> + connection after placeholder conversion, thus the resulting string is
> + subject to the same restrictions alreadby mentioned above.
>
> This description doesn't need to be added if 0001 patch is applied, is it? Because
> 0001 patch adds very similar description into the docs.

+1, removed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Attachment Content-Type Size
v24_0002_allow_escapes.patch application/octet-stream 9.4 KB
v24_0001_update_descriptions.patch application/octet-stream 1.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2021-12-09 12:22:30 Re: parallel vacuum comments
Previous Message Amit Kapila 2021-12-09 10:51:55 Re: Data is copied twice when specifying both child and parent table in publication