Dear Fujii-san,
Thank you for giving comments! I attached new patches.
> 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.
> + 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.
We can force parse_pgfdw_appname() not to be called if MyProcPort does not exist,
but I don't think it is needed now.
> 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.
> + if (appname == NULL || *appname
> == '\0')
> + appname = "[unknown]";
>
> Do we really want to replace the application name with "[unknown]"
> when application_name in the local server is not set? At least for me,
> it's intuitive to replace it with empty string in that case,
> in postgres_fdw application_name.
Yeah, I agreed that empty string should be keep here.
Currently I kept NULL case because of the above reason.
> The patch now fails to be applied to the master. Could you rebase it?
Thanks, rebased. I just moved test to the end of the sql file.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED