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
v21_0002_allow_escapes.patch
Description: v21_0002_allow_escapes.patch
v21_0001_export_padding_function.patch
Description: v21_0001_export_padding_function.patch