Обсуждение: Re: Allow escape in application_name

At Sat, 4 Dec 2021 00:07:05 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/11/08 22:40, kuroda.hayato@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;
> <snip>
> +        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 <xref linkend="guc-log-line-prefix"/>, this is a
> +      <function>printf</function>-style string. Accepted escapes are
> +      bit different from <xref linkend="guc-log-line-prefix"/>,
> +      but padding can be used like as it.
> 
> This description needs to be updated.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

On 2021/12/07 18:48, kuroda.hayato@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
tocheck whether they are NULL or not for some reasons or future usage, e.g., background worker not logging into any
specifieddatabase may set application_name to '%d' and connect to a foreign server in the future, let's change the code
laterwith 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
containeven non-ASCII characters, unlike application_name. So how about using the following description, instead?
 

-----------------
<varname>postgres_fdw.application_name</varname> can be any string of any length and contain even non-ASCII characters.
Howeverwhen it's passed to and used as <varname>application_name</varname> in a foreign server, note that it will be
truncatedto less than <symbol>NAMEDATALEN</symbol> characters and any characters other than printable ASCII ones in it
willbe replaced with question marks (<literal>?</literal>).
 
-----------------

+        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_namestring will be released soon? Or it's better to call pfree()?
 

+      Same as <xref linkend="guc-log-line-prefix"/>, this is a
+      <function>printf</function>-style string. Unlike <xref 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?

-----------------
<literal>%</literal> characters begin <quote>escape sequences</quote> that are replaced with status information as
outlinedbelow. Unrecognized escapes are ignored. Other characters are copied straight to the application name. Note
thatit's not allowed to specify a plus/minus sign or a numeric literal after the <literal>%</literal> and before the
option,for alignment and padding.
 
-----------------

+         <entry><literal>%u</literal></entry>
+         <entry>Local user name</entry>

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
descriptioninto the docs.
 

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

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?
> 
> -----------------
> <varname>postgres_fdw.application_name</varname> can be any string of any
> length and contain even non-ASCII characters. However when it's passed to and
> used as <varname>application_name</varname> in a foreign server, note that it
> will be truncated to less than <symbol>NAMEDATALEN</symbol> characters
> and any characters other than printable ASCII ones in it will be replaced with
> question marks (<literal>?</literal>).
> -----------------

+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 <xref linkend="guc-log-line-prefix"/>, this is a
> +      <function>printf</function>-style string. Unlike <xref
> 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?
> 
> -----------------
> <literal>%</literal> characters begin <quote>escape sequences</quote> 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
> <literal>%</literal> and before the option, for alignment and padding.
> -----------------

Fixed.

> +         <entry><literal>%u</literal></entry>
> +         <entry>Local user name</entry>
> 
> 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

On 2021/12/09 19:52, kuroda.hayato@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?
>>
>> -----------------
>> <varname>postgres_fdw.application_name</varname> can be any string of any
>> length and contain even non-ASCII characters. However when it's passed to and
>> used as <varname>application_name</varname> in a foreign server, note that it
>> will be truncated to less than <symbol>NAMEDATALEN</symbol> characters
>> and any characters other than printable ASCII ones in it will be replaced with
>> question marks (<literal>?</literal>).
>> -----------------
> 
> +1, Fixed.

Could you tell me why you added new paragraph into the middle of existing paragraph? This change caused the following
errorwhen compiling the docs.
 

postgres-fdw.sgml:952: parser error : Opening and ending tag mismatch: listitem line 934 and para
      </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
NULLjust after pfree(). Otherwise pfree()'d "data" can be pfree()'d again at the end of connect_pg_server().
 

>> +         <entry><literal>%u</literal></entry>
>> +         <entry>Local user name</entry>
>>
>> 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_namein 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

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
>       </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

On 2021/12/10 16:35, kuroda.hayato@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 <xref linkend="guc-application-name"/> 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 CORPORATION

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

On 2021/12/16 11:53, kuroda.hayato@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
bedropped. 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
addedthe 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
readthe patch, ISTM that the patch has no same issue. But could you double check that?
 

[1]
https://postgr.es/m/848ff477-effd-42b9-8b25-3f7cfe289398@oss.nttdata.com

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

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

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

At Fri, 17 Dec 2021 02:42:25 +0000, "kuroda.hayato@fujitsu.com" <kuroda.hayato@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

On 2021/12/17 12:06, Kyotaro Horiguchi wrote:
> At Fri, 17 Dec 2021 02:42:25 +0000, "kuroda.hayato@fujitsu.com" <kuroda.hayato@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
checkwe can avoid unnecessary call of process_pgfdw_appname() when the value is NULL. OTOH, of course we can also
removethe 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
casewhether we add the check or not can affect the behavior (i.e., escape sequence is replace with "[unknown]" or not).
SoISTM 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
stringrather than "[unknown]" when appname or username, etc was NULL or '\0'. To implement this behavior, I argued to
removethe 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-035a4a68fc61@oss.nttdata.com

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Fri, 17 Dec 2021 14:50:37 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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

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
thatcase.
 

> 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

At Thu, 23 Dec 2021 23:10:38 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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

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

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

On 2021/12/27 10:40, kuroda.hayato@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
thetest stable even in that case, the patch uses substring() is truncate application_name string in the test query's
conditionto less than NAMEDATALEN.
 

[1]
https://postgr.es/m/20211224.184406.814784272581964942.horikyota.ntt@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 CORPORATION

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

On Mon, Dec 27, 2021 at 1:40 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2021/12/27 10:40, kuroda.hayato@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
makethe test stable even in that case, the patch uses substring() is truncate application_name string in the test
query'scondition 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/

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

On Tue, Dec 28, 2021 at 8:57 AM kuroda.hayato@fujitsu.com
<kuroda.hayato@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/

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
versionof 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 CORPORATION

On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao <masao.fujii@oss.nttdata.com> 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
versionof 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/

At Wed, 29 Dec 2021 10:34:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao <masao.fujii@oss.nttdata.com> 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
versionof 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