Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)

On 2021/08/05 12:18, kuroda.hayato@fujitsu.com wrote:

Dear Hackers, Tom,

(I changed subject because this is no longer postgres_fdw's patch)

What would be better to think about is how to let users specify this
kind of behavior for themselves. I think it's possible to set
application_name as part of a foreign server's connection options,
but at present the result would only be a constant string. Somebody
who wished the PID to be in there would like to have some sort of
formatting escape, say "%p" for PID. Extrapolating wildly, maybe we
could make all the %-codes known to log_line_prefix available here.

I think your argument is better than mine. I will try to implement this approach.

I made a patch based on your advice. I add parse and rewrite function in connectOptions2().
I implemented in libpq layer, hence any other application can be used.
Here is an example:

```
$ env PGAPPNAME=000%p%utest000 psql -d postgres -c "show application_name"
application_name
-----------------------
00025632hayatotest000
(1 row)
```

Why did you make even %u and %d available in application_name?
With the patch, they are replaced with the user name to connect as
and the database name to connect to, respectively. Unlike %p
(i.e., PID in *client* side), those information can be easily viewed via
log_line_prefix and pg_stat_activity, etc in the server side.
So I'm not sure how much helpful to expose them also in application_name.

I don't think this is a great idea as-is.  People who need to do this
sort of thing will all have their own ideas of what they need to track
--- most obviously, it might be appropriate to include the originating
server's name, else you don't know what machine the PID is for.

So some people may want to specify their own ID in application_name
when connecting to the foreign server. For now they can do that by
setting application_name per foreign server object. But this means that
every session connecting to the same foreign server basically should
use the same application_name. If they want to change the setting,
they need to execute "ALTER SERAVER ... OPTIONS ...". Isn't this inconvenient?
If yes, what about allowing us to specify foreign server's application_name
per session? For example, we can add postgres_fdw.application_name GUC,
and then if it's set postgres_fdw always uses it instead of the server-level
option when connecting to the foreign server. Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Dear Fujii-san,

Thank you for replying! I attached new version.

Why did you make even %u and %d available in application_name?

Actually no particular reason. I added them because they can easily add...
And I agree what you say, so removed.

So some people may want to specify their own ID in application_name
when connecting to the foreign server. For now they can do that by
setting application_name per foreign server object. But this means that
every session connecting to the same foreign server basically should
use the same application_name. If they want to change the setting,
they need to execute "ALTER SERAVER ... OPTIONS ...". Isn't this inconvenient?

Yeah, currently such users must execute ALTER command for each time.

If yes, what about allowing us to specify foreign server's application_name
per session? For example, we can add postgres_fdw.application_name GUC,
and then if it's set postgres_fdw always uses it instead of the server-level
option when connecting to the foreign server. Thought?

OK, I added the GUC parameter. My patch Defines new GUC at _PG_init().
The value is used when backends started to connect to remote server.
Normal users can modify the value by SET commands but
that change does not propagate to the established connections.

I'm not sure about the versioning policy about contrib.
Do we have to make new version? Any other objects are not changed.

Finally, I and Fujii-san were now discussing locally whether
this replacement should be in libpq or postgres_fdw layer.
I started to think that it should be postgres_fdw, because:
* previously aplications could distinguish by using current applicaiton_name,
* libpq is used almost all uses so some modifications might affect someone, and
* libpq might be just a tool, and should not be intelligent

I will make and attach another version that new codes move to postgres_fdw.
So could you vote which version is better?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Fujii-san,

I attached new version, that almost all codes
moved from libpq to postgres_fdw.

Now we can accept four types of escapes.
All escapes will be rewritten to connection souce's information:

* application_name,
* user name,
* database name, and
* backend's pid.

These are cannot be set by log_line_prefix, so I think it is useful.

We can use escape sequences in two ways:
* Sets postgres_fdw.application_name GUC paramter, or
* Sets application_name option in CREATE SERVER command.

Which version do you like?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 2021/08/31 16:11, kuroda.hayato@fujitsu.com wrote:

Dear Fujii-san,

I attached new version, that almost all codes
moved from libpq to postgres_fdw.

Thanks for updating the patch!

Can we split the patch into two as follows? If so, we can review
and commit them one by one.

#1. Add application_name GUC into postgres_fdw
#2. Allow to specify special escape characters in application_name that postgres_fdw uses.

Now we can accept four types of escapes.
All escapes will be rewritten to connection souce's information:

* application_name,
* user name,
* database name, and
* backend's pid.

+1

These are cannot be set by log_line_prefix, so I think it is useful.

We can use escape sequences in two ways:
* Sets postgres_fdw.application_name GUC paramter, or
* Sets application_name option in CREATE SERVER command.

OK.

Which version do you like?

For now I've not found real use case that implementing the feature
in libpq would introduce. So IMO postgres_fdw version is better.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Dear Fujii-san,

Can we split the patch into two as follows? If so, we can review
and commit them one by one.

#1. Add application_name GUC into postgres_fdw
#2. Allow to specify special escape characters in application_name that
postgres_fdw uses.

OK, I split and attached like that. 0001 adds new GUC, and
0002 allows to accept escapes.

For now I've not found real use case that implementing the feature
in libpq would introduce. So IMO postgres_fdw version is better.

+1, so I'll focus on the this version.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 2021/09/01 19:04, kuroda.hayato@fujitsu.com wrote:

OK, I split and attached like that. 0001 adds new GUC, and
0002 allows to accept escapes.

Thanks for splitting and updating the patches!

Here are the comments for 0001 patch.

-		/* Use "postgres_fdw" as fallback_application_name. */
+		/* Use GUC paramter if set */
+		if (pgfdw_application_name && *pgfdw_application_name != '\0')

This GUC parameter should be set after the options of foreign server
are set so that its setting can override the server-level ones.
Isn't it better to comment this?

+static bool
+check_pgfdw_application_name(char **newval, void **extra, GucSource source)
+{
+	/* Only allow clean ASCII chars in the application name */
+	if (*newval)
+		pg_clean_ascii(*newval);
+	return true;

Do we really need this check hook? Even without that, any non-ASCII characters
in application_name would be replaced with "?" in the foreign PostgreSQL server
when it's passed to that.

On the other hand, if it's really necessary, application_name set in foreign
server object also needs to be processed in the same way.

+							   NULL,
+							   PGC_USERSET,
+							   GUC_IS_NAME,

Why is GUC_IS_NAME flag necessary?

postgres_fdw.application_name overrides application_name set in foreign server object.
Shouldn't this information be documented?

Isn't it better to have the regression test for this feature?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Dear Fujii-san,

Thank you for reviewing!

This GUC parameter should be set after the options of foreign server
are set so that its setting can override the server-level ones.
Isn't it better to comment this?

I added the following comments:

```diff
-               /* Use "postgres_fdw" as fallback_application_name. */
+               /*
+                * Use pgfdw_application_name as application_name.
+                *
+                * Note that this check must be behind constructing generic options
+                * because pgfdw_application_name has higher priority.
+                */
```

Do we really need this check hook? Even without that, any non-ASCII characters
in application_name would be replaced with "?" in the foreign PostgreSQL server
when it's passed to that.

On the other hand, if it's really necessary, application_name set in foreign
server object also needs to be processed in the same way.

I added check_hook because I want to make sure that
input for parse function has only ascii characters.
But in this phase we don't have such a function, so removed.
(Actually I'm not sure it is really needed, so I will check until next phase)

Why is GUC_IS_NAME flag necessary?

I thought again and I noticed even if extremely long string is specified
it will be truncated at server-side. This check is duplicated so removed.
Maybe such a check is a user-responsibility.

postgres_fdw.application_name overrides application_name set in foreign server
object.
Shouldn't this information be documented?

I added descriptions to postgres_fdw.sgml.

Isn't it better to have the regression test for this feature?

+1, added. In that test SET the parameter and check pg_stat_activity.

Attached is the fixed version. 0002 will be rebased later.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 2021/09/02 18:27, kuroda.hayato@fujitsu.com wrote:

I added the following comments:

```diff
-               /* Use "postgres_fdw" as fallback_application_name. */
+               /*
+                * Use pgfdw_application_name as application_name.
+                *
+                * Note that this check must be behind constructing generic options
+                * because pgfdw_application_name has higher priority.
+                */
```

Thanks! What about updating the comments furthermore as follows?

---------------------------------
Use pgfdw_application_name as application_name if set.

PQconnectdbParams() processes the parameter arrays from start to end.
If any key word is repeated, the last value is used. Therefore note that
pgfdw_application_name must be added to the arrays after options of
ForeignServer are, so that it can override application_name set in
ForeignServer.
---------------------------------

Attached is the fixed version. 0002 will be rebased later.

Thanks for updating the patch!

+		}
+		/* Use "postgres_fdw" as fallback_application_name */

It's better to add new empty line between these two lines.

+-- Disconnect once because the value is used only when establishing connections
+DO $$
+	BEGIN
+		PERFORM postgres_fdw_disconnect_all();
+	END
+$$;

Why does DO command need to be used here to execute
postgres_fdw_disconnect_all()? Instead, we can just execute
"SELECT 1 FROM postgres_fdw_disconnect_all();"?

For test coverage, it's better to test at least the following three cases?

(1) appname is set in neither GUC nor foreign server
-> "postgres_fdw" set in fallback_application_name is used
(2) appname is set in foreign server, but not in GUC
-> appname in foreign server is used
(3) appname is set both in GUC and foreign server
-> appname in GUC is used

+SELECT FROM ft1 LIMIT 1;

"1" should be added just after SELECT in the above statement?
Because postgres_fdw regression test basically uses "SELECT 1 FROM ..."
in other places.

+	DefineCustomStringVariable("postgres_fdw.application_name",
+							   "Sets the application name. This is used when connects to the remote server.",

What about simplifying this description as follows?

---------------
Sets the application name to be used on the remote server.
---------------

+  <title> Configuration Parameters </title>
+  <variablelist>

The empty characters just after <title> and before </title> should be removed?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Dear Fujii-san,

Thank you for your great works. Attached is the latest version.

Thanks! What about updating the comments furthermore as follows?

---------------------------------
Use pgfdw_application_name as application_name if set.

PQconnectdbParams() processes the parameter arrays from start to end.
If any key word is repeated, the last value is used. Therefore note that
pgfdw_application_name must be added to the arrays after options of
ForeignServer are, so that it can override application_name set in
ForeignServer.
---------------------------------

It's more friendly than mine because it mentions
about specification about PQconnectdbParams().
Fixed like yours.

+		}
+		/* Use "postgres_fdw" as fallback_application_name */

It's better to add new empty line between these two lines.

Fixed.

+-- Disconnect once because the value is used only when establishing
connections
+DO $$
+	BEGIN
+		PERFORM postgres_fdw_disconnect_all();
+	END
+$$;

Why does DO command need to be used here to execute
postgres_fdw_disconnect_all()? Instead, we can just execute
"SELECT 1 FROM postgres_fdw_disconnect_all();"?

DO command was used because I want to
ignore the returning value of postgres_fdw_disconnect_all().
Currently this function retruns false, but if other tests are modified,
some connection may remain and the function may become to return true.

I seeked sql file and I found that this function was called by your way.
Hence I fixed.

For test coverage, it's better to test at least the following three cases?

(1) appname is set in neither GUC nor foreign server
-> "postgres_fdw" set in fallback_application_name is used
(2) appname is set in foreign server, but not in GUC
-> appname in foreign server is used
(3) appname is set both in GUC and foreign server
-> appname in GUC is used

I set four testcases:

(1) Sets neither GUC nor server option
(2) Sets server option, but not GUC
(3) Sets GUC but not server option
(4) Sets both GUC and server option

I confirmed it almost works fine, but I found that
fallback_application_name will be never used in our test enviroment.
It is caused because our test runner pg_regress sets PGAPPNAME to "pg_regress" and
libpq prefer the environment variable to fallback_appname.
(I tried to control it by \setenv, but failed...)

+SELECT FROM ft1 LIMIT 1;

"1" should be added just after SELECT in the above statement?
Because postgres_fdw regression test basically uses "SELECT 1 FROM ..."
in other places.

Fixed.

+	DefineCustomStringVariable("postgres_fdw.application_name",
+							   "Sets the application name. This is used when connects to the remote server.",

What about simplifying this description as follows?

---------------
Sets the application name to be used on the remote server.
---------------

+1.

+  <title> Configuration Parameters </title>
+  <variablelist>

The empty characters just after <title> and before </title> should be removed?

I checked other sgml file and agreed. Fixed.

And I found that including string.h is no more needed. Hence it is removed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 2021/09/03 14:56, kuroda.hayato@fujitsu.com wrote:

Dear Fujii-san,

Thank you for your great works. Attached is the latest version.

Thanks a lot!

I set four testcases:

(1) Sets neither GUC nor server option
(2) Sets server option, but not GUC
(3) Sets GUC but not server option
(4) Sets both GUC and server option

OK.

I confirmed it almost works fine, but I found that
fallback_application_name will be never used in our test enviroment.
It is caused because our test runner pg_regress sets PGAPPNAME to "pg_regress" and
libpq prefer the environment variable to fallback_appname.
(I tried to control it by \setenv, but failed...)

make check uses "pg_regress", but I guess that make installcheck uses
"postgres_fdw". So the patch would cause make installcheck to fail.
I think that the case (1) is not so important, so can be removed. Thought?

Attached is the updated version of the patch. I removed the test
for case (1). And I arranged the regression tests so that they are based
on debug_discard_caches, to simplify them. Also I added and updated
some comments and docs. Could you review this version?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Dear Fujii-san,

Thank you for updating! Your modification is very interesting and
I learn something new.

Attached is the updated version of the patch. I removed the test
for case (1). And I arranged the regression tests so that they are based
on debug_discard_caches, to simplify them. Also I added and updated
some comments and docs. Could you review this version?

I agree removing (1) because it is obvious case.

```diff
+-- If appname is set both as GUC and as options of server object,
+-- the GUC setting overrides appname of server object and is used.
+SET postgres_fdw.application_name TO 'fdw_guc_appname';
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column? 
+----------
+        1
+(1 row)
+
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column? 
+----------
+        1
+(1 row)
+
+SELECT application_name FROM pg_stat_activity
+       WHERE application_name IN ('loopback2', 'fdw_guc_appname');
+ application_name 
+------------------
+ fdw_guc_appname
+(1 row)
```

I think we should SELECT ft6 because foreign server 'loopback'
doesn't have application_name server option.

I have no comments anymore.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hi, Kuroda-san, Fujii-san

I agree that this feature is useful. Thanks for working this.
I've tried to use the patches and read them. I have some questions.

1)

Why does postgres_fdw.application_name override the server's option?

+      establishes a connection to a foreign server.  This overrides
+      <varname>application_name</varname> option of the server object.
+      Note that change of this parameter doesn't affect any existing
+      connections until they are re-established.

My expected behavior was that application_name option of server object
overrides the GUC because other GUCs seems behave so. For example,
log_statement in postgresql.conf can be overrided by ALTER ROLE for
only specific user. Should the narrower scope setting takes precedence?

2)

Is it better to specify that we can use the escaped format
for application_name option of the server object in the document?
I think it's new feature too.

3)

