Horton

Thread

Commits

  1. Fix GRANTED BY support in REVOKE ROLE statements

  2. Allow GRANTED BY clause in normal GRANT and REVOKE statements

  3. Try to stabilize output from rolenames regression test.

  4. Allow CURRENT_ROLE where CURRENT_USER is accepted

  1. Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-24T06:35:58Z

    I was checking some loose ends in SQL conformance, when I noticed: We 
support GRANT role ... GRANTED BY CURRENT_USER, but we don't support 
CURRENT_ROLE in that place, even though in PostgreSQL they are 
equivalent.  Here is a trivial patch to add that.

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

  2. Re: Allow CURRENT_ROLE in GRANTED BY

    Vik Fearing <vik@postgresfriends.org> — 2020-06-24T08:12:38Z

    On 6/24/20 8:35 AM, Peter Eisentraut wrote:
> I was checking some loose ends in SQL conformance, when I noticed: We
> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> CURRENT_ROLE in that place, even though in PostgreSQL they are
> equivalent.  Here is a trivial patch to add that.


The only thing that isn't dead-obvious about this patch is the commit
message says "[PATCH 1/2]".  What is in the other part?

Assuming that's just a remnant of development, this LGTM.
-- 
Vik Fearing

  3. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-24T18:21:29Z

    On 2020-06-24 10:12, Vik Fearing wrote:
> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>> I was checking some loose ends in SQL conformance, when I noticed: We
>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>> equivalent.  Here is a trivial patch to add that.
> 
> 
> The only thing that isn't dead-obvious about this patch is the commit
> message says "[PATCH 1/2]".  What is in the other part?

Hehe.  The second patch is some in-progress work to add the GRANTED BY 
clause to the regular GRANT command.  More on that perhaps at a later date.

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

  4. Re: Allow CURRENT_ROLE in GRANTED BY

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-06-24T21:08:31Z

    On 2020-Jun-24, Peter Eisentraut wrote:

> I was checking some loose ends in SQL conformance, when I noticed: We
> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> CURRENT_ROLE in that place, even though in PostgreSQL they are equivalent.
> Here is a trivial patch to add that.

Hmm, since this adds to RoleSpec, this change makes every place that
uses that production also take CURRENT_ROLE, so we'd need to document in
all those places.  For example, alter_role.sgml, create_schema.sgml,
etc.

This also affects role_list (but maybe the docs for those are already
vague enough -- eg. ALTER INDEX .. OWNED BY only says "role_name" with
no further explanation, even though it does take "current_user".)

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

  5. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-29T12:47:38Z

    On 2020-06-24 23:08, Alvaro Herrera wrote:
> On 2020-Jun-24, Peter Eisentraut wrote:
> 
>> I was checking some loose ends in SQL conformance, when I noticed: We
>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>> CURRENT_ROLE in that place, even though in PostgreSQL they are equivalent.
>> Here is a trivial patch to add that.
> 
> Hmm, since this adds to RoleSpec, this change makes every place that
> uses that production also take CURRENT_ROLE, so we'd need to document in
> all those places.  For example, alter_role.sgml, create_schema.sgml,
> etc.

Good point.  Here is an updated patch that updates all the documentation 
places where CURRENT_USER is mentioned.

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

  6. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-08-26T11:00:57Z

    On 2020-06-29 14:47, Peter Eisentraut wrote:
> On 2020-06-24 23:08, Alvaro Herrera wrote:
>> On 2020-Jun-24, Peter Eisentraut wrote:
>>
>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>> CURRENT_ROLE in that place, even though in PostgreSQL they are equivalent.
>>> Here is a trivial patch to add that.
>>
>> Hmm, since this adds to RoleSpec, this change makes every place that
>> uses that production also take CURRENT_ROLE, so we'd need to document in
>> all those places.  For example, alter_role.sgml, create_schema.sgml,
>> etc.
> 
> Good point.  Here is an updated patch that updates all the documentation
> places where CURRENT_USER is mentioned.

Here is another patch that also makes comprehensive updates to the 
rolenames tests under src/test/modules/unsafe_tests/.

I think this should now cover all the required ancillary changes.

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

  7. Re: Allow CURRENT_ROLE in GRANTED BY

    Asif Rehman <asifr.rehman@gmail.com> — 2020-09-07T10:02:49Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

