From: | "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Fujii Masao' <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "ikedamsh(at)oss(dot)nttdata(dot)com" <ikedamsh(at)oss(dot)nttdata(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, 'Kyotaro Horiguchi' <horikyota(dot)ntt(at)gmail(dot)com> |
Subject: | RE: Allow escape in application_name |
Date: | 2021-11-04 11:42:37 |
Message-ID: | TYAPR01MB58666640BD10FBCF439B1400F58D9@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v21_0002_allow_escapes.patch | application/octet-stream | 10.6 KB |
v21_0001_export_padding_function.patch | application/octet-stream | 2.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2021-11-04 11:45:23 | Re: lastOverflowedXid does not handle transaction ID wraparound |
Previous Message | Magnus Hagander | 2021-11-04 11:03:53 | Re: PROXY protocol support |