Re: Allow escape in application_name

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