On 2021/11/04 20:42, kuroda.hay...@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
