Re: Allow escape in application_name

On 2021/12/09 19:52, kuroda(dot)hayato(at)fujitsu(dot)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 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?

>> 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().

>> + <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_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