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
