Re: Support escape sequence for cluster_name in postgres_fdw.application_name
On 2022/02/15 8:52, r.takahash...@fujitsu.com wrote: Hi Fujii san, Thank you for updating the patch. I have no additional comments. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Support escape sequence for cluster_name in postgres_fdw.application_name
Hi Fujii san, Thank you for updating the patch. I have no additional comments. Regards, Ryohei Takahashi
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
On 2022/02/09 9:19, r.takahash...@fujitsu.com wrote:
Hi,
Thank you for updating the patch.
I agree with the documentation and program.
How about adding the test for %c (Session ID)?
(Adding the test for %C (cluster_name) seems difficult.)
Ok, I added the tests for %c and %C escape sequences.
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index b2e02caefe..057342083c 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10910,6 +10910,26 @@ SELECT pg_terminate_backend(pid, 18) FROM
pg_stat_activity
t
(1 row)
+-- Test %c (session ID) and %C (cluster name) escape sequences.
+SET postgres_fdw.application_name TO 'fdw_%C%c';
+SELECT 1 FROM ft6 LIMIT 1;
+ ?column?
+--
+1
+(1 row)
+
+SELECT pg_terminate_backend(pid, 18) FROM pg_stat_activity
+ WHERE application_name =
+substring('fdw_' || current_setting('cluster_name') ||
+ to_hex(trunc(EXTRACT(EPOCH FROM (SELECT backend_start FROM
+ pg_stat_get_activity(pg_backend_pid()::integer) || '.' ||
+ to_hex(pg_backend_pid())
+ for current_setting('max_identifier_length')::int);
+ pg_terminate_backend
+--
+ t
+(1 row)
+
--Clean up
RESET postgres_fdw.application_name;
RESET debug_discard_caches;
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index fc3ce6a53a..af38e956e7 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -489,6 +489,12 @@ process_pgfdw_appname(const char *appname)
case 'a':
appendStringInfoString(, application_name);
break;
+ case 'c':
+ appendStringInfo(, "%lx.%x", (long)
(MyStartTime), MyProcPid);
+ break;
+ case 'C':
+ appendStringInfoString(, cluster_name);
+ break;
case 'd':
appendStringInfoString(,
MyProcPort->database_name);
break;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e050639b57..6c9f579c41 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3501,6 +3501,17 @@ SELECT pg_terminate_backend(pid, 18) FROM
pg_stat_activity
substring('fdw_' || current_setting('application_name') ||
CURRENT_USER || '%' for current_setting('max_identifier_length')::int);
+-- Test %c (session ID) and %C (cluster name) escape sequences.
+SET postgres_fdw.application_name TO 'fdw_%C%c';
+SELECT 1 FROM ft6 LIMIT 1;
+SELECT pg_terminate_backend(pid, 18) FROM pg_stat_activity
+ WHERE application_name =
+substring('fdw_' || current_setting('cluster_name') ||
+ to_hex(trunc(EXTRACT(EPOCH FROM (SELECT backend_start FROM
+ pg_stat_get_activity(pg_backend_pid()::integer) || '.' ||
+ to_hex(pg_backend_pid())
+ for current_setting('max_identifier_length')::int);
+
--Clean up
RESET postgres_fdw.application_name;
RESET debug_discard_caches;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 2bb31f1125..17cd90ab12 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -983,6 +983,20 @@ postgres=# SELECT postgres_fdw_disconnect_all();
%a
Application name on local server
+
+ %c
+
+ Session ID on local server
+ (see for details)
+
+
+
+ %C
+
+ Cluster name in local server
+ (see for details)
+
+
%u
User name on local server
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 6bb81707b0..f1bfe79feb 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -271,7 +271,7 @@ extern int temp_file_limit;
extern int num_temp_buffers;
-extern char *cluster_name;
+extern PGDLLIMPORT char *cluster_name;
extern PGDLLIMPORT char *ConfigFileName;
extern char *HbaFileName;
extern char *IdentFileName;
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
Sorry for missing this. At Thu, 27 Jan 2022 19:26:39 +0900, Fujii Masao wrote in > > On 2022/01/27 17:10, Kyotaro Horiguchi wrote: > > I don't object to adding more meaningful replacements, but more escape > > sequence makes me anxious about the increased easiness of exceeding > > the size limit of application_name. > > If this is really an issue, it might be time to reconsider the size > limit of application_name. If it's considered too short, the patch > that enlarges it should be proposed separately. That makes sense. > > Considering that it is used to > > identify fdw-initinator server, we might need to add padding (or > > rather truncating) option in the escape sequence syntax, then warn > > about truncated application_names for safety. > > I failed to understand this. Could you tell me why we might need to > add padding option here? My point was "truncating" option, which limits the length of the replacement string. But expanding the application_name limit is more sensible. > > Is the reason for 'C' in upper-case to avoid possible conflict with > > 'c' of log_line_prefix? > > Yes. > > > I'm not sure that preventive measure is worth > > doing. Looking the escape-sequence spec alone, it seems to me rather > > strange that an upper-case letter is used in spite of its lower-case > > is not used yet. > > I have no strong opinion about using %C. If there is better character > for the escape sequence, I'm happy to use it. So what character is > more proper? %c? I think so. > > Otherwise all looks fine to me except the lack of documentation. > > The patch updated postgres-fdw.sgml, but you imply there are other > documents that the patch should update? Could you tell me where the > patch should update? Mmm. I should have missed that part. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
RE: Support escape sequence for cluster_name in postgres_fdw.application_name
Hi, Thank you for updating the patch. I agree with the documentation and program. How about adding the test for %c (Session ID)? (Adding the test for %C (cluster_name) seems difficult.) Regards, Ryohei Takahashi
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
On 2022/01/28 14:07, r.takahash...@fujitsu.com wrote: I think %c of log_line_prefix (Session ID) is also useful for postgres_fdw.application_name. Therefore, how about adding both %c (Session ID) and %C (cluster_name)? +1 Attached is the updated version of the patch. It adds those escape sequences %c and %C. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATIONdiff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index fc3ce6a53a..af38e956e7 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -489,6 +489,12 @@ process_pgfdw_appname(const char *appname) case 'a': appendStringInfoString(, application_name); break; + case 'c': + appendStringInfo(, "%lx.%x", (long) (MyStartTime), MyProcPid); + break; + case 'C': + appendStringInfoString(, cluster_name); + break; case 'd': appendStringInfoString(, MyProcPort->database_name); break; diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 2bb31f1125..17cd90ab12 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -983,6 +983,20 @@ postgres=# SELECT postgres_fdw_disconnect_all(); %a Application name on local server + + %c + + Session ID on local server + (see for details) + + + + %C + + Cluster name in local server + (see for details) + + %u User name on local server diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 6bb81707b0..f1bfe79feb 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -271,7 +271,7 @@ extern int temp_file_limit; extern int num_temp_buffers; -extern char *cluster_name; +extern PGDLLIMPORT char *cluster_name; extern PGDLLIMPORT char *ConfigFileName; extern char *HbaFileName; extern char *IdentFileName;
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
On Thu, Jan 27, 2022 at 3:10 AM Kyotaro Horiguchi wrote: > Is the reason for 'C' in upper-case to avoid possible conflict with > 'c' of log_line_prefix? I'm not sure that preventive measure is worth > doing. Looking the escape-sequence spec alone, it seems to me rather > strange that an upper-case letter is used in spite of its lower-case > is not used yet. It's good to be consistent, though. -- Robert Haas EDB: http://www.enterprisedb.com
RE: Support escape sequence for cluster_name in postgres_fdw.application_name
Hi, Thank you for developing this feature. I think adding escape sequence for cluster_name is useful too. > Is the reason for 'C' in upper-case to avoid possible conflict with > 'c' of log_line_prefix? I'm not sure that preventive measure is worth > doing. Looking the escape-sequence spec alone, it seems to me rather > strange that an upper-case letter is used in spite of its lower-case > is not used yet. I think %c of log_line_prefix (Session ID) is also useful for postgres_fdw.application_name. Therefore, how about adding both %c (Session ID) and %C (cluster_name)? Regards, Ryohei Takahashi
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
On 2022/01/27 17:10, Kyotaro Horiguchi wrote: At Tue, 25 Jan 2022 16:02:39 +0900, Fujii Masao wrote in Hi, Commit 6e0cb3dec1 allowed postgres_fdw.application_name to include escape sequences %a (application name), %d (database name), %u (user name) and %p (pid). In addition to them, I'd like to support the escape sequence (e.g., %C) for cluster name there. This escape sequence is helpful to investigate where each remote transactions came from. Thought? Patch attached. I don't object to adding more meaningful replacements, but more escape sequence makes me anxious about the increased easiness of exceeding the size limit of application_name. If this is really an issue, it might be time to reconsider the size limit of application_name. If it's considered too short, the patch that enlarges it should be proposed separately. Considering that it is used to identify fdw-initinator server, we might need to add padding (or rather truncating) option in the escape sequence syntax, then warn about truncated application_names for safety. I failed to understand this. Could you tell me why we might need to add padding option here? Is the reason for 'C' in upper-case to avoid possible conflict with 'c' of log_line_prefix? Yes. I'm not sure that preventive measure is worth doing. Looking the escape-sequence spec alone, it seems to me rather strange that an upper-case letter is used in spite of its lower-case is not used yet. I have no strong opinion about using %C. If there is better character for the escape sequence, I'm happy to use it. So what character is more proper? %c? Otherwise all looks fine to me except the lack of documentation. The patch updated postgres-fdw.sgml, but you imply there are other documents that the patch should update? Could you tell me where the patch should update? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
At Tue, 25 Jan 2022 16:02:39 +0900, Fujii Masao wrote in > Hi, > > Commit 6e0cb3dec1 allowed postgres_fdw.application_name to include > escape sequences %a (application name), %d (database name), %u (user > name) and %p (pid). In addition to them, I'd like to support the > escape sequence (e.g., %C) for cluster name there. This escape > sequence is helpful to investigate where each remote transactions came > from. Thought? > > Patch attached. I don't object to adding more meaningful replacements, but more escape sequence makes me anxious about the increased easiness of exceeding the size limit of application_name. Considering that it is used to identify fdw-initinator server, we might need to add padding (or rather truncating) option in the escape sequence syntax, then warn about truncated application_names for safety. Is the reason for 'C' in upper-case to avoid possible conflict with 'c' of log_line_prefix? I'm not sure that preventive measure is worth doing. Looking the escape-sequence spec alone, it seems to me rather strange that an upper-case letter is used in spite of its lower-case is not used yet. Otherwise all looks fine to me except the lack of documentation. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