Is the following expected behavior?

* 1. [coorinator] configure "postgres_fdw.application_name" as "[%d]".
* 2. [coodinator] connect *Non-ASCII* database.
* 3. [coodinator] execute queries for remote server.
* 4. [remote] execute the following query.

```
postgres(2654193)=# SELECT application_name FROM pg_stat_activity WHERE
backend_type = 'client backend' ;
application_name
---------------------------
psql
[?????????]
```

One of the difference between log_line_prefix and application_name
is that whether it only allow clean ASCII chars or not. So, the above
behavior is expected.

I think there are three choices for handling the above case.

1) Accept the above case because Non-ASCII rarely be used(?), and
describe the limitation in the document.
2) Add a new characters for oid of database.
3) Lease the limitation of application_name and make it accept
Non-ASCII.

As a user, 3) is best for me.
But it seems be out of scope this threads, so will you select 1)?

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Dear Ikeda-san,

I agree that this feature is useful. Thanks for working this.

Thanks :-).

1)

Why does postgres_fdw.application_name override the server's option?

+      establishes a connection to a foreign server.  This overrides
+      <varname>application_name</varname> option of the server object.
+      Note that change of this parameter doesn't affect any existing
+      connections until they are re-established.

My expected behavior was that application_name option of server object
overrides the GUC because other GUCs seems behave so. For example,
log_statement in postgresql.conf can be overrided by ALTER ROLE for
only specific user. Should the narrower scope setting takes precedence?

