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

Dear Fujii-san, Horiguchi-san

I'm sorry for sending a bad patch...

> -------------------
> elog.c:2785:14: warning: expression which evaluates to zero treated as a null
> pointer constant of type 'char *' [-Wnon-literal-null-conversion]
> *endptr = '\0';
> ^~~~
> 1 warning generated.
> -------------------
>
> I got the above compiler warning.

Fixed. *endptr points (char *)p in any cases, that means
this parameter is invalid.

> + * Note: StringInfo datatype cannot be accepted
> + * because elog.h should not include postgres-original header file.
>
> How about moving the function to guc.c from elog.c because it's for
> the parameters, i.e., log_line_prefix and postgres_fdw.application_name?
> This allows us to use StringInfo in the function?

Yeah, StringInfo can be used in guc.c. Hence moved it.
Some variables and functions for timestamp became non-static function,
because they were used both normal logging and log_line_prefix.
I think their name is not too generic so I did not fix them.

> + parse_pgfdw_appname(buf, values[i]);
> + /*
> + * Note that appname may becomes an empty
> string
> + * if an input string has wrong format.
> + */
> + values[i] = *buf;
>
> If postgres_fdw.application_name contains only invalid escape characters like
> "%b", parse_pgfdw_appname() returns an empty string. We discussed
> there are four options to handle this case and we concluded (4) is better.
> Right? But ISTM that the patch uses (2).
>
> > (1) Use the original postgres_fdw.application_name like "%b"
> > (2) Use the application_name of the server object (if set)
> > (3) Use fallback_application_name
> > (4) Use empty string as application_name

Yeah, currently my patch uses case (2). I tried to implement (4),
but I found that libpq function(may be conninfo_array_parse()) must be modified in order to that.
We moved the functionality to libpq layer because we want to avoid some side effects,
so we started to think case (4) might be wrong.

Now we propose the following senario:
1. Use postgres_fdw.application_name when it is set and the parsing result is not empty
2. If not, use the foreign-server option when it is set and the parsing result is not empty
3. If not, use fallback_application_name

How do you think?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Attachment Content-Type Size
v18_0002_allow_escapes.patch application/octet-stream 33.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2021-09-27 04:31:03 Re: Remove page-read callback from XLogReaderState.
Previous Message Amit Kapila 2021-09-27 04:02:40 Re: row filtering for logical replication