Horton

Thread

Commits

  1. Teach SHOW ALL to honor pg_read_all_settings membership

  2. Default monitoring roles

  1. SHOW ALL does not honor pg_read_all_settings membership

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-03-01T15:22:38Z

    I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.

Patch attached; I think this should be backpatched.

Yours,
Laurenz Albe

  2. Re: SHOW ALL does not honor pg_read_all_settings membership

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-04-16T12:32:10Z

    On Thu, 2018-03-01 at 16:22 +0100, I wrote:
> I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
> to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.
> 
> Patch attached; I think this should be backpatched.

Now that the dust from the last commitfest is settling, I'll make a second
attempt to attract attention for this small bug fix.

The original commit was Simon's.

Yours,
Laurenz Albe

  3. Re: SHOW ALL does not honor pg_read_all_settings membership

    Michael Paquier <michael@paquier.xyz> — 2018-04-17T03:28:58Z

    On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote:
> Now that the dust from the last commitfest is settling, I'll make a second
> attempt to attract attention for this small bug fix.
> 
> The original commit was Simon's.

Thanks for the ping.

This was new as of v10, so this cannot be listed as an open item still I
have added that under the section for older bugs, because you are right
as far as I can see.

GetConfigOption is wrong by the way, as restrict_superuser means that
all members of the group pg_read_all_settings can read
GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at
least needs a fix, the variable ought to be renamed as well.
--
Michael

  4. Re: SHOW ALL does not honor pg_read_all_settings membership

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-04-20T11:21:46Z

    Michael Paquier wrote:
> On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote:
> > Now that the dust from the last commitfest is settling, I'll make a second
> > attempt to attract attention for this small bug fix.
> > 
> > The original commit was Simon's.
> 
> Thanks for the ping.
> 
> This was new as of v10, so this cannot be listed as an open item still I
> have added that under the section for older bugs, because you are right
> as far as I can see.
> 
> GetConfigOption is wrong by the way, as restrict_superuser means that
> all members of the group pg_read_all_settings can read
> GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at
> least needs a fix, the variable ought to be renamed as well.

Thanks for the review!

I agree; here is a patch for that.

Yours,
Laurenz Albe

  5. Re: SHOW ALL does not honor pg_read_all_settings membership

    Michael Paquier <michael@paquier.xyz> — 2018-04-20T13:58:06Z

    On Fri, Apr 20, 2018 at 01:21:46PM +0200, Laurenz Albe wrote:
> I agree; here is a patch for that.

Thanks for taking care of that as well this looks fine to me.  Note to
committer: this needs to be merged with the first patch actually fixing
the SHOW ALL issue.

All the four core callers of GetConfigOption() actually do not use
restrict_superuser set to true...  Modules may use this option, so of
course let's not remove it.
--
Michael

  6. Re: SHOW ALL does not honor pg_read_all_settings membership

    Simon Riggs <simon@2ndquadrant.com> — 2018-05-31T18:05:58Z

    On 17 April 2018 at 04:28, Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Apr 16, 2018 at 02:32:10PM +0200, Laurenz Albe wrote:
>> Now that the dust from the last commitfest is settling, I'll make a second
>> attempt to attract attention for this small bug fix.
>>
>> The original commit was Simon's.
>
> Thanks for the ping.
>
> This was new as of v10, so this cannot be listed as an open item still I
> have added that under the section for older bugs, because you are right
> as far as I can see.

OK, agreed, its a bug.

Any objections to backpatch to v10?

> GetConfigOption is wrong by the way, as restrict_superuser means that
> all members of the group pg_read_all_settings can read
> GUC_SUPERUSER_ONLY params, and not only superusers, so the comment at
> least needs a fix, the variable ought to be renamed as well.

OK

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  7. Re: SHOW ALL does not honor pg_read_all_settings membership

    Michael Paquier <michael@paquier.xyz> — 2018-05-31T18:42:42Z

    On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote:
> Any objections to backpatch to v10?

A backpatch is acceptable in my opinion.
--
Michael

  8. Re: SHOW ALL does not honor pg_read_all_settings membership

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-07T21:23:57Z

    On 2018-May-31, Michael Paquier wrote:

> On Thu, May 31, 2018 at 07:05:58PM +0100, Simon Riggs wrote:
> > Any objections to backpatch to v10?
> 
> A backpatch is acceptable in my opinion.

Agreed on backpatching.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: SHOW ALL does not honor pg_read_all_settings membership

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T19:08:25Z

    BTW a further bug here seems to be that "select * from pg_settings()"
does not show the source file/line for members of the role, which seems
to be documented to occur.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: SHOW ALL does not honor pg_read_all_settings membership

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T19:13:57Z

    On 2018-Jun-08, Alvaro Herrera wrote:

> BTW a further bug here seems to be that "select * from pg_settings()"
> does not show the source file/line for members of the role, which seems
> to be documented to occur.

And I think this fixes it.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: SHOW ALL does not honor pg_read_all_settings membership

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-06-08T20:35:52Z

    On 2018-Mar-01, Laurenz Albe wrote:

> I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
> to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.
> 
> Patch attached; I think this should be backpatched.

Done, with the further fixes downthread.  Thanks!

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: SHOW ALL does not honor pg_read_all_settings membership

    Michael Paquier <michael@paquier.xyz> — 2018-06-11T01:38:14Z

    On Fri, Jun 08, 2018 at 03:13:57PM -0400, Alvaro Herrera wrote:
> And I think this fixes it.

-    if (conf->source == PGC_S_FILE && superuser())
+    if (conf->source == PGC_S_FILE &&
+        is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))

Thanks Alvaro!  This bit in GetConfigOptionByNum() looks fine to me.
--
Michael

  13. Re: SHOW ALL does not honor pg_read_all_settings membership

    Laurenz Albe <laurenz.albe@cybertec.at> — 2018-06-11T06:28:23Z

    Alvaro Herrera wrote:
> On 2018-Mar-01, Laurenz Albe wrote:
> 
> > I noticed that commit 25fff40798fc4ac11a241bfd9ab0c45c085e2212 forgot
> > to teach SHOW ALL to show all GUCs when the user belongs to pg_read_all_settings.
> > 
> > Patch attached; I think this should be backpatched.
> 
> Done, with the further fixes downthread.  Thanks!

Thank you!

Laurenz Albe