Re: Allow escape in application_name

Fujii Masao <masao.fujii@oss.nttdata.com>

From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: kuroda.hayato@fujitsu.com, houzj.fnst@fujitsu.com, ikedamsh@oss.nttdata.com, pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us
Date: 2021-12-23T14:10:38Z
Lists: pgsql-hackers

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 that case.

> 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



Commits

  1. postgres_fdw: Add regression test for postgres_fdw.application_name GUC.

  2. postgres_fdw: Allow postgres_fdw.application_name to include escape sequences.

  3. doc: Add note about postgres_fdw.application_name.

  4. postgres_fdw: Revert unstable tests for postgres_fdw.application_name.

  5. postgres_fdw: Allow application_name of remote connection to be set via GUC.