Support escape sequence for cluster_name in postgres_fdw.application_name
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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
pgfdw_appname_cluster_name_v1.patchtext/plain; charset=UTF-8; name=pgfdw_appname_cluster_name_v1.patchDownload
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index fc3ce6a53a..4120c21d09 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -489,6 +489,9 @@ process_pgfdw_appname(const char *appname)
case 'a':
appendStringInfoString(&buf, application_name);
break;
+ case 'C':
+ appendStringInfoString(&buf, cluster_name);
+ break;
case 'd':
appendStringInfoString(&buf, MyProcPort->database_name);
break;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 2bb31f1125..d7f545958c 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -983,6 +983,13 @@ postgres=# SELECT postgres_fdw_disconnect_all();
<entry><literal>%a</literal></entry>
<entry>Application name on local server</entry>
</row>
+ <row>
+ <entry><literal>%C</literal></entry>
+ <entry>
+ Cluster name in local server
+ (see <xref linkend="guc-cluster-name"/> for details)
+ </entry>
+ </row>
<row>
<entry><literal>%u</literal></entry>
<entry>User name on local server</entry>
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;
At Tue, 25 Jan 2022 16:02:39 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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
On 2022/01/27 17:10, Kyotaro Horiguchi wrote:
At Tue, 25 Jan 2022 16:02:39 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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
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
On Thu, Jan 27, 2022 at 3:10 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> 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
On 2022/01/28 14:07, r.takahashi_2@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 CORPORATION
Attachments:
pgfdw_appname_cluster_name_v2.patchtext/plain; charset=UTF-8; name=pgfdw_appname_cluster_name_v2.patchDownload
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(&buf, application_name);
break;
+ case 'c':
+ appendStringInfo(&buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
+ break;
+ case 'C':
+ appendStringInfoString(&buf, cluster_name);
+ break;
case 'd':
appendStringInfoString(&buf, 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();
<entry><literal>%a</literal></entry>
<entry>Application name on local server</entry>
</row>
+ <row>
+ <entry><literal>%c</literal></entry>
+ <entry>
+ Session ID on local server
+ (see <xref linkend="guc-log-line-prefix"/> for details)
+ </entry>
+ </row>
+ <row>
+ <entry><literal>%C</literal></entry>
+ <entry>
+ Cluster name in local server
+ (see <xref linkend="guc-cluster-name"/> for details)
+ </entry>
+ </row>
<row>
<entry><literal>%u</literal></entry>
<entry>User name on local server</entry>
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;
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
Sorry for missing this.
At Thu, 27 Jan 2022 19:26:39 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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
On 2022/02/09 9:19, r.takahashi_2@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 CORPORATION
Attachments:
pgfdw_appname_cluster_name_v3.patchtext/plain; charset=UTF-8; name=pgfdw_appname_cluster_name_v3.patchDownload
diff --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, 180000) 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, 180000) 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(&buf, application_name);
break;
+ case 'c':
+ appendStringInfo(&buf, "%lx.%x", (long) (MyStartTime), MyProcPid);
+ break;
+ case 'C':
+ appendStringInfoString(&buf, cluster_name);
+ break;
case 'd':
appendStringInfoString(&buf, 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, 180000) 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, 180000) 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();
<entry><literal>%a</literal></entry>
<entry>Application name on local server</entry>
</row>
+ <row>
+ <entry><literal>%c</literal></entry>
+ <entry>
+ Session ID on local server
+ (see <xref linkend="guc-log-line-prefix"/> for details)
+ </entry>
+ </row>
+ <row>
+ <entry><literal>%C</literal></entry>
+ <entry>
+ Cluster name in local server
+ (see <xref linkend="guc-cluster-name"/> for details)
+ </entry>
+ </row>
<row>
<entry><literal>%u</literal></entry>
<entry>User name on local server</entry>
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;
Hi Fujii san,
Thank you for updating the patch.
I have no additional comments.
Regards,
Ryohei Takahashi
On 2022/02/15 8:52, r.takahashi_2@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