Hmm... I didn't know the policy in other options, thank you.
I set higher priority because of the following reason.
When users set app name as server option and
create remote connection from same backend process,
they will use same even if GUC sets.
In this case users must execute ALTER SERVERS every session,
and I think it is inconvenient.

I think both approaches are reasonable, so I also want to ask committer.
Fujii-san, how do you think?

2)

Is it better to specify that we can use the escaped format
for application_name option of the server object in the document?
I think it's new feature too.

Yeah, I agree. I will add at `Connection Options` section in 0002 patch.
Is it OK? Do we have more good place?

3)

Is the following expected behavior?

* 1. [coorinator] configure "postgres_fdw.application_name" as "[%d]".
* 2. [coodinator] connect *Non-ASCII* database.
* 3. [coodinator] execute queries for remote server.
* 4. [remote] execute the following query.

```
postgres(2654193)=# SELECT application_name FROM pg_stat_activity WHERE
backend_type = 'client backend' ;
application_name
---------------------------
psql
[?????????]
```

One of the difference between log_line_prefix and application_name
is that whether it only allow clean ASCII chars or not. So, the above
behavior is expected.

I think there are three choices for handling the above case.

1) Accept the above case because Non-ASCII rarely be used(?), and
describe the limitation in the document.
2) Add a new characters for oid of database.
3) Lease the limitation of application_name and make it accept
Non-ASCII.

