Re: Allow escape in application_name

2022-01-03 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2022-01-03 Thread Masahiko Sawada
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

Re: Allow escape in application_name

2022-01-03 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-12-28 Thread Masahiko Sawada
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

Re: Allow escape in application_name

2021-12-27 Thread Fujii Masao
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)

Re: Allow escape in application_name

2021-12-27 Thread Masahiko Sawada
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: > > ``` >

RE: Allow escape in application_name

2021-12-27 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-12-27 Thread Masahiko Sawada
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

RE: Allow escape in application_name

2021-12-26 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-12-26 Thread Fujii Masao
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?

RE: Allow escape in application_name

2021-12-26 Thread kuroda.hay...@fujitsu.com
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

2021-12-24 Thread Fujii Masao
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.

Re: Allow escape in application_name

2021-12-23 Thread Kyotaro Horiguchi
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) > >>{ > >>

Re: Allow escape in application_name

2021-12-23 Thread Fujii Masao
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]); /*

Re: Allow escape in application_name

2021-12-16 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-12-16 Thread Fujii Masao
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 &&

Re: Allow escape in application_name

2021-12-16 Thread Kyotaro Horiguchi
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') > > >

RE: Allow escape in application_name

2021-12-16 Thread kuroda.hay...@fujitsu.com
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

RE: Allow escape in application_name

2021-12-16 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-12-15 Thread Fujii Masao
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]) !=

RE: Allow escape in application_name

2021-12-15 Thread kuroda.hay...@fujitsu.com
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]

Re: Allow escape in application_name

2021-12-15 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-12-09 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-12-09 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-12-09 Thread kuroda.hay...@fujitsu.com
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 >

Re: Allow escape in application_name

2021-12-08 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-12-07 Thread kuroda.hay...@fujitsu.com
Dear Horiguchi-san Thank you for giving comments! I attached new patches. > > + if (appname == NULL) > > + appname = "[unknown]"; > > + if (username == NULL || *username > == '\0') > > +

RE: Allow escape in application_name

2021-12-07 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-12-05 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-12-03 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-11-08 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-11-07 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-11-06 Thread Fujii Masao
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

Re: Allow escape in application_name

2021-11-04 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-11-04 Thread Fujii Masao
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 (?). +

RE: Allow escape in application_name

2021-11-04 Thread kuroda.hay...@fujitsu.com
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 >

Re: Allow escape in application_name

2021-11-02 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-10-15 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-10-13 Thread Kyotaro Horiguchi
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

RE: Allow escape in application_name

2021-10-13 Thread kuroda.hay...@fujitsu.com
> 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

2021-10-13 Thread kuroda.hay...@fujitsu.com
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. "^-" :

Re: Allow escape in application_name

2021-10-12 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-10-12 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-10-12 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-10-11 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-10-06 Thread kuroda.hay...@fujitsu.com
Dear Fujii-san, Thank you for reviewing! > + else if (*p == '-' ? isdigit(p[1]) : isdigit(p[0])) > + { > + char *endptr; > + padding = strtol(p, , 10); > > Maybe isdigit() and strtol() work differently depending on locale setting?

Re: Allow escape in application_name

2021-10-05 Thread Fujii Masao
On 2021/10/04 21:53, kuroda.hay...@fujitsu.com wrote: if (*p == '-' ? isdigit(p[1]) : isdigit(p[0])) { char *endptr; padding = strtol(p, , 10); if (p == endptr)

RE: Allow escape in application_name

2021-10-04 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-10-01 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name

2021-09-30 Thread Kyotaro Horiguchi
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

RE: Allow escape in application_name

2021-09-26 Thread kuroda.hay...@fujitsu.com
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'; >

Re: Allow escape in application_name

2021-09-23 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-09-21 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-09-15 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-09-15 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-09-15 Thread Fujii Masao
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

RE: Allow escape in application_name

2021-09-13 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-09-13 Thread Kyotaro Horiguchi
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

RE: Allow escape in application_name

2021-09-09 Thread kuroda.hay...@fujitsu.com
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,

RE: Allow escape in application_name

2021-09-09 Thread houzj.f...@fujitsu.com
>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 > >

RE: Allow escape in application_name

2021-09-09 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-09-09 Thread Fujii Masao
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. + /* +

RE: Allow escape in application_name

2021-09-09 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-09-09 Thread Fujii Masao
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],

RE: Allow escape in application_name

2021-09-08 Thread kuroda.hay...@fujitsu.com
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)

Re: Allow escape in application_name

2021-09-07 Thread Kyotaro Horiguchi
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-07 Thread Fujii Masao
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-07 Thread Tom Lane
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

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-07 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-06 Thread Fujii Masao
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

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-06 Thread kuroda.hay...@fujitsu.com
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

2021-09-06 Thread Masahiro Ikeda
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

Re: Allow escape in application_name

2021-09-06 Thread Fujii Masao
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-06 Thread Fujii Masao
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

Re: Allow escape in application_name

2021-09-06 Thread Masahiro Ikeda
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 >

RE: Allow escape in application_name

2021-09-05 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name

2021-09-05 Thread Masahiro Ikeda
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

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-05 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-03 Thread Fujii Masao
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)

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-02 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-02 Thread Fujii Masao
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. +* +*

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-02 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-01 Thread Fujii Masao
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

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-09-01 Thread kuroda.hay...@fujitsu.com
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.

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-08-31 Thread Fujii Masao
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

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-08-31 Thread kuroda.hay...@fujitsu.com
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

RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-08-26 Thread kuroda.hay...@fujitsu.com
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

Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-08-19 Thread Fujii Masao
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

Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

2021-08-04 Thread kuroda.hay...@fujitsu.com
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,