Re: 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

On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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

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.

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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

On Mon, Jun 16, 2014 at 4:14 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)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

On Fri, Jun 13, 2014 at 11:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/13/2014 07:29 AM, Tom Lane wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
>> <masao(dot)fujii(at)gmail(dot)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-----

On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 06/13/2014 07:29 AM, Tom Lane wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
>>> <masao(dot)fujii(at)gmail(dot)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.
http://www.postgresql.org/message-id/1402675662004-5807224.post@n5.nabble.com

Regards,

--
Fujii Masao

On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 06/13/2014 07:29 AM, Tom Lane wrote:
>>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>>> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
>>>> <masao(dot)fujii(at)gmail(dot)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.
> http://www.postgresql.org/message-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

On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 06/13/2014 07:29 AM, Tom Lane wrote:
>>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>>> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
>>>> <masao(dot)fujii(at)gmail(dot)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

-----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-----

On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
> -----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.

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

-----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-----

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail(at)joeconway(dot)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

At 2014-07-02 12:52:51 -0700, mail(at)joeconway(dot)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

Abhijit Menon-Sen wrote:
> At 2014-07-02 12:52:51 -0700, mail(at)joeconway(dot)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

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

At 2014-07-02 16:47:16 -0400, alvherre(at)2ndquadrant(dot)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

Abhijit Menon-Sen wrote:
> At 2014-07-02 16:47:16 -0400, alvherre(at)2ndquadrant(dot)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

-----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(at)2ndquadrant(dot)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-----

On Wed, Jul 2, 2014 at 10:52 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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/

On Thu, Jul 3, 2014 at 1:13 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail(at)joeconway(dot)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(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, Jul 3, 2014 at 1:13 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway <mail(at)joeconway(dot)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

On Tue, Jul 15, 2014 at 10:45 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Wed, Jul 2, 2014 at 10:52 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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

On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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

On Thu, Aug 28, 2014 at 11:23 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>
>> On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> > On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.

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

On Wed, Sep 3, 2014 at 8:09 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Thu, Aug 28, 2014 at 11:23 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
wrote:
> >>
> >> On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> >> wrote:
> >> > On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.
>
> 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(dot)kapila16(at)gmail(dot)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