As a user, 3) is best for me.
But it seems be out of scope this threads, so will you select 1)?

Actually I did not considering about such a situation...
But I want to choose (1) because the limitation is caused by libpq layer
and the modification is almost unrelated to this patch.
I'm not sure why appname have such a limitation
so at first we should investigate it. How do you think?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 2021-09-06 13:21, kuroda.hayato@fujitsu.com wrote:

Dear Ikeda-san,

I agree that this feature is useful. Thanks for working this.

Thanks :-).

1)

Why does postgres_fdw.application_name override the server's option?

+      establishes a connection to a foreign server.  This overrides
+      <varname>application_name</varname> option of the server object.
+      Note that change of this parameter doesn't affect any existing
+      connections until they are re-established.

My expected behavior was that application_name option of server object
overrides the GUC because other GUCs seems behave so. For example,
log_statement in postgresql.conf can be overrided by ALTER ROLE for
only specific user. Should the narrower scope setting takes
precedence?

Hmm... I didn't know the policy in other options, thank you.
I set higher priority because of the following reason.
When users set app name as server option and
create remote connection from same backend process,
they will use same even if GUC sets.
In this case users must execute ALTER SERVERS every session,
and I think it is inconvenient.

Thanks for explaining what you thought. I understood your idea.

IIUC, we can execute "ALTER SERVERS" commands automatically using
scripts?
If so, to have finer control seems to be more important than to reduce
the effort of
overwriting every configuration.

