Audit of logout
Hi,
Some users enable log_disconnections in postgresql.conf to audit all logouts.
But since log_disconnections is defined with PGC_BACKEND, it can be changed
at connection start. This means that any client (even nonsuperuser) can freely
disable log_disconnections not to log his or her logout even when the
system admin
enables it in postgresql.conf. Isn't this problematic for audit?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
Some users enable log_disconnections in postgresql.conf to audit all logouts.
But since log_disconnections is defined with PGC_BACKEND, it can be changed
at connection start. This means that any client (even nonsuperuser) can freely
disable log_disconnections not to log his or her logout even when the
system admin
enables it in postgresql.conf. Isn't this problematic for audit?
That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order
to forbid non-superusers from changing its setting. Attached
patch does this.
Also defining log_disconnections with PGC_BACKEND itself seems strange.
Since it's used only at connection termination, there seems to be
no need to fix its setting value at connection startup. No? OTOH,
for example, log_connections and post_auth_delay are defined with
PGC_BACKEND and their settings can be changed only at connection startup.
This seems intuitive because they are used only at connection
startup and it's useless to change their settings after that. But
the situation of log_disconnections seems different from them.
Am I missing something?
One concern is; the patch may break the existing application if it
relies on the current behavior of log_disconnections. But I'm
wondering if such applications really exist.
Thought?
Regards,
--
Fujii Masao
Attachments:
0001-Make-log_disconnections-PGC_SUSET-rather-than-PGC_BA.patchtext/x-patch; charset=US-ASCII; name=0001-Make-log_disconnections-PGC_SUSET-rather-than-PGC_BA.patchDownload
From d3e6db1516a8cbb557e38a56b26c34ed7e51d9e1 Mon Sep 17 00:00:00 2001
From: MasaoFujii <masao.fujii@gmail.com>
Date: Fri, 13 Jun 2014 22:09:39 +0900
Subject: [PATCH] Make log_disconnections PGC_SUSET rather than PGC_BACKEND.
So far even non-superusers could disable log_disconnections in order to
prevent their session logout from being logged because the parameter was
defined with PGC_BACKEND. This was harmful in the systems which need to
audit all session logouts by using log_disconnections. For this problem,
this commit changes the GUC context of log_disconnections to PGC_SUSET
and forbids non-superuser from changing its setting.
---
doc/src/sgml/config.sgml | 2 +-
src/backend/utils/misc/guc.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 697cf99..184d864 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4234,7 +4234,7 @@ local0.* /var/log/postgresql
<varname>log_connections</varname> but at session termination,
and includes the duration of the session. This is off by
default.
- This parameter cannot be changed after session start.
+ Only superusers can change this setting.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1d094f0..7c84c9f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -922,7 +922,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"log_disconnections", PGC_BACKEND, LOGGING_WHAT,
+ {"log_disconnections", PGC_SUSET, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
--
1.7.1
Fujii Masao <masao.fujii@gmail.com> writes:
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Some users enable log_disconnections in postgresql.conf to audit all logouts.
But since log_disconnections is defined with PGC_BACKEND, it can be changed
at connection start. This means that any client (even nonsuperuser) can freely
disable log_disconnections not to log his or her logout even when the
system admin
enables it in postgresql.conf. Isn't this problematic for audit?
That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order
to forbid non-superusers from changing its setting. Attached
patch does this.
TBH, this complaint seems entirely artificial. If somebody needs to audit
disconnections, and they see a connection record with no disconnection
record but the session's no longer there, they certainly know that a
disconnection occurred. And if there wasn't a server crash to explain it,
they go slap the wrist of whoever violated corporate policy by turning off
log_disconnections. Why do we need system-level enforcement of this?
Moreover, your proposed fix breaks the entire point of the PGC_BACKEND
setting, which was to try to prevent disconnections from being logged
or not logged when the corresponding connection was not logged or logged
respectively. If an auditor wants the system to enforce that there are
matched pairs of those records, he's not exactly going to be satisfied by
being told that only superusers can cause them to not match.
I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when log_connections
is set.
Another answer is to make both variables PGC_SIGHUP, on the grounds
that it doesn't make much sense for them not to be applied system-wide;
except that I think there was some idea that logging might be enabled
per-user or per-database using ALTER ROLE/DATABASE.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane-2 wrote
Another answer is to make both variables PGC_SIGHUP, on the grounds
that it doesn't make much sense for them not to be applied system-wide;
except that I think there was some idea that logging might be enabled
per-user or per-database using ALTER ROLE/DATABASE.
From a trouble-shooting standpoint if I know that client software in
question is focused on particular users/databases being able to only enable
connection logging for those would make sense. Whether that is a true
production concern is another matter but the possibility does exist.
I personally do not get how a logoff is a risk auditing event. Logons and
actual queries I can understand.
For the same reason keeping them separate has merit since for imaginable
circumstances the logoffs are noise but the logons are important - and
forcing them to be on/off in tandem disallows the option to remove the noise
from the logs.
David J.
--
View this message in context: http://postgresql.1045698.n5.nabble.com/Audit-of-logout-tp5806985p5807224.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order
to forbid non-superusers from changing its setting. Attached
patch does this.
I tend to agree with this- those are things which regular users really
shouldn't be able to modify. Policy enforcement can be done when there
isn't system enforcement, but you really don't want to fall back to
policy enforcement when you don't need to.
TBH, this complaint seems entirely artificial. If somebody needs to audit
disconnections, and they see a connection record with no disconnection
record but the session's no longer there, they certainly know that a
disconnection occurred. And if there wasn't a server crash to explain it,
they go slap the wrist of whoever violated corporate policy by turning off
log_disconnections. Why do we need system-level enforcement of this?
Going through every log file, and writing something to address log file
changes, to hunt for those cases is no small amount of effort which
you're asking every administrator with an audit requirement to write
code to do to provide something which we make it appear as if we're
doing for them. It's certainly a violation of POLA that users can
decide on their own that their disconnections don't get logged.
Moreover, your proposed fix breaks the entire point of the PGC_BACKEND
setting, which was to try to prevent disconnections from being logged
or not logged when the corresponding connection was not logged or logged
respectively. If an auditor wants the system to enforce that there are
matched pairs of those records, he's not exactly going to be satisfied by
being told that only superusers can cause them to not match.
This is only accurate when a superuser exists in the system and even so,
superuser access can be much more easily reviewed as it's going to be a
lot less traffic and there may be other auditing mechanisms in place for
that access.
I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when log_connections
is set.Another answer is to make both variables PGC_SIGHUP, on the grounds
that it doesn't make much sense for them not to be applied system-wide;
except that I think there was some idea that logging might be enabled
per-user or per-database using ALTER ROLE/DATABASE.
Both of these options look pretty reasonable to me as a way to improve
the current situation. I'm not really sure that I see the use-case for
only logging connections/disconnections on a per-user or per-database
basis; in my experience it's not a lot of traffic to log it all and I
don't recall ever seeing anyone set those anything other than
system-wide.
I like the idea of flexibility in what's logged, I just don't see this
specific case as really needing it.
Thanks,
Stephen
At 2014-06-13 10:29:24 -0400, tgl@sss.pgh.pa.us wrote:
I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when log_connections
is set.
I like that idea.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 16, 2014 at 4:14 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order
to forbid non-superusers from changing its setting. Attached
patch does this.I tend to agree with this- those are things which regular users really
shouldn't be able to modify. Policy enforcement can be done when there
isn't system enforcement, but you really don't want to fall back to
policy enforcement when you don't need to.
+1.
TBH, this complaint seems entirely artificial. If somebody needs to audit
disconnections, and they see a connection record with no disconnection
record but the session's no longer there, they certainly know that a
disconnection occurred. And if there wasn't a server crash to explain it,
they go slap the wrist of whoever violated corporate policy by turning off
log_disconnections. Why do we need system-level enforcement of this?Going through every log file, and writing something to address log file
changes, to hunt for those cases is no small amount of effort which
you're asking every administrator with an audit requirement to write
code to do to provide something which we make it appear as if we're
doing for them. It's certainly a violation of POLA that users can
decide on their own that their disconnections don't get logged.
+1.
I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when log_connections
is set.Another answer is to make both variables PGC_SIGHUP, on the grounds
that it doesn't make much sense for them not to be applied system-wide;
except that I think there was some idea that logging might be enabled
per-user or per-database using ALTER ROLE/DATABASE.Both of these options look pretty reasonable to me as a way to improve
the current situation. I'm not really sure that I see the use-case for
only logging connections/disconnections on a per-user or per-database
basis; in my experience it's not a lot of traffic to log it all and I
don't recall ever seeing anyone set those anything other than
system-wide.I like the idea of flexibility in what's logged, I just don't see this
specific case as really needing it.
I don't really like either of these ideas much, and I don't really see
the problem with Fujii Masao's original proposal. It's true that some
installations may find it valuable to preserve the property that a
disconnect is logged whenever they connect is logged, but if that's
really what's at issue, then presumably the superuser is not going to
be flipping these settings on and off on the fly *anyway*.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 13, 2014 at 11:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Some users enable log_disconnections in postgresql.conf to audit all logouts.
But since log_disconnections is defined with PGC_BACKEND, it can be changed
at connection start. This means that any client (even nonsuperuser) can freely
disable log_disconnections not to log his or her logout even when the
system admin
enables it in postgresql.conf. Isn't this problematic for audit?That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order
to forbid non-superusers from changing its setting. Attached
patch does this.TBH, this complaint seems entirely artificial. If somebody needs to audit
disconnections, and they see a connection record with no disconnection
record but the session's no longer there, they certainly know that a
disconnection occurred. And if there wasn't a server crash to explain it,
they go slap the wrist of whoever violated corporate policy by turning off
log_disconnections. Why do we need system-level enforcement of this?Moreover, your proposed fix breaks the entire point of the PGC_BACKEND
setting, which was to try to prevent disconnections from being logged
or not logged when the corresponding connection was not logged or logged
respectively. If an auditor wants the system to enforce that there are
matched pairs of those records, he's not exactly going to be satisfied by
being told that only superusers can cause them to not match.I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when log_connections
is set.Another answer is to make both variables PGC_SIGHUP, on the grounds
that it doesn't make much sense for them not to be applied system-wide;
except that I think there was some idea that logging might be enabled
per-user or per-database using ALTER ROLE/DATABASE.
But, IIUC, since PGC_BACKEND parameters cannot be set per-role and per-database,
such idea seems impractical. No? ISTM that there is no big
user-visible problematic
change of the behavior even if we redefine log_connections and
log_disconnections
as PGC_SIGHUP.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/13/2014 07:29 AM, Tom Lane wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
<masao.fujii@gmail.com> wrote:Some users enable log_disconnections in postgresql.conf to
audit all logouts. But since log_disconnections is defined with
PGC_BACKEND, it can be changed at connection start. This means
that any client (even nonsuperuser) can freely disable
log_disconnections not to log his or her logout even when the
system admin enables it in postgresql.conf. Isn't this
problematic for audit?That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
forbid non-superusers from changing its setting. Attached patch
does this.
This whole argument seems wrong unless I'm missing something:
test=# set log_connections = on;
ERROR: parameter "log_connections" cannot be set after connection start
test=# set log_disconnections = off;
ERROR: parameter "log_disconnections" cannot be set after connection
start
I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when
log_connections is set.
That might be a good idea though.
Another answer is to make both variables PGC_SIGHUP, on the
grounds that it doesn't make much sense for them not to be applied
system-wide; except that I think there was some idea that logging
might be enabled per-user or per-database using ALTER
ROLE/DATABASE.
I don't think this is a good idea because of the reason you mention.
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTpQMcAAoJEDfy90M199hltHAP/2hEnKymoEq6zryaSpHZ2j0O
mj/8bEzCgYR/S4KUW8uqCzYK0g3HD5ncXJZkqpnaYvySV5YnopeUjuHaXxZOmuxx
GSbtmxo0wE5cYfEartVsX+ve0j7uSUwXBYZWD3em9FXNwFMnfVt3E/izwmHEnC7u
pIFHz6wKn6/QKaU9u/XRln4SZOAzeh4aYaHZy+5mhmGoU8fIJtZvdjEJSuAxxgzm
LMKGM/hgF23itpjjutDxQNoTUP+JGh0WzwqeW1t4+Y6T7HqXeTeT4IWsw3AH5sPg
e/NM+x4oeX9In6Gn4MLwT4R5Qai/JnaKGpzUv0jXlWPPvB23ilsb87eJ0BdbKDu1
LyxH16bH23DYL9LW+GAULRoMP78PLMKh4Mx2pe9KSL9tEBENvYpf+ew3IOfRmTlD
MAQRvhzspjPWp1AMQ9eNjX+63mpAeTBfHOBlVKUznhljHdDN7rcwpOzL82ecowDi
nM9bC+Me1jabaxRdu2cxt+p28BB5Ez3CX2wOz2JpM0ObruneoFhYCKXM9fUaD1d2
zJXiNtD7VgsUUtz+DGrNB32PyvzguhK0MXpX6/kRl5L1Xkpa4L+AV1nXWCkJYD6D
+btVgDscfnlWo1lQimq7B0KVET4zXnyI97vE7Xx0U7mvo8FZ8SQQHhbA7iy4P2SI
HUlqaKVcx2PLgoRAEEfL
=vQd8
-----END PGP SIGNATURE-------
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 06/13/2014 07:29 AM, Tom Lane wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
<masao.fujii@gmail.com> wrote:Some users enable log_disconnections in postgresql.conf to
audit all logouts. But since log_disconnections is defined with
PGC_BACKEND, it can be changed at connection start. This means
that any client (even nonsuperuser) can freely disable
log_disconnections not to log his or her logout even when the
system admin enables it in postgresql.conf. Isn't this
problematic for audit?That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
forbid non-superusers from changing its setting. Attached patch
does this.This whole argument seems wrong unless I'm missing something:
test=# set log_connections = on;
ERROR: parameter "log_connections" cannot be set after connection start
test=# set log_disconnections = off;
ERROR: parameter "log_disconnections" cannot be set after connection
start
You can change log_connections/disconnections via connection option as follows
$ grep log_disconnections $PGDATA/postgresql.conf
log_disconnections = on
$ psql -U hoge -d "options='-c log_disconnections=off'"
=> show log_disconnections ;
log_disconnections
--------------------
off
(1 row)
=> \du
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------+-----------
hoge | | {}
postgres | Superuser, Create role, Create DB, Replication | {}
I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when
log_connections is set.That might be a good idea though.
David pointed the merit of keeping those two parameters separate upthread
and I understand his thought.
/messages/by-id/1402675662004-5807224.post@n5.nabble.com
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 06/13/2014 07:29 AM, Tom Lane wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
<masao.fujii@gmail.com> wrote:Some users enable log_disconnections in postgresql.conf to
audit all logouts. But since log_disconnections is defined with
PGC_BACKEND, it can be changed at connection start. This means
that any client (even nonsuperuser) can freely disable
log_disconnections not to log his or her logout even when the
system admin enables it in postgresql.conf. Isn't this
problematic for audit?That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
forbid non-superusers from changing its setting. Attached patch
does this.This whole argument seems wrong unless I'm missing something:
test=# set log_connections = on;
ERROR: parameter "log_connections" cannot be set after connection start
test=# set log_disconnections = off;
ERROR: parameter "log_disconnections" cannot be set after connection
startYou can change log_connections/disconnections via connection option as follows
$ grep log_disconnections $PGDATA/postgresql.conf
log_disconnections = on$ psql -U hoge -d "options='-c log_disconnections=off'"
=> show log_disconnections ;
log_disconnections
--------------------
off
(1 row)=> \du
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------+-----------
hoge | | {}
postgres | Superuser, Create role, Create DB, Replication | {}I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when
log_connections is set.That might be a good idea though.
David pointed the merit of keeping those two parameters separate upthread
and I understand his thought.
/messages/by-id/1402675662004-5807224.post@n5.nabble.com
Let me explain the problem which I'd like to address here, again.
The problem is that any client (even non-superuser) can change the setting of
log_disconnections when it connects to the server. For example, you can do by
using "options" connection parameter as follows.
$ psql -U hoge -d "options='-c log_disconnections=off'"
=> show log_disconnections ;
log_disconnections
--------------------
off
(1 row)
This means that, even when DBA enables log_disconnections in postgresql.conf
in order to audit all the logouts, a client can freely avoid logging
of his or her
logout for some reasons. I think this situation problematic especially for audit
purpose.
You may think that logging logins is enough for audit purpose and
logging logouts
is not so important. But imagine the case where all logins are logged but some
corresponding logouts are not logged. This would make it difficult to check and
verify the validities of audit logs. It's not easy to handle such
users that only
login is logged.
Any client can enable/disable log_disconnections because it's defined with
PGC_BACKEND context. By definition, GUC parameters with PGC_BACKEND can
be changed at connection startup. But it cannot be changed after connection is
established.
BTW, log_connections is also defined with PGC_BACKEND, so any client can
change its setting at connection startup. But *fortunately* it's used
by the server
before the changed value is given from a client at connection startup. So, the
server always uses the setting value in postgresql.conf, whether a
client tries to
change log_connections or not. Therefore log_connections doesn't cause the
problem which I described the above at all. That's good for audit purpose. OTOH,
ISTM that defining log_connections with PGC_BACKEND is a bit strange since
a client cannot change it at all.
To address the problem, I'm thinking to change the GUC context of
log_disconnections from PGC_BACKEND to PGC_SIGHUP. This prevents a client
from changing the setting. In the top of the thread, I posted the patch which
changed the context to PGC_SUSET, but now I'm thinking PGC_SIGHUP is better.
Since log_disconnections is used (if it's enabled, the callback
function which logs
lotout is registered) just after connection is established, using PGC_SUSET and
allowing superuser to change the setting by SET command is useless.
Changing the GUC context of log_disconnections cause two behavior changes.
(1) As I explained the above, it prevents a client from changing the setting.
Yeah, this is what I want.
(2) It allows DBA to change log_disconnections of running backends by
reloading the configuration file. But such changed value has no effect on
whether logout is logged or not because running backend will never use it
except SHOW command. As I explained the above, log_disconnections is
*fixed* only just after connection is established.
SHOW command displays the changed value. But it might not be the value
which the running backend really uses. You may think that this
inconsistency
is a problem. For this, we can add new GUC flag which forbid the reload of
configuration file to change the setting, and redefine log_disconnections
with that flag. Do we need this?
I don't think that my idea causes serious behavior changes which can break
existing application. So I think that it's not problematic to change the GUC
context of log_disconnections to PGC_SIGHUP. Thought?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 06/13/2014 07:29 AM, Tom Lane wrote:
Fujii Masao <masao.fujii@gmail.com> writes:
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
<masao.fujii@gmail.com> wrote:Some users enable log_disconnections in postgresql.conf to
audit all logouts. But since log_disconnections is defined with
PGC_BACKEND, it can be changed at connection start. This means
that any client (even nonsuperuser) can freely disable
log_disconnections not to log his or her logout even when the
system admin enables it in postgresql.conf. Isn't this
problematic for audit?That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
forbid non-superusers from changing its setting. Attached patch
does this.This whole argument seems wrong unless I'm missing something:
test=# set log_connections = on;
ERROR: parameter "log_connections" cannot be set after connection start
test=# set log_disconnections = off;
ERROR: parameter "log_disconnections" cannot be set after connection
start
Hmm... I found that you had marked this proposal as "Returned with Feedback".
But I don't think that we reached the consensus to do that. I think that it's
still worth discussing this topic in this CF. So I marked this as "Needs Review"
again.
If you strongly think that this proposal should be marked as
"Returned with Feedback", could you let me know why you think so?
Regards
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/01/2014 11:55 PM, Fujii Masao wrote:
Hmm... I found that you had marked this proposal as "Returned with
Feedback". But I don't think that we reached the consensus to do
that. I think that it's still worth discussing this topic in this
CF. So I marked this as "Needs Review" again.If you strongly think that this proposal should be marked as
"Returned with Feedback", could you let me know why you think so?
Returned with Feedback means, well exactly that ;-) -- the patch as
linked to the CF page is wrong and cannot be applied the way it is
currently. It is therefore returned to you to be fixed. It does not
mean "Rejected" which is what you seem to infer.
As mentioned on the CF the documentation change is flat wrong, and you
yourself have said that you think PGC_SUSET is wrong now and
PGC_SIGHUP should be used instead.
Furthermore I doubt you have tested the change to PGC_SIGHUP because I
did a quick test, and for some reason I haven't yet tracked down SHOW
does not see the change to log_disconnections as you said it would
after a reload, unlike other PGC_SIGHUP vars. So there is more
thought, testing, and possibly coding needed to make this a viable change.
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTtBm6AAoJEDfy90M199hlEzQQAJOCZrUb1fQw5ArGCBRm3T2n
gB1SobGGbmSweY3M8D/U55/IpjWEp3Emma3869tTVG51d6r1vRchwax1Ol2IUuek
Z7YwdgysoUOY1RNbqsa/WUVS23djKplgA7azrDu+MXFJpupBQjCH7lumtJ/L1dbC
1ua3NKZg914HkDTNHkD2AKL6o4LupfRM0hcSmBUZRG0fWLgSBiHza+idzFR1TfK2
6SpK0T6u3H6R7eU/7YluapY6LC/nA0bx+zM7sBEsWE2Wb1NQ/DER/fkuSO0V0N3X
/ljRUP7sds2KOlIJ95081JVbN5lTM2rQfd6ZLu5CsTKEDKR+PL8rOGgCX2mMoTS5
gc8vDLtyArqzcZpmRTufyBUvQAAS7CyIG7JxJNyWkEDwnn/B8HqBuGdLIdS8VdV5
oEdhbcKuN5cMTodMAv1h/QPcpSE72O/zZ6XxJGD5Wcpury7BTmBkLNJnZOsY4GU0
Nlxcc3tTvMsZYpvrJYBVfypQ6J7PCCwx1qra2GLtA7fkSeH+tXuqQ0IKVXi2bYEr
TDC2Cx/37cn56Z9PObAUJlYmoolix8MD27m6cEW0PvkJrDe7tEPWpEf38MUeZ0ae
kinXrB0MtxrrqICsWBjtxz0HGdsbQUreYs3JadnmkAR8cvhunDuHFShZv6GGzaZL
PzhOgGxxHemWGF7er2YO
=tiPs
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail@joeconway.com> wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1On 07/01/2014 11:55 PM, Fujii Masao wrote:
Hmm... I found that you had marked this proposal as "Returned with
Feedback". But I don't think that we reached the consensus to do
that. I think that it's still worth discussing this topic in this
CF. So I marked this as "Needs Review" again.If you strongly think that this proposal should be marked as
"Returned with Feedback", could you let me know why you think so?Returned with Feedback means, well exactly that ;-) -- the patch as
linked to the CF page is wrong and cannot be applied the way it is
currently. It is therefore returned to you to be fixed. It does not
mean "Rejected" which is what you seem to infer.
I think that we should use "Waiting on Author" in that case.
"Returned with Feedback" is not "Rejected". That's right. But it basically
means that the bugfixed or revised version of the patch will NOT be
reviewed in this CF. IOW, the author has to wait for the patch review
until next CF.
As mentioned on the CF the documentation change is flat wrong, and you
yourself have said that you think PGC_SUSET is wrong now and
PGC_SIGHUP should be used instead.Furthermore I doubt you have tested the change to PGC_SIGHUP because I
did a quick test, and for some reason I haven't yet tracked down SHOW
does not see the change to log_disconnections as you said it would
after a reload, unlike other PGC_SIGHUP vars.
No. If we change it to PGC_SIGHUP, SHOW command does display
the changed value after a reload. It's the same behavior as other
PGC_SIGHUP parameters do. Attached patch just changes it to PGC_SIGHUP.
You can test that by using the patch.
OTOH, the current behavior, i.e., log_disconnections with PGC_BACKEND,
is that SHOW command doesn't display the changed value after a reload.
So there is more
thought, testing, and possibly coding needed to make this a viable change.
+1
Regards,
--
Fujii Masao
Attachments:
0001-Make-log_disconnections-PGC_SUSET-rather-than-PGC_SI.patchtext/x-diff; charset=US-ASCII; name=0001-Make-log_disconnections-PGC_SUSET-rather-than-PGC_SI.patchDownload
From 098f11cfbf8f9370829e24e9f8d704c050c98875 Mon Sep 17 00:00:00 2001
From: MasaoFujii <masao.fujii@gmail.com>
Date: Fri, 13 Jun 2014 22:09:39 +0900
Subject: [PATCH] Make log_disconnections PGC_SUSET rather than PGC_SIGHUP.
So far even non-superusers could disable log_disconnections in order to
prevent their session logout from being logged because the parameter was
defined with PGC_BACKEND. This was harmful in the systems which need to
audit all session logouts by using log_disconnections. For this problem,
this commit changes the GUC context of log_disconnections to PGC_SIGHUP
and forbids any client from changing its setting.
---
doc/src/sgml/config.sgml | 3 ++-
src/backend/utils/misc/guc.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 37d78b3..f5f0324 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4257,7 +4257,8 @@ local0.* /var/log/postgresql
<varname>log_connections</varname> but at session termination,
and includes the duration of the session. This is off by
default.
- This parameter cannot be changed after session start.
+ This parameter can only be set in the <filename>postgresql.conf</>
+ file or on the server command line.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 3a31a75..19e521b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -919,7 +919,7 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- {"log_disconnections", PGC_BACKEND, LOGGING_WHAT,
+ {"log_disconnections", PGC_SIGHUP, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
--
1.7.12.4 (Apple Git-37)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/02/2014 12:43 PM, Fujii Masao wrote:
I think that we should use "Waiting on Author" in that case.
"Returned with Feedback" is not "Rejected". That's right. But it
basically means that the bugfixed or revised version of the patch
will NOT be reviewed in this CF. IOW, the author has to wait for
the patch review until next CF.
Doesn't mean that to me but feel free to change it to Waiting on
Author if you prefer :-)
Is there any official explanation as to what those states mean
documented anywhere?
No. If we change it to PGC_SIGHUP, SHOW command does display the
changed value after a reload. It's the same behavior as other
PGC_SIGHUP parameters do. Attached patch just changes it to
PGC_SIGHUP. You can test that by using the patch.
I tried it already myself and it did not work the same for me. Perhaps
I messed something up but I tried two or three times and the value of
log_disconnections did not change in the open session when I did SHOW
after reload, but in new backends it did. On the other hand I also
tried a different SIGHUP var (log_checkpoints) and saw that effect
immediately in the open session. Was too tired to follow up last night
though. Have you verified for yourself that it behaves the way you say?
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTtGMTAAoJEDfy90M199hlZs0P/A4ptorZOFkvW3DJRDfKoE2f
v9wiWHYQJtN31sdDdljRD+3WvfiGJhMVCVukzZbaZejVGsEOModEOSLJZgkPhqa/
n/TSD77KdfHWkiAENkBmsBJLfqccysRxpWsJ5rPSiXqmyz+hxHJ6u8R4RGf/gPXn
15Rq3ZYNtGP25pTlHUr869y1D9dqnEzYdhf69ql4iIQPgsGow+bppZGVEEFd6MsF
2rpUTgdczhn4yl/kMTW25CFm19dHjj+m/v+Yv9uTg0JWX8xBtQ5hT9Z8Pgc/xU2U
WDsdfQ46ORinBOfPd/RPoJoIjxpMGMtuB5dX0mHsVzp7+kqSdKI+5amSz8YxqBE8
ii01AOfH2fOws236QQtVywWRHJMs7xWj5k/YNe1ABNoh5wFQRaaFkVC5r/i+Url2
3lDHUN/MjTeXieBQMUDNmqQbrRShYQqBnWl05n8dn/6V5U42eundJCr0tO+awW9V
3gTVTpwymsv7AkJfk3LjUybTYYDee3YM1A2YhdufYrhmQJttjh1kaeai9G05y5SZ
vU6DL+FJkVukxq5n6MTDzcxKrL0SYcRUVW0FZmng/Wn5SrsBC8AifkuR3Z1uFY5s
TjgCMDr/RzTGlXjITJQOl9smc6P/0yI8JKvdkAGnjeKY9zYsVn35fs/6HGMPmusZ
DWEp8jAdDohoWgiZCz4x
=0qGo
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fujii Masao <masao.fujii@gmail.com> writes:
On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail@joeconway.com> wrote:
Returned with Feedback means, well exactly that ;-) -- the patch as
linked to the CF page is wrong and cannot be applied the way it is
currently. It is therefore returned to you to be fixed. It does not
mean "Rejected" which is what you seem to infer.
I think that we should use "Waiting on Author" in that case.
I don't think there's a huge distinction between Waiting on Author and
Returned with Feedback. The former means we think the author will
probably submit a new version shortly, the latter means we think it'll
take awhile, but in any particular case those predictions could turn
out wrong. I don't have a problem with moving a patch from Returned with
Feedback back to Needs Review if the author is able to turn it around
while the CF is still going on.
As far as the particular case goes, it strikes me that the real issue
here is that we're confusing privilege level with time-duration of the
setting's effect. The point of marking log_connections as PGC_BACKEND is
that changing it within a session after a given session starts is useless,
and it's probably better to freeze it so it can tell you what was actually
done by the current session. The point of marking log_disconnections as
PGC_BACKEND is so that you can freeze the determination of whether a given
session will log its disconnection at the same time that its determination
of whether to log its connection got frozen, and thus ensure that each
connection log entry should eventually have a disconnection log entry,
assuming you have both enabled. These considerations are not invalidated
by questions of which users should be allowed to adjust the value.
In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
are allowed to change it. I don't have any objection to making these two
settings only adjustable by superusers --- I just don't want to give up
the existing timing restrictions for them.
(If we did this, there are probably some other settings that should become
PGC_SU_BACKEND, eg session_preload_libraries ...)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
At 2014-07-02 12:52:51 -0700, mail@joeconway.com wrote:
Doesn't mean that to me but feel free to change it to Waiting on
Author if you prefer :-)Is there any official explanation as to what those states mean
documented anywhere?
I don't know if there's an official definition, but my understanding of
"returned with feedback" was always pretty much in agreement with what
Fujii wrote. If the author is expected to post a revised patch soon, it
gets marked "waiting on author", and "returned with feedback" means it
will take longer, probably in the next CF.
But I also treat these labels as a matter of convenience, and definitely
not some mark of shame where the author should feel upset that the patch
was put in one state or the other. As far as I'm concerned, patches can
be switched from "returned with feedback" to "needs review" to "ready
for committer" at the drop of a hat if updates are made in time.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Abhijit Menon-Sen wrote:
At 2014-07-02 12:52:51 -0700, mail@joeconway.com wrote:
Doesn't mean that to me but feel free to change it to Waiting on
Author if you prefer :-)Is there any official explanation as to what those states mean
documented anywhere?I don't know if there's an official definition, but my understanding of
"returned with feedback" was always pretty much in agreement with what
Fujii wrote. If the author is expected to post a revised patch soon, it
gets marked "waiting on author", and "returned with feedback" means it
will take longer, probably in the next CF.
I think the main thing with "returned with feedback" is the patch is not
supposed to be handled any further in the current commitfest. Normally
if a new version is submitted, it's opened as a new entry in a future
commitfest. So it's one of the three final states for a patch, the
other two being "committed" and "rejected". The other status values,
"needs review", "waiting on author", and "ready for committer" are
transient and can change to any other state.
So I disagree with you here:
But I also treat these labels as a matter of convenience, and definitely
not some mark of shame where the author should feel upset that the patch
was put in one state or the other. As far as I'm concerned, patches can
be switched from "returned with feedback" to "needs review" to "ready
for committer" at the drop of a hat if updates are made in time.
A patch that is Returned with Feedback is *not* supposed to go back to
"needs review" or any of the other states. If we expect that the author
is going to update the patch, the right state to use is "Waiting on
author".
In any case, no state is a mark of shame on the author. We are not
supposed to judge people here.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
are allowed to change it. I don't have any objection to making these two
settings only adjustable by superusers --- I just don't want to give up
the existing timing restrictions for them.
Another idea would be to get rid of PGC_SUSET as a separate category, and
instead have a superuser-only bit in the GUC flags, which would apply to
all categories. This would be a bit more orthogonal, though likely a
much more invasive change.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
At 2014-07-02 16:47:16 -0400, alvherre@2ndquadrant.com wrote:
If we expect that the author is going to update the patch, the right
state to use is "Waiting on author".
Quite so. That's how I understand it, and what I've been doing. But what
if we really *don't* expect the author to update the patch, but they do
it anyway? That's the only case I was referring to.
If the right thing to do is to open an entry in the next CF (as you said
earlier in your mail), that's all right with me.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Abhijit Menon-Sen wrote:
At 2014-07-02 16:47:16 -0400, alvherre@2ndquadrant.com wrote:
If we expect that the author is going to update the patch, the right
state to use is "Waiting on author".Quite so. That's how I understand it, and what I've been doing. But what
if we really *don't* expect the author to update the patch, but they do
it anyway? That's the only case I was referring to.If the right thing to do is to open an entry in the next CF (as you said
earlier in your mail), that's all right with me.
As Tom says I think we should be open to the possibility that we made a
mistake and that it should return to "needs review", when reasonable.
For example if the author takes long to update and we're in the final
steps of closing the commitfest, I don't think we need to feel forced to
re-examine the patch in the same commitfest.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 07/02/2014 02:15 PM, Abhijit Menon-Sen wrote:
At 2014-07-02 16:47:16 -0400, alvherre@2ndquadrant.com wrote:
If we expect that the author is going to update the patch, the
right state to use is "Waiting on author".Quite so. That's how I understand it, and what I've been doing. But
what if we really *don't* expect the author to update the patch,
but they do it anyway? That's the only case I was referring to.If the right thing to do is to open an entry in the next CF (as you
said earlier in your mail), that's all right with me.
In any case I think this side discussion has proven there is not
universal understanding on the particulars and in any case we ought to
have clear definitions (probably with examples) documented somewhere
if anyone is expected to take them quite so specifically.
Also, given Tom's remarks on the other part of this thread, I would
not be surprised if this turned out to be a much larger change than
Fujii-san originally hoped for, and thus may in fact get delayed to
next CF.
Joe
- --
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJTtHhTAAoJEDfy90M199hlPmYP/09Cfq8zktVIEePqp/IN9bTo
RW2xL8C/+TsDlHcHNmwB46PpP4xOYnOP6my7EOS4xRKJrF6fwybZUu2nJ2T+ifkP
VHNTRHbW2ndpOR1CkmiJvbNGdr4uWHdzt1RsMrbPP+qIQl5tBnb/vBfI33B8/qzC
tkYCSbhcVaYACajCJaobelWfUbREgBf255x0VdprdyBjmSRq/d1trm3sh2IshKCz
KV2YjLWN6lVENAtp8LLFbpZLVOQPQ0direY1bwcSM+/SC9XqhQeaEWWp7WMgeGtl
2VNObiFJIGDWjQFc2qW/VIyKVHGhvkxpuQ3KSw4gj64EQ561OTGYIW0S+QMHXwt8
krW50yVHw/MJ6efmx2rlBbBqugMB/rw3qe6YpuEcF5s8ezQt9b5ejQC9hZxAl8ln
d8BFISjlT5nroBRCvPYeJO8k7mFB9JFfaOi/GWru+dg/J4Wz/V6Rjr+n4yybjg2V
vNRbJZgAJBIn6iTlKHrx3/QtU7tt4kUIr7gUCVEhYLLAFikPIItARzNYcJZ77jVK
lX30v95gw8IJm1MmhEEkQFKmXzoTYNhh8sEn6t3a8zxRRcIIy+l0jD5lEDxSEirj
lRIgvHtU+LyNkCY6d+V3jPuyg1FlJcNhotUr7KSgwUuij+MrVMxook5IiiJsu/3s
VayKFPq0FLkpkJXSIaha
=afxT
-----END PGP SIGNATURE-----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 2, 2014 at 10:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
are allowed to change it. I don't have any objection to making these two
settings only adjustable by superusers --- I just don't want to give up
the existing timing restrictions for them.Another idea would be to get rid of PGC_SUSET as a separate category, and
instead have a superuser-only bit in the GUC flags, which would apply to
all categories. This would be a bit more orthogonal, though likely a
much more invasive change.
That could become interesting in the futuren ow that we have ALTER
SYSTEM SET. It could allow a non-superuser to make persistent
configuration changes. Now, I'm not sure we actually *want* that
though... But having it as a separate bit would make it possible for
ALTER SYSTEM SET to say that for example regular users would be able
to change work_mem persistently. But if we want to go down that route,
we might need a more fine grained permissions model than just
superuser vs non-superuser...
I think going with the PGC_SU_BACKEND is the right choice at this
time, until we have an actual usecase for the other :)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 3, 2014 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail@joeconway.com> wrote:
No. If we change it to PGC_SIGHUP, SHOW command does display
the changed value after a reload. It's the same behavior as other
PGC_SIGHUP parameters do. Attached patch just changes it to PGC_SIGHUP.
You can test that by using the patch.
As this patch is marked as Needs Review, so I went ahead and
picked up for review, however after reading mail chain, it seems to
me that there is a general inclination to have a new category in
GucContext for this feature. I don't see the patch implementing the
same in this thread, so I think it is better to move it to next CF
(2014-08).
Whats your opinion?
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Mon, Jul 28, 2014 at 12:22 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 3, 2014 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail@joeconway.com> wrote:
No. If we change it to PGC_SIGHUP, SHOW command does display
the changed value after a reload. It's the same behavior as other
PGC_SIGHUP parameters do. Attached patch just changes it to PGC_SIGHUP.
You can test that by using the patch.As this patch is marked as Needs Review, so I went ahead and
picked up for review, however after reading mail chain, it seems to
me that there is a general inclination to have a new category in
GucContext for this feature. I don't see the patch implementing the
same in this thread, so I think it is better to move it to next CF
(2014-08).
Yep, agreed. I just moved this to next CF.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jul 15, 2014 at 10:45 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Wed, Jul 2, 2014 at 10:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
are allowed to change it. I don't have any objection to making these two
settings only adjustable by superusers --- I just don't want to give up
the existing timing restrictions for them.Another idea would be to get rid of PGC_SUSET as a separate category, and
instead have a superuser-only bit in the GUC flags, which would apply to
all categories. This would be a bit more orthogonal, though likely a
much more invasive change.That could become interesting in the futuren ow that we have ALTER
SYSTEM SET. It could allow a non-superuser to make persistent
configuration changes. Now, I'm not sure we actually *want* that
though... But having it as a separate bit would make it possible for
ALTER SYSTEM SET to say that for example regular users would be able
to change work_mem persistently. But if we want to go down that route,
we might need a more fine grained permissions model than just
superuser vs non-superuser...I think going with the PGC_SU_BACKEND is the right choice at this
time, until we have an actual usecase for the other :)
Yep, the attached patch introduces PGC_SU_BACKEND and
changes the contexts of log_connections and log_disconnections
to PGC_SU_BACKEND. Review?
Regards,
--
Fujii Masao
Attachments:
pgc-su-backend_v1.patchtext/x-patch; charset=US-ASCII; name=pgc-su-backend_v1.patchDownload
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 3258,3264 **** get_stats_option_name(const char *arg)
* argv[0] is ignored in either case (it's assumed to be the program name).
*
* ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
! * coming from the client, or PGC_SUSET for insecure options coming from
* a superuser client.
*
* If a database name is present in the command line arguments, it's
--- 3258,3264 ----
* argv[0] is ignored in either case (it's assumed to be the program name).
*
* ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
! * coming from the client, or PGC_SU_BACKEND for insecure options coming from
* a superuser client.
*
* If a database name is present in the command line arguments, it's
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
***************
*** 957,963 **** process_startup_options(Port *port, bool am_superuser)
GucContext gucctx;
ListCell *gucopts;
! gucctx = am_superuser ? PGC_SUSET : PGC_BACKEND;
/*
* First process any command-line switches that were included in the
--- 957,963 ----
GucContext gucctx;
ListCell *gucopts;
! gucctx = am_superuser ? PGC_SU_BACKEND : PGC_BACKEND;
/*
* First process any command-line switches that were included in the
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 512,517 **** const char *const GucContext_Names[] =
--- 512,518 ----
/* PGC_INTERNAL */ "internal",
/* PGC_POSTMASTER */ "postmaster",
/* PGC_SIGHUP */ "sighup",
+ /* PGC_SU_BACKEND */ "superuser-backend",
/* PGC_BACKEND */ "backend",
/* PGC_SUSET */ "superuser",
/* PGC_USERSET */ "user"
***************
*** 910,916 **** static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
! {"log_connections", PGC_BACKEND, LOGGING_WHAT,
gettext_noop("Logs each successful connection."),
NULL
},
--- 911,917 ----
NULL, NULL, NULL
},
{
! {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT,
gettext_noop("Logs each successful connection."),
NULL
},
***************
*** 919,925 **** static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
! {"log_disconnections", PGC_BACKEND, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
--- 920,926 ----
NULL, NULL, NULL
},
{
! {"log_disconnections", PGC_SU_BACKEND, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
***************
*** 5681,5696 **** set_config_option(const char *name, const char *value,
* signals to individual backends only.
*/
break;
case PGC_BACKEND:
if (context == PGC_SIGHUP)
{
/*
! * If a PGC_BACKEND parameter is changed in the config file,
! * we want to accept the new value in the postmaster (whence
! * it will propagate to subsequently-started backends), but
! * ignore it in existing backends. This is a tad klugy, but
! * necessary because we don't re-read the config file during
! * backend start.
*
* In EXEC_BACKEND builds, this works differently: we load all
* nondefault settings from the CONFIG_EXEC_PARAMS file during
--- 5682,5706 ----
* signals to individual backends only.
*/
break;
+ case PGC_SU_BACKEND:
+ if (context == PGC_BACKEND)
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to set parameter \"%s\"",
+ name)));
+ return 0;
+ }
case PGC_BACKEND:
if (context == PGC_SIGHUP)
{
/*
! * If a PGC_SU_BACKEND or PGC_BACKEND parameter is changed in
! * the config file, we want to accept the new value in the
! * postmaster (whence it will propagate to subsequently-started
! * backends), but ignore it in existing backends. This is a tad
! * klugy, but necessary because we don't re-read the config file
! * during backend start.
*
* In EXEC_BACKEND builds, this works differently: we load all
* nondefault settings from the CONFIG_EXEC_PARAMS file during
***************
*** 5709,5716 **** set_config_option(const char *name, const char *value,
return -1;
#endif
}
! else if (context != PGC_POSTMASTER && context != PGC_BACKEND &&
! source != PGC_S_CLIENT)
{
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
--- 5719,5726 ----
return -1;
#endif
}
! else if (context != PGC_POSTMASTER && context != PGC_SU_BACKEND &&
! context != PGC_SU_BACKEND && source != PGC_S_CLIENT)
{
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
***************
*** 8353,8360 **** read_nondefault_variables(void)
GucContext varscontext;
/*
! * Assert that PGC_BACKEND case in set_config_option() will do the right
! * thing.
*/
Assert(IsInitProcessingMode());
--- 8363,8370 ----
GucContext varscontext;
/*
! * Assert that PGC_SU_BACKEND and PGC_BACKEND case in set_config_option()
! * will do the right thing.
*/
Assert(IsInitProcessingMode());
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
***************
*** 36,47 ****
* certain point in their main loop. It's safer to wait than to read a
* file asynchronously.)
*
! * BACKEND options can only be set at postmaster startup, from the
* configuration file, or by client request in the connection startup
! * packet (e.g., from libpq's PGOPTIONS variable). Furthermore, an
! * already-started backend will ignore changes to such an option in the
! * configuration file. The idea is that these options are fixed for a
! * given backend once it's started, but they can vary across backends.
*
* SUSET options can be set at postmaster startup, with the SIGHUP
* mechanism, or from SQL if you're a superuser.
--- 36,52 ----
* certain point in their main loop. It's safer to wait than to read a
* file asynchronously.)
*
! * SU_BACKEND options can only be set at postmaster startup, from the
* configuration file, or by client request in the connection startup
! * packet (e.g., from libpq's PGOPTIONS variable) if you're a superuser.
! * Furthermore, an already-started backend will ignore changes to
! * such an option in the configuration file. The idea is that these options
! * are fixed for a given backend once it's started, but they can vary
! * across backends.
! *
! * BACKEND options are the same as SU_BACKEND ones, but they can
! * be set by client request in the connection startup packet even when
! * you're not a superuser.
*
* SUSET options can be set at postmaster startup, with the SIGHUP
* mechanism, or from SQL if you're a superuser.
***************
*** 53,58 **** typedef enum
--- 58,64 ----
PGC_INTERNAL,
PGC_POSTMASTER,
PGC_SIGHUP,
+ PGC_SU_BACKEND,
PGC_BACKEND,
PGC_SUSET,
PGC_USERSET
On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Yep, the attached patch introduces PGC_SU_BACKEND and
changes the contexts of log_connections and log_disconnections
to PGC_SU_BACKEND. Review?
1.
! else if (context != PGC_POSTMASTER && context != PGC_SU_BACKEND &&
! context != PGC_SU_BACKEND && source != PGC_S_CLIENT)
In the above check for PGC_SU_BACKEND is repeated, here
one of the check should be PGC_SU_BACKEND and other
should be PGC_BACKEND.
2.
+ case PGC_SU_BACKEND:
+ if (context == PGC_BACKEND)
+ {
..
..
+ return 0;
+ }
case PGC_BACKEND:
if (context == PGC_SIGHUP)Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?
Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f. show log_connections; --This step shows the value as on,
--whereas I think it should have
been off
This test has been performed on *Windows*.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Yep, the attached patch introduces PGC_SU_BACKEND and
changes the contexts of log_connections and log_disconnections
to PGC_SU_BACKEND. Review?
Thanks for reviewing the patch!
1.
! else if (context != PGC_POSTMASTER && context != PGC_SU_BACKEND &&
! context != PGC_SU_BACKEND && source != PGC_S_CLIENT)In the above check for PGC_SU_BACKEND is repeated, here
one of the check should be PGC_SU_BACKEND and other
should be PGC_BACKEND.
Right. Fixed. Attached is the updated version of the patch.
BTW, I also added the following into the document of log_connections
and log_disconnections.
Only superusers can change this setting at session start.
2. + case PGC_SU_BACKEND: + if (context == PGC_BACKEND) + { .. .. + return 0; + } case PGC_BACKEND: if (context == PGC_SIGHUP)Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f. show log_connections; --This step shows the value as on,
--whereas I think it should have been
off
In this case, log_connections is changed in postgresql.conf and it's
reloaded, so ISTM that it's natural that even non-superuser sees the
changed value. No? Maybe I'm missing something.
Regards,
--
Fujii Masao
Attachments:
pgc-su-backend_v2.patchtext/x-patch; charset=US-ASCII; name=pgc-su-backend_v2.patchDownload
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 4231,4236 **** local0.* /var/log/postgresql
--- 4231,4237 ----
<para>
Causes each attempted connection to the server to be logged,
as well as successful completion of client authentication.
+ Only superusers can change this setting at session start.
This parameter cannot be changed after session start.
The default is off.
</para>
***************
*** 4258,4263 **** local0.* /var/log/postgresql
--- 4259,4265 ----
<varname>log_connections</varname> but at session termination,
and includes the duration of the session. This is off by
default.
+ Only superusers can change this setting at session start.
This parameter cannot be changed after session start.
</para>
</listitem>
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 3258,3264 **** get_stats_option_name(const char *arg)
* argv[0] is ignored in either case (it's assumed to be the program name).
*
* ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
! * coming from the client, or PGC_SUSET for insecure options coming from
* a superuser client.
*
* If a database name is present in the command line arguments, it's
--- 3258,3264 ----
* argv[0] is ignored in either case (it's assumed to be the program name).
*
* ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
! * coming from the client, or PGC_SU_BACKEND for insecure options coming from
* a superuser client.
*
* If a database name is present in the command line arguments, it's
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
***************
*** 957,963 **** process_startup_options(Port *port, bool am_superuser)
GucContext gucctx;
ListCell *gucopts;
! gucctx = am_superuser ? PGC_SUSET : PGC_BACKEND;
/*
* First process any command-line switches that were included in the
--- 957,963 ----
GucContext gucctx;
ListCell *gucopts;
! gucctx = am_superuser ? PGC_SU_BACKEND : PGC_BACKEND;
/*
* First process any command-line switches that were included in the
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 509,514 **** const char *const GucContext_Names[] =
--- 509,515 ----
/* PGC_INTERNAL */ "internal",
/* PGC_POSTMASTER */ "postmaster",
/* PGC_SIGHUP */ "sighup",
+ /* PGC_SU_BACKEND */ "superuser-backend",
/* PGC_BACKEND */ "backend",
/* PGC_SUSET */ "superuser",
/* PGC_USERSET */ "user"
***************
*** 907,913 **** static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
! {"log_connections", PGC_BACKEND, LOGGING_WHAT,
gettext_noop("Logs each successful connection."),
NULL
},
--- 908,914 ----
NULL, NULL, NULL
},
{
! {"log_connections", PGC_SU_BACKEND, LOGGING_WHAT,
gettext_noop("Logs each successful connection."),
NULL
},
***************
*** 916,922 **** static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
! {"log_disconnections", PGC_BACKEND, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
--- 917,923 ----
NULL, NULL, NULL
},
{
! {"log_disconnections", PGC_SU_BACKEND, LOGGING_WHAT,
gettext_noop("Logs end of a session, including duration."),
NULL
},
***************
*** 5685,5700 **** set_config_option(const char *name, const char *value,
* signals to individual backends only.
*/
break;
case PGC_BACKEND:
if (context == PGC_SIGHUP)
{
/*
! * If a PGC_BACKEND parameter is changed in the config file,
! * we want to accept the new value in the postmaster (whence
! * it will propagate to subsequently-started backends), but
! * ignore it in existing backends. This is a tad klugy, but
! * necessary because we don't re-read the config file during
! * backend start.
*
* In EXEC_BACKEND builds, this works differently: we load all
* nondefault settings from the CONFIG_EXEC_PARAMS file during
--- 5686,5710 ----
* signals to individual backends only.
*/
break;
+ case PGC_SU_BACKEND:
+ if (context == PGC_BACKEND)
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to set parameter \"%s\"",
+ name)));
+ return 0;
+ }
case PGC_BACKEND:
if (context == PGC_SIGHUP)
{
/*
! * If a PGC_SU_BACKEND or PGC_BACKEND parameter is changed in
! * the config file, we want to accept the new value in the
! * postmaster (whence it will propagate to subsequently-started
! * backends), but ignore it in existing backends. This is a tad
! * klugy, but necessary because we don't re-read the config file
! * during backend start.
*
* In EXEC_BACKEND builds, this works differently: we load all
* nondefault settings from the CONFIG_EXEC_PARAMS file during
***************
*** 5714,5720 **** set_config_option(const char *name, const char *value,
#endif
}
else if (context != PGC_POSTMASTER && context != PGC_BACKEND &&
! source != PGC_S_CLIENT)
{
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
--- 5724,5730 ----
#endif
}
else if (context != PGC_POSTMASTER && context != PGC_BACKEND &&
! context != PGC_SU_BACKEND && source != PGC_S_CLIENT)
{
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
***************
*** 8357,8364 **** read_nondefault_variables(void)
GucContext varscontext;
/*
! * Assert that PGC_BACKEND case in set_config_option() will do the right
! * thing.
*/
Assert(IsInitProcessingMode());
--- 8367,8374 ----
GucContext varscontext;
/*
! * Assert that PGC_SU_BACKEND and PGC_BACKEND case in set_config_option()
! * will do the right thing.
*/
Assert(IsInitProcessingMode());
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
***************
*** 36,47 ****
* certain point in their main loop. It's safer to wait than to read a
* file asynchronously.)
*
! * BACKEND options can only be set at postmaster startup, from the
* configuration file, or by client request in the connection startup
! * packet (e.g., from libpq's PGOPTIONS variable). Furthermore, an
! * already-started backend will ignore changes to such an option in the
! * configuration file. The idea is that these options are fixed for a
! * given backend once it's started, but they can vary across backends.
*
* SUSET options can be set at postmaster startup, with the SIGHUP
* mechanism, or from SQL if you're a superuser.
--- 36,52 ----
* certain point in their main loop. It's safer to wait than to read a
* file asynchronously.)
*
! * SU_BACKEND options can only be set at postmaster startup, from the
* configuration file, or by client request in the connection startup
! * packet (e.g., from libpq's PGOPTIONS variable) if you're a superuser.
! * Furthermore, an already-started backend will ignore changes to
! * such an option in the configuration file. The idea is that these options
! * are fixed for a given backend once it's started, but they can vary
! * across backends.
! *
! * BACKEND options are the same as SU_BACKEND ones, but they can
! * be set by client request in the connection startup packet even when
! * you're not a superuser.
*
* SUSET options can be set at postmaster startup, with the SIGHUP
* mechanism, or from SQL if you're a superuser.
***************
*** 53,58 **** typedef enum
--- 58,64 ----
PGC_INTERNAL,
PGC_POSTMASTER,
PGC_SIGHUP,
+ PGC_SU_BACKEND,
PGC_BACKEND,
PGC_SUSET,
PGC_USERSET
On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:
On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:
Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f. show log_connections; --This step shows the value as on,
--whereas I think it should have
been
off
In this case, log_connections is changed in postgresql.conf and it's
reloaded, so ISTM that it's natural that even non-superuser sees the
changed value. No? Maybe I'm missing something.
Yeah, you are right.
With the latest patch, I am getting one regression failure on windows.
Attached is the regression diff.
Can we improve this line a bit?
! * BACKEND options are the same as SU_BACKEND ones, but they can
BACKEND options can be set same as SU_BACKEND ones, ......
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
regression.diffsapplication/octet-stream; name=regression.diffsDownload
*** e:/WorkSpace/PostgreSQL/master/postgresql/src/test/regress/expected/plpgsql.out Wed Apr 9 09:39:57 2014
--- e:/WorkSpace/PostgreSQL/master/postgresql/src/test/regress/results/plpgsql.out Thu Aug 28 19:23:21 2014
***************
*** 4823,4828 ****
--- 4823,4829 ----
drop function scope_test();
-- Check handling of conflicts between plpgsql vars and table columns.
set plpgsql.variable_conflict = error;
+ ERROR: parameter "plpgsql.variable_conflict" cannot be set after connection start
create function conflict_test() returns setof int8_tbl as $$
declare r record;
q1 bigint := 42;
======================================================================
On Thu, Aug 28, 2014 at 11:23 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:
Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f. show log_connections; --This step shows the value as on,
--whereas I think it should have
been
offIn this case, log_connections is changed in postgresql.conf and it's
reloaded, so ISTM that it's natural that even non-superuser sees the
changed value. No? Maybe I'm missing something.Yeah, you are right.
With the latest patch, I am getting one regression failure on windows.
Attached is the regression diff.
Thanks for testing the patch!
That regression failure looks strange, I'm not sure yet why that happened...
Does it happen only on Windows?
Can we improve this line a bit?
! * BACKEND options are the same as SU_BACKEND ones, but they can
BACKEND options can be set same as SU_BACKEND ones, ......
Yep.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 3, 2014 at 8:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Aug 28, 2014 at 11:23 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:
On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:
On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:
Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f. show log_connections; --This step shows the value as on,
--whereas I think it should
have
been
offIn this case, log_connections is changed in postgresql.conf and it's
reloaded, so ISTM that it's natural that even non-superuser sees the
changed value. No? Maybe I'm missing something.Yeah, you are right.
With the latest patch, I am getting one regression failure on windows.
Attached is the regression diff.Thanks for testing the patch!
That regression failure looks strange, I'm not sure yet why that
happened...
Does it happen only on Windows?
Yes, it was failing only on windows. Today when I further tried to
look into the issue, I found that if I rebuild plpgsql, it didn't occurred.
So the conclusion is that it occurred due to build mismatch, however I
am not sure why a rebuild of plpgsql is required for this patch.
Sorry for the noise.
There are no more comments from myside, so I will mark this as
"Ready For Committer".
Can we improve this line a bit?
! * BACKEND options are the same as SU_BACKEND ones, but they can
BACKEND options can be set same as SU_BACKEND ones, ......Yep.
Okay.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
Yes, it was failing only on windows. Today when I further tried to
look into the issue, I found that if I rebuild plpgsql, it didn't occurred.
So the conclusion is that it occurred due to build mismatch, however I
am not sure why a rebuild of plpgsql is required for this patch.
Sorry for the noise.
plpgsql contains references to PGC_SUSET and PGC_USERSET, whose values are
changed by this patch, so failure is unsurprising if you don't rebuild it.
(I saw failures in the regression tests even without Windows until I
updated plpgsql.)
Committed with minor cosmetic adjustments; mostly, rewriting some
comments.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers