Re: Allow escape in application_name

On 2021/11/04 20:42, kuroda.hayato@fujitsu.com wrote:
> Dear Fujii-san,
> 
> Thank you for giving comments! I attached new patches.

Thanks for updating the patch!

+     <para>
+     Note that if embedded strings have Non-ASCII,
+     these characters will be replaced with the question marks (<literal>?</literal>).
+     This limitation is caused by <literal>application_name</literal>.
+     </para>

Isn't this descripton confusing because postgres_fdw actually doesn't do this?
postgres_fdw just passses the application_name as it is to the remote server.


>> On second thought, do we really need padding support for application_name
>> in postgres_fdw? I just thought that when I read the description
>> "Padding can be useful to aid human readability in log files." in the docs
>> about log_line_prefix.
> 
> My feelings was that we don't have any reasons not to support,
> but I cannot found some good use-cases.
> Now I decided to keep supporting that,
> but if we face another problem I will cut that.

I'd like to hear more opinions about this from others.
But *if* there is actually no use case, I'd like not to add
the feature, to make the code simpler.


>> +            case 'a':
>> +                if (MyProcPort)
>> +                {
>>
>> I commented that the check of MyProcPort is necessary because background
>> worker not having MyProcPort may access to the remote server. The example
>> of such process is the resolver process that Sawada-san was proposing for
>> atomic commit feature. But the proposal was withdrawn and for now
>> there seems no such process. If this is true, we can safely remove the check
>> of MyProcPort? If so, something like Assert(MyProcPort != NULL) may need
>> to be added, instead.
> 
> My understating was that we don't have to assume committing the Sawada-san's patch.
> I think that FDW is only available from backend processes in the current implementation,
> and MyProcPort will be substituted when processes are initialized() - in BackendInitialize().
> Since the backend will execute BackendInitialize() after forking() from the postmaster,
> we can assume that everyone who operates FDW has a valid value for MyProcPort.
> I removed if statement and added assertion.

+            case 'u':
+                Assert(MyProcPort != NULL);

Isn't it enough to perform this assertion check only once
at the top of parse_pgfdw_appname()?


> We can force parse_pgfdw_appname() not to be called if MyProcPort does not exist,
> but I don't think it is needed now.

Yes.


>> If user name or database name is not set, the name is replaced with
>> "[unknown]". log_line_prefix needs this because log message may be
>> output when they have not been set yet, e.g., at early stage of backend
>> startup. But I wonder if application_name in postgres_fdw actually
>> need that.. Thought?
> 
> Hmm, I think all of backend processes have username and database, but
> here has been followed from Horiguchi-san's suggestion:
> 
> ```
> I'm not sure but even if user_name doesn't seem to be NULL, don't we
> want to do the same thing with %u of log_line_prefix for safety?
> Namely, setting [unknown] if user_name is NULL or "". The same can be
> said for %d.
> ```
> 
> But actually I don't have strong opinions.

Ok, we can wait for more opinions about this to come.
But if user name and database name should NOT be NULL
in postgres_fdw, I think that it's better to add the assertion
check for those conditions and get rid of "[unknown]" part.

Regards,

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