The patch applies cleanly and looks fine to me. However wouldn't it be better to just map the CURRENT_ROLE to CURRENT_USER in backend grammar?

The new status of this patch is: Waiting on Author

  8. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-09-10T16:21:54Z

    On 2020-09-07 12:02, Asif Rehman wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
> 
> The patch applies cleanly and looks fine to me. However wouldn't it be better to just map the CURRENT_ROLE to CURRENT_USER in backend grammar?

Existing code treats them differently.  I think, given that the code is 
already written, it is good to preserve what the user wrote.

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

  9. Re: Allow CURRENT_ROLE in GRANTED BY

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-11T20:05:13Z

    On 2020-Aug-26, Peter Eisentraut wrote:

> Here is another patch that also makes comprehensive updates to the rolenames
> tests under src/test/modules/unsafe_tests/.

Looks good to me.  You need to DROP ROLE "current_role" at the bottom of
rolenames.sql, though (as well as DROP OWNED BY, I suppose.)

> I think this should now cover all the required ancillary changes.

\o/

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

  10. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-09-17T10:02:35Z

    On 2020-09-11 22:05, Alvaro Herrera wrote:
> On 2020-Aug-26, Peter Eisentraut wrote:
> 
>> Here is another patch that also makes comprehensive updates to the rolenames
>> tests under src/test/modules/unsafe_tests/.
> 
> Looks good to me.  You need to DROP ROLE "current_role" at the bottom of
> rolenames.sql, though (as well as DROP OWNED BY, I suppose.)
> 
>> I think this should now cover all the required ancillary changes.
> 
> \o/
> 

committed


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

  11. Re: Allow CURRENT_ROLE in GRANTED BY

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-17T14:12:04Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> committed

A couple of buildfarm animals are reporting instability in the
modified rolenames test, eg

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2020-09-17%2010%3A27%3A36
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2020-09-17%2011%3A17%3A08
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&dt=2020-09-17%2011%3A47%3A07

			regards, tom lane

  12. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-12-10T18:39:49Z

    On 2020-06-24 20:21, Peter Eisentraut wrote:
> On 2020-06-24 10:12, Vik Fearing wrote:
>> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>>> equivalent.  Here is a trivial patch to add that.
>>
>>
>> The only thing that isn't dead-obvious about this patch is the commit
>> message says "[PATCH 1/2]".  What is in the other part?
> 
> Hehe.  The second patch is some in-progress work to add the GRANTED BY
> clause to the regular GRANT command.  More on that perhaps at a later date.

Here is the highly anticipated and quite underwhelming second part of 
this patch set.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

  13. Re: Allow CURRENT_ROLE in GRANTED BY

    Simon Riggs <simon@2ndquadrant.com> — 2020-12-30T12:43:39Z

    On Thu, 10 Dec 2020 at 18:40, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-06-24 20:21, Peter Eisentraut wrote:
> > On 2020-06-24 10:12, Vik Fearing wrote:
> >> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
> >>> I was checking some loose ends in SQL conformance, when I noticed: We
> >>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> >>> CURRENT_ROLE in that place, even though in PostgreSQL they are
> >>> equivalent.  Here is a trivial patch to add that.
> >>
> >>
> >> The only thing that isn't dead-obvious about this patch is the commit
> >> message says "[PATCH 1/2]".  What is in the other part?
> >
> > Hehe.  The second patch is some in-progress work to add the GRANTED BY
> > clause to the regular GRANT command.  More on that perhaps at a later date.
>
> Here is the highly anticipated and quite underwhelming second part of
> this patch set.

Looks great, but no test to confirm it works. I would suggest adding a
test and committing directly since I don't see any cause for further
discussion.

-- 
Simon Riggs                http://www.EnterpriseDB.com/

  14. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-01-30T08:51:03Z

    On 2020-12-30 13:43, Simon Riggs wrote:
> On Thu, 10 Dec 2020 at 18:40, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>>
>> On 2020-06-24 20:21, Peter Eisentraut wrote:
>>> On 2020-06-24 10:12, Vik Fearing wrote:
>>>> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>>>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>>>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>>>>> equivalent.  Here is a trivial patch to add that.
>>>>
>>>>
>>>> The only thing that isn't dead-obvious about this patch is the commit
>>>> message says "[PATCH 1/2]".  What is in the other part?
>>>
>>> Hehe.  The second patch is some in-progress work to add the GRANTED BY
>>> clause to the regular GRANT command.  More on that perhaps at a later date.
>>
>> Here is the highly anticipated and quite underwhelming second part of
>> this patch set.
> 
> Looks great, but no test to confirm it works. I would suggest adding a
> test and committing directly since I don't see any cause for further
> discussion.

Committed with some tests.  Thanks.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

  15. Re: Allow CURRENT_ROLE in GRANTED BY

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-16T14:04:11Z

    > On 30 Jan 2021, at 09:51, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> 
> On 2020-12-30 13:43, Simon Riggs wrote:
>> On Thu, 10 Dec 2020 at 18:40, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> 
>>> On 2020-06-24 20:21, Peter Eisentraut wrote:
>>>> On 2020-06-24 10:12, Vik Fearing wrote:
>>>>> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>>>>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>>>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>>>>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>>>>>> equivalent.  Here is a trivial patch to add that.
>>>>> 
>>>>> 
>>>>> The only thing that isn't dead-obvious about this patch is the commit
>>>>> message says "[PATCH 1/2]".  What is in the other part?
>>>> 
>>>> Hehe.  The second patch is some in-progress work to add the GRANTED BY
>>>> clause to the regular GRANT command.  More on that perhaps at a later date.
>>> 
>>> Here is the highly anticipated and quite underwhelming second part of
>>> this patch set.
>> Looks great, but no test to confirm it works. I would suggest adding a
>> test and committing directly since I don't see any cause for further
>> discussion.
> 
> Committed with some tests.  Thanks.

While looking at the proposed privileges.sql test patch from Mark Dilger [0] I
realized that the commit above seems to have missed the RevokeRoleStmt syntax.
As per the SQL Spec it should be supported there as well AFAICT.  Was this
intentional or should the attached small diff be applied to fix it?

--
Daniel Gustafsson		https://vmware.com/

[0] 333B0203-D19B-4335-AE64-90EB0FAF46F0@enterprisedb.com

  16. Re: Allow CURRENT_ROLE in GRANTED BY

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-16T14:27:25Z

    > On 16 Nov 2021, at 15:04, Daniel Gustafsson <daniel@yesql.se> wrote:

> ..or should the attached small diff be applied to fix it?

Actually it shouldn't, I realized when hitting Send that it was the wrong
version.  The attached is the proposed diff.

--
Daniel Gustafsson		https://vmware.com/

  17. Re: Allow CURRENT_ROLE in GRANTED BY

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-18T13:41:07Z

    On 16.11.21 15:27, Daniel Gustafsson wrote:
>> On 16 Nov 2021, at 15:04, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> ..or should the attached small diff be applied to fix it?
> 
> Actually it shouldn't, I realized when hitting Send that it was the wrong
> version.  The attached is the proposed diff.

This appears to have been an oversight.

  18. Re: Allow CURRENT_ROLE in GRANTED BY

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-18T13:42:46Z

    
> On 18 Nov 2021, at 14:41, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> On 16.11.21 15:27, Daniel Gustafsson wrote:
>>>> On 16 Nov 2021, at 15:04, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> ..or should the attached small diff be applied to fix it?
>> Actually it shouldn't, I realized when hitting Send that it was the wrong
>> version.  The attached is the proposed diff.
> 
> This appears to have been an oversight.

Thanks for confirming, I’ll take another pass over the proposed diff in a bit.

  19. Re: Allow CURRENT_ROLE in GRANTED BY

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-26T13:17:05Z

    > On 18 Nov 2021, at 14:42, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 18 Nov 2021, at 14:41, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>> 
>> On 16.11.21 15:27, Daniel Gustafsson wrote:
>>>>> On 16 Nov 2021, at 15:04, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>> ..or should the attached small diff be applied to fix it?
>>> Actually it shouldn't, I realized when hitting Send that it was the wrong
>>> version.  The attached is the proposed diff.
>> 
>> This appears to have been an oversight.
> 
> Thanks for confirming, I’ll take another pass over the proposed diff in a bit.

Polished a little and pushed to master with a backpatch to 14 where it was
introduced.

--
Daniel Gustafsson		https://vmware.com/