Thread: pgsql: postgres_fdw: Allow postgres_fdw.application_name to include esc

postgres_fdw: Allow postgres_fdw.application_name to include escape sequences.

application_name that used when postgres_fdw establishes a connection to
a foreign server can be specified in either or both a connection parameter
of a server object and GUC postgres_fdw.application_name. This commit
allows those parameters to include escape sequences that begins with
% character. Then postgres_fdw replaces those escape sequences with
status information. For example, %d and %u are replaced with user name
and database name in local server, respectively. This feature enables us
to add information more easily to track remote transactions or queries,
into application_name of a remote connection.

Author: Hayato Kuroda
Reviewed-by: Kyotaro Horiguchi, Masahiro Ikeda, Hou Zhijie, Fujii Masao
Discussion: https://postgr.es/m/TYAPR01MB5866FAE71C66547C64616584F5EB9@TYAPR01MB5866.jpnprd01.prod.outlook.com
Discussion: https://postgr.es/m/TYCPR01MB5870D1E8B949DAF6D3B84E02F5F29@TYCPR01MB5870.jpnprd01.prod.outlook.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6e0cb3dec10e460288d68a128e3d79d16a230cdb

Modified Files
--------------
contrib/postgres_fdw/connection.c              | 36 ++++++++++++++
contrib/postgres_fdw/expected/postgres_fdw.out | 32 ++++++++++++
contrib/postgres_fdw/option.c                  | 62 ++++++++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.h            |  1 +
contrib/postgres_fdw/sql/postgres_fdw.sql      | 21 ++++++++
doc/src/sgml/postgres-fdw.sgml                 | 67 +++++++++++++++++++++++++-
6 files changed, 218 insertions(+), 1 deletion(-)

On 2021/12/24 16:58, Fujii Masao wrote:
> postgres_fdw: Allow postgres_fdw.application_name to include escape sequences.

Hmm... some buildfarm animals reported a failure. So I will investigate the issue.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

I saw the test has been revertd.

At Fri, 24 Dec 2021 17:20:18 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> Hmm... some buildfarm animals reported a failure. So I will
> investigate the issue.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25

> NOTICE:  identifier
> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%"
> will be truncated to
> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw"

It's 70 characters long..

application_name: pg_regress/postgres_fdw
user_name       : buildfarm
database_name   : contrib_regression_postgres_fdw
Source PID      : 757365

Maybe we can distribute the placeholders into several sessions.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2021/12/24 18:00, Kyotaro Horiguchi wrote:
> I saw the test has been revertd.

Yes, I reverted the added unstable tests not to prevent
buildfarm from testing other patches while I'm doing
the investigation.

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25
> 
>> NOTICE:  identifier
>> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%"
>> will be truncated to
>> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw"
> 
> It's 70 characters long..
> 
> application_name: pg_regress/postgres_fdw
> user_name       : buildfarm
> database_name   : contrib_regression_postgres_fdw
> Source PID      : 757365
> 
> Maybe we can distribute the placeholders into several sessions.

Or probably we don't need to test all escape sequences. How about picking up one or two from them? But even if only one
ortwo are picked up, application_name still can be larger than 63 characters. So probably we also should use
substring()in the test query, for that case. For example,
 

SELECT count(*) > 0 FROM pg_stat_activity
   WHERE application_name = substring(current_setting('application_name') ||
     CURRENT_USER || current_database() || pg_backend_pid() || '%', 1, 63);

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Fri, 24 Dec 2021 18:24:39 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/12/24 18:00, Kyotaro Horiguchi wrote:
> > I saw the test has been revertd.
> 
> Yes, I reverted the added unstable tests not to prevent
> buildfarm from testing other patches while I'm doing
> the investigation.
> 
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25
> > 
> >> NOTICE:  identifier
> >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%"
> >> will be truncated to
> >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw"
> > It's 70 characters long..
> > application_name: pg_regress/postgres_fdw
> > user_name       : buildfarm
> > database_name   : contrib_regression_postgres_fdw
> > Source PID      : 757365
> > Maybe we can distribute the placeholders into several sessions.
> 
> Or probably we don't need to test all escape sequences. How about
> picking up one or two from them? But even if only one or two are
> picked up, application_name still can be larger than 63 characters. So
> probably we also should use substring() in the test query, for that
> case. For example,
> 
> SELECT count(*) > 0 FROM pg_stat_activity
>   WHERE application_name = substring(current_setting('application_name')
>   ||
>     CURRENT_USER || current_database() || pg_backend_pid() || '%', 1, 63);

I once thought the same, but that change causes buildfarms *always*
truncate the resulting application name. I believe the base
application_name and database_name never grows further longer. So I'd
like to split the check to %a%u%% and %d%p.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 24 Dec 2021 18:44:06 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Fri, 24 Dec 2021 18:24:39 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> > 
> > 
> > On 2021/12/24 18:00, Kyotaro Horiguchi wrote:
> > > I saw the test has been revertd.
> > 
> > Yes, I reverted the added unstable tests not to prevent
> > buildfarm from testing other patches while I'm doing
> > the investigation.
> > 
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-12-24%2008%3A02%3A25
> > > 
> > >> NOTICE:  identifier
> > >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw757365%"
> > >> will be truncated to
> > >> "pg_regress/postgres_fdwbuildfarmcontrib_regression_postgres_fdw"
> > > It's 70 characters long..
> > > application_name: pg_regress/postgres_fdw
> > > user_name       : buildfarm
> > > database_name   : contrib_regression_postgres_fdw
> > > Source PID      : 757365
> > > Maybe we can distribute the placeholders into several sessions.
> > 
> > Or probably we don't need to test all escape sequences. How about
> > picking up one or two from them? But even if only one or two are
> > picked up, application_name still can be larger than 63 characters. So
> > probably we also should use substring() in the test query, for that
> > case. For example,
> > 
> > SELECT count(*) > 0 FROM pg_stat_activity
> >   WHERE application_name = substring(current_setting('application_name')
> >   ||
> >     CURRENT_USER || current_database() || pg_backend_pid() || '%', 1, 63);
> 
> I once thought the same, but that change causes buildfarms *always*
> truncate the resulting application name. I believe the base
> application_name and database_name never grows further longer. So I'd
> like to split the check to %a%u%% and %d%p.

Or if we are convinced that the names won't get longer, we could set
application_name on the local session as something like 'X' so that the
result fits 63 character-long..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2021/12/24 18:44, Kyotaro Horiguchi wrote:
> I once thought the same, but that change causes buildfarms *always*
> truncate the resulting application name. I believe the base
> application_name and database_name never grows further longer. So I'd
> like to split the check to %a%u%% and %d%p.

Yeah, probably it's better to split the test in that way. So I attached the patch doing that at the original thread [1]
inpgsql-hackers. Let's discuss this there.
 

[1]
https://postgr.es/m/e7b61420-a97b-8246-77c4-a0d48fba5a45@oss.nttdata.com

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION