Re: Allow escape in application_name

On 2021/11/05 12:17, Kyotaro Horiguchi wrote:
> I'm not sure that that distinction is so clear for users. So I feel we
> want a notice something like this. But it doesn't seem correct as it
> is.  When the user name of the session consists of non-ascii
> characters, %u is finally seen as a sequence of '?'.  It is not a
> embedded strings in pgfdw_application_name.  I didn't notice this
> behavior.
> 
> "pgfdw_application_name is set to application_name of a pgfdw
> connection after placeholder conversion, thus the resulting string is
> subject to the same restrictions to application_names.". Maybe
> followed by "If session user name or database name contains non-ascii
> characters, they are replaced by question marks "?"".

If we add something to the docs about this, we should clearly
describe what postgres_fdw.application_name does and
what postgres_fdw in a foreign server does. BTW, this kind of
information was already commented in option.c.
Probably it's better to mention the limitations on not only
ASCII characters but also the length of application name.

          * Unlike application_name GUC, don't set GUC_IS_NAME flag nor check_hook
     * to allow postgres_fdw.application_name to be any string more than
     * NAMEDATALEN characters and to include non-ASCII characters. Instead,
     * remote server truncates application_name of remote connection to less
     * than NAMEDATALEN and replaces any non-ASCII characters in it with a '?'
     * character.

If possible, I'd like to see this change as a separate patch
and commt it first because this is the description for
the existing parameter postgres_fdw.application_name.


>> 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.
> 
> I think padding is useful because it alingns the significant content
> of log lines by equating the length of the leading fixed
> components. application_name doesn't make any other visible components
> unaligned even when shown in the pg_stat_activity view.  It is not
> useful in that sense.
> 
> Padding make the string itself make look maybe nicer, but gives no
> other advantage.
> 
> I doubt people complain to the lack of the padding feature.  Addition
> to that, since pgfdw_application_name and log_line_prefix work
> different way in regard to multibyte characters, we don't need to
> struggle so hard to consilidate them in their behavior.
> 
> # I noticed that the paddig feature doesn't consider multibyte
> # characters. (I'm not suggesting them ought to be handled
> # "prpoerly").
> 
> In short, I'm for to removing it by +0.7.

So our current consensus is to remove the padding part
from postgres_fdw.application_name.


>> +            case 'u':
>> +                Assert(MyProcPort != NULL);
>>
>> Isn't it enough to perform this assertion check only once
>> at the top of parse_pgfdw_appname()?
> 
> Yeah, in either way, we should treat them in the same way.
> 
>>> 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.
> 
> (I assume you said "it is needed now".)  I'm not sure how to force
> that but if it means a NULL MyProcPort cuases a crash, I think
> crashing server is needlessly too aggressive as the penatly.

I said "Yes" for Kuroda-san's comment "I don't think it is
needed now". So I meant that "it is NOT needed now".
Sorry for unclear comment..

His idea was to skip calling parse_pgfdw_appname() if
MyProcPort is NULL, so as to prevent parse_pgfdw_appname()
from seeing NULL pointer of MyProcPort. But he thought
it's not necessary now, and I agree with him because
the idea would cause more confusing behavior.


> It seems to me that postgres-fdw asumes a valid user id, but doesn't
> make no use of databsae, server port, and process id.  What I thought
> here is that making it an assertion is too much. So just ignoring the
> replacement is also fine to me.
> 
> That being said, if we are eager not to have unused code paths, it is
> reasonable enough.  I don't object strongly to replace it with an
> assertion.

So no one strongly objects to the addition of assertion?

Regards,

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