RE: Allow escape in application_name

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

In response to

Responses

Browse pgsql-hackers by date

  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