Re: Allow escape in application_name
At Tue, 4 Jan 2022 12:36:32 +0900, Masahiko Sawada wrote in > On Tue, Jan 4, 2022 at 12:05 PM Kyotaro Horiguchi > wrote: > > pg_terminate_backend returns just after kill() returns. So I'm afraid > > that there's a case where the next access to ft6 happens before the > > old connection actually ends on slow machines or under heavy load. > > The test does pg_terminate_backend() with a timeout, and in this case, > don't we wait for the backend to end after sending the signal? Oops! I missed that part. I agree that it works. Thanks! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name
On Tue, Jan 4, 2022 at 12:05 PM Kyotaro Horiguchi
wrote:
>
> At Wed, 29 Dec 2021 10:34:31 +0900, Masahiko Sawada
> wrote in
> > On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao
> > wrote:
> > >
> > >
> > >
> > > On 2021/12/28 9:32, Masahiko Sawada wrote:
> > > > Doesn't this query return 64? So the expression "substring(str for
> > > > (SELECT max_identifier_length FROM pg_control_init()))" returns the
> > > > first 64 characters of the given string while the application_name is
> > > > truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
> > > > fine to use current_setting('max_identifier_length') instead of
> > > > max_identifier_length of pg_control_init().
> > >
> > > Interesting! I agree that current_setting('max_identifier_length') should
> > > be used instead. Attached is the updated version of the patch. It uses
> > > current_setting('max_identifier_length').
> >
> > Thank you for updating the patch! It looks good to me.
>
> pg_terminate_backend returns just after kill() returns. So I'm afraid
> that there's a case where the next access to ft6 happens before the
> old connection actually ends on slow machines or under heavy load.
The test does pg_terminate_backend() with a timeout, and in this case,
don't we wait for the backend to end after sending the signal?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: Allow escape in application_name
At Wed, 29 Dec 2021 10:34:31 +0900, Masahiko Sawada
wrote in
> On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao
> wrote:
> >
> >
> >
> > On 2021/12/28 9:32, Masahiko Sawada wrote:
> > > Doesn't this query return 64? So the expression "substring(str for
> > > (SELECT max_identifier_length FROM pg_control_init()))" returns the
> > > first 64 characters of the given string while the application_name is
> > > truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
> > > fine to use current_setting('max_identifier_length') instead of
> > > max_identifier_length of pg_control_init().
> >
> > Interesting! I agree that current_setting('max_identifier_length') should
> > be used instead. Attached is the updated version of the patch. It uses
> > current_setting('max_identifier_length').
>
> Thank you for updating the patch! It looks good to me.
pg_terminate_backend returns just after kill() returns. So I'm afraid
that there's a case where the next access to ft6 happens before the
old connection actually ends on slow machines or under heavy load.
> > BTW it seems confusing that pg_control_init() (also pg_controldata) and GUC
> > report different values as max_identifier_length. Probably they should
> > return the same value such as NAMEDATALEN - 1. But this change might be
> > overkill...
>
> Agreed. Probably we can fix it in a separate patch if necessary.
Agree to another patch, but I think we should at least add a caution
that they are different. I'm not sure we can change the context of
ControlFileData.nameDataLen.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Allow escape in application_name
On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao wrote:
>
>
>
> On 2021/12/28 9:32, Masahiko Sawada wrote:
> > Doesn't this query return 64? So the expression "substring(str for
> > (SELECT max_identifier_length FROM pg_control_init()))" returns the
> > first 64 characters of the given string while the application_name is
> > truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
> > fine to use current_setting('max_identifier_length') instead of
> > max_identifier_length of pg_control_init().
>
> Interesting! I agree that current_setting('max_identifier_length') should be
> used instead. Attached is the updated version of the patch. It uses
> current_setting('max_identifier_length').
Thank you for updating the patch! It looks good to me.
>
> BTW it seems confusing that pg_control_init() (also pg_controldata) and GUC
> report different values as max_identifier_length. Probably they should return
> the same value such as NAMEDATALEN - 1. But this change might be overkill...
Agreed. Probably we can fix it in a separate patch if necessary.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: Allow escape in application_name
On 2021/12/28 9:32, Masahiko Sawada wrote:
Doesn't this query return 64? So the expression "substring(str for
(SELECT max_identifier_length FROM pg_control_init()))" returns the
first 64 characters of the given string while the application_name is
truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
fine to use current_setting('max_identifier_length') instead of
max_identifier_length of pg_control_init().
Interesting! I agree that current_setting('max_identifier_length') should be
used instead. Attached is the updated version of the patch. It uses
current_setting('max_identifier_length').
BTW it seems confusing that pg_control_init() (also pg_controldata) and GUC
report different values as max_identifier_length. Probably they should return
the same value such as NAMEDATALEN - 1. But this change might be overkill...
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 7720ab9c58..7d6f7d9e3d 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10825,3 +10825,54 @@ ERROR: invalid value for integer option "batch_size":
100$%$#$#
ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
ERROR: invalid option "nonexistent"
HINT: There are no valid options in this context.
+-- ===
+-- test postgres_fdw.application_name GUC
+-- ===
+--- Turn debug_discard_caches off for this test to make sure that
+--- the remote connection is alive when checking its application_name.
+SET debug_discard_caches = 0;
+-- Specify escape sequences in application_name option of a server
+-- object so as to test that they are replaced with status information
+-- expectedly.
+--
+-- Since pg_stat_activity.application_name may be truncated to less than
+-- NAMEDATALEN characters, note that substring() needs to be used
+-- at the condition of test query to make sure that the string consisting
+-- of database name and process ID is also less than that.
+ALTER SERVER loopback2 OPTIONS (application_name 'fdw_%d%p');
+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_database() || pg_backend_pid() for
+ current_setting('max_identifier_length')::int);
+ pg_terminate_backend
+--
+ t
+(1 row)
+
+-- postgres_fdw.application_name overrides application_name option
+-- of a server object if both settings are present.
+SET postgres_fdw.application_name TO 'fdw_%a%u%%';
+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('application_name') ||
+ CURRENT_USER || '%' 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/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index beeac8af1e..9eb673e369 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3452,3 +3452,38 @@ CREATE FOREIGN TABLE inv_bsz (c1 int )
-- No option is allowed to be specified at foreign data wrapper level
ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
+
+-- ===
+-- test postgres_fdw.application_name GUC
+-- ===
+--- Turn debug_discard_caches off for this test to make sure that
+--- the remote connection is alive when checking its application_name.
+SET debug_discard_caches = 0;
+
+-- Specify escape sequences in application_name option of a server
+-- object so as to test that they are replaced with status information
+-- expectedly.
+--
+-- Since pg_stat_activity.application_name may be truncated to less than
+-- NAMEDATALEN characters, note that substring() needs to be used
+-- at the condition of test query to make sure that the string consisting
+-- of database name and process ID is also less than that.
+ALTER SERVER loopback2 OPTIONS (application_name 'fdw_%d%p');
+SELECT 1 FROM ft6 LIMIT 1;
+SELECT pg_terminate_backend(pid, 18) FROM pg_stat_activity
+ WHERE application_name =
+substring('fdw_' || current_database() || pg_backend_pid() for
+ current_setting('max_identifier_length')::int);
+
+-- postgres_fdw.application_name overrides application_name option
+-- of a server object if both settings are present.
+SET postgres_fdw.applicat
Re: Allow escape in application_name
On Tue, Dec 28, 2021 at 8:57 AM kuroda.hay...@fujitsu.com
wrote:
>
> Dear Sawada-san,
>
> > If so, we need to do substring(... for
> > 63) instead.
Just to be clear, I meant substring(... for NAMEDATALEN - 1).
>
> Yeah, the parameter will be truncated as one less than NAMEDATALEN:
>
> ```
> max_identifier_length (integer)
> Reports the maximum identifier length. It is determined as one less than the
> value of NAMEDATALEN when building the server.
> The default value of NAMEDATALEN is 64; therefore the default
> max_identifier_length is 63 bytes,
> which can be less than 63 characters when using multibyte encodings.
> ```
I think this is the description of the max_identifier_length GUC parameter.
>
> But in Fujii-san's patch length is picked up by the following SQL, so I think
> it works well.
>
> ```
> SELECT max_identifier_length FROM pg_control_init()
> ```
Doesn't this query return 64? So the expression "substring(str for
(SELECT max_identifier_length FROM pg_control_init()))" returns the
first 64 characters of the given string while the application_name is
truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
fine to use current_setting('max_identifier_length') instead of
max_identifier_length of pg_control_init().
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
RE: Allow escape in application_name
Dear Sawada-san, > Good idea. But the application_name is actually truncated to 63 > characters (NAMEDATALEN - 1)? If so, we need to do substring(... for > 63) instead. Yeah, the parameter will be truncated as one less than NAMEDATALEN: ``` max_identifier_length (integer) Reports the maximum identifier length. It is determined as one less than the value of NAMEDATALEN when building the server. The default value of NAMEDATALEN is 64; therefore the default max_identifier_length is 63 bytes, which can be less than 63 characters when using multibyte encodings. ``` But in Fujii-san's patch length is picked up by the following SQL, so I think it works well. ``` SELECT max_identifier_length FROM pg_control_init() ``` Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
On Mon, Dec 27, 2021 at 1:40 PM Fujii Masao wrote: > > > > On 2021/12/27 10:40, kuroda.hay...@fujitsu.com wrote: > > Dear Fujii-san, Horiguchi-san, > > > > I confirmed that the feature was committed but reverted the test. > > Now I'm checking buildfarm. > > Attached is the patch that adds the regression test for > postgres_fdw.application_name. Could you review this? > > As Horiguchi-san suggested at [1], I split the test into two, and then > tweaked them as follows. > > 1. Set application_name option of a server option to 'fdw_%d%p' > 2. Set postgres_fdw.application_name to 'fdw_%a%u%%' > > 'fdw_%d%p' and 'fdw_%a%u%%' still may be larger than NAMEDATALEN depending on > the regression test environment. To make the test stable even in that case, > the patch uses substring() is truncate application_name string in the test > query's condition to less than NAMEDATALEN. Good idea. But the application_name is actually truncated to 63 characters (NAMEDATALEN - 1)? If so, we need to do substring(... for 63) instead. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
RE: Allow escape in application_name
Dear Fujii-san, > Attached is the patch that adds the regression test for > postgres_fdw.application_name. Could you review this? > > As Horiguchi-san suggested at [1], I split the test into two, and then > tweaked them > as follows. > > 1. Set application_name option of a server option to 'fdw_%d%p' > 2. Set postgres_fdw.application_name to 'fdw_%a%u%%' > > 'fdw_%d%p' and 'fdw_%a%u%%' still may be larger than NAMEDATALEN > depending on the regression test environment. To make the test stable even in > that case, the patch uses substring() is truncate application_name string in > the > test query's condition to less than NAMEDATALEN. I think it's good because we can care about max_identifier_length, and both of setting methods are used. Also it's smart using pg_terminate_backend(). (Actually I'm writing a test that separates test cases into five parts, but your patch is better.) I think the test failure should be noticed by me, but I missed, sorry. Do you know how to apply my patch and test by buildfarm animals? Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
On 2021/12/27 10:40, kuroda.hay...@fujitsu.com wrote:
Dear Fujii-san, Horiguchi-san,
I confirmed that the feature was committed but reverted the test.
Now I'm checking buildfarm.
Attached is the patch that adds the regression test for
postgres_fdw.application_name. Could you review this?
As Horiguchi-san suggested at [1], I split the test into two, and then tweaked
them as follows.
1. Set application_name option of a server option to 'fdw_%d%p'
2. Set postgres_fdw.application_name to 'fdw_%a%u%%'
'fdw_%d%p' and 'fdw_%a%u%%' still may be larger than NAMEDATALEN depending on
the regression test environment. To make the test stable even in that case, the
patch uses substring() is truncate application_name string in the test query's
condition to less than NAMEDATALEN.
[1]
https://postgr.es/m/20211224.184406.814784272581964942.horikyota@gmail.com
But anyway I want to say thank you for your contribution!
Thanks!
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 7720ab9c58..ad716004a5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10825,3 +10825,55 @@ ERROR: invalid value for integer option "batch_size":
100$%$#$#
ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
ERROR: invalid option "nonexistent"
HINT: There are no valid options in this context.
+-- ===
+-- test postgres_fdw.application_name GUC
+-- ===
+--- Turn debug_discard_caches off for this test to make sure that
+--- the remote connection is alive when checking its application_name.
+SET debug_discard_caches = 0;
+-- Specify escape sequences in application_name option of a server
+-- object so as to test that they are replaced with status information
+-- expectedly.
+--
+-- Since pg_stat_activity.application_name may be truncated to less than
+-- NAMEDATALEN characters, note that substring() needs to be used
+-- at the condition of test query to make sure that the string consisting
+-- of database name and process ID is also less than that.
+ALTER SERVER loopback2 OPTIONS (application_name 'fdw_%d%p');
+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_database() || pg_backend_pid() for
+ (SELECT max_identifier_length FROM pg_control_init()));
+ pg_terminate_backend
+--
+ t
+(1 row)
+
+-- postgres_fdw.application_name overrides application_name option
+-- of a server object if both settings are present.
+SET postgres_fdw.application_name TO 'fdw_%a%u%%';
+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('application_name') ||
+ CURRENT_USER || '%' for
+ (SELECT max_identifier_length FROM pg_control_init()));
+ pg_terminate_backend
+--
+ t
+(1 row)
+
+--Clean up
+RESET postgres_fdw.application_name;
+RESET debug_discard_caches;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index beeac8af1e..d811f160cf 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3452,3 +3452,39 @@ CREATE FOREIGN TABLE inv_bsz (c1 int )
-- No option is allowed to be specified at foreign data wrapper level
ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
+
+-- ===
+-- test postgres_fdw.application_name GUC
+-- ===
+--- Turn debug_discard_caches off for this test to make sure that
+--- the remote connection is alive when checking its application_name.
+SET debug_discard_caches = 0;
+
+-- Specify escape sequences in application_name option of a server
+-- object so as to test that they are replaced with status information
+-- expectedly.
+--
+-- Since pg_stat_activity.application_name may be truncated to less than
+-- NAMEDATALEN characters, note that substring() needs to be used
+-- at the condition of test query to make sure that the string consisting
+-- of database name and process ID is also less than that.
+ALTER SERVER loopback2 OPTIONS (application_name 'fdw_%d%p');
+SELECT 1 FROM ft6 LIMIT 1;
+SELECT pg_terminate_backend(pid, 18) FROM pg_stat_activity
+ WHERE application_name =
+substring('fdw_' || current_database() || pg_backend_pid() for
+ (SELECT max_identifier_length FROM pg_control_init()));
+
+-- postgres_fdw.applicat
RE: Allow escape in application_name
Dear Fujii-san, Horiguchi-san, I confirmed that the feature was committed but reverted the test. Now I'm checking buildfarm. But anyway I want to say thank you for your contribution! Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
On 2021/12/24 13:49, Kyotaro Horiguchi wrote: I'm ok to remove the check "values[i] != NULL", but think that it's better to keep the other check "*(values[i]) != '\0'" as it is. Because *(values[i]) can be null character and it's a waste of cycles to call process_pgfdw_appname() in that case. Right. I removed too much. Thanks for the check! So I kept the check "*(values[i]) != '\0'" as it is and pushed the patch. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Allow escape in application_name
At Thu, 23 Dec 2021 23:10:38 +0900, Fujii Masao
wrote in
>
>
> On 2021/12/17 16:50, Kyotaro Horiguchi wrote:
> > Thus rewriting the code we're focusing on like the following would
> > make sense to me.
> >
> >>if (strcmp(keywords[i], "application_name") == 0)
> >>{
> >>values[i] = process_pgfdw_appname(values[i]);
> >>
> >>/*
> >> * Break if we have a non-empty string. If we end up failing
> >> with
> >> * all candidates, fallback_application_name would work.
> >> */
> >>if (appanme[0] != '\0')
(appname?)
> >>break;
> >>}
>
> I'm ok to remove the check "values[i] != NULL", but think that it's
> better to keep the other check "*(values[i]) != '\0'" as it
> is. Because *(values[i]) can be null character and it's a waste of
> cycles to call process_pgfdw_appname() in that case.
Right. I removed too much.
> > Thanks for revisiting.
> >
> >> #1. use "[unknown]"
> >> #2. add the check but not use "[unknown]"
> >> #3. don't add the check (i.e., what the current patch does)
> >>
> >> For now, I'm ok to choose #2 or #3.
> > As I said before, given that we don't show "unkown" or somethig like
> > as the fallback, I'm fine with not having a NULL check since anyway it
> > bumps into SEGV immediately. In short I'm fine with #3 here.
>
> Yep, let's use #3 approach.
Agreed.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Allow escape in application_name
On 2021/12/17 16:50, Kyotaro Horiguchi wrote:
Thus rewriting the code we're focusing on like the following would
make sense to me.
if (strcmp(keywords[i], "application_name") == 0)
{
values[i] = process_pgfdw_appname(values[i]);
/*
* Break if we have a non-empty string. If we end up failing
with
* all candidates, fallback_application_name would work.
*/
if (appanme[0] != '\0')
break;
}
I'm ok to remove the check "values[i] != NULL", but think that it's better to keep the
other check "*(values[i]) != '\0'" as it is. Because *(values[i]) can be null character
and it's a waste of cycles to call process_pgfdw_appname() in that case.
Thanks for revisiting.
#1. use "[unknown]"
#2. add the check but not use "[unknown]"
#3. don't add the check (i.e., what the current patch does)
For now, I'm ok to choose #2 or #3.
As I said before, given that we don't show "unkown" or somethig like
as the fallback, I'm fine with not having a NULL check since anyway it
bumps into SEGV immediately. In short I'm fine with #3 here.
Yep, let's use #3 approach.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Allow escape in application_name
At Fri, 17 Dec 2021 14:50:37 +0900, Fujii Masao
wrote in
> > FWIW, I don't understand why we care of the case where the function
> > itself changes its mind to set values[i] to null
>
> Whether we add this check or not, the behavior is the same, i.e., that
> setting value is not used. But by adding the check we can avoid
> unnecessary call of process_pgfdw_appname() when the value is
> NULL. OTOH, of course we can also remove the check. So I'm ok to
> remove that if you're thinking it's more consistent to remove that.
That depends. It seems to me defGetString is assumed to return a valid
pointer, since (AFAIS) all of the callers don't check against NULL. On
the other hand the length of the string may be zero. Actually
check_conn_params() called just after makes the same assumption (only
on "password", though). On the other hand PQconnectdbParams assumes
NULL value as not-set.
So assumption on the NULL value differs in some places and at least
postgres_fdw doesn't use NULL to represent "not exists".
Thus rewriting the code we're focusing on like the following would
make sense to me.
> if (strcmp(keywords[i], "application_name") == 0)
> {
> values[i] = process_pgfdw_appname(values[i]);
>
> /*
>* Break if we have a non-empty string. If we end up failing
> with
>* all candidates, fallback_application_name would work.
>*/
> if (appanme[0] != '\0')
> break;
> }
> Probably now it's good chance to revisit this issue. As I commented at
> [1], at least for me it's intuitive to use empty string rather than
> "[unknown]" when appname or username, etc was NULL or '\0'. To
> implement this behavior, I argued to remove the check itself. But
> there are several options:
Thanks for revisiting.
> #1. use "[unknown]"
> #2. add the check but not use "[unknown]"
> #3. don't add the check (i.e., what the current patch does)
>
> For now, I'm ok to choose #2 or #3.
As I said before, given that we don't show "unkown" or somethig like
as the fallback, I'm fine with not having a NULL check since anyway it
bumps into SEGV immediately. In short I'm fine with #3 here.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Allow escape in application_name
On 2021/12/17 12:06, Kyotaro Horiguchi wrote: At Fri, 17 Dec 2021 02:42:25 +, "kuroda.hay...@fujitsu.com" wrote in Dear Fujii-san, Sorry I forgot replying your messages. if (strcmp(keywords[i], "application_name") == 0 && values[i] != NULL && *(values[i]) != '\0') I'm not sure but do we have a case that values[i] becomes NULL even if keywords[i] is "application_name"? No for now, I guess. But isn't it safer to check that, too? I also could not find strong reason why that check should be dropped. But you'd like to drop that? No, I agreed the new checking. I'm just afraid of my code missing. FWIW, I don't understand why we care of the case where the function itself changes its mind to set values[i] to null Whether we add this check or not, the behavior is the same, i.e., that setting value is not used. But by adding the check we can avoid unnecessary call of process_pgfdw_appname() when the value is NULL. OTOH, of course we can also remove the check. So I'm ok to remove that if you're thinking it's more consistent to remove that. while we ignore the possibility that some module at remote is modified so that some global variables to be NULL. I don't mind wheter we care for NULLs or not but I think we should be consistent at least in a single patch. Probably you're mentioning that we got rid of something like the following code from process_pgfdw_appname(). In this case whether we add the check or not can affect the behavior (i.e., escape sequence is replace with "[unknown]" or not). So ISTM that the situations are similar but not the same. + if (appname == NULL || *appname == '\0') + appname = "[unknown]"; Probably now it's good chance to revisit this issue. As I commented at [1], at least for me it's intuitive to use empty string rather than "[unknown]" when appname or username, etc was NULL or '\0'. To implement this behavior, I argued to remove the check itself. But there are several options: #1. use "[unknown]" #2. add the check but not use "[unknown]" #3. don't add the check (i.e., what the current patch does) For now, I'm ok to choose #2 or #3. [1] https://postgr.es/m/0dbe50c3-c528-74b1-c577-035a4a68f...@oss.nttdata.com Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Allow escape in application_name
At Fri, 17 Dec 2021 02:42:25 +, "kuroda.hay...@fujitsu.com" wrote in > Dear Fujii-san, > > Sorry I forgot replying your messages. > > > >> if (strcmp(keywords[i], "application_name") == 0 && > > >> values[i] != NULL && *(values[i]) != '\0') > > > > > > I'm not sure but do we have a case that values[i] becomes NULL > > > even if keywords[i] is "application_name"? > > > > No for now, I guess. But isn't it safer to check that, too? I also could > > not find strong > > reason why that check should be dropped. But you'd like to drop that? > > No, I agreed the new checking. I'm just afraid of my code missing. FWIW, I don't understand why we care of the case where the function itself changes its mind to set values[i] to null while we ignore the possibility that some module at remote is modified so that some global variables to be NULL. I don't mind wheter we care for NULLs or not but I think we should be consistent at least in a single patch. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
RE: Allow escape in application_name
Dear Fujii-san, Sorry I forgot replying your messages. > >>if (strcmp(keywords[i], "application_name") == 0 && > >>values[i] != NULL && *(values[i]) != '\0') > > > > I'm not sure but do we have a case that values[i] becomes NULL > > even if keywords[i] is "application_name"? > > No for now, I guess. But isn't it safer to check that, too? I also could not > find strong > reason why that check should be dropped. But you'd like to drop that? No, I agreed the new checking. I'm just afraid of my code missing. Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Allow escape in application_name
Dear Fujii-san, > Thanks for the review! At first I pushed 0001 patch. I found your commit. Thanks! > BTW, 0002 patch adds the regression test that checks > pg_stat_activity.application_name. But three months before, we added the > similar > test when introducing postgres_fdw.application_name GUC and > reverted/removed it because it's not stable [1]. So we should review carefully > whether the test 0002 patch adds may have the same issue or not. As far as I > read > the patch, ISTM that the patch has no same issue. But could you double check > that? I agreed we will not face the problem. When postgres_fdw_disconnect_all() is performed, we just send a character 'X' to remote backends(in sendTerminateConn() and lower functions) and return without any blockings. After receiving 'X' message in remote backends, proc_exit() is performed and processes will be died. The test failure is caused because SELECT statement is performed before dying backends perfectly. Currently we search pg_stat_activity and this is not affected by residual rows because the condition is too strict to exist others. Hence I think this test is stable. How do you think? Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
On 2021/12/16 11:53, kuroda.hay...@fujitsu.com wrote: Dear Fujii-san, Thank you for updating! I read your patches and I have only one comment. if (strcmp(keywords[i], "application_name") == 0 && values[i] != NULL && *(values[i]) != '\0') I'm not sure but do we have a case that values[i] becomes NULL even if keywords[i] is "application_name"? No for now, I guess. But isn't it safer to check that, too? I also could not find strong reason why that check should be dropped. But you'd like to drop that? I think other parts are perfect. Thanks for the review! At first I pushed 0001 patch. BTW, 0002 patch adds the regression test that checks pg_stat_activity.application_name. But three months before, we added the similar test when introducing postgres_fdw.application_name GUC and reverted/removed it because it's not stable [1]. So we should review carefully whether the test 0002 patch adds may have the same issue or not. As far as I read the patch, ISTM that the patch has no same issue. But could you double check that? [1] https://postgr.es/m/848ff477-effd-42b9-8b25-3f7cfe289...@oss.nttdata.com Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san, Thank you for updating! I read your patches and I have only one comment. > if (strcmp(keywords[i], "application_name") == 0 && > values[i] != NULL && *(values[i]) != '\0') I'm not sure but do we have a case that values[i] becomes NULL even if keywords[i] is "application_name"? I think other parts are perfect. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
On 2021/12/10 16:35, kuroda.hay...@fujitsu.com wrote:
How about adding that new paragraph just after the existing one, instead?
Fixed.
Thanks for the fix! Attached is the updated version of 0001 patch.
I added "See for details."
into the description. Barring any objection, I will commit
this patch at first.
Thanks! But, since the term "local server" is already used in the docs, we can
use
"the setting value of application_name in local server" etc?
I like the word "local server," so I reworte descriptions.
Thanks! Attached is the updated version of 0002 patch. I applied
some cosmetic changes, improved comments and docs. Could you review
this version?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 41c952fbe3..f4210c878f 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -936,6 +936,16 @@ postgres=# SELECT postgres_fdw_disconnect_all();
Note that change of this parameter doesn't affect any existing
connections until they are re-established.
+
+ postgres_fdw.application_name can be any string
+ of any length and contain even non-ASCII characters. However when
+ it's passed to and used as application_name
+ in a foreign server, note that it will be truncated to less than
+ NAMEDATALEN characters and any characters other
+ than printable ASCII ones in it will be replaced with question
+ marks (?).
+ See for details.
+
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 6bac4ad23e..664487b013 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -348,6 +348,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
{
const char **keywords;
const char **values;
+ char *appname = NULL;
int n;
/*
@@ -383,6 +384,39 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
n++;
}
+ /*
+* Search the parameter arrays to find application_name
setting, and
+* replace escape sequences in it with status information if
found.
+* The arrays are searched backwards because the last value is
used if
+* application_name is repeatedly set.
+*/
+ for (int i = n - 1; i >= 0; i--)
+ {
+ if (strcmp(keywords[i], "application_name") == 0 &&
+ values[i] != NULL && *(values[i]) != '\0')
+ {
+ /*
+* Use this application_name setting if it's
not empty string
+* even after any escape sequences in it are
replaced.
+*/
+ appname = process_pgfdw_appname(values[i]);
+ if (appname[0] != '\0')
+ {
+ values[i] = appname;
+ break;
+ }
+
+ /*
+* This empty application_name is not used, so
we set
+* values[i] to NULL and keep searching the
array to find the
+* next one.
+*/
+ values[i] = NULL;
+ pfree(appname);
+ appname = NULL;
+ }
+ }
+
/* Use "postgres_fdw" as fallback_application_name */
keywords[n] = "fallback_application_name";
values[n] = "postgres_fdw";
@@ -452,6 +486,8 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* Prepare new session for use */
configure_remote_session(conn);
+ if (appname != NULL)
+ pfree(appname);
pfree(keywords);
pfree(values);
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index 5196e4797a..ca6ae6749b 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10822,3 +10822,35 @@ ERROR: invalid value for integer option "batch_size":
100$%$#$#
ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
ERROR: invalid option "nonexistent"
HINT: There are no valid options in this context.
+-- ===
+-- test postgres_fdw.application_name GUC
+
RE: Allow escape in application_name
Dear Fujii-san, I apologize for sending bad patches... I confirmed that it can be built and passed a test. > Could you tell me why you added new paragraph into the middle of existing > paragraph? This change caused the following error when compiling the docs. > > postgres-fdw.sgml:952: parser error : Opening and ending tag mismatch: > listitem > line 934 and para > > > How about adding that new paragraph just after the existing one, instead? Fixed. > + /* > + * The parsing result became an empty string, > + * and that means other "application_name" > will be used > + * for connecting to the remote server. > + * So we must set values[i] to an empty string > + * and search the array again. > + */ > + values[i] = ""; > > Isn't pfree() necessary inside the loop also when data[0]=='\0' case? If so, > probably "data" would need to be set to NULL just after pfree(). Otherwise > pfree()'d "data" can be pfree()'d again at the end of connect_pg_server(). I confirmed the source, and I agreed your argument because initStringInfo() allocates memory firstly. Hence pfree() was added. > Thanks! But, since the term "local server" is already used in the docs, we > can use > "the setting value of application_name in local server" etc? I like the word "local server," so I reworte descriptions. Best Regards, Hayato Kuroda FUJITSU LIMITED v25_0002_allow_escapes.patch Description: v25_0002_allow_escapes.patch v25_0001_update_descriptions.patch Description: v25_0001_update_descriptions.patch
Re: Allow escape in application_name
On 2021/12/09 19:52, kuroda.hay...@fujitsu.com wrote: Dear Fujii-san Thank you for quick reviewing! I attached new ones. Thanks for updating the patches! Regarding 0001 patch, IMO it's better to explain that postgres_fdw.application_name can be any string of any length and contain even non-ASCII characters, unlike application_name. So how about using the following description, instead? - postgres_fdw.application_name can be any string of any length and contain even non-ASCII characters. However when it's passed to and used as application_name in a foreign server, note that it will be truncated to less than NAMEDATALEN characters and any characters other than printable ASCII ones in it will be replaced with question marks (?). - +1, Fixed. Could you tell me why you added new paragraph into the middle of existing paragraph? This change caused the following error when compiling the docs. postgres-fdw.sgml:952: parser error : Opening and ending tag mismatch: listitem line 934 and para How about adding that new paragraph just after the existing one, instead? Seems you removed the calls to pfree() from the patch. That's because the memory context used for the replaced application_name string will be released soon? Or it's better to call pfree()? Because I used escape_param_str() and get_connect_string() as reference, they did not release the memory. I reconsidered here, however, and I agreed it is confusing that only keywords and values are pfree()'d. I exported char* data and execute pfree() if it is used. Understood. + /* +* The parsing result became an empty string, +* and that means other "application_name" will be used +* for connecting to the remote server. +* So we must set values[i] to an empty string +* and search the array again. +*/ + values[i] = ""; Isn't pfree() necessary inside the loop also when data[0]=='\0' case? If so, probably "data" would need to be set to NULL just after pfree(). Otherwise pfree()'d "data" can be pfree()'d again at the end of connect_pg_server(). + %u + Local user name Average users can understand what "Local" here means? Maybe not. I added descriptions and an example. Thanks! But, since the term "local server" is already used in the docs, we can use "the setting value of application_name in local server" etc? I agree that it's good idea to add the example about how escape sequences are replaced. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san Thank you for quick reviewing! I attached new ones. > Regarding 0001 patch, IMO it's better to explain that > postgres_fdw.application_name can be any string of any length and contain even > non-ASCII characters, unlike application_name. So how about using the > following > description, instead? > > - > postgres_fdw.application_name can be any string of any > length and contain even non-ASCII characters. However when it's passed to and > used as application_name in a foreign server, note that it > will be truncated to less than NAMEDATALEN characters > and any characters other than printable ASCII ones in it will be replaced with > question marks (?). > - +1, Fixed. > + int i; > + for (i = n - 1; i >= 0; i--) > > As I told before, it's better to simplify this to "for (int i = n - 1; i >= > 0; i--)". Sorry, I missed. Fixed. > Seems you removed the calls to pfree() from the patch. That's because the > memory context used for the replaced application_name string will be released > soon? Or it's better to call pfree()? Because I used escape_param_str() and get_connect_string() as reference, they did not release the memory. I reconsidered here, however, and I agreed it is confusing that only keywords and values are pfree()'d. I exported char* data and execute pfree() if it is used. > + Same as , this is a > + printf-style string. Unlike linkend="guc-log-line-prefix"/>, > + paddings are not allowed. Accepted escapes are as follows: > > Isn't it better to explain escape sequences in postgres_fdw.application_name > more explicitly, as follows? > > - > % characters begin escape sequences that > are replaced with status information as outlined below. Unrecognized escapes > are > ignored. Other characters are copied straight to the application name. Note > that > it's not allowed to specify a plus/minus sign or a numeric literal after the > % and before the option, for alignment and padding. > - Fixed. > + %u > + Local user name > > Average users can understand what "Local" here means? Maybe not. I added descriptions and an example. > + postgres_fdw.application_name is set to application_name of a pgfdw > + connection after placeholder conversion, thus the resulting string is > + subject to the same restrictions alreadby mentioned above. > > This description doesn't need to be added if 0001 patch is applied, is it? > Because > 0001 patch adds very similar description into the docs. +1, removed. Best Regards, Hayato Kuroda FUJITSU LIMITED v24_0002_allow_escapes.patch Description: v24_0002_allow_escapes.patch v24_0001_update_descriptions.patch Description: v24_0001_update_descriptions.patch
Re: Allow escape in application_name
On 2021/12/07 18:48, kuroda.hay...@fujitsu.com wrote: Yeah, I followed your suggestion. But we deiced to keep codes clean, hence I removed the if-statements. +1 because neither application_name, user_name nor database_name should be NULL for current usage. But if it's better to check whether they are NULL or not for some reasons or future usage, e.g., background worker not logging into any specified database may set application_name to '%d' and connect to a foreign server in the future, let's change the code later with comments. Thanks for updating the patch! Regarding 0001 patch, IMO it's better to explain that postgres_fdw.application_name can be any string of any length and contain even non-ASCII characters, unlike application_name. So how about using the following description, instead? - postgres_fdw.application_name can be any string of any length and contain even non-ASCII characters. However when it's passed to and used as application_name in a foreign server, note that it will be truncated to less than NAMEDATALEN characters and any characters other than printable ASCII ones in it will be replaced with question marks (?). - + int i; + for (i = n - 1; i >= 0; i--) As I told before, it's better to simplify this to "for (int i = n - 1; i >= 0; i--)". Seems you removed the calls to pfree() from the patch. That's because the memory context used for the replaced application_name string will be released soon? Or it's better to call pfree()? + Same as , this is a + printf-style string. Unlike , + paddings are not allowed. Accepted escapes are as follows: Isn't it better to explain escape sequences in postgres_fdw.application_name more explicitly, as follows? - % characters begin escape sequences that are replaced with status information as outlined below. Unrecognized escapes are ignored. Other characters are copied straight to the application name. Note that it's not allowed to specify a plus/minus sign or a numeric literal after the % and before the option, for alignment and padding. - + %u + Local user name Average users can understand what "Local" here means? + postgres_fdw.application_name is set to application_name of a pgfdw + connection after placeholder conversion, thus the resulting string is + subject to the same restrictions alreadby mentioned above. This description doesn't need to be added if 0001 patch is applied, is it? Because 0001 patch adds very similar description into the docs. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Horiguchi-san Thank you for giving comments! I attached new patches. > > + if (appname == NULL) > > + appname = "[unknown]"; > > + if (username == NULL || *username > == '\0') > > + username = "[unknown]"; > > + if (dbname == NULL || *dbname == > '\0') > > + dbname = "[unknown]"; > > > > I'm still not sure why these are necessary. Could you clarify that? > > It probably comes from my request, just for safety for uncertain > future. They are actually not needed under the current usage of the > function, so *I* would not object to removing them if you are strongly > feel them out of place. In that case, since NULL's leads to SEGV > outright, we would even not need assertions instead. Yeah, I followed your suggestion. But we deiced to keep codes clean, hence I removed the if-statements. Best Regards, Hayato Kuroda FUJITSU LIMITED v23_0001_update_descriptions.patch Description: v23_0001_update_descriptions.patch v23_0002_allow_escapes.patch Description: v23_0002_allow_escapes.patch
RE: Allow escape in application_name
Dear Fujii-san, Thank you for reviewing! I'll post the latest version. > Currently StringInfo variable is defined in connect_pg_server() and passed to > parse_pgfdw_appname(). But there is no strong reason why the variable needs to > be defined connect_pg_server(). Right? If so how about refactoring > parse_pgfdw_appname() so that it defines StringInfoData variable and returns > the > processed character string? You can see such coding at, for example, > escape_param_str() in dblink.c. > If this refactoring is done, probably we can get rid of "#include > "lib/stringinfo.h"" > line from connection.c because connect_pg_server() no longer needs to use > StringInfo. I refactored that. Thanks! > + int i; > > + for (i = n - 1; i >= 0; i--) > > I'm tempted to simplify these into "for (int i = n - 1; i >= 0; i--)". > > + /* > + * Search application_name and replace it if found. > + * > + * We search paramters from the end because the later > + * one have higher priority. See also the above comment. > + */ > > How about updating these comments into the following? > > --- > Search the parameter arrays to find application_name setting, > and replace escape sequences in it with status information if found. > The arrays are searched backwards because the last value > is used if application_name is repeatedly set. Prefer yours. Fixed. > + if (strcmp(buf->data, "") != 0) > > Is this condition the same as "data[0] == '\0'"? Maybe "data[0] != '\0'", but fixed. > + * parse postgres_fdw.application_name and set escaped string. > + * This function is almost same as log_line_prefix(), but > + * accepted escape sequence is different and this cannot understand > + * padding integer. > + * > + * Note that argument buf must be initialized. > > How about updating these comments into the following? > I thought that using the term "parse" was a bit confusing because what the > function actually does is to replace escape seequences in application_name. > Probably it's also better to rename the function, e.g., to > process_pgfdw_appname . > > - > Replace escape sequences beginning with % character in the given > application_name with status information, and return it. > - Thank you for suggestions. I confused the word "parse." Fixed and renamed. > + Same as , this is a > + printf-style string. Accepted escapes are > + bit different from , > + but padding can be used like as it. > > This description needs to be updated. I missed there. Fixed. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
At Sat, 4 Dec 2021 00:07:05 +0900, Fujii Masao wrote in > > > On 2021/11/08 22:40, kuroda.hay...@fujitsu.com wrote: > > Attached patches are the latest version. > > Thanks for updating the patch! > > + buf = makeStringInfo(); > > This is necessary only when application_name is set. So it's better to > avoid this if appname is not set. > > Currently StringInfo variable is defined in connect_pg_server() and > passed to parse_pgfdw_appname(). But there is no strong reason why the > variable needs to be defined connect_pg_server(). Right? If so how > about refactoring parse_pgfdw_appname() so that it defines > StringInfoData variable and returns the processed character string? > You can see such coding at, for example, escape_param_str() in > dblink.c. > > If this refactoring is done, probably we can get rid of "#include > "lib/stringinfo.h"" line from connection.c because connect_pg_server() > no longer needs to use StringInfo. > > + int i; > > + for (i = n - 1; i >= 0; i--) > > I'm tempted to simplify these into "for (int i = n - 1; i >= 0; i--)". > > + /* > + * Search application_name and replace it if found. > + * > + * We search paramters from the end because the later > + * one have higher priority. See also the above comment. > + */ > > How about updating these comments into the following? > > --- > Search the parameter arrays to find application_name setting, > and replace escape sequences in it with status information if found. > The arrays are searched backwards because the last value > is used if application_name is repeatedly set. > --- > > + if (strcmp(buf->data, "") != 0) > > Is this condition the same as "data[0] == '\0'"? FWIW, I agree to these. > + * parse postgres_fdw.application_name and set escaped string. > + * This function is almost same as log_line_prefix(), but > + * accepted escape sequence is different and this cannot understand > + * padding integer. > + * > + * Note that argument buf must be initialized. > > How about updating these comments into the following? > I thought that using the term "parse" was a bit confusing because what > the function actually does is to replace escape seequences in > application_name. Probably it's also better to rename the function, > e.g., to process_pgfdw_appname . It is consistent to how we have described the similar features. > - > Replace escape sequences beginning with % character in the given > application_name with status information, and return it. > - > > + if (appname == NULL) > + appname = "[unknown]"; > + if (username == NULL || *username == > '\0') > + username = "[unknown]"; > + if (dbname == NULL || *dbname == '\0') > + dbname = "[unknown]"; > > I'm still not sure why these are necessary. Could you clarify that? It probably comes from my request, just for safety for uncertain future. They are actually not needed under the current usage of the function, so *I* would not object to removing them if you are strongly feel them out of place. In that case, since NULL's leads to SEGV outright, we would even not need assertions instead. > + Same as , this is a > + printf-style string. Accepted escapes are > + bit different from , > + but padding can be used like as it. > > This description needs to be updated. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name
On 2021/11/08 22:40, kuroda.hay...@fujitsu.com wrote: Attached patches are the latest version. Thanks for updating the patch! + buf = makeStringInfo(); This is necessary only when application_name is set. So it's better to avoid this if appname is not set. Currently StringInfo variable is defined in connect_pg_server() and passed to parse_pgfdw_appname(). But there is no strong reason why the variable needs to be defined connect_pg_server(). Right? If so how about refactoring parse_pgfdw_appname() so that it defines StringInfoData variable and returns the processed character string? You can see such coding at, for example, escape_param_str() in dblink.c. If this refactoring is done, probably we can get rid of "#include "lib/stringinfo.h"" line from connection.c because connect_pg_server() no longer needs to use StringInfo. + int i; + for (i = n - 1; i >= 0; i--) I'm tempted to simplify these into "for (int i = n - 1; i >= 0; i--)". + /* +* Search application_name and replace it if found. +* +* We search paramters from the end because the later +* one have higher priority. See also the above comment. +*/ How about updating these comments into the following? --- Search the parameter arrays to find application_name setting, and replace escape sequences in it with status information if found. The arrays are searched backwards because the last value is used if application_name is repeatedly set. --- + if (strcmp(buf->data, "") != 0) Is this condition the same as "data[0] == '\0'"? + * parse postgres_fdw.application_name and set escaped string. + * This function is almost same as log_line_prefix(), but + * accepted escape sequence is different and this cannot understand + * padding integer. + * + * Note that argument buf must be initialized. How about updating these comments into the following? I thought that using the term "parse" was a bit confusing because what the function actually does is to replace escape seequences in application_name. Probably it's also better to rename the function, e.g., to process_pgfdw_appname . - Replace escape sequences beginning with % character in the given application_name with status information, and return it. - + if (appname == NULL) + appname = "[unknown]"; + if (username == NULL || *username == '\0') + username = "[unknown]"; + if (dbname == NULL || *dbname == '\0') + dbname = "[unknown]"; I'm still not sure why these are necessary. Could you clarify that? + Same as , this is a + printf-style string. Accepted escapes are + bit different from , + but padding can be used like as it. This description needs to be updated. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Horiguchi-san, Fujii-san Thank you for discussing! > Isn't it enough to perform this assertion check only once > at the top of parse_pgfdw_appname()? Fixed. > If possible, I'd like to see this change as a separate patch > and commt it first because this is the description for > the existing parameter postgres_fdw.application_name. I attached a small doc patch for this. > "pgfdw_application_name is set to application_name of a pgfdw > connection after placeholder conversion, thus the resulting string is > subject to the same restrictions to application_names.". Maybe > followed by "If session user name or database name contains non-ascii > characters, they are replaced by question marks "?"". This part was also added. Thanks! > So our current consensus is to remove the padding part > from postgres_fdw.application_name. Yeah I removed. Attached patches are the latest version. Best Regards, Hayato Kuroda FUJITSU LIMITED v22_0002_allow_escapes.patch Description: v22_0002_allow_escapes.patch v22_0001_update_descriptions.patch Description: v22_0001_update_descriptions.patch
Re: Allow escape in application_name
At Sun, 7 Nov 2021 13:35:39 +0900, Fujii Masao wrote in > > > On 2021/11/05 12:17, Kyotaro Horiguchi wrote: > If possible, I'd like to see this change as a separate patch > and commt it first because this is the description for > the existing parameter postgres_fdw.application_name. Fair enough. > >> I'd like to hear more opinions about this from others. > >> But *if* there is actually no use case, I'd like not to add > >> the feature, to make the code simpler. > > I think padding is useful because it alingns the significant content > > of log lines by equating the length of the leading fixed > > In short, I'm for to removing it by +0.7. > > So our current consensus is to remove the padding part > from postgres_fdw.application_name. I think so. > >> + case 'u': > >> + Assert(MyProcPort != NULL); > >> > >> Isn't it enough to perform this assertion check only once > >> at the top of parse_pgfdw_appname()? > > Yeah, in either way, we should treat them in the same way. > > > >>> We can force parse_pgfdw_appname() not to be called if MyProcPort does > >>> not exist, > >>> but I don't think it is needed now. > >> > >> Yes. > > (I assume you said "it is needed now".) I'm not sure how to force > > that but if it means a NULL MyProcPort cuases a crash, I think > > crashing server is needlessly too aggressive as the penatly. > > I said "Yes" for Kuroda-san's comment "I don't think it is > needed now". So I meant that "it is NOT needed now". > Sorry for unclear comment.. > > His idea was to skip calling parse_pgfdw_appname() if > MyProcPort is NULL, so as to prevent parse_pgfdw_appname() > from seeing NULL pointer of MyProcPort. But he thought > it's not necessary now, and I agree with him because > the idea would cause more confusing behavior. > > > > It seems to me that postgres-fdw asumes a valid user id, but doesn't > > make no use of databsae, server port, and process id. What I thought > > here is that making it an assertion is too much. So just ignoring the > > replacement is also fine to me. > > That being said, if we are eager not to have unused code paths, it is > > reasonable enough. I don't object strongly to replace it with an > > assertion. > > So no one strongly objects to the addition of assertion? It seems to me so. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name
On 2021/11/05 12:17, Kyotaro Horiguchi wrote: I'm not sure that that distinction is so clear for users. So I feel we want a notice something like this. But it doesn't seem correct as it is. When the user name of the session consists of non-ascii characters, %u is finally seen as a sequence of '?'. It is not a embedded strings in pgfdw_application_name. I didn't notice this behavior. "pgfdw_application_name is set to application_name of a pgfdw connection after placeholder conversion, thus the resulting string is subject to the same restrictions to application_names.". Maybe followed by "If session user name or database name contains non-ascii characters, they are replaced by question marks "?"". If we add something to the docs about this, we should clearly describe what postgres_fdw.application_name does and what postgres_fdw in a foreign server does. BTW, this kind of information was already commented in option.c. Probably it's better to mention the limitations on not only ASCII characters but also the length of application name. * Unlike application_name GUC, don't set GUC_IS_NAME flag nor check_hook * to allow postgres_fdw.application_name to be any string more than * NAMEDATALEN characters and to include non-ASCII characters. Instead, * remote server truncates application_name of remote connection to less * than NAMEDATALEN and replaces any non-ASCII characters in it with a '?' * character. If possible, I'd like to see this change as a separate patch and commt it first because this is the description for the existing parameter postgres_fdw.application_name. I'd like to hear more opinions about this from others. But *if* there is actually no use case, I'd like not to add the feature, to make the code simpler. I think padding is useful because it alingns the significant content of log lines by equating the length of the leading fixed components. application_name doesn't make any other visible components unaligned even when shown in the pg_stat_activity view. It is not useful in that sense. Padding make the string itself make look maybe nicer, but gives no other advantage. I doubt people complain to the lack of the padding feature. Addition to that, since pgfdw_application_name and log_line_prefix work different way in regard to multibyte characters, we don't need to struggle so hard to consilidate them in their behavior. # I noticed that the paddig feature doesn't consider multibyte # characters. (I'm not suggesting them ought to be handled # "prpoerly"). In short, I'm for to removing it by +0.7. So our current consensus is to remove the padding part from postgres_fdw.application_name. + case 'u': + Assert(MyProcPort != NULL); Isn't it enough to perform this assertion check only once at the top of parse_pgfdw_appname()? Yeah, in either way, we should treat them in the same way. We can force parse_pgfdw_appname() not to be called if MyProcPort does not exist, but I don't think it is needed now. Yes. (I assume you said "it is needed now".) I'm not sure how to force that but if it means a NULL MyProcPort cuases a crash, I think crashing server is needlessly too aggressive as the penatly. I said "Yes" for Kuroda-san's comment "I don't think it is needed now". So I meant that "it is NOT needed now". Sorry for unclear comment.. His idea was to skip calling parse_pgfdw_appname() if MyProcPort is NULL, so as to prevent parse_pgfdw_appname() from seeing NULL pointer of MyProcPort. But he thought it's not necessary now, and I agree with him because the idea would cause more confusing behavior. It seems to me that postgres-fdw asumes a valid user id, but doesn't make no use of databsae, server port, and process id. What I thought here is that making it an assertion is too much. So just ignoring the replacement is also fine to me. That being said, if we are eager not to have unused code paths, it is reasonable enough. I don't object strongly to replace it with an assertion. So no one strongly objects to the addition of assertion? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Allow escape in application_name
At Fri, 5 Nov 2021 03:14:00 +0900, Fujii Masao
wrote in
>
>
> On 2021/11/04 20:42, kuroda.hay...@fujitsu.com wrote:
> > Dear Fujii-san,
> >Thank you for giving comments! I attached new patches.
>
> Thanks for updating the patch!
>
> +
> + Note that if embedded strings have Non-ASCII,
> + these characters will be replaced with the question marks
> (?).
> + This limitation is caused by application_name.
> +
>
> Isn't this descripton confusing because postgres_fdw actually doesn't
> do this?
> postgres_fdw just passses the application_name as it is to the remote
> server.
I'm not sure that that distinction is so clear for users. So I feel we
want a notice something like this. But it doesn't seem correct as it
is. When the user name of the session consists of non-ascii
characters, %u is finally seen as a sequence of '?'. It is not a
embedded strings in pgfdw_application_name. I didn't notice this
behavior.
"pgfdw_application_name is set to application_name of a pgfdw
connection after placeholder conversion, thus the resulting string is
subject to the same restrictions to application_names.". Maybe
followed by "If session user name or database name contains non-ascii
characters, they are replaced by question marks "?"".
Sidetracking a bit, considering this restriction, we might need to
reconsider about %u and %d. session-id might be useful as it is
ascii-string that can identify a session exactly.
> >> On second thought, do we really need padding support for
> >> application_name
> >> in postgres_fdw? I just thought that when I read the description
> >> "Padding can be useful to aid human readability in log files." in the
> >> docs
> >> about log_line_prefix.
> >My feelings was that we don't have any reasons not to support,
> > but I cannot found some good use-cases.
> > Now I decided to keep supporting that,
> > but if we face another problem I will cut that.
>
> I'd like to hear more opinions about this from others.
> But *if* there is actually no use case, I'd like not to add
> the feature, to make the code simpler.
I think padding is useful because it alingns the significant content
of log lines by equating the length of the leading fixed
components. application_name doesn't make any other visible components
unaligned even when shown in the pg_stat_activity view. It is not
useful in that sense.
Padding make the string itself make look maybe nicer, but gives no
other advantage.
I doubt people complain to the lack of the padding feature. Addition
to that, since pgfdw_application_name and log_line_prefix work
different way in regard to multibyte characters, we don't need to
struggle so hard to consilidate them in their behavior.
# I noticed that the paddig feature doesn't consider multibyte
# characters. (I'm not suggesting them ought to be handled
# "prpoerly").
In short, I'm for to removing it by +0.7.
> >> + case 'a':
> >> + if (MyProcPort)
> >> + {
> >>
> >> I commented that the check of MyProcPort is necessary because
> >> background
> >> worker not having MyProcPort may access to the remote server. The
> >> example
> >> of such process is the resolver process that Sawada-san was proposing
> >> for
> >> atomic commit feature. But the proposal was withdrawn and for now
> >> there seems no such process. If this is true, we can safely remove the
> >> check
> >> of MyProcPort? If so, something like Assert(MyProcPort != NULL) may
> >> need
> >> to be added, instead.
> >My understating was that we don't have to assume committing the
> >Sawada-san's patch.
> > I think that FDW is only available from backend processes in the
> > current implementation,
> > and MyProcPort will be substituted when processes are initialized() -
> > in BackendInitialize().
> > Since the backend will execute BackendInitialize() after forking()
> > from the postmaster,
> > we can assume that everyone who operates FDW has a valid value for
> > MyProcPort.
> > I removed if statement and added assertion.
I think it is right.
> + case 'u':
> + Assert(MyProcPort != NULL);
>
> Isn't it enough to perform this assertion check only once
> at the top of parse_pgfdw_appname()?
Yeah, in either way, we should treat them in the same way.
> > We can force parse_pgfdw_appname() not to be called if MyProcPort does
> > not exist,
> > but I don't think it is needed now.
>
> Yes.
(I assume you said "it is needed now".) I'm not sure how to force
that but if it means a NULL MyProcPort cuases a crash, I think
crashing server is needlessly too aggressive as the penatly.
> >> If user name or database name is not set, the name is replaced with
> >> "[unknown]". log_line_prefix needs this because log message may be
> >> output when they have not been set yet, e.g., at early stage of
> >> backend
> >> startup. But I wonder if application_name in postgres_fdw actually
> >> need that.. Thought?
> >Hmm, I t
Re: Allow escape in application_name
On 2021/11/04 20:42, kuroda.hay...@fujitsu.com wrote:
Dear Fujii-san,
Thank you for giving comments! I attached new patches.
Thanks for updating the patch!
+
+ Note that if embedded strings have Non-ASCII,
+ these characters will be replaced with the question marks
(?).
+ This limitation is caused by application_name.
+
Isn't this descripton confusing because postgres_fdw actually doesn't do this?
postgres_fdw just passses the application_name as it is to the remote server.
On second thought, do we really need padding support for application_name
in postgres_fdw? I just thought that when I read the description
"Padding can be useful to aid human readability in log files." in the docs
about log_line_prefix.
My feelings was that we don't have any reasons not to support,
but I cannot found some good use-cases.
Now I decided to keep supporting that,
but if we face another problem I will cut that.
I'd like to hear more opinions about this from others.
But *if* there is actually no use case, I'd like not to add
the feature, to make the code simpler.
+ case 'a':
+ if (MyProcPort)
+ {
I commented that the check of MyProcPort is necessary because background
worker not having MyProcPort may access to the remote server. The example
of such process is the resolver process that Sawada-san was proposing for
atomic commit feature. But the proposal was withdrawn and for now
there seems no such process. If this is true, we can safely remove the check
of MyProcPort? If so, something like Assert(MyProcPort != NULL) may need
to be added, instead.
My understating was that we don't have to assume committing the Sawada-san's
patch.
I think that FDW is only available from backend processes in the current
implementation,
and MyProcPort will be substituted when processes are initialized() - in
BackendInitialize().
Since the backend will execute BackendInitialize() after forking() from the
postmaster,
we can assume that everyone who operates FDW has a valid value for MyProcPort.
I removed if statement and added assertion.
+ case 'u':
+ Assert(MyProcPort != NULL);
Isn't it enough to perform this assertion check only once
at the top of parse_pgfdw_appname()?
We can force parse_pgfdw_appname() not to be called if MyProcPort does not
exist,
but I don't think it is needed now.
Yes.
If user name or database name is not set, the name is replaced with
"[unknown]". log_line_prefix needs this because log message may be
output when they have not been set yet, e.g., at early stage of backend
startup. But I wonder if application_name in postgres_fdw actually
need that.. Thought?
Hmm, I think all of backend processes have username and database, but
here has been followed from Horiguchi-san's suggestion:
```
I'm not sure but even if user_name doesn't seem to be NULL, don't we
want to do the same thing with %u of log_line_prefix for safety?
Namely, setting [unknown] if user_name is NULL or "". The same can be
said for %d.
```
But actually I don't have strong opinions.
Ok, we can wait for more opinions about this to come.
But if user name and database name should NOT be NULL
in postgres_fdw, I think that it's better to add the assertion
check for those conditions and get rid of "[unknown]" part.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san,
Thank you for giving comments! I attached new patches.
> On second thought, do we really need padding support for application_name
> in postgres_fdw? I just thought that when I read the description
> "Padding can be useful to aid human readability in log files." in the docs
> about log_line_prefix.
My feelings was that we don't have any reasons not to support,
but I cannot found some good use-cases.
Now I decided to keep supporting that,
but if we face another problem I will cut that.
> + case 'a':
> + if (MyProcPort)
> + {
>
> I commented that the check of MyProcPort is necessary because background
> worker not having MyProcPort may access to the remote server. The example
> of such process is the resolver process that Sawada-san was proposing for
> atomic commit feature. But the proposal was withdrawn and for now
> there seems no such process. If this is true, we can safely remove the check
> of MyProcPort? If so, something like Assert(MyProcPort != NULL) may need
> to be added, instead.
My understating was that we don't have to assume committing the Sawada-san's
patch.
I think that FDW is only available from backend processes in the current
implementation,
and MyProcPort will be substituted when processes are initialized() - in
BackendInitialize().
Since the backend will execute BackendInitialize() after forking() from the
postmaster,
we can assume that everyone who operates FDW has a valid value for MyProcPort.
I removed if statement and added assertion.
We can force parse_pgfdw_appname() not to be called if MyProcPort does not
exist,
but I don't think it is needed now.
> If user name or database name is not set, the name is replaced with
> "[unknown]". log_line_prefix needs this because log message may be
> output when they have not been set yet, e.g., at early stage of backend
> startup. But I wonder if application_name in postgres_fdw actually
> need that.. Thought?
Hmm, I think all of backend processes have username and database, but
here has been followed from Horiguchi-san's suggestion:
```
I'm not sure but even if user_name doesn't seem to be NULL, don't we
want to do the same thing with %u of log_line_prefix for safety?
Namely, setting [unknown] if user_name is NULL or "". The same can be
said for %d.
```
But actually I don't have strong opinions.
> + if (appname == NULL || *appname
> == '\0')
> + appname = "[unknown]";
>
> Do we really want to replace the application name with "[unknown]"
> when application_name in the local server is not set? At least for me,
> it's intuitive to replace it with empty string in that case,
> in postgres_fdw application_name.
Yeah, I agreed that empty string should be keep here.
Currently I kept NULL case because of the above reason.
> The patch now fails to be applied to the master. Could you rebase it?
Thanks, rebased. I just moved test to the end of the sql file.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v21_0002_allow_escapes.patch
Description: v21_0002_allow_escapes.patch
v21_0001_export_padding_function.patch
Description: v21_0001_export_padding_function.patch
Re: Allow escape in application_name
On 2021/10/15 17:45, kuroda.hay...@fujitsu.com wrote:
Dear Horiguchi-san,
I'm not sure. All of it is a if-story. z/OS's isdigit is defined as
"Test for a decimal digit, as defined in the digit locale source file
and in the digit class of the LC_CTYPE category of the current
locale.", which I read it as "it accepts multibyte strings" but of
course I'm not sure that's correct.
On CentOS8, and for Japanese letters, both iswdigit(L'ー') and
iswdigit(L'０') return false so, assuming the same character
classfication scheme that is implementation dependent, I guess z/OS's
isdigit would behave the same way with CentOS's isdigit.
Hoever, regardless of the precise behavior,
But, of cause I agreed this is the crazy case.
Yes, I meant that that doesn't matter.
Hmm.. of cause I want to do because I believe this is corner case,
but we should avoid regression...
If no one can say it completely some existing methods probably should be used.
Therefore, I would like to suggest using the export and support functions again.
On second thought, do we really need padding support for application_name
in postgres_fdw? I just thought that when I read the description
"Padding can be useful to aid human readability in log files." in the docs
about log_line_prefix.
+ case 'a':
+ if (MyProcPort)
+ {
I commented that the check of MyProcPort is necessary because background
worker not having MyProcPort may access to the remote server. The example
of such process is the resolver process that Sawada-san was proposing for
atomic commit feature. But the proposal was withdrawn and for now
there seems no such process. If this is true, we can safely remove the check
of MyProcPort? If so, something like Assert(MyProcPort != NULL) may need
to be added, instead.
+ if (username == NULL || *username ==
'\0')
+ username = "[unknown]";
If user name or database name is not set, the name is replaced with
"[unknown]". log_line_prefix needs this because log message may be
output when they have not been set yet, e.g., at early stage of backend
startup. But I wonder if application_name in postgres_fdw actually
need that.. Thought?
+ if (appname == NULL || *appname == '\0')
+ appname = "[unknown]";
Do we really want to replace the application name with "[unknown]"
when application_name in the local server is not set? At least for me,
it's intuitive to replace it with empty string in that case,
in postgres_fdw application_name.
The patch now fails to be applied to the master. Could you rebase it?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Horiguchi-san, > I'm not sure. All of it is a if-story. z/OS's isdigit is defined as > "Test for a decimal digit, as defined in the digit locale source file > and in the digit class of the LC_CTYPE category of the current > locale.", which I read it as "it accepts multibyte strings" but of > course I'm not sure that's correct. > > On CentOS8, and for Japanese letters, both iswdigit(L'ー') and > iswdigit(L'０') return false so, assuming the same character > classfication scheme that is implementation dependent, I guess z/OS's > isdigit would behave the same way with CentOS's isdigit. > > Hoever, regardless of the precise behavior, > > > But, of cause I agreed this is the crazy case. > > Yes, I meant that that doesn't matter. Hmm.. of cause I want to do because I believe this is corner case, but we should avoid regression... If no one can say it completely some existing methods probably should be used. Therefore, I would like to suggest using the export and support functions again. The name is converted to parse_XXX and it locates in elog.c. How is it? Best Regards, Hayato Kuroda FUJITSU LIMITED v20_0002_allow_escapes.patch Description: v20_0002_allow_escapes.patch v20_0001_export_padding_function.patch Description: v20_0001_export_padding_function.patch
Re: Allow escape in application_name
At Wed, 13 Oct 2021 11:05:19 +, "kuroda.hay...@fujitsu.com"
wrote in
> Dear Horiguchi-san, Fujii-san,
>
> Perfect work... Thank you for replying and analyzing!
>
> > A. "^-?[0-9]+.*" : returns valid padding. p goes after the last digit.
> > B. "^[^0-9-].*" : padding = 0, p doesn't advance.
> > C. "^-[^0-9].*" : padding = 0, p advances by 1 byte.
> > D. "^-" : padding = 0, p advances by 1 byte.
> > (if *p == 0 then breaks)
>
> I confirmed them and your patterns are correct.
Thanks for the confirmation.
> > If we wan to make the behaviors C and D same with the current, the
> > else clause should be like the follows, but I don't think we need to
> > do that.
> > else
> > {
> > padding = 0;
> > if (*p == '-')
> > p++;
> > }
>
> This treatments is not complex so I want to add them if possible.
Also I don't oppose to do that.
> > One possible cause of a difference in behavior is character class
> > handling including multibyte characters of isdigit and strtol. If
> > isdigit accepts '一' as a digit (some platforms might do this) , and
> > strtol doesn't (I believe it is universal behavior), '%一0p' is
> > converted to '%' and the pointer moves onto '一'. But I don't think we
> > need to do something for such a crazy specification.
>
> Does isdigit() understand multi-byte character correctly? The arguments
> of isdigit() is just a unsigned char, and this is 1byte.
> Hence I thought that they cannot distinguish 'ー'.
..
> Sorry, but I referred some wrong doc. Please ignore here...
I'm not sure. All of it is a if-story. z/OS's isdigit is defined as
"Test for a decimal digit, as defined in the digit locale source file
and in the digit class of the LC_CTYPE category of the current
locale.", which I read it as "it accepts multibyte strings" but of
course I'm not sure that's correct.
On CentOS8, and for Japanese letters, both iswdigit(L'ー') and
iswdigit(L'０') return false so, assuming the same character
classfication scheme that is implementation dependent, I guess z/OS's
isdigit would behave the same way with CentOS's isdigit.
Hoever, regardless of the precise behavior,
> But, of cause I agreed this is the crazy case.
Yes, I meant that that doesn't matter.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
RE: Allow escape in application_name
> Does isdigit() understand multi-byte character correctly? The arguments > of isdigit() is just a unsigned char, and this is 1byte. > Hence I thought that they cannot distinguish 'ー'. Sorry, but I referred some wrong doc. Please ignore here... Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Allow escape in application_name
Dear Horiguchi-san, Fujii-san,
Perfect work... Thank you for replying and analyzing!
> A. "^-?[0-9]+.*" : returns valid padding. p goes after the last digit.
> B. "^[^0-9-].*" : padding = 0, p doesn't advance.
> C. "^-[^0-9].*" : padding = 0, p advances by 1 byte.
> D. "^-" : padding = 0, p advances by 1 byte.
> (if *p == 0 then breaks)
I confirmed them and your patterns are correct.
> If we wan to make the behaviors C and D same with the current, the
> else clause should be like the follows, but I don't think we need to
> do that.
> else
> {
> padding = 0;
> if (*p == '-')
> p++;
> }
This treatments is not complex so I want to add them if possible.
> One possible cause of a difference in behavior is character class
> handling including multibyte characters of isdigit and strtol. If
> isdigit accepts '一' as a digit (some platforms might do this) , and
> strtol doesn't (I believe it is universal behavior), '%一0p' is
> converted to '%' and the pointer moves onto '一'. But I don't think we
> need to do something for such a crazy specification.
Does isdigit() understand multi-byte character correctly? The arguments
of isdigit() is just a unsigned char, and this is 1byte.
Hence I thought that they cannot distinguish 'ー'.
Actually I considered about another thing. Maybe isdigit() just checks
whether the value of the argument is in (int)48 and (int)57, and that means that
the first part of some multi-byte characters may be accepted as digit in some
locales.
But, of cause I agreed this is the crazy case.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Re: Allow escape in application_name
At Tue, 12 Oct 2021 15:42:23 +0900 (JST), Kyotaro Horiguchi wrote in > If we code as the following: ... > A. "^-?[0-9]+.*" : same to the current > B. "^[^0-9-].*" : same to the current > C. "^-[^0-9].*" : padding = 0, p doesn't advance. > D. "^-" : padding = 0, p doesn't advance. One possible cause of a difference in behavior is character class handling including multibyte characters of isdigit and strtol. If isdigit accepts '一' as a digit (some platforms might do this) , and strtol doesn't (I believe it is universal behavior), '%一0p' is converted to '%' and the pointer moves onto '一'. But I don't think we need to do something for such a crazy specification. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name
At Tue, 12 Oct 2021 15:06:11 +0900 (JST), Kyotaro Horiguchi
wrote in
> "%4%5%6%7p" is converted to "57p". Do we need to imitate that bug
> with this patch?
So.. I try to describe the behavior for exhaustive patterns..
current:
A. "^-?[0-9]+.*" : returns valid padding. p goes after the last digit.
B. "^[^0-9-].*" : padding = 0, p doesn't advance.
C. "^-[^0-9].*" : padding = 0, p advances by 1 byte.
D. "^-" : padding = 0, p advances by 1 byte.
(if *p == 0 then breaks)
I think the patterns covers the all possibilities.
If we code as the following:
if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
{
char *endptr;
padding = strtol(p, &endptr, 10);
Assert(endptr > p);
if (*endptr == '\0')
break;
p = endptr;
}
else
padding = 0;
A. "^-?[0-9]+.*" : same to the current
B. "^[^0-9-].*" : same to the current
C. "^-[^0-9].*" : padding = 0, p doesn't advance.
D. "^-" : padding = 0, p doesn't advance.
If we wan to make the behaviors C and D same with the current, the
else clause should be like the follows, but I don't think we need to
do that.
else
{
padding = 0;
if (*p == '-')
p++;
}
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Allow escape in application_name
At Tue, 12 Oct 2021 13:25:01 +0900, Fujii Masao wrote in > > > On 2021/10/07 11:46, kuroda.hay...@fujitsu.com wrote: > > So now we can choose from followings: > >* ignore such differences and use isdigit() and strtol() > > * give up using them and implement two static support functions > >How do you think? Someone knows any other knowledge about locale? > > Replacing process_log_prefix_padding() with isdigit()+strtol() is > just refactoring and doesn't provide any new feature. So they > basically should work in the same way. If the behavior of > isdigit()+strtol() > can be different from process_log_prefix_padding(), I'd prefer to > the latter option you suggested, i.e., give up using > isdigit()+strtol(). > > OTOH, of course if the behaviors of them are the same, > I'm ok to use isdigit()+strtol(), though. Hmm. It look like behaving a bit xdifferently. At least for example, for "%-X", current code treats it as 0 padding but the patch treats it as -1. By the way, the current code is already a kind of buggy. I think I showed an example like: "%4%5%6%7p" is converted to "57p". Do we need to imitate that bug with this patch? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name
On 2021/10/07 11:46, kuroda.hay...@fujitsu.com wrote: So now we can choose from followings: * ignore such differences and use isdigit() and strtol() * give up using them and implement two static support functions How do you think? Someone knows any other knowledge about locale? Replacing process_log_prefix_padding() with isdigit()+strtol() is just refactoring and doesn't provide any new feature. So they basically should work in the same way. If the behavior of isdigit()+strtol() can be different from process_log_prefix_padding(), I'd prefer to the latter option you suggested, i.e., give up using isdigit()+strtol(). OTOH, of course if the behaviors of them are the same, I'm ok to use isdigit()+strtol(), though. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san,
Thank you for reviewing!
> + else if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
> + {
> + char *endptr;
> + padding = strtol(p, &endptr, 10);
>
> Maybe isdigit() and strtol() work differently depending on locale setting?
> If so, I'm afraid that the behavior of this new code would be different from
> process_log_prefix_padding().
(Maybe strtol() should be strtoint(). This is my fault... but anyway)
You afraid that these functions may return non-zero value if locale is not "C"
and inputs are non-ascii characters?
I read the POSIX specification(isdigit: [1], strtol: [2]) and I found that
it suggests such functions are affected by locale settings.
For isdigit():
> The isdigit() and isdigit_l() functions shall test whether c is a character
> of class digit in the current locale,
> or in the locale represented by locale, respectively; see XBD Locale.
For strtol()
> In other than the C or POSIX locale, additional locale-specific subject
> sequence forms may be accepted.
I seeked other sources, but I thought that they did not consider about
locale settings. For example, functions in numutils.c are use such functions
without any locale consideration.
And I considered executing setlocale(LC_CTYPE, "C"), but I found
the following comment pg_locale.c and I did not want to violate the policy.
> * Here is how the locale stuff is handled: LC_COLLATE and LC_CTYPE
> * are fixed at CREATE DATABASE time, stored in pg_database, and cannot
> * be changed. Thus, the effects of strcoll(), strxfrm(), isupper(),
> * toupper(), etc. are always in the same fixed locale.
But isdigit() and strtol() cannot accept multi byte character,
so I also thought we don't have to consider
that some unexpected character is set in LC_CTYPE digit category.
So now we can choose from followings:
* ignore such differences and use isdigit() and strtol()
* give up using them and implement two static support functions
How do you think? Someone knows any other knowledge about locale?
[1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/isdigit.html
[2]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Re: Allow escape in application_name
On 2021/10/04 21:53, kuroda.hay...@fujitsu.com wrote:
if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
{
char *endptr;
padding = strtol(p, &endptr, 10);
if (p == endptr)
break;
p = endptr;
}
else
padding = 0;
I removed the support function based on your suggestion,
but added some if-statement in order to treats the special case: '%-p'.
+ else if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
+ {
+ char *endptr;
+ padding = strtol(p, &endptr, 10);
Maybe isdigit() and strtol() work differently depending on locale setting?
If so, I'm afraid that the behavior of this new code would be different from
process_log_prefix_padding().
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Horiguchi-san,
Thank you for giving many comments! I attached new patches.
I'm sorry for the late reply.
> I think we don't have a predecessor of the case like this where a
> behavior is decided from object option and GUC.
>
> I'm a bit uncomfortable with .conf configuration overrides server
> options, but I also think in-session-set GUC should override server
> options. So, it's slightly against POLA but from the standpoint of
> usability +0.5 to that prioritization since I cannot come up with a
> better idea.
OK, I keep the current spec. Please tell me if you come up with something.
> I thought it is nice to share process_format_string but the function
> is too tightly coupled with ErrorData detail as you pointed off-list.
> So I came to think we cannot use the function from outside. It could
> be a possibility to make the function be just a skeleton that takes a
> list of pairs of an escaped character and the associated callback
> function but that is apparently too-much complex. (It would be worth
> doing if we share the same backbone processing with archive_command,
> restore_command, recover_end_command and so on, but that is
> necessarily accompanied by the need to change the behavior either
> log_line_prefix or others.)
I agree that combining them makes source too complex.
And we have an another problem about native language support.
Sometimes espaces becomes "[unkown]" string in log_line_prefix(),
and they are gettext()'s target. So if we combine log_line_prefix() and
parse_pgfdw_appname(),
some non-ascii characters may be set to postgres_fdw.applicaiton_name.
Of cause we can implement callback function, but I think it is the next step.
> I personally don't care even if this feature implements
> padding-reading logic differently from process_format_string, but if
> we don't like that, I would return to suggest using strtol in both
> functions.
>
> As Fujii-san pointed upthread, strtol behaves a bit different way than
> we expect here, but that can be handled by checking the starting
> characters.
> if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
> {
> char *endptr;
> padding = strtol(p, &endptr, 10);
> if (p == endptr)
> break;
> p = endptr;
> }
> else
> padding = 0;
I removed the support function based on your suggestion,
but added some if-statement in order to treats the special case: '%-p'.
And I found and fixed another bug. If users set application_name
both in server option and GUC, concatenated string was set as appname.
This is caused because we reused same StringInfo and parsing all appname string
even if we sucsess it. Now the array search will stop if sucseed,
and in failure case do resetStringInfo() and re-search.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v19_0002_allow_escapes.patch
Description: v19_0002_allow_escapes.patch
v19_0001_remove_padding_function.patch
Description: v19_0001_remove_padding_function.patch
Re: Allow escape in application_name
At Fri, 01 Oct 2021 11:23:33 +0900 (JST), Kyotaro Horiguchi wrote in > function but that is apparently too-much complex. (It would be worth > doing if we share the same backbone processing with archive_command, > restore_command, recover_end_command and so on, but that is > necessarily accompanied by the need to change the behavior either > log_line_prefix or others.) So I was daydreaming over the idea and concluded as it is no-go. I said that the fdw-application name is close to archive_command rather than log_line_prefix, but that is totally missing the point. log_line_prefix and the fdw-application name share the concept of formatting, which is not a part of archive_command and friends. The latter is made very light weight and simple due to not only the formatless-ness but also the narrow choices of escape characters. Also they work on fixed-length buffer instead of StringInfo. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name
At Mon, 27 Sep 2021 04:10:50 +, "kuroda.hay...@fujitsu.com"
wrote in
> I'm sorry for sending a bad patch...
Thank you for the new version, and sorry for making the discussion go
back and forth:p
> > + * 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?
I think we don't have a predecessor of the case like this where a
behavior is decided from object option and GUC.
I'm a bit uncomfortable with .conf configuration overrides server
options, but I also think in-session-set GUC should override server
options. So, it's slightly against POLA but from the standpoint of
usability +0.5 to that prioritization since I cannot come up with a
better idea.
I thought it is nice to share process_format_string but the function
is too tightly coupled with ErrorData detail as you pointed off-list.
So I came to think we cannot use the function from outside. It could
be a possibility to make the function be just a skeleton that takes a
list of pairs of an escaped character and the associated callback
function but that is apparently too-much complex. (It would be worth
doing if we share the same backbone processing with archive_command,
restore_command, recover_end_command and so on, but that is
necessarily accompanied by the need to change the behavior either
log_line_prefix or others.)
I personally don't care even if this feature implements
padding-reading logic differently from process_format_string, but if
we don't like that, I would return to suggest using strtol in both
functions.
As Fujii-san pointed upthread, strtol behaves a bit different way than
we expect here, but that can be handled by checking the starting
characters.
> if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
> {
> char *endptr;
> padding = strtol(p, &endptr, 10);
> if (p == endptr)
> break;
> p = endptr;
> }
> else
> padding = 0;
The code gets a bit more complex but simplification by removing the
helper function wins. strtol is slower than the original function but
it can be thought in noise-level? isdigit on some platforms seems
following locale, but it is already widely used for example while
numeric parsing so I don't think that matters. (Of course we can get
rid of isdigit by using bare comparisons.)
I think it can be a separate preparatory patch of this patch.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
RE: Allow escape in application_name
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 v18_0002_allow_escapes.patch Description: v18_0002_allow_escapes.patch
Re: Allow escape in application_name
On 2021/09/21 15:08, kuroda.hay...@fujitsu.com wrote: Dear Fujii-san, Horiguchi-san, Based on your advice, I made a patch that communize two parsing functions into one. new internal function parse_format_string() was added. (This name may be too generic...) log_line_prefix() and parse_pgfdw_appname() become just the wrapper function. My prerpimary checking was passed for server and postgres_fdw, but could you review that? Yes. Thanks for updating the 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. + * 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? + 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 Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san, Horiguchi-san, Based on your advice, I made a patch that communize two parsing functions into one. new internal function parse_format_string() was added. (This name may be too generic...) log_line_prefix() and parse_pgfdw_appname() become just the wrapper function. My prerpimary checking was passed for server and postgres_fdw, but could you review that? Best Regards, Hayato Kuroda FUJITSU LIMITED v17_0002_allow_escapes.patch Description: v17_0002_allow_escapes.patch
Re: Allow escape in application_name
On 2021/09/16 12:36, kuroda.hay...@fujitsu.com wrote: you mean that this is not strtoXXX, right? Yes, the behavior of process_log_prefix_padding() is different from strtoint() or pg_strtoint32(). For example, how the heading space characters are handled is different. If we use the name like pg_fast_strtoint(), we might be likely to replace the existing strtoint() or pg_strtoint32() with pg_fast_strtoint() because its name contains "fast", for better performance. But we should not do that because their behavior is different. If so we should go back to process_padding() Horiguchi-san, do you have any idea? At least for me process_padding() sounds like the function is actually doing the padding operation. This is not what the function does. So how about something like parse_padding(), parse_padding_format()? And I added pg_restrict keywords for compiler optimizations. I'm not sure how much it's worth doing this here. If this change is helpful for better performance or something, IMO it's better to propose this separately. I agreed that (2) and (3) breaks the rule which should override server option. Hence I chose (4), values[i] substituted to buf.data in any case. Attached is the latest version. Thanks! Will review the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san, Thanks for comments! > >> Thanks for the new version. I don't object for reusing > >> process_log_prefix_padding, but I still find it strange that the > >> function with the name 'process_padding' is in string.c. If we move > >> it to string.c, I'd like to name it "pg_fast_strtoi" or something and > >> change the interface to int pg_fast_strtoi(const char *p, char > >> **endptr) that is (roughly) compatible to strtol. What do (both) you > >> think about this? > > > > I agree that this interface might be confused. > > I changed its name and interface. How do you think? > > Actually I cannot distinguish the name is good or not, > > but I could not think of anything else... > > The name using the word "strtoint" sounds confusing to me > because the behavior of the function is different from strtoint() or > pg_strtoint32(), etc. Otherwise we can easily misunderstand that > pg_fast_strtoint() can be used as alternative of strtoint() or > pg_strtoint32(). I have no better idea for the name, though.. you mean that this is not strtoXXX, right? If so we should go back to process_padding() Horiguchi-san, do you have any idea? And I added pg_restrict keywords for compiler optimizations. > >> I didn't fully checked in what case parse_pgfdw_appname gives "" as > >> result, I feel that we should use the original value in that > >> case. That is, > >> > >>> parse_pgfdw_appname(&buf, vaues[i]); > >>> > >>> /* use the result if any, otherwise use the original string */ > >>> if (buf.data[0] != 0) > >>> values[i] = buf.data; > >>> > >>> /* break if it results in non-empty string */ > >>> if (values[0][0] != 0) > >>> break; > > Agreed. It's strange to use the application_name of the server > object in that case. There seems to be four options: > > (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 > > (2) and (3) look strange to me because we expect that > postgres_fdw.application_name should override application_name > of the server object and fallback_application_mame. > > Also reporting invalid escape sequence in application_name as it is, > i.e., (1), looks strange to me. > > So (4) looks most intuitive and similar behavior to log_line_prefix. > Thought? I agreed that (2) and (3) breaks the rule which should override server option. Hence I chose (4), values[i] substituted to buf.data in any case. Attached is the latest version. Best Regards, Hayato Kuroda FUJITSU LIMITED v16_0002_allow_escapes.patch Description: v16_0002_allow_escapes.patch
Re: Allow escape in application_name
On 2021/09/14 13:42, kuroda.hay...@fujitsu.com wrote: Dear Horiguchi-san, Thank you for giving comments! Thanks for the new version. I don't object for reusing process_log_prefix_padding, but I still find it strange that the function with the name 'process_padding' is in string.c. If we move it to string.c, I'd like to name it "pg_fast_strtoi" or something and change the interface to int pg_fast_strtoi(const char *p, char **endptr) that is (roughly) compatible to strtol. What do (both) you think about this? I agree that this interface might be confused. I changed its name and interface. How do you think? Actually I cannot distinguish the name is good or not, but I could not think of anything else... The name using the word "strtoint" sounds confusing to me because the behavior of the function is different from strtoint() or pg_strtoint32(), etc. Otherwise we can easily misunderstand that pg_fast_strtoint() can be used as alternative of strtoint() or pg_strtoint32(). I have no better idea for the name, though.. I didn't fully checked in what case parse_pgfdw_appname gives "" as result, I feel that we should use the original value in that case. That is, parse_pgfdw_appname(&buf, vaues[i]); /* use the result if any, otherwise use the original string */ if (buf.data[0] != 0) values[i] = buf.data; /* break if it results in non-empty string */ if (values[0][0] != 0) break; Agreed. It's strange to use the application_name of the server object in that case. There seems to be four options: (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 (2) and (3) look strange to me because we expect that postgres_fdw.application_name should override application_name of the server object and fallback_application_mame. Also reporting invalid escape sequence in application_name as it is, i.e., (1), looks strange to me. So (4) looks most intuitive and similar behavior to log_line_prefix. Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Horiguchi-san, Thank you for giving comments! > Thanks for the new version. I don't object for reusing > process_log_prefix_padding, but I still find it strange that the > function with the name 'process_padding' is in string.c. If we move > it to string.c, I'd like to name it "pg_fast_strtoi" or something and > change the interface to int pg_fast_strtoi(const char *p, char > **endptr) that is (roughly) compatible to strtol. What do (both) you > think about this? I agree that this interface might be confused. I changed its name and interface. How do you think? Actually I cannot distinguish the name is good or not, but I could not think of anything else... > I didn't fully checked in what case parse_pgfdw_appname gives "" as > result, I feel that we should use the original value in that > case. That is, > > > parse_pgfdw_appname(&buf, vaues[i]); > > > > /* use the result if any, otherwise use the original string */ > > if (buf.data[0] != 0) > > values[i] = buf.data; > > > > /* break if it results in non-empty string */ > > if (values[0][0] != 0) > > break; Hmm. I tested log_line_prefix() by setting it as '%z' and I found that any prefixes were not output. I think we should follow it, so currently not fixed. Best Regards, Hayato Kuroda FUJITSU LIMITED v15_0002_allow_escapes.patch Description: v15_0002_allow_escapes.patch
Re: Allow escape in application_name
At Fri, 10 Sep 2021 04:33:53 +, "kuroda.hay...@fujitsu.com"
wrote in
> Dear Hou,
>
> > I found one minor thing in the patch.
>
> Thanks!
>
> > Is it better to use Abs(padding) here ?
> > Although some existing codes are using " padding > 0 ? padding : -padding ".
>
> +1, fixed.
>
> And I found that some comments were not added above padding calculation,
> so added.
Thanks for the new version. I don't object for reusing
process_log_prefix_padding, but I still find it strange that the
function with the name 'process_padding' is in string.c. If we move
it to string.c, I'd like to name it "pg_fast_strtoi" or something and
change the interface to int pg_fast_strtoi(const char *p, char
**endptr) that is (roughly) compatible to strtol. What do (both) you
think about this?
+ /*
+* Search application_name and replace it if found.
+*
+* We search paramters from the end because the later
+* one have higher priority. See also the above comment.
+*/
+ for (i = n - 1; i >= 0; i--)
+ {
+ if (strcmp(keywords[i], "application_name") == 0)
+ {
+ parse_pgfdw_appname(&buf, values[i]);
+ values[i] = buf.data;
+
+ /*
+* If the input format is wrong and the string
becomes '\0',
+* this parameter is no longer used.
+* We conitnue searching application_name.
+*/
+ if (*values[i] != '\0')
+ break;
+ }
+ }
I didn't fully checked in what case parse_pgfdw_appname gives "" as
result, I feel that we should use the original value in that
case. That is,
> parse_pgfdw_appname(&buf, vaues[i]);
>
> /* use the result if any, otherwise use the original string */
> if (buf.data[0] != 0)
> values[i] = buf.data;
>
> /* break if it results in non-empty string */
> if (values[0][0] != 0)
> break;
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
RE: Allow escape in application_name
Dear Hou, > I found one minor thing in the patch. Thanks! > Is it better to use Abs(padding) here ? > Although some existing codes are using " padding > 0 ? padding : -padding ". +1, fixed. And I found that some comments were not added above padding calculation, so added. Best Regards, Hayato Kuroda FUJITSU LIMITED v14_0002_allow_escapes.patch Description: v14_0002_allow_escapes.patch
RE: Allow escape in application_name
>From Friday, September 10, 2021 11:24 AM kuroda.hay...@fujitsu.com > > > We can simplify the code as follows. > > > > if (values[i] != '\0') > > break; > > Fixed. And type mismatching was also fixed. > > > IMO it's better to use process_padding() to process log_line_prefix > > and postgres_fdw.application in the same way as possible. > > Which would be less confusing. > > OK, I followed that. Some comments were added above the function. Hi Kuroda-san, I found one minor thing in the patch. + appendStringInfoSpaces(buf, + padding > 0 ? padding : -padding); Is it better to use Abs(padding) here ? Although some existing codes are using " padding > 0 ? padding : -padding ". Best regards, Hou zj
RE: Allow escape in application_name
Dear Fujii-san, Thank you for quick reviewing! > We can simplify the code as follows. > > if (values[i] != '\0') > break; Fixed. And type mismatching was also fixed. > IMO it's better to use process_padding() to process log_line_prefix and > postgres_fdw.application in the same way as possible. > Which would be less confusing. OK, I followed that. Some comments were added above the function. Best Regards, Hayato Kuroda FUJITSU LIMITED v13_0002_allow_escapes.patch Description: v13_0002_allow_escapes.patch
Re: Allow escape in application_name
On 2021/09/09 21:36, kuroda.hay...@fujitsu.com wrote: In this case, I expected to use fallback_application_name instead of appname, but I missed the case where appname was set as a server option. Maybe we should do continue when buf.data is \0. Yes. + /* +* If the input format is wrong and the string becomes '\0', +* this parameter is no longer used. +* We conitnue searching application_name. +*/ + if (values[i] == '\0') + continue; + else + break; We can simplify the code as follows. if (values[i] != '\0') break; Yeah, and I found that "%+5p" is another example. Our padding function does not understand plus sign (and maybe this is the specification of printf()), but strtol() reads. I did not fix here because I lost my way. Do we treats these cases and continue to use strtol(), or use process_padding()? IMO it's better to use process_padding() to process log_line_prefix and postgres_fdw.application in the same way as possible. Which would be less confusing. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Fujii-san,
Thank you for reviewing!
> postgres_fdw gets out of the loop after processing appname even
> when buf.data is '\0'. Is this expected behavior? Because of this,
> when postgres_fdw.application_name = '%b', unprocessed appname
> of the server object is used.
In this case, I expected to use fallback_application_name instead of appname,
but I missed the case where appname was set as a server option.
Maybe we should do continue when buf.data is \0.
> + char *endptr = NULL;
> + padding = (int)strtol(p, &endptr, 10);
>
> strtol() seems to work differently from process_log_prefix_padding(),
> for example, when the input string is "%-p".
Yeah, and I found that "%+5p" is another example.
Our padding function does not understand plus sign
(and maybe this is the specification of printf()), but strtol() reads.
I did not fix here because I lost my way. Do we treats these cases
and continue to use strtol(), or use process_padding()?
> Could you tell me why the statement checking
> application_name should be wrapped in a function?
> Instead, we can just execute something like the following?
>
> SELECT COUNT(*) FROM pg_stat_activity WHERE application_name =
> current_setting('application_name') || current_user || current_database() ||
> pg_backend_pid() || '%';
I did not know current_setting() function...
The function was replaced as you expected.
> When log_line_prefix() processes "%a", "%u" or "%d" characters in
> log_line_prefix, it checks whether MyProcPort is NULL or not.
> Likewise shouldn't parse_pgfdw_appname() do the same thing?
> For now it's ok not to check that because only process having
> MyProcPort can use postgres_fdw. But in the future the process
> not having that may use postgres_fdw, for example, 2PC resolver
> process that Sawada-san is proposing to add to support automatic
> 2PC among multiple remote servers.
Sure, I did not consider about other patches. I added checks.
> +You can use escape sequences for application_name
> even if
> +it is set as a connection or a user mapping option.
> +Please refer the later section.
>
> I was thinking that application_name cannot be set in a user mapping.
I confirmed codes and found that is_valid_option() returns false
if libpq options are set. So removed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v12_0002_allow_escapes.patch
Description: v12_0002_allow_escapes.patch
Re: Allow escape in application_name
On 2021/09/08 21:32, kuroda.hay...@fujitsu.com wrote:
Dear Horiguchi-san,
Thank you for reviewing! I attached the fixed version.
Thanks for updating the patch!
+ for (i = n - 1; i >= 0; i--)
+ {
+ if (strcmp(keywords[i], "application_name") == 0)
+ {
+ parse_pgfdw_appname(&buf, values[i]);
+ values[i] = buf.data;
+ break;
+ }
postgres_fdw gets out of the loop after processing appname even
when buf.data is '\0'. Is this expected behavior? Because of this,
when postgres_fdw.application_name = '%b', unprocessed appname
of the server object is used.
+CREATE FUNCTION for_escapes() RETURNS bool AS $$
+DECLARE
+appname text;
+c bool;
+BEGIN
+SHOW application_name INTO appname;
+EXECUTE 'SELECT COUNT(*) FROM pg_stat_activity WHERE application_name
LIKE '''
Could you tell me why the statement checking
application_name should be wrapped in a function?
Instead, we can just execute something like the following?
SELECT COUNT(*) FROM pg_stat_activity WHERE application_name =
current_setting('application_name') || current_user || current_database() ||
pg_backend_pid() || '%';
+ char *endptr = NULL;
+ padding = (int)strtol(p, &endptr, 10);
strtol() seems to work differently from process_log_prefix_padding(),
for example, when the input string is "%-p".
+ case 'a':
+ {
+ const char *appname = application_name;
When log_line_prefix() processes "%a", "%u" or "%d" characters in
log_line_prefix, it checks whether MyProcPort is NULL or not.
Likewise shouldn't parse_pgfdw_appname() do the same thing?
For now it's ok not to check that because only process having
MyProcPort can use postgres_fdw. But in the future the process
not having that may use postgres_fdw, for example, 2PC resolver
process that Sawada-san is proposing to add to support automatic
2PC among multiple remote servers.
+You can use escape sequences for application_name even
if
+it is set as a connection or a user mapping option.
+Please refer the later section.
I was thinking that application_name cannot be set in a user mapping.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Horiguchi-san, Thank you for reviewing! I attached the fixed version. > Is "the previous comment" "the comment above"? Yeah, fixed. > + for (i = n -1; i >= 0; i--) > > You might want a space between - and 1. Fixed. > +parse_application_name(StringInfo buf, const char *name) > > The name seems a bit too generic as it is a function only for > postgres_fdw. Indeed. I renamed to parse_pgfdw_appname(). > + /* must be a '%', so skip to the next char */ > + p++; > + if (*p == '\0') > + break; /* format error - > ignore it */ > > I'm surprised by finding that undefined %-escapes and stray % behave > differently between archive_command and log_line_prefix. I understand > this behaves like the latter. Indeed. pgarch_archiveXlog() treats undefined escapes as nothing special, but log_line_prefix() stop parsing immediately. They have no description about it in docs. I will not treat it in this thread and follow log_line_prefix(), but I agree it is strange. > + const char *username = > MyProcPort->user_name; > > I'm not sure but even if user_name doesn't seem to be NULL, don't we > want to do the same thing with %u of log_line_prefix for safety? > Namely, setting [unknown] if user_name is NULL or "". The same can be > said for %d. I think they will be never NULL in current implementation, but your suggestion is better. Checks were added in %a, %u and %d. > + * process_padding --- helper function for processing the format > + * string in log_line_prefix > > Since this is no longer a static helper function for a specific > function, the name and the comment should be more descriptive. > > That being said, in the first place the function seems reducible > almost to a call to atol. By a quick measurement the function is about > 38% faster (0.024us/call(the function) vs 0.039us/call(strtol) so I'm > not sure we want to replace process_log_prefix_padding(), but we don't > need to reuse the function in parse_application_name since it is > called only once per connection. My first impression is that they use the function because they want to know the end of sequence and do error-handling, but I agree this can be replaced by strtol(). I changed the function to strtol() and restored process_log_prefix_padding(). How do you think? Does it follow your suggestion? Best Regards, Hayato Kuroda FUJITSU LIMITED v11_0002_allow_escapes.patch Description: v11_0002_allow_escapes.patch
Re: Allow escape in application_name
At Tue, 7 Sep 2021 11:30:27 +, "kuroda.hay...@fujitsu.com" wrote in > I attached the rebased version. Tests and descriptions were added. > In my understanding Ikeda-san's indication is included. I have some comments by a quick look. +* one have higher priority. See also the previous comment. Is "the previous comment" "the comment above"? + for (i = n -1; i >= 0; i--) You might want a space between - and 1. +parse_application_name(StringInfo buf, const char *name) The name seems a bit too generic as it is a function only for postgres_fdw. + /* must be a '%', so skip to the next char */ + p++; + if (*p == '\0') + break; /* format error - ignore it */ I'm surprised by finding that undefined %-escapes and stray % behave differently between archive_command and log_line_prefix. I understand this behaves like the latter. + const char *username = MyProcPort->user_name; I'm not sure but even if user_name doesn't seem to be NULL, don't we want to do the same thing with %u of log_line_prefix for safety? Namely, setting [unknown] if user_name is NULL or "". The same can be said for %d. + * process_padding --- helper function for processing the format + * string in log_line_prefix Since this is no longer a static helper function for a specific function, the name and the comment should be more descriptive. That being said, in the first place the function seems reducible almost to a call to atol. By a quick measurement the function is about 38% faster (0.024us/call(the function) vs 0.039us/call(strtol) so I'm not sure we want to replace process_log_prefix_padding(), but we don't need to reuse the function in parse_application_name since it is called only once per connection. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/09/08 7:46, Tom Lane wrote: Fujii Masao writes: Pushed 0001 patch. Thanks! The buildfarm shows that this test case isn't terribly reliable. Yes... Thanks for reporting this! TBH, I think you should just remove the test case altogether. It does not look useful enough to justify a permanent expenditure of testing cycles, never mind the amount of effort that would be needed to stabilize it. Ok, I will remove the tests. Kuroda-san is proposing the additional feature which allows postgres_fdw.application_name to include escape characters. If we commit the feature, it's worth adding the similar tests checking that the escape characters there are replaced with expected values. So it's better to investigate what made the tests fail, not to cause the same tests failure later. The tests use postgres_fdw_disconnect_all() to close the existing remote connections before establishing new remote connection with new application_name setting. Then they check that the expected application_name appears in pg_stat_activity. The cause of the issue seems to be that it can take a bit time for the existing remote connection (i.e., its corresponding backend) to exit after postgres_fdw_disconect_all() is called. So application_name of the existing remote connection also could appear in pg_stat_activity when it's checked next time. If this analysis is right, the tests that the upcoming patch adds should handle the case where the existing remote connection can appear in pg_stat_activity after postgres_fdw_disconect_all(). Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Fujii Masao writes: > Pushed 0001 patch. Thanks! The buildfarm shows that this test case isn't terribly reliable. TBH, I think you should just remove the test case altogether. It does not look useful enough to justify a permanent expenditure of testing cycles, never mind the amount of effort that would be needed to stabilize it. regards, tom lane
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san, Ikeda-san, > Pushed 0001 patch. Thanks! I confirmed your commit. Thanks! I attached the rebased version. Tests and descriptions were added. In my understanding Ikeda-san's indication is included. Best Regards, Hayato Kuroda FUJITSU LIMITED v10_0002_allow_escapes.patch Description: v10_0002_allow_escapes.patch
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/09/07 10:32, kuroda.hay...@fujitsu.com wrote: Dear Fujii-san, I confirmed it and I think it's OK. Other comments should be included in 0002. Pushed 0001 patch. Thanks! Could you rebase 0002 patch? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san, I confirmed it and I think it's OK. Other comments should be included in 0002. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
On 2021/09/06 21:28, Fujii Masao wrote: > > > On 2021/09/06 16:48, Masahiro Ikeda wrote: >> On 2021-09-06 13:21, kuroda.hay...@fujitsu.com wrote: >>> Dear Ikeda-san, >>> I agree that this feature is useful. Thanks for working this. >>> >>> Thanks . >>> 1) Why does postgres_fdw.application_name override the server's option? > + establishes a connection to a foreign server. This overrides > + application_name option of the server object. > + Note that change of this parameter doesn't affect any existing > + connections until they are re-established. My expected behavior was that application_name option of server object overrides the GUC because other GUCs seems behave so. For example, log_statement in postgresql.conf can be overrided by ALTER ROLE for only specific user. Should the narrower scope setting takes precedence? > > An option of a role doesn't always override a GUC setting. For example, > GUC with PGC_USERSET like work_mem, work_mem setting of a role overrides > that set in postgresql.conf, as you told. But if work_mem is set by > SET command, its value overrides the option of a role. So if we should treat > an option of foreign server object like an option of a role, we should handle > applicaion_name option for postgres_fdw in that way. > > If we want to implement this, we need to check the context of > postgres_fdw.application_name when using it. If its context is PGC_SIGHUP > or PGC_POSTMASTER, it needs to be added the connection arrays that > PQconnectParams() uses before options of server object are processed, > i.e., application_name of server object overrides the other in this case. > On the other hand, if its context is higher than PGC_SIGHUP, it needs to > be added to the arrays after options of server object are processed. > > I'm OK with the current proposal for now (i.e., postgres_fdw.application_name > always overrides that of server object) at least as the first step beucase > it's simple. But if you want that feature and can simply implement it, I don't > object to that. Thanks for explaining. I forgot to consider about SET command. I understood that SET command should override the old value, and it's difficult to manage the contexts for server options and the GUC. Sorry for my impractical proposal. >> IIUC, we can execute "ALTER SERVERS" commands automatically using scripts? >> If so, to have finer control seems to be more important than to reduce the >> effort of >> overwriting every configuration. > > You mean to execute ALTER SERVER automatically via script whenever > a client connects to the server (i.e., a client passes its application_name > to the server, the server automatically sets it to the foreign server object, > then the server uses it as application_name of the remote connection)? Sorry, Kuroda-san and Fujii-san are right. If SET command is used, ALTER SERVER doesn't work. Is it better to specify that we can use the escaped format for application_name option of the server object in the document? I think it's new feature too. >>> >>> Yeah, I agree. I will add at `Connection Options` section in 0002 patch. >>> Is it OK? Do we have more good place? >> >> +1 > > +1 > >>> Actually I did not considering about such a situation... >>> But I want to choose (1) because the limitation is caused by libpq layer >>> and the modification is almost unrelated to this patch. >>> I'm not sure why appname have such a limitation >>> so at first we should investigate it. How do you think? >> >> OK. I agree that (1) is enough for the first step. > > +1 > >> If I have any time, I'll investigate it too. > > Maybe we need to consider what happens if different clients use > application_name with different encodings. How should > pg_stat_activity.application_name and log_line_prefix, etc handle such case? Thanks. That makes sense. I'll check them. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
Re: Allow escape in application_name
On 2021/09/06 16:48, Masahiro Ikeda wrote: On 2021-09-06 13:21, kuroda.hay...@fujitsu.com wrote: Dear Ikeda-san, I agree that this feature is useful. Thanks for working this. Thanks :-). 1) Why does postgres_fdw.application_name override the server's option? > + establishes a connection to a foreign server. This overrides > + application_name option of the server object. > + Note that change of this parameter doesn't affect any existing > + connections until they are re-established. My expected behavior was that application_name option of server object overrides the GUC because other GUCs seems behave so. For example, log_statement in postgresql.conf can be overrided by ALTER ROLE for only specific user. Should the narrower scope setting takes precedence? An option of a role doesn't always override a GUC setting. For example, GUC with PGC_USERSET like work_mem, work_mem setting of a role overrides that set in postgresql.conf, as you told. But if work_mem is set by SET command, its value overrides the option of a role. So if we should treat an option of foreign server object like an option of a role, we should handle applicaion_name option for postgres_fdw in that way. If we want to implement this, we need to check the context of postgres_fdw.application_name when using it. If its context is PGC_SIGHUP or PGC_POSTMASTER, it needs to be added the connection arrays that PQconnectParams() uses before options of server object are processed, i.e., application_name of server object overrides the other in this case. On the other hand, if its context is higher than PGC_SIGHUP, it needs to be added to the arrays after options of server object are processed. I'm OK with the current proposal for now (i.e., postgres_fdw.application_name always overrides that of server object) at least as the first step beucase it's simple. But if you want that feature and can simply implement it, I don't object to that. IIUC, we can execute "ALTER SERVERS" commands automatically using scripts? If so, to have finer control seems to be more important than to reduce the effort of overwriting every configuration. You mean to execute ALTER SERVER automatically via script whenever a client connects to the server (i.e., a client passes its application_name to the server, the server automatically sets it to the foreign server object, then the server uses it as application_name of the remote connection)? Is it better to specify that we can use the escaped format for application_name option of the server object in the document? I think it's new feature too. Yeah, I agree. I will add at `Connection Options` section in 0002 patch. Is it OK? Do we have more good place? +1 +1 Actually I did not considering about such a situation... But I want to choose (1) because the limitation is caused by libpq layer and the modification is almost unrelated to this patch. I'm not sure why appname have such a limitation so at first we should investigate it. How do you think? OK. I agree that (1) is enough for the first step. +1 If I have any time, I'll investigate it too. Maybe we need to consider what happens if different clients use application_name with different encodings. How should pg_stat_activity.application_name and log_line_prefix, etc handle such case? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/09/06 10:32, kuroda.hay...@fujitsu.com wrote:
I think we should SELECT ft6 because foreign server 'loopback'
doesn't have application_name server option.
Yes. I forgot to update that... Thanks for the review!
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 82aa14a65d..705f60a3ae 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -353,10 +353,11 @@ connect_pg_server(ForeignServer *server, UserMapping
*user)
/*
* Construct connection params from generic options of
ForeignServer
* and UserMapping. (Some of them might not be libpq options,
in
-* which case we'll just waste a few array slots.) Add 3 extra
slots
-* for fallback_application_name, client_encoding, end marker.
+* which case we'll just waste a few array slots.) Add 4 extra
slots
+* for application_name, fallback_application_name,
client_encoding,
+* end marker.
*/
- n = list_length(server->options) + list_length(user->options) +
3;
+ n = list_length(server->options) + list_length(user->options) +
4;
keywords = (const char **) palloc(n * sizeof(char *));
values = (const char **) palloc(n * sizeof(char *));
@@ -366,7 +367,23 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
n += ExtractConnectionOptions(user->options,
keywords + n, values + n);
- /* Use "postgres_fdw" as fallback_application_name. */
+ /*
+* Use pgfdw_application_name as application_name if set.
+*
+* PQconnectdbParams() processes the parameter arrays from
start to
+* end. If any key word is repeated, the last value is used.
Therefore
+* note that pgfdw_application_name must be added to the arrays
after
+* options of ForeignServer are, so that it can override
+* application_name set in ForeignServer.
+*/
+ if (pgfdw_application_name && *pgfdw_application_name != '\0')
+ {
+ keywords[n] = "application_name";
+ values[n] = pgfdw_application_name;
+ n++;
+ }
+
+ /* Use "postgres_fdw" as fallback_application_name */
keywords[n] = "fallback_application_name";
values[n] = "postgres_fdw";
n++;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index e3ee30f1aa..39befa394a 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10761,3 +10761,82 @@ ERROR: invalid value for integer option "fetch_size":
100$%$#$#
CREATE FOREIGN TABLE inv_bsz (c1 int )
SERVER loopback OPTIONS (batch_size '100$%$#$#');
ERROR: invalid value for integer option "batch_size": 100$%$#$#
+-- ===
+-- test postgres_fdw.application_name GUC
+-- ===
+-- Turn debug_discard_caches off for this test to make that
+-- the remote connection is alive when checking its application_name.
+-- For each test, close all the existing cached connections manually and
+-- establish connection with new setting of application_name.
+SET debug_discard_caches = 0;
+-- If appname is set as GUC but not as options of server object,
+-- the GUC setting is used as application_name of remote connection.
+SET postgres_fdw.application_name TO 'fdw_guc_appname';
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column?
+--
+1
+(1 row)
+
+SELECT 1 FROM ft6 LIMIT 1;
+ ?column?
+--
+1
+(1 row)
+
+SELECT application_name FROM pg_stat_activity
+ WHERE application_name IN ('loopback2', 'fdw_guc_appname');
+ application_name
+--
+ fdw_guc_appname
+(1 row)
+
+-- If appname is set as options of server object but not as GUC,
+-- appname of server object is used.
+RESET postgres_fdw.application_name;
+ALTER SERVER loopback2 OPTIONS (ADD application_name 'loopback2');
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column?
+--
+1
+(1 row)
+
+SELECT 1 FROM ft6 LIMIT 1;
+ ?column?
+--
+1
+(1 row)
+
+SELECT application_name FROM pg_stat_activity
+ WHERE application_name IN ('loopback2', 'fdw_guc_appname');
+ application_name
+--
+ loopback2
+(1 row)
+
+-- If appname is set both as GUC and as options
Re: Allow escape in application_name
On 2021-09-06 13:21, kuroda.hay...@fujitsu.com wrote: Dear Ikeda-san, I agree that this feature is useful. Thanks for working this. Thanks :-). 1) Why does postgres_fdw.application_name override the server's option? > + establishes a connection to a foreign server. This overrides > + application_name option of the server object. > + Note that change of this parameter doesn't affect any existing > + connections until they are re-established. My expected behavior was that application_name option of server object overrides the GUC because other GUCs seems behave so. For example, log_statement in postgresql.conf can be overrided by ALTER ROLE for only specific user. Should the narrower scope setting takes precedence? Hmm... I didn't know the policy in other options, thank you. I set higher priority because of the following reason. When users set app name as server option and create remote connection from same backend process, they will use same even if GUC sets. In this case users must execute ALTER SERVERS every session, and I think it is inconvenient. Thanks for explaining what you thought. I understood your idea. IIUC, we can execute "ALTER SERVERS" commands automatically using scripts? If so, to have finer control seems to be more important than to reduce the effort of overwriting every configuration. But this is just my feeling. I agree to hear comments from Fujii-san and so on. I think both approaches are reasonable, so I also want to ask committer. Fujii-san, how do you think? 2) Is it better to specify that we can use the escaped format for application_name option of the server object in the document? I think it's new feature too. Yeah, I agree. I will add at `Connection Options` section in 0002 patch. Is it OK? Do we have more good place? +1 I found that '%%' need to be described in the documents of postgres_fdw.application_name. What do you think? 3) Is the following expected behavior? * 1. [coorinator] configure "postgres_fdw.application_name" as "[%d]". * 2. [coodinator] connect *Non-ASCII* database. * 3. [coodinator] execute queries for remote server. * 4. [remote] execute the following query. ``` postgres(2654193)=# SELECT application_name FROM pg_stat_activity WHERE backend_type = 'client backend' ; application_name --- psql [?] ``` One of the difference between log_line_prefix and application_name is that whether it only allow clean ASCII chars or not. So, the above behavior is expected. I think there are three choices for handling the above case. 1) Accept the above case because Non-ASCII rarely be used(?), and describe the limitation in the document. 2) Add a new characters for oid of database. 3) Lease the limitation of application_name and make it accept Non-ASCII. As a user, 3) is best for me. But it seems be out of scope this threads, so will you select 1)? Actually I did not considering about such a situation... But I want to choose (1) because the limitation is caused by libpq layer and the modification is almost unrelated to this patch. I'm not sure why appname have such a limitation so at first we should investigate it. How do you think? OK. I agree that (1) is enough for the first step. If I have any time, I'll investigate it too. Regards, -- Masahiro Ikeda NTT DATA CORPORATION
RE: Allow escape in application_name
Dear Ikeda-san, > I agree that this feature is useful. Thanks for working this. Thanks :-). > 1) > > Why does postgres_fdw.application_name override the server's option? > > > + establishes a connection to a foreign server. This overrides > > + application_name option of the server object. > > + Note that change of this parameter doesn't affect any existing > > + connections until they are re-established. > > My expected behavior was that application_name option of server object > overrides the GUC because other GUCs seems behave so. For example, > log_statement in postgresql.conf can be overrided by ALTER ROLE for > only specific user. Should the narrower scope setting takes precedence? Hmm... I didn't know the policy in other options, thank you. I set higher priority because of the following reason. When users set app name as server option and create remote connection from same backend process, they will use same even if GUC sets. In this case users must execute ALTER SERVERS every session, and I think it is inconvenient. I think both approaches are reasonable, so I also want to ask committer. Fujii-san, how do you think? > 2) > > Is it better to specify that we can use the escaped format > for application_name option of the server object in the document? > I think it's new feature too. Yeah, I agree. I will add at `Connection Options` section in 0002 patch. Is it OK? Do we have more good place? > 3) > > Is the following expected behavior? > > * 1. [coorinator] configure "postgres_fdw.application_name" as "[%d]". > * 2. [coodinator] connect *Non-ASCII* database. > * 3. [coodinator] execute queries for remote server. > * 4. [remote] execute the following query. > > ``` > postgres(2654193)=# SELECT application_name FROM pg_stat_activity WHERE > backend_type = 'client backend' ; > application_name > --- > psql > [?] > ``` > > One of the difference between log_line_prefix and application_name > is that whether it only allow clean ASCII chars or not. So, the above > behavior is expected. > > I think there are three choices for handling the above case. > > 1) Accept the above case because Non-ASCII rarely be used(?), and > describe the limitation in the document. > 2) Add a new characters for oid of database. > 3) Lease the limitation of application_name and make it accept > Non-ASCII. > > As a user, 3) is best for me. > But it seems be out of scope this threads, so will you select 1)? Actually I did not considering about such a situation... But I want to choose (1) because the limitation is caused by libpq layer and the modification is almost unrelated to this patch. I'm not sure why appname have such a limitation so at first we should investigate it. How do you think? Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Allow escape in application_name
Hi, Kuroda-san, Fujii-san I agree that this feature is useful. Thanks for working this. I've tried to use the patches and read them. I have some questions. 1) Why does postgres_fdw.application_name override the server's option? + establishes a connection to a foreign server. This overrides + application_name option of the server object. + Note that change of this parameter doesn't affect any existing + connections until they are re-established. My expected behavior was that application_name option of server object overrides the GUC because other GUCs seems behave so. For example, log_statement in postgresql.conf can be overrided by ALTER ROLE for only specific user. Should the narrower scope setting takes precedence? 2) Is it better to specify that we can use the escaped format for application_name option of the server object in the document? I think it's new feature too. 3) Is the following expected behavior? * 1. [coorinator] configure "postgres_fdw.application_name" as "[%d]". * 2. [coodinator] connect *Non-ASCII* database. * 3. [coodinator] execute queries for remote server. * 4. [remote] execute the following query. ``` postgres(2654193)=# SELECT application_name FROM pg_stat_activity WHERE backend_type = 'client backend' ; application_name --- psql [?] ``` One of the difference between log_line_prefix and application_name is that whether it only allow clean ASCII chars or not. So, the above behavior is expected. I think there are three choices for handling the above case. 1) Accept the above case because Non-ASCII rarely be used(?), and describe the limitation in the document. 2) Add a new characters for oid of database. 3) Lease the limitation of application_name and make it accept Non-ASCII. As a user, 3) is best for me. But it seems be out of scope this threads, so will you select 1)? Regards, -- Masahiro Ikeda NTT DATA CORPORATION
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san,
Thank you for updating! Your modification is very interesting and
I learn something new.
> Attached is the updated version of the patch. I removed the test
> for case (1). And I arranged the regression tests so that they are based
> on debug_discard_caches, to simplify them. Also I added and updated
> some comments and docs. Could you review this version?
I agree removing (1) because it is obvious case.
```diff
+-- If appname is set both as GUC and as options of server object,
+-- the GUC setting overrides appname of server object and is used.
+SET postgres_fdw.application_name TO 'fdw_guc_appname';
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column?
+--
+1
+(1 row)
+
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+--
+1
+(1 row)
+
+SELECT application_name FROM pg_stat_activity
+ WHERE application_name IN ('loopback2', 'fdw_guc_appname');
+ application_name
+--
+ fdw_guc_appname
+(1 row)
```
I think we should SELECT ft6 because foreign server 'loopback'
doesn't have application_name server option.
I have no comments anymore.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/09/03 14:56, kuroda.hay...@fujitsu.com wrote:
Dear Fujii-san,
Thank you for your great works. Attached is the latest version.
Thanks a lot!
I set four testcases:
(1) Sets neither GUC nor server option
(2) Sets server option, but not GUC
(3) Sets GUC but not server option
(4) Sets both GUC and server option
OK.
I confirmed it almost works fine, but I found that
fallback_application_name will be never used in our test enviroment.
It is caused because our test runner pg_regress sets PGAPPNAME to "pg_regress"
and
libpq prefer the environment variable to fallback_appname.
(I tried to control it by \setenv, but failed...)
make check uses "pg_regress", but I guess that make installcheck uses
"postgres_fdw". So the patch would cause make installcheck to fail.
I think that the case (1) is not so important, so can be removed. Thought?
Attached is the updated version of the patch. I removed the test
for case (1). And I arranged the regression tests so that they are based
on debug_discard_caches, to simplify them. Also I added and updated
some comments and docs. Could you review this version?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 82aa14a65d..705f60a3ae 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -353,10 +353,11 @@ connect_pg_server(ForeignServer *server, UserMapping
*user)
/*
* Construct connection params from generic options of
ForeignServer
* and UserMapping. (Some of them might not be libpq options,
in
-* which case we'll just waste a few array slots.) Add 3 extra
slots
-* for fallback_application_name, client_encoding, end marker.
+* which case we'll just waste a few array slots.) Add 4 extra
slots
+* for application_name, fallback_application_name,
client_encoding,
+* end marker.
*/
- n = list_length(server->options) + list_length(user->options) +
3;
+ n = list_length(server->options) + list_length(user->options) +
4;
keywords = (const char **) palloc(n * sizeof(char *));
values = (const char **) palloc(n * sizeof(char *));
@@ -366,7 +367,23 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
n += ExtractConnectionOptions(user->options,
keywords + n, values + n);
- /* Use "postgres_fdw" as fallback_application_name. */
+ /*
+* Use pgfdw_application_name as application_name if set.
+*
+* PQconnectdbParams() processes the parameter arrays from
start to
+* end. If any key word is repeated, the last value is used.
Therefore
+* note that pgfdw_application_name must be added to the arrays
after
+* options of ForeignServer are, so that it can override
+* application_name set in ForeignServer.
+*/
+ if (pgfdw_application_name && *pgfdw_application_name != '\0')
+ {
+ keywords[n] = "application_name";
+ values[n] = pgfdw_application_name;
+ n++;
+ }
+
+ /* Use "postgres_fdw" as fallback_application_name */
keywords[n] = "fallback_application_name";
values[n] = "postgres_fdw";
n++;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index e3ee30f1aa..6a7d83c81f 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -10761,3 +10761,82 @@ ERROR: invalid value for integer option "fetch_size":
100$%$#$#
CREATE FOREIGN TABLE inv_bsz (c1 int )
SERVER loopback OPTIONS (batch_size '100$%$#$#');
ERROR: invalid value for integer option "batch_size": 100$%$#$#
+-- ===
+-- test postgres_fdw.application_name GUC
+-- ===
+-- Turn debug_discard_caches off for this test to make that
+-- the remote connection is alive when checking its application_name.
+-- For each test, close all the existing cached connections manually and
+-- establish connection with new setting of application_name.
+SET debug_discard_caches = 0;
+-- If appname is set as GUC but not as options of server object,
+-- the GUC setting is used as application_name of remote connection.
+SET postgres_fdw.application_name TO 'fdw_guc_appname';
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column?
+--
+1
+(1 row)
+
+SE
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san,
Thank you for your great works. Attached is the latest version.
> Thanks! What about updating the comments furthermore as follows?
>
> -
> Use pgfdw_application_name as application_name if set.
>
> PQconnectdbParams() processes the parameter arrays from start to end.
> If any key word is repeated, the last value is used. Therefore note that
> pgfdw_application_name must be added to the arrays after options of
> ForeignServer are, so that it can override application_name set in
> ForeignServer.
> -
It's more friendly than mine because it mentions
about specification about PQconnectdbParams().
Fixed like yours.
> + }
> + /* Use "postgres_fdw" as fallback_application_name */
>
> It's better to add new empty line between these two lines.
Fixed.
> +-- Disconnect once because the value is used only when establishing
> connections
> +DO $$
> + BEGIN
> + PERFORM postgres_fdw_disconnect_all();
> + END
> +$$;
>
> Why does DO command need to be used here to execute
> postgres_fdw_disconnect_all()? Instead, we can just execute
> "SELECT 1 FROM postgres_fdw_disconnect_all();"?
DO command was used because I want to
ignore the returning value of postgres_fdw_disconnect_all().
Currently this function retruns false, but if other tests are modified,
some connection may remain and the function may become to return true.
I seeked sql file and I found that this function was called by your way.
Hence I fixed.
> For test coverage, it's better to test at least the following three cases?
>
> (1) appname is set in neither GUC nor foreign server
> -> "postgres_fdw" set in fallback_application_name is used
> (2) appname is set in foreign server, but not in GUC
> -> appname in foreign server is used
> (3) appname is set both in GUC and foreign server
>-> appname in GUC is used
I set four testcases:
(1) Sets neither GUC nor server option
(2) Sets server option, but not GUC
(3) Sets GUC but not server option
(4) Sets both GUC and server option
I confirmed it almost works fine, but I found that
fallback_application_name will be never used in our test enviroment.
It is caused because our test runner pg_regress sets PGAPPNAME to "pg_regress"
and
libpq prefer the environment variable to fallback_appname.
(I tried to control it by \setenv, but failed...)
> +SELECT FROM ft1 LIMIT 1;
>
> "1" should be added just after SELECT in the above statement?
> Because postgres_fdw regression test basically uses "SELECT 1 FROM ..."
> in other places.
Fixed.
> + DefineCustomStringVariable("postgres_fdw.application_name",
> +"Sets the
> application name. This is used when connects to the remote server.",
>
> What about simplifying this description as follows?
>
> ---
> Sets the application name to be used on the remote server.
> ---
+1.
> + Configuration Parameters
> +
>
> The empty characters just after and before should be removed?
I checked other sgml file and agreed. Fixed.
And I found that including string.h is no more needed. Hence it is removed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v07_0001_add_application_name_GUC.patch
Description: v07_0001_add_application_name_GUC.patch
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/09/02 18:27, kuroda.hay...@fujitsu.com wrote:
I added the following comments:
```diff
- /* Use "postgres_fdw" as fallback_application_name. */
+ /*
+* Use pgfdw_application_name as application_name.
+*
+* Note that this check must be behind constructing generic
options
+* because pgfdw_application_name has higher priority.
+*/
```
Thanks! What about updating the comments furthermore as follows?
-
Use pgfdw_application_name as application_name if set.
PQconnectdbParams() processes the parameter arrays from start to end.
If any key word is repeated, the last value is used. Therefore note that
pgfdw_application_name must be added to the arrays after options of
ForeignServer are, so that it can override application_name set in
ForeignServer.
-
Attached is the fixed version. 0002 will be rebased later.
Thanks for updating the patch!
+ }
+ /* Use "postgres_fdw" as fallback_application_name */
It's better to add new empty line between these two lines.
+-- Disconnect once because the value is used only when establishing connections
+DO $$
+ BEGIN
+ PERFORM postgres_fdw_disconnect_all();
+ END
+$$;
Why does DO command need to be used here to execute
postgres_fdw_disconnect_all()? Instead, we can just execute
"SELECT 1 FROM postgres_fdw_disconnect_all();"?
For test coverage, it's better to test at least the following three cases?
(1) appname is set in neither GUC nor foreign server
-> "postgres_fdw" set in fallback_application_name is used
(2) appname is set in foreign server, but not in GUC
-> appname in foreign server is used
(3) appname is set both in GUC and foreign server
-> appname in GUC is used
+SELECT FROM ft1 LIMIT 1;
"1" should be added just after SELECT in the above statement?
Because postgres_fdw regression test basically uses "SELECT 1 FROM ..."
in other places.
+ DefineCustomStringVariable("postgres_fdw.application_name",
+ "Sets the application
name. This is used when connects to the remote server.",
What about simplifying this description as follows?
---
Sets the application name to be used on the remote server.
---
+ Configuration Parameters
+
The empty characters just after and before should be removed?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san, Thank you for reviewing! > This GUC parameter should be set after the options of foreign server > are set so that its setting can override the server-level ones. > Isn't it better to comment this? I added the following comments: ```diff - /* Use "postgres_fdw" as fallback_application_name. */ + /* +* Use pgfdw_application_name as application_name. +* +* Note that this check must be behind constructing generic options +* because pgfdw_application_name has higher priority. +*/ ``` > Do we really need this check hook? Even without that, any non-ASCII characters > in application_name would be replaced with "?" in the foreign PostgreSQL > server > when it's passed to that. > > On the other hand, if it's really necessary, application_name set in foreign > server object also needs to be processed in the same way. I added check_hook because I want to make sure that input for parse function has only ascii characters. But in this phase we don't have such a function, so removed. (Actually I'm not sure it is really needed, so I will check until next phase) > Why is GUC_IS_NAME flag necessary? I thought again and I noticed even if extremely long string is specified it will be truncated at server-side. This check is duplicated so removed. Maybe such a check is a user-responsibility. > postgres_fdw.application_name overrides application_name set in foreign server > object. > Shouldn't this information be documented? I added descriptions to postgres_fdw.sgml. > Isn't it better to have the regression test for this feature? +1, added. In that test SET the parameter and check pg_stat_activity. Attached is the fixed version. 0002 will be rebased later. Best Regards, Hayato Kuroda FUJITSU LIMITED v06_0001_add_application_name_GUC.patch Description: v06_0001_add_application_name_GUC.patch
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/09/01 19:04, kuroda.hay...@fujitsu.com wrote:
OK, I split and attached like that. 0001 adds new GUC, and
0002 allows to accept escapes.
Thanks for splitting and updating the patches!
Here are the comments for 0001 patch.
- /* Use "postgres_fdw" as fallback_application_name. */
+ /* Use GUC paramter if set */
+ if (pgfdw_application_name && *pgfdw_application_name != '\0')
This GUC parameter should be set after the options of foreign server
are set so that its setting can override the server-level ones.
Isn't it better to comment this?
+static bool
+check_pgfdw_application_name(char **newval, void **extra, GucSource source)
+{
+ /* Only allow clean ASCII chars in the application name */
+ if (*newval)
+ pg_clean_ascii(*newval);
+ return true;
Do we really need this check hook? Even without that, any non-ASCII characters
in application_name would be replaced with "?" in the foreign PostgreSQL server
when it's passed to that.
On the other hand, if it's really necessary, application_name set in foreign
server object also needs to be processed in the same way.
+ NULL,
+ PGC_USERSET,
+ GUC_IS_NAME,
Why is GUC_IS_NAME flag necessary?
postgres_fdw.application_name overrides application_name set in foreign server
object.
Shouldn't this information be documented?
Isn't it better to have the regression test for this feature?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san, > Can we split the patch into two as follows? If so, we can review > and commit them one by one. > > #1. Add application_name GUC into postgres_fdw > #2. Allow to specify special escape characters in application_name that > postgres_fdw uses. OK, I split and attached like that. 0001 adds new GUC, and 0002 allows to accept escapes. > For now I've not found real use case that implementing the feature > in libpq would introduce. So IMO postgres_fdw version is better. +1, so I'll focus on the this version. Best Regards, Hayato Kuroda FUJITSU LIMITED v05_0002_allow_to_escape.patch Description: v05_0002_allow_to_escape.patch v05_0001_add_application_name_GUC.patch Description: v05_0001_add_application_name_GUC.patch
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/08/31 16:11, kuroda.hay...@fujitsu.com wrote: Dear Fujii-san, I attached new version, that almost all codes moved from libpq to postgres_fdw. Thanks for updating the patch! Can we split the patch into two as follows? If so, we can review and commit them one by one. #1. Add application_name GUC into postgres_fdw #2. Allow to specify special escape characters in application_name that postgres_fdw uses. Now we can accept four types of escapes. All escapes will be rewritten to connection souce's information: * application_name, * user name, * database name, and * backend's pid. +1 These are cannot be set by log_line_prefix, so I think it is useful. We can use escape sequences in two ways: * Sets postgres_fdw.application_name GUC paramter, or * Sets application_name option in CREATE SERVER command. OK. Which version do you like? For now I've not found real use case that implementing the feature in libpq would introduce. So IMO postgres_fdw version is better. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san, I attached new version, that almost all codes moved from libpq to postgres_fdw. Now we can accept four types of escapes. All escapes will be rewritten to connection souce's information: * application_name, * user name, * database name, and * backend's pid. These are cannot be set by log_line_prefix, so I think it is useful. We can use escape sequences in two ways: * Sets postgres_fdw.application_name GUC paramter, or * Sets application_name option in CREATE SERVER command. Which version do you like? Best Regards, Hayato Kuroda FUJITSU LIMITED v04_0002_add_guc.patch Description: v04_0002_add_guc.patch v04_0001_move_process_padding.patch Description: v04_0001_move_process_padding.patch
RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Dear Fujii-san, Thank you for replying! I attached new version. > Why did you make even %u and %d available in application_name? Actually no particular reason. I added them because they can easily add... And I agree what you say, so removed. > So some people may want to specify their own ID in application_name > when connecting to the foreign server. For now they can do that by > setting application_name per foreign server object. But this means that > every session connecting to the same foreign server basically should > use the same application_name. If they want to change the setting, > they need to execute "ALTER SERAVER ... OPTIONS ...". Isn't this inconvenient? Yeah, currently such users must execute ALTER command for each time. > If yes, what about allowing us to specify foreign server's application_name > per session? For example, we can add postgres_fdw.application_name GUC, > and then if it's set postgres_fdw always uses it instead of the server-level > option when connecting to the foreign server. Thought? OK, I added the GUC parameter. My patch Defines new GUC at _PG_init(). The value is used when backends started to connect to remote server. Normal users can modify the value by SET commands but that change does not propagate to the established connections. I'm not sure about the versioning policy about contrib. Do we have to make new version? Any other objects are not changed. Finally, I and Fujii-san were now discussing locally whether this replacement should be in libpq or postgres_fdw layer. I started to think that it should be postgres_fdw, because: * previously aplications could distinguish by using current applicaiton_name, * libpq is used almost all uses so some modifications might affect someone, and * libpq might be just a tool, and should not be intelligent I will make and attach another version that new codes move to postgres_fdw. So could you vote which version is better? Best Regards, Hayato Kuroda FUJITSU LIMITED v02_0002_add_guc.patch Description: v02_0002_add_guc.patch v02_0001_allow_escape_in_application_name.patch Description: v02_0001_allow_escape_in_application_name.patch
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
On 2021/08/05 12:18, kuroda.hay...@fujitsu.com wrote: Dear Hackers, Tom, (I changed subject because this is no longer postgres_fdw's patch) What would be better to think about is how to let users specify this kind of behavior for themselves. I think it's possible to set application_name as part of a foreign server's connection options, but at present the result would only be a constant string. Somebody who wished the PID to be in there would like to have some sort of formatting escape, say "%p" for PID. Extrapolating wildly, maybe we could make all the %-codes known to log_line_prefix available here. I think your argument is better than mine. I will try to implement this approach. I made a patch based on your advice. I add parse and rewrite function in connectOptions2(). I implemented in libpq layer, hence any other application can be used. Here is an example: ``` $ env PGAPPNAME=000%p%utest000 psql -d postgres -c "show application_name" application_name --- 00025632hayatotest000 (1 row) ``` Why did you make even %u and %d available in application_name? With the patch, they are replaced with the user name to connect as and the database name to connect to, respectively. Unlike %p (i.e., PID in *client* side), those information can be easily viewed via log_line_prefix and pg_stat_activity, etc in the server side. So I'm not sure how much helpful to expose them also in application_name. I don't think this is a great idea as-is. People who need to do this sort of thing will all have their own ideas of what they need to track --- most obviously, it might be appropriate to include the originating server's name, else you don't know what machine the PID is for. So some people may want to specify their own ID in application_name when connecting to the foreign server. For now they can do that by setting application_name per foreign server object. But this means that every session connecting to the same foreign server basically should use the same application_name. If they want to change the setting, they need to execute "ALTER SERAVER ... OPTIONS ...". Isn't this inconvenient? If yes, what about allowing us to specify foreign server's application_name per session? For example, we can add postgres_fdw.application_name GUC, and then if it's set postgres_fdw always uses it instead of the server-level option when connecting to the foreign server. Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
