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

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