On 2021/09/08 21:32, kuroda.hay...@fujitsu.com wrote:
Dear Horiguchi-san,

Thank you for reviewing! I attached the fixed version.

Thanks for updating the patch!

+               for (i = n - 1; i >= 0; i--)
+               {
+                       if (strcmp(keywords[i], "application_name") == 0)
+                       {
+                               parse_pgfdw_appname(&buf, values[i]);
+                               values[i] = buf.data;
+                               break;
+                       }

postgres_fdw gets out of the loop after processing appname even
when buf.data is '\0'. Is this expected behavior? Because of this,
when postgres_fdw.application_name = '%b', unprocessed appname
of the server object is used.


+CREATE FUNCTION for_escapes() RETURNS bool AS $$
+    DECLARE
+        appname text;
+        c bool;
+    BEGIN
+        SHOW application_name INTO appname;
+        EXECUTE 'SELECT COUNT(*) FROM pg_stat_activity WHERE application_name 
LIKE '''

Could you tell me why the statement checking
application_name should be wrapped in a function?
Instead, we can just execute something like the following?

SELECT COUNT(*) FROM pg_stat_activity WHERE application_name = 
current_setting('application_name') || current_user || current_database() || 
pg_backend_pid() || '%';


+                       char *endptr = NULL;
+                       padding = (int)strtol(p, &endptr, 10);

strtol() seems to work differently from process_log_prefix_padding(),
for example, when the input string is "%-p".


+                       case 'a':
+                               {
+                                       const char *appname = application_name;

When log_line_prefix() processes "%a", "%u" or "%d" characters in
log_line_prefix, it checks whether MyProcPort is NULL or not.
Likewise shouldn't parse_pgfdw_appname() do the same thing?
For now it's ok not to check that because only process having
MyProcPort can use postgres_fdw. But in the future the process
not having that may use postgres_fdw, for example, 2PC resolver
process that Sawada-san is proposing to add to support automatic
2PC among multiple remote servers.


+    You can use escape sequences for <literal>application_name</literal> even 
if
+    it is set as a connection or a user mapping option.
+    Please refer the later section.

I was thinking that application_name cannot be set in a user mapping.

Regards,

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


Reply via email to