Thread: 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 namethere. 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
Attachment
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
From
Kyotaro Horiguchi
Date:
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
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
From
Fujii Masao
Date:
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 whatcharacter 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 tellme 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
From
"r.takahashi_2@fujitsu.com"
Date:
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
From
Robert Haas
Date:
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
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
From
Fujii Masao
Date:
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
Attachment
RE: Support escape sequence for cluster_name in postgres_fdw.application_name
From
"r.takahashi_2@fujitsu.com"
Date:
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
From
Kyotaro Horiguchi
Date:
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
Re: Support escape sequence for cluster_name in postgres_fdw.application_name
From
Fujii Masao
Date:
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
Attachment
RE: Support escape sequence for cluster_name in postgres_fdw.application_name
From
"r.takahashi_2@fujitsu.com"
Date:
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
From
Fujii Masao
Date:
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