But this is just my feeling.
I agree to hear comments from Fujii-san and so on.

I think both approaches are reasonable, so I also want to ask
committer.
Fujii-san, how do you think?

2)

Is it better to specify that we can use the escaped format
for application_name option of the server object in the document?
I think it's new feature too.

Yeah, I agree. I will add at `Connection Options` section in 0002
patch.
Is it OK? Do we have more good place?

+1

I found that '%%' need to be described in the documents
of postgres_fdw.application_name. What do you think?

3)

Is the following expected behavior?

* 1. [coorinator] configure "postgres_fdw.application_name" as "[%d]".
* 2. [coodinator] connect *Non-ASCII* database.
* 3. [coodinator] execute queries for remote server.
* 4. [remote] execute the following query.

```
postgres(2654193)=# SELECT application_name FROM pg_stat_activity
WHERE
backend_type = 'client backend' ;
application_name
---------------------------
psql
[?????????]
```

One of the difference between log_line_prefix and application_name
is that whether it only allow clean ASCII chars or not. So, the above
behavior is expected.

I think there are three choices for handling the above case.

1) Accept the above case because Non-ASCII rarely be used(?), and
describe the limitation in the document.
2) Add a new characters for oid of database.
3) Lease the limitation of application_name and make it accept
Non-ASCII.

As a user, 3) is best for me.
But it seems be out of scope this threads, so will you select 1)?

Actually I did not considering about such a situation...
But I want to choose (1) because the limitation is caused by libpq
layer
and the modification is almost unrelated to this patch.
I'm not sure why appname have such a limitation
so at first we should investigate it. How do you think?

OK. I agree that (1) is enough for the first step.
If I have any time, I'll investigate it too.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2021/09/06 10:32, kuroda.hayato@fujitsu.com wrote:

I think we should SELECT ft6 because foreign server 'loopback'
doesn't have application_name server option.

Yes. I forgot to update that... Thanks for the review!
Attached is the updated version of the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/09/06 16:48, Masahiro Ikeda wrote:

On 2021-09-06 13:21, kuroda.hayato@fujitsu.com wrote:

Dear Ikeda-san,

I agree that this feature is useful. Thanks for working this.

Thanks :-).

1)

Why does postgres_fdw.application_name override the server's option?

+      establishes a connection to a foreign server.  This overrides
+      <varname>application_name</varname> option of the server object.
+      Note that change of this parameter doesn't affect any existing
+      connections until they are re-established.

My expected behavior was that application_name option of server object
overrides the GUC because other GUCs seems behave so. For example,
log_statement in postgresql.conf can be overrided by ALTER ROLE for
only specific user. Should the narrower scope setting takes precedence?

An option of a role doesn't always override a GUC setting. For example,
GUC with PGC_USERSET like work_mem, work_mem setting of a role overrides
that set in postgresql.conf, as you told. But if work_mem is set by
SET command, its value overrides the option of a role. So if we should treat
an option of foreign server object like an option of a role, we should handle
applicaion_name option for postgres_fdw in that way.

If we want to implement this, we need to check the context of
postgres_fdw.application_name when using it. If its context is PGC_SIGHUP
or PGC_POSTMASTER, it needs to be added the connection arrays that
PQconnectParams() uses before options of server object are processed,
i.e., application_name of server object overrides the other in this case.
On the other hand, if its context is higher than PGC_SIGHUP, it needs to
be added to the arrays after options of server object are processed.

I'm OK with the current proposal for now (i.e., postgres_fdw.application_name
always overrides that of server object) at least as the first step beucase
it's simple. But if you want that feature and can simply implement it, I don't
object to that.

IIUC, we can execute "ALTER SERVERS" commands automatically using scripts?
If so, to have finer control seems to be more important than to reduce the effort of
overwriting every configuration.

You mean to execute ALTER SERVER automatically via script whenever
a client connects to the server (i.e., a client passes its application_name
to the server, the server automatically sets it to the foreign server object,
then the server uses it as application_name of the remote connection)?

Is it better to specify that we can use the escaped format
for application_name option of the server object in the document?
I think it's new feature too.

Yeah, I agree. I will add at `Connection Options` section in 0002 patch.
Is it OK? Do we have more good place?

+1

+1

Actually I did not considering about such a situation...
But I want to choose (1) because the limitation is caused by libpq layer
and the modification is almost unrelated to this patch.
I'm not sure why appname have such a limitation
so at first we should investigate it. How do you think?

OK. I agree that (1) is enough for the first step.

+1

If I have any time, I'll investigate it too.

Maybe we need to consider what happens if different clients use
application_name with different encodings. How should
pg_stat_activity.application_name and log_line_prefix, etc handle such case?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/09/06 21:28, Fujii Masao wrote:

On 2021/09/06 16:48, Masahiro Ikeda wrote:

On 2021-09-06 13:21, kuroda.hayato@fujitsu.com wrote:

Dear Ikeda-san,

I agree that this feature is useful. Thanks for working this.

Thanks .

1)

Why does postgres_fdw.application_name override the server's option?

+      establishes a connection to a foreign server.  This overrides
+      <varname>application_name</varname> option of the server object.
+      Note that change of this parameter doesn't affect any existing
+      connections until they are re-established.

My expected behavior was that application_name option of server object
overrides the GUC because other GUCs seems behave so. For example,
log_statement in postgresql.conf can be overrided by ALTER ROLE for
only specific user. Should the narrower scope setting takes precedence?

An option of a role doesn't always override a GUC setting. For example,
GUC with PGC_USERSET like work_mem, work_mem setting of a role overrides
that set in postgresql.conf, as you told. But if work_mem is set by
SET command, its value overrides the option of a role. So if we should treat
an option of foreign server object like an option of a role, we should handle
applicaion_name option for postgres_fdw in that way.

If we want to implement this, we need to check the context of
postgres_fdw.application_name when using it. If its context is PGC_SIGHUP
or PGC_POSTMASTER, it needs to be added the connection arrays that
PQconnectParams() uses before options of server object are processed,
i.e., application_name of server object overrides the other in this case.
On the other hand, if its context is higher than PGC_SIGHUP, it needs to
be added to the arrays after options of server object are processed.

I'm OK with the current proposal for now (i.e., postgres_fdw.application_name
always overrides that of server object) at least as the first step beucase
it's simple. But if you want that feature and can simply implement it, I don't
object to that.

Thanks for explaining.

I forgot to consider about SET command. I understood that SET command should
override the old value, and it's difficult to manage the contexts for server
options and the GUC. Sorry for my impractical proposal.

IIUC, we can execute "ALTER SERVERS" commands automatically using scripts?
If so, to have finer control seems to be more important than to reduce the
effort of
overwriting every configuration.

You mean to execute ALTER SERVER automatically via script whenever
a client connects to the server (i.e., a client passes its application_name
to the server, the server automatically sets it to the foreign server object,
then the server uses it as application_name of the remote connection)?

Sorry, Kuroda-san and Fujii-san are right.
If SET command is used, ALTER SERVER doesn't work.

Is it better to specify that we can use the escaped format
for application_name option of the server object in the document?
I think it's new feature too.

Yeah, I agree. I will add at `Connection Options` section in 0002 patch.
Is it OK? Do we have more good place?

+1

+1

Actually I did not considering about such a situation...
But I want to choose (1) because the limitation is caused by libpq layer
and the modification is almost unrelated to this patch.
I'm not sure why appname have such a limitation
so at first we should investigate it. How do you think?

OK. I agree that (1) is enough for the first step.

+1

If I have any time, I'll investigate it too.

Maybe we need to consider what happens if different clients use
application_name with different encodings. How should
pg_stat_activity.application_name and log_line_prefix, etc handle such case?

Thanks. That makes sense.
I'll check them.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

Dear Fujii-san,

I confirmed it and I think it's OK.
Other comments should be included in 0002.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On 2021/09/07 10:32, kuroda.hayato@fujitsu.com wrote:

Dear Fujii-san,

I confirmed it and I think it's OK.
Other comments should be included in 0002.

Pushed 0001 patch. Thanks!
Could you rebase 0002 patch?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION