Re: Allow escape in application_name

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