RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name) - Mailing list pgsql-hackers
From | kuroda.hayato@fujitsu.com |
---|---|
Subject | RE: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name) |
Date | |
Msg-id | TYAPR01MB58669D67A505AD8995BBD2D6F5CF9@TYAPR01MB5866.jpnprd01.prod.outlook.com Whole thread Raw |
In response to | Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name) (Fujii Masao <masao.fujii@oss.nttdata.com>) |
Responses |
Re: Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
(Fujii Masao <masao.fujii@oss.nttdata.com>)
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: