Thread: Setting log_connection in connection string doesn't work
Hello. I noticed that the following command doesn't leave connection log in log file. > psql "host=localhost options=-c\ log_connections=on" The reason is we log connections before the options is processed. We need to move the code from BackendInitialize to InitPostgres where that options are processed if we want that option to work. However, I'm not sure we can delay connection-log until that point, since that movement changes the meaning of the log message. Another option is to log connections in InitPostgres if not yet and log_connections is turned on by connection options. Futher another option is we don't make it work and write in the document that it doesn't work. (I didn't find that, but...) Opinions and suggestions are welcome. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> I noticed that the following command doesn't leave connection log in
> log file.
>> psql "host=localhost options=-c\ log_connections=on"
[ shrug... ] Why would you expect it to? Should "-c log_connections=off"
be able to hide a connection from the log?
regards, tom lane
At Tue, 26 Oct 2021 09:39:12 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > I noticed that the following command doesn't leave connection log in
> > log file.
> >> psql "host=localhost options=-c\ log_connections=on"
>
> [ shrug... ] Why would you expect it to? Should "-c log_connections=off"
> be able to hide a connection from the log?
I don't know. The fact is that it's a superuser-backend variable that
is silently ignored (but acutally seems to be set in the session).
Setting log_disconnection the same way works (of course the impliction
of this is far less significant that the log_connection case).
If we want to refuse them to be set at session start (and I think so),
shouldn't they be changed to SIGHUP? (I forgot to mention this choice
in the previous mail..)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e91d5a3cfd..57d810c80d 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1353,7 +1353,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT,
+ {"log_connections", PGC_SIGHUP, LOGGING_WHAT,
gettext_noop("Logs each successful connection."),
NULL
},
@@ -1362,7 +1362,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"log_disconnections", PGC_SU_BACKEND, LOGGING_WHAT,
+ {"log_disconnections", PGC_SIGHUP, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
On Wed, Oct 27, 2021 at 10:24:05AM +0900, Kyotaro Horiguchi wrote: > I don't know. The fact is that it's a superuser-backend variable that > is silently ignored (but acutally seems to be set in the session). > Setting log_disconnection the same way works (of course the impliction > of this is far less significant that the log_connection case). fe550b2 is the commit that has changed both those parameters to be PGC_SU_BACKEND, with the commit log mentioning the case you are describing. That would be the area of this thread: https://www.postgresql.org/message-id/20408.1404329822@sss.pgh.pa.us As Tom and this thread are saying, there may be a use-case for making log_connections more effective at startup so as superusers could hide their logs at will. However, honestly, I am not sure that this is worth spending time improving this as the use-case looks rather thin to me. Perhaps you are right and we could just mark both of those GUCs as PGC_SIGHUP, making the whole easier to understand and more consistent, though. If we do that, the patch is wrong, as the docs would also need a refresh. -- Michael
Attachment
At Wed, 27 Oct 2021 10:55:31 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Wed, Oct 27, 2021 at 10:24:05AM +0900, Kyotaro Horiguchi wrote: > > I don't know. The fact is that it's a superuser-backend variable that > > is silently ignored (but acutally seems to be set in the session). > > Setting log_disconnection the same way works (of course the impliction > > of this is far less significant that the log_connection case). > > fe550b2 is the commit that has changed both those parameters to be > PGC_SU_BACKEND, with the commit log mentioning the case you are > describing. That would be the area of this thread: > https://www.postgresql.org/message-id/20408.1404329822@sss.pgh.pa.us Thanks for the pointer. (I didn't remember of that thread..) > As Tom and this thread are saying, there may be a use-case for > making log_connections more effective at startup so as superusers > could hide their logs at will. However, honestly, I am not sure that > this is worth spending time improving this as the use-case looks > rather thin to me. Perhaps you are right and we could just mark both I tend to agree. > of those GUCs as PGC_SIGHUP, making the whole easier to understand and > more consistent, though. If we do that, the patch is wrong, as the > docs would also need a refresh. Yeah, this is the full version of the patch. regards. -- Kyotaro Horiguchi NTT Open Source Software Center From 11a9612c2590f57f431c3918d5b62c08a5b29efb Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Date: Wed, 27 Oct 2021 11:39:02 +0900 Subject: [PATCH] Change log_(dis)connections to PGC_SIGHUP log_connections is not effective when it is given in connection options. Since no complaint has been heard for this behavior the use-case looks rather thin. Thus we change it to PGC_SIGHUP, rahther than putting efforts to make it effective for the use-case. log_disconnections is working with the usage but be consistent by treating it the same way with log_connection. --- doc/src/sgml/config.sgml | 8 ++++---- src/backend/utils/misc/guc.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index de77f14573..64b04a47d2 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -6800,8 +6800,8 @@ local0.* /var/log/postgresql Causes each attempted connection to the server to be logged, as well as successful completion of both client authentication (if necessary) and authorization. - Only superusers can change this parameter at session start, - and it cannot be changed at all within a session. + This parameter can only be set in the <filename>postgresql.conf</filename> + file or on the server command line. The default is <literal>off</literal>. </para> @@ -6827,8 +6827,8 @@ local0.* /var/log/postgresql Causes session terminations to be logged. The log output provides information similar to <varname>log_connections</varname>, plus the duration of the session. - Only superusers can change this parameter at session start, - and it cannot be changed at all within a session. + This parameter can only be set in the <filename>postgresql.conf</filename> + file or on the server command line. The default is <literal>off</literal>. </para> </listitem> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e91d5a3cfd..57d810c80d 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1353,7 +1353,7 @@ static struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, { - {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT, + {"log_connections", PGC_SIGHUP, LOGGING_WHAT, gettext_noop("Logs each successful connection."), NULL }, @@ -1362,7 +1362,7 @@ static struct config_bool ConfigureNamesBool[] = NULL, NULL, NULL }, { - {"log_disconnections", PGC_SU_BACKEND, LOGGING_WHAT, + {"log_disconnections", PGC_SIGHUP, LOGGING_WHAT, gettext_noop("Logs end of a session, including duration."), NULL }, -- 2.27.0
This patch is from October of 2021. I don't see any commitfest entry for it. Should it be applied? --------------------------------------------------------------------------- On Wed, Oct 27, 2021 at 11:53:09AM +0900, Kyotaro Horiguchi wrote: > At Wed, 27 Oct 2021 10:55:31 +0900, Michael Paquier <michael@paquier.xyz> wrote in > > On Wed, Oct 27, 2021 at 10:24:05AM +0900, Kyotaro Horiguchi wrote: > > > I don't know. The fact is that it's a superuser-backend variable that > > > is silently ignored (but acutally seems to be set in the session). > > > Setting log_disconnection the same way works (of course the impliction > > > of this is far less significant that the log_connection case). > > > > fe550b2 is the commit that has changed both those parameters to be > > PGC_SU_BACKEND, with the commit log mentioning the case you are > > describing. That would be the area of this thread: > > https://www.postgresql.org/message-id/20408.1404329822@sss.pgh.pa.us > > Thanks for the pointer. (I didn't remember of that thread..) > > > As Tom and this thread are saying, there may be a use-case for > > making log_connections more effective at startup so as superusers > > could hide their logs at will. However, honestly, I am not sure that > > this is worth spending time improving this as the use-case looks > > rather thin to me. Perhaps you are right and we could just mark both > > I tend to agree. > > > of those GUCs as PGC_SIGHUP, making the whole easier to understand and > > more consistent, though. If we do that, the patch is wrong, as the > > docs would also need a refresh. > > Yeah, this is the full version of the patch. > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > >From 11a9612c2590f57f431c3918d5b62c08a5b29efb Mon Sep 17 00:00:00 2001 > From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> > Date: Wed, 27 Oct 2021 11:39:02 +0900 > Subject: [PATCH] Change log_(dis)connections to PGC_SIGHUP > > log_connections is not effective when it is given in connection > options. Since no complaint has been heard for this behavior the > use-case looks rather thin. Thus we change it to PGC_SIGHUP, rahther > than putting efforts to make it effective for the > use-case. log_disconnections is working with the usage but be > consistent by treating it the same way with log_connection. > --- > doc/src/sgml/config.sgml | 8 ++++---- > src/backend/utils/misc/guc.c | 4 ++-- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml > index de77f14573..64b04a47d2 100644 > --- a/doc/src/sgml/config.sgml > +++ b/doc/src/sgml/config.sgml > @@ -6800,8 +6800,8 @@ local0.* /var/log/postgresql > Causes each attempted connection to the server to be logged, > as well as successful completion of both client authentication (if > necessary) and authorization. > - Only superusers can change this parameter at session start, > - and it cannot be changed at all within a session. > + This parameter can only be set in the <filename>postgresql.conf</filename> > + file or on the server command line. > The default is <literal>off</literal>. > </para> > > @@ -6827,8 +6827,8 @@ local0.* /var/log/postgresql > Causes session terminations to be logged. The log output > provides information similar to <varname>log_connections</varname>, > plus the duration of the session. > - Only superusers can change this parameter at session start, > - and it cannot be changed at all within a session. > + This parameter can only be set in the <filename>postgresql.conf</filename> > + file or on the server command line. > The default is <literal>off</literal>. > </para> > </listitem> > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > index e91d5a3cfd..57d810c80d 100644 > --- a/src/backend/utils/misc/guc.c > +++ b/src/backend/utils/misc/guc.c > @@ -1353,7 +1353,7 @@ static struct config_bool ConfigureNamesBool[] = > NULL, NULL, NULL > }, > { > - {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT, > + {"log_connections", PGC_SIGHUP, LOGGING_WHAT, > gettext_noop("Logs each successful connection."), > NULL > }, > @@ -1362,7 +1362,7 @@ static struct config_bool ConfigureNamesBool[] = > NULL, NULL, NULL > }, > { > - {"log_disconnections", PGC_SU_BACKEND, LOGGING_WHAT, > + {"log_disconnections", PGC_SIGHUP, LOGGING_WHAT, > gettext_noop("Logs end of a session, including duration."), > NULL > }, > -- > 2.27.0 > -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Bruce Momjian <bruce@momjian.us> writes:
> This patch is from October of 2021. I don't see any commitfest entry
> for it. Should it be applied?
I think we decided not to. The original argument for having these
be PGC_SU_BACKEND was to try to ensure that you got matching
connection and disconnection log entries for any one session,
and I don't see anything that supersedes that plan.
regards, tom lane
On Wed, Aug 17, 2022 at 10:29:26AM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > This patch is from October of 2021. I don't see any commitfest entry > > for it. Should it be applied? > > I think we decided not to. The original argument for having these > be PGC_SU_BACKEND was to try to ensure that you got matching > connection and disconnection log entries for any one session, > and I don't see anything that supersedes that plan. Okay, thanks for the feedback. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson