Re: Logging which local address was connected to in log_line_prefix - Mailing list pgsql-hackers

From Tom Lane
Subject Re: Logging which local address was connected to in log_line_prefix
Date
Msg-id 3101493.1743976861@sss.pgh.pa.us
Whole thread Raw
In response to Re: Logging which local address was connected to in log_line_prefix  (Greg Sabino Mullane <htamfids@gmail.com>)
Responses Re: Logging which local address was connected to in log_line_prefix
Re: Logging which local address was connected to in log_line_prefix
List pgsql-hackers
Greg Sabino Mullane <htamfids@gmail.com> writes:
> I have not attempted the caching change yet.

After some thought I concluded that caching the local-address string
in MyProcPort itself would be the most robust way of making that work.
Otherwise you need some way to update the cache when MyProcPort is
created (in case the process already emitted some messages before
that), and the patch starts to spread into other places.

I think 0004 attached is about committable, but there is one
definitional point that is troubling me slightly: our choice to
emit "[none]" when there's no port isn't consistent with the
log_line_prefix documentation's statement that

         ... Some escapes are only recognized by session processes,
         and will be treated as empty by background processes such as
         the main server process.

Since we've marked %L as "Session only" = yes, this implies
that it should print as an empty string not "[none]".
We could either

1. Ignore the inconsistency, commit 0004 as-is.

2. Change the output to be an empty string in background processes.
   This is consistent, but it goes against our upthread feeling
   that "[none]" would avoid confusion.

3. Mark %L as "Session only" = no.  This seems a little weird,
   but it'd also be consistent.

4. Add something to the above-quoted text about %L being an exception.

I don't really care for #3 or #4, but I'm ambivalent between #1 and
#2.  I think the worry about confusion originated when the patch
would print "[local]" for either a Unix socket or a background
process, and that certainly was confusing.  "[local]" versus
an empty string is not so ambiguous, so maybe it's fine.

Thoughts?

            regards, tom lane

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index fea683cb49c..a8542fe41ce 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7689,6 +7689,12 @@ local0.*    /var/log/postgresql
              <entry>Remote host name or IP address</entry>
              <entry>yes</entry>
             </row>
+            <row>
+             <entry><literal>%L</literal></entry>
+             <entry>Local address (the IP address on the server that the
+             client connected to)</entry>
+             <entry>yes</entry>
+            </row>
             <row>
              <entry><literal>%b</literal></entry>
              <entry>Backend type</entry>
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8a6b6905079..d6299633ab7 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -67,6 +67,7 @@
 #endif

 #include "access/xact.h"
+#include "common/ip.h"
 #include "libpq/libpq.h"
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
@@ -3084,6 +3085,38 @@ log_status_format(StringInfo buf, const char *format, ErrorData *edata)
                     appendStringInfoSpaces(buf,
                                            padding > 0 ? padding : -padding);
                 break;
+            case 'L':
+                {
+                    const char *local_host;
+
+                    if (MyProcPort)
+                    {
+                        if (MyProcPort->local_host[0] == '\0')
+                        {
+                            /*
+                             * First time through: cache the lookup, since it
+                             * might not have trivial cost.
+                             */
+                            (void) pg_getnameinfo_all(&MyProcPort->laddr.addr,
+                                                      MyProcPort->laddr.salen,
+                                                      MyProcPort->local_host,
+                                                      sizeof(MyProcPort->local_host),
+                                                      NULL, 0,
+                                                      NI_NUMERICHOST | NI_NUMERICSERV);
+                        }
+                        local_host = MyProcPort->local_host;
+                    }
+                    else
+                    {
+                        /* Background process, or connection not yet made */
+                        local_host = "[none]";
+                    }
+                    if (padding != 0)
+                        appendStringInfo(buf, "%*s", padding, local_host);
+                    else
+                        appendStringInfoString(buf, local_host);
+                }
+                break;
             case 'r':
                 if (MyProcPort && MyProcPort->remote_host)
                 {
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ff56a1f0732..f154906c2fa 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -603,6 +603,7 @@
                     #   %d = database name
                     #   %r = remote host and port
                     #   %h = remote host
+                    #   %L = local address
                     #   %b = backend type
                     #   %p = process ID
                     #   %P = process ID of parallel group leader
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 0d1f1838f73..d6e671a6382 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -139,6 +139,9 @@ typedef struct Port
     int            remote_hostname_errcode;    /* see above */
     char       *remote_port;    /* text rep of remote port */

+    /* local_host is filled only if needed (see log_status_format) */
+    char        local_host[64]; /* ip addr of local socket for client conn */
+
     /*
      * Information that needs to be saved from the startup packet and passed
      * into backend execution.  "char *" fields are NULL if not set.

pgsql-hackers by date:

Previous
From: Daniel Gustafsson
Date:
Subject: Re: Possibly hard-to-read message
Next
From: Michael Paquier
Date:
Subject: Re: Logging which local address was connected to in log_line_prefix