Обсуждение: 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.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
Вложения
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
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@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
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
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
-----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-----
On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail@joeconway.com> wrote: > -----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 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 rolesRole 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.fujii@gmail.com> wrote: > On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail@joeconway.com> wrote: >> -----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 > > 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 changedvalue has no effect on whether logout is logged or not because running backend will never use it except SHOWcommand. 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. Youmay think that this inconsistency is a problem. For this, we can add new GUC flag which forbid the reload of configuration file to changethe 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.fujii@gmail.com> wrote: > On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail@joeconway.com> wrote: >> -----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 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@joeconway.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.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
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
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
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
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
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
-----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-----
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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
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.
> 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?
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
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
Вложения
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.
>
> 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)
+ {
..
+ if (context == PGC_BACKEND)
+ {
..
..
+ return 0;
+ }
case PGC_BACKEND:
if (context == PGC_SIGHUP)
+ 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*.
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
Вложения
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.
>
> 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, ......
BACKEND options can be set same as SU_BACKEND ones, ......
Вложения
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 >> > 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.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
> >> > 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
> > 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.
>
> 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
> >> > 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.
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