Обсуждение: Re: Version 14/15 documentation Section "Alter Default Privileges"
On Fri, 2022-11-04 at 10:49 +0100, Laurenz Albe wrote: > On Thu, 2022-11-03 at 11:32 +0100, Laurenz Albe wrote: > > On Wed, 2022-11-02 at 19:29 +0000, David Burns wrote: > > > > > Some additional clarity in the versions 14/15 documentation would be helpful specifically > > > surrounding the "target_role" clause for the ALTER DEFAULT PRIVILEGES command. > > > To the uninitiated, the current description seems vague. Maybe something like the following would help: > > After some more thinking, I came up with the attached patch. I'm sending a reply to the hackers list, so that I can add the patch to the commitfest. Yours, Laurenz Albe
Hi, On Fri, Oct 27, 2023 at 09:03:04AM +0200, Laurenz Albe wrote: > On Fri, 2022-11-04 at 10:49 +0100, Laurenz Albe wrote: > > On Thu, 2022-11-03 at 11:32 +0100, Laurenz Albe wrote: > > > On Wed, 2022-11-02 at 19:29 +0000, David Burns wrote: > > > > > > > Some additional clarity in the versions 14/15 documentation > > > > would be helpful specifically surrounding the "target_role" > > > > clause for the ALTER DEFAULT PRIVILEGES command. To the > > > > uninitiated, the current description seems vague. Maybe > > > > something like the following would help: > > > > After some more thinking, I came up with the attached patch. > > I'm sending a reply to the hackers list, so that I can add the patch > to the commitfest. I think something like this is highly useful because I have also seen people very confused why default privileges are not applied. However, maybe it could be made even clearer if also the main description is amended, like "You can change default privileges only for objects that will be created by yourself or by roles that you are a member of (via target_role)." or something. Michael
On Fri, 2023-10-27 at 11:34 +0200, Michael Banck wrote: > On Fri, Oct 27, 2023 at 09:03:04AM +0200, Laurenz Albe wrote: > > On Fri, 2022-11-04 at 10:49 +0100, Laurenz Albe wrote: > > > On Thu, 2022-11-03 at 11:32 +0100, Laurenz Albe wrote: > > > > On Wed, 2022-11-02 at 19:29 +0000, David Burns wrote: > > > > > Some additional clarity in the versions 14/15 documentation > > > > > would be helpful specifically surrounding the "target_role" > > > > > clause for the ALTER DEFAULT PRIVILEGES command. To the > > > > > uninitiated, the current description seems vague. Maybe > > > > > something like the following would help: > > > > > > After some more thinking, I came up with the attached patch. > > > I think something like this is highly useful because I have also seen > people very confused why default privileges are not applied. > > However, maybe it could be made even clearer if also the main > description is amended, like > > "You can change default privileges only for objects that will be created > by yourself or by roles that you are a member of (via target_role)." > > or something. True. I have done that in the attached patch. In this patch, it is mentioned *twice* that ALTER DEFAULT PRIVILEGES only affects objects created by the current user. I thought that would not harm, but if it is too redundant, I can remove the second mention. Yours, Laurenz Albe
Вложения
Hi, On Fri, Oct 27, 2023 at 05:49:42PM +0200, Laurenz Albe wrote: > On Fri, 2023-10-27 at 11:34 +0200, Michael Banck wrote: > > On Fri, Oct 27, 2023 at 09:03:04AM +0200, Laurenz Albe wrote: > > > On Fri, 2022-11-04 at 10:49 +0100, Laurenz Albe wrote: > > > > On Thu, 2022-11-03 at 11:32 +0100, Laurenz Albe wrote: > > > > > On Wed, 2022-11-02 at 19:29 +0000, David Burns wrote: > > > > > > Some additional clarity in the versions 14/15 documentation > > > > > > would be helpful specifically surrounding the "target_role" > > > > > > clause for the ALTER DEFAULT PRIVILEGES command. To the > > > > > > uninitiated, the current description seems vague. Maybe > > > > > > something like the following would help: > > > > > > > > After some more thinking, I came up with the attached patch. > > > > > I think something like this is highly useful because I have also seen > > people very confused why default privileges are not applied. > > > > However, maybe it could be made even clearer if also the main > > description is amended, like > > > > "You can change default privileges only for objects that will be created > > by yourself or by roles that you are a member of (via target_role)." > > > > or something. > > True. I have done that in the attached patch. > In this patch, it is mentioned *twice* that ALTER DEFAULT PRIVILEGES > only affects objects created by the current user. I thought that > would not harm, but if it is too redundant, I can remove the second > mention. I think it is fine, and I have marked the patch as ready-for-committer. I think it should be applied to all branches, not just 14/15 as mentioned in the subject. Michael
On Sat, Oct 28, 2023 at 11:01:59AM +0200, Michael Banck wrote: > On Fri, Oct 27, 2023 at 05:49:42PM +0200, Laurenz Albe wrote: > > True. I have done that in the attached patch. > > In this patch, it is mentioned *twice* that ALTER DEFAULT PRIVILEGES > > only affects objects created by the current user. I thought that > > would not harm, but if it is too redundant, I can remove the second > > mention. > > I think it is fine, and I have marked the patch as ready-for-committer. > > I think it should be applied to all branches, not just 14/15 as > mentioned in the subject. I have developed the attached patch on top of the alter default patch I just applied. It is more radical, making FOR ROLE clearer, and also moving my new FOR ROLE text up to the first paragraph, and reordering the paragraphs to be clearer. I think this is too radical for backpatch to 11/12, but I think 16/master makes sense after the minor releases next week. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
On Fri, 2023-11-03 at 12:53 -0400, Bruce Momjian wrote: > I have developed the attached patch on top of the alter default patch I > just applied. It is more radical, making FOR ROLE clearer, and also > moving my new FOR ROLE text up to the first paragraph, and reordering > the paragraphs to be clearer. > > I think this is too radical for backpatch to 11/12, but I think > 16/master makes sense after the minor releases next week. I think it is a good idea to move part of the text to a new paragraph. > --- a/doc/src/sgml/ref/alter_default_privileges.sgml > +++ b/doc/src/sgml/ref/alter_default_privileges.sgml > @@ -90,23 +90,14 @@ REVOKE [ GRANT OPTION FOR ] > [...] > + As a non-superuser, you can change default privileges only for yourself > + and for roles that you are a member of. These privileges are not > + inherited, so member roles must use <command>SET ROLE</command> to > + access these privileges, or <command>ALTER DEFAULT PRIVILEGES</command> > + must be run for each member role. Privileges can be set globally > + (i.e., for all objects created in the current database), or just for > + objects created in specified schemas. That this paragraph is not clear enough about who gets the privileges and who creates the objects, and that is one of the difficulties in understanding ALTER DEFAULT PRIVILEGES. Perhaps: <para> <command>ALTER DEFAULT PRIVILEGES</command> allows you to set the privileges that will be applied to objects created in the future. (It does not affect privileges assigned to already-existing objects.) Privileges can be set globally (i.e., for all objects created in the current database), or just for objects created in specified schemas. </para> <para> As a non-superuser, you can change default privileges only on objects created by yourself or by roles that you are a member of. If you alter the default privileges for a role, only objects created by that role will be affected. It is not sufficient to be a member of that role; member roles must use <command>SET ROLE</command> to assume the identity of the role for which default privileges were altered. </para> <para> There is no way to change the default privileges for objects created by any role. You have run <command>ALTER DEFAULT PRIVILEGES</command> for all roles that can create objects whose default privileges should be modified. </para> > @@ -136,12 +140,9 @@ REVOKE [ GRANT OPTION FOR ] > <term><replaceable>target_role</replaceable></term> > <listitem> > <para> > - The name of an existing role of which the current role is a member. > - Default access privileges are not inherited, so member roles > - must use <command>SET ROLE</command> to access these privileges, > - or <command>ALTER DEFAULT PRIVILEGES</command> must be run for > - each member role. If <literal>FOR ROLE</literal> is omitted, > - the current role is assumed. > + If <literal>FOR ROLE</literal> is specified, this is the role that > + will be assigned the new default privileges, or the current role > + if not specified. This is downright wrong; the "target_role" will *not* be assigned any privileges. Perhaps: <para> Default privileges are changed only for objects created by <replaceable>target_role</replaceable>. If <literal>FOR ROLE</literal> is omitted, the current role is assumed. </para> Yours, Laurenz Albe
On Sat, Nov 4, 2023 at 07:05:28AM +0100, Laurenz Albe wrote: > On Fri, 2023-11-03 at 12:53 -0400, Bruce Momjian wrote: > > I have developed the attached patch on top of the alter default patch I > > just applied. It is more radical, making FOR ROLE clearer, and also > > moving my new FOR ROLE text up to the first paragraph, and reordering > > the paragraphs to be clearer. > > > > I think this is too radical for backpatch to 11/12, but I think > > 16/master makes sense after the minor releases next week. > > I think it is a good idea to move part of the text to a new paragraph. Yeah, kind of radical but I think it needed to be done. > > --- a/doc/src/sgml/ref/alter_default_privileges.sgml > > +++ b/doc/src/sgml/ref/alter_default_privileges.sgml > > @@ -90,23 +90,14 @@ REVOKE [ GRANT OPTION FOR ] > > [...] > > + As a non-superuser, you can change default privileges only for yourself > > + and for roles that you are a member of. These privileges are not > > + inherited, so member roles must use <command>SET ROLE</command> to > > + access these privileges, or <command>ALTER DEFAULT PRIVILEGES</command> > > + must be run for each member role. Privileges can be set globally > > + (i.e., for all objects created in the current database), or just for > > + objects created in specified schemas. > > That this paragraph is not clear enough about who gets the privileges and > who creates the objects, and that is one of the difficulties in understanding > ALTER DEFAULT PRIVILEGES. Yes, I like your new paragraphs better than I what I had. > This is downright wrong; the "target_role" will *not* be assigned any > privileges. > > Perhaps: > > <para> > Default privileges are changed only for objects created by > <replaceable>target_role</replaceable>. If <literal>FOR ROLE</literal> > is omitted, the current role is assumed. > </para> Yes, I see your point. Updated patch attached. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
On Sat, 2023-11-04 at 14:20 -0400, Bruce Momjian wrote: > Yes, I see your point. Updated patch attached. Almost perfect, except: + Change default privileges for objects created by + <replaceable>target_role</replaceable>; if omitted, the current + role is modified. It is not the role that is modified. Perhaps: [...]; if omitted, the current role is used. Yours, Laurenz Albe
On Sat, Nov 4, 2023 at 10:12:42PM +0100, Laurenz Albe wrote: > On Sat, 2023-11-04 at 14:20 -0400, Bruce Momjian wrote: > > Yes, I see your point. Updated patch attached. > > Almost perfect, except: > > + Change default privileges for objects created by > + <replaceable>target_role</replaceable>; if omitted, the current > + role is modified. > > It is not the role that is modified. Perhaps: > > [...]; if omitted, the current role is used. Sure, attached. Here is the issue I have though, we are really not changing default privileges for objects created in the future, we are changing the role _now_ so future objects will have different default privileges, right? I think wording like the above is kind of odd. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
Hi, On Sat, Nov 04, 2023 at 09:14:01PM -0400, Bruce Momjian wrote: > + There is no way to change the default privileges for objects created by > + any role. You have run <command>ALTER DEFAULT PRIVILEGES</command> for all > + roles that can create objects whose default privileges should be modified. That second sentence is broken, it should be "You have to run [...]" I think. Michael
On Sat, 2023-11-04 at 21:14 -0400, Bruce Momjian wrote: > > It is not the role that is modified. Perhaps: > > > > [...]; if omitted, the current role is used. > > Sure, attached. Here is the issue I have though, we are really not > changing default privileges for objects created in the future, we are > changing the role _now_ so future objects will have different default > privileges, right? I think wording like the above is kind of odd. I see what you mean. The alternative is to be precise, at the risk of repeating ourselves: if omitted, default privileges will be changed for objects created by the current role. Yours, Laurenz Albe
On Mon, Nov 6, 2023 at 09:32:27AM +0100, Michael Banck wrote: > Hi, > > On Sat, Nov 04, 2023 at 09:14:01PM -0400, Bruce Momjian wrote: > > + There is no way to change the default privileges for objects created by > > + any role. You have run <command>ALTER DEFAULT PRIVILEGES</command> for all > > + roles that can create objects whose default privileges should be modified. > > That second sentence is broken, it should be "You have to run [...]" I > think. Agreed, fixed, thanks. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Mon, Nov 6, 2023 at 09:44:14AM +0100, Laurenz Albe wrote: > On Sat, 2023-11-04 at 21:14 -0400, Bruce Momjian wrote: > > > It is not the role that is modified. Perhaps: > > > > > > [...]; if omitted, the current role is used. > > > > Sure, attached. Here is the issue I have though, we are really not > > changing default privileges for objects created in the future, we are > > changing the role _now_ so future objects will have different default > > privileges, right? I think wording like the above is kind of odd. > > I see what you mean. The alternative is to be precise, at the risk of > repeating ourselves: > > if omitted, default privileges will be changed for objects created by > the current role. Okay, I think I have good wording for this. I didn't like the wording of other roles, so I restructured that in the attached patch too. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
On Mon, 2023-11-06 at 10:55 -0500, Bruce Momjian wrote: > Okay, I think I have good wording for this. I didn't like the wording > of other roles, so I restructured that in the attached patch too. > <para> > ! Default privileges apply only to the active role; the default > ! privileges of member roles have no affect on object permissions. > ! <command>SET ROLE</command> can be used to change the active user and > ! apply their default privileges. > ! </para> You don't mean member roles, but roles that the active role is a member of, right? How do you like my version, as attached? Yours, Laurenz Albe
Вложения
On Mon, Nov 6, 2023 at 09:53:50PM +0100, Laurenz Albe wrote: > On Mon, 2023-11-06 at 10:55 -0500, Bruce Momjian wrote: > > Okay, I think I have good wording for this. I didn't like the wording > > of other roles, so I restructured that in the attached patch too. > > > <para> > > ! Default privileges apply only to the active role; the default > > ! privileges of member roles have no affect on object permissions. > > ! <command>SET ROLE</command> can be used to change the active user and > > ! apply their default privileges. > > ! </para> > > You don't mean member roles, but roles that the active role is a member of, > right? Yes, sorry fixed in the attached patch. > + <para> > + As a non-superuser, you can change default privileges only on objects created > + by yourself or by roles that you are a member of. However, you don't inherit > + altered default privileges from roles you are a member of; objects you create > + will receive the default privileges for your current role. > + </para> I went with different wording since I found the above confusing. You didn't seem to like my SET ROLE suggestion so I removed it. > + > + <para> > + There is no way to change the default privileges for objects created by > + arbitrary roles. You have run <command>ALTER DEFAULT PRIVILEGES</command> I find the above sentence odd. What is its purpose? > + for any role that can create objects whose default privileges should be > + modified. > + </para> > + > + <para> > + Currently, > + only the privileges for schemas, tables (including views and foreign > + tables), sequences, functions, and types (including domains) can be > + altered. For this command, functions include aggregates and procedures. > + The words <literal>FUNCTIONS</literal> and <literal>ROUTINES</literal> are > + equivalent in this command. (<literal>ROUTINES</literal> is preferred > + going forward as the standard term for functions and procedures taken > + together. In earlier PostgreSQL releases, only the > + word <literal>FUNCTIONS</literal> was allowed. It is not possible to set > + default privileges for functions and procedures separately.) > + </para> > + > <para> > Default privileges that are specified per-schema are added to whatever > the global default privileges are for the particular object type. > @@ -136,8 +149,9 @@ REVOKE [ GRANT OPTION FOR ] > <term><replaceable>target_role</replaceable></term> > <listitem> > <para> > - The name of an existing role of which the current role is a member. > - If <literal>FOR ROLE</literal> is omitted, the current role is assumed. > + Default privileges are changed for objects created by the > + <replaceable>target_role</replaceable>, or the current > + role if unspecified. I like a verb to be first, like "Change" rather than "default privileges". Patch attached. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
Hi, On Tue, Nov 07, 2023 at 05:30:20PM -0500, Bruce Momjian wrote: > On Mon, Nov 6, 2023 at 09:53:50PM +0100, Laurenz Albe wrote: > > + <para> > > + There is no way to change the default privileges for objects created by > > + arbitrary roles. You have run <command>ALTER DEFAULT PRIVILEGES</command> > > I find the above sentence odd. What is its purpose? I guess it is to address the main purpose of this patch/confusion with users: they believe setting DEFAULT PRIVILEGES will set grants accordingly for all objects created in the future, no matter who creates them. So hammering in that this is not the case seems fine from my side (modulo the "You have to run" typo). Michael
On Tue, 2023-11-07 at 17:30 -0500, Bruce Momjian wrote: > You didn't seem to like my SET ROLE suggestion so I removed it. I thought that the information that you can use SET ROLE to assume the identity of another role is correct, but leads a bit too far in the manual page of ALTER DEFAULT PRIVILEGES. > > + <para> > > + There is no way to change the default privileges for objects created by > > + arbitrary roles. You have run <command>ALTER DEFAULT PRIVILEGES</command> > > I find the above sentence odd. What is its purpose? I cannot count how many times I have seen the complaint "I have run ALTER DEFAULT PRIVILEGES, and now when some other user creates a table, the permissions are unchanged". People tend to think that if you omit FOR ROLE, the change applies to PUBLIC. Your improved documentation of "target_role" already covers that somewhat, so if you don't like the repetition, I'm alright with that. I just thought it might be worth stating it explicitly. I think your patch is fine and ready to go. Yours, Laurenz Albe >
On Wed, Nov 8, 2023 at 01:12:24PM +0100, Laurenz Albe wrote: > On Tue, 2023-11-07 at 17:30 -0500, Bruce Momjian wrote: > > You didn't seem to like my SET ROLE suggestion so I removed it. > > I thought that the information that you can use SET ROLE to assume > the identity of another role is correct, but leads a bit too far > in the manual page of ALTER DEFAULT PRIVILEGES. Agreed, it was a stretch. > > > + <para> > > > + There is no way to change the default privileges for objects created by > > > + arbitrary roles. You have run <command>ALTER DEFAULT PRIVILEGES</command> > > > > I find the above sentence odd. What is its purpose? > > I cannot count how many times I have seen the complaint "I have run ALTER DEFAULT > PRIVILEGES, and now when some other user creates a table, the permissions are > unchanged". People tend to think that if you omit FOR ROLE, the change applies to > PUBLIC. I agree we have to be clear, and this is complex, which is why we are struggling. I feel we have to be clear about who is allowed to modify which default privileges, and what default privileges are active during object creation. I ended up basically saying you can modify the default privileges of roles you are member of, but they don't apply at creation time for your own role. I am open to better wording. > Your improved documentation of "target_role" already covers that somewhat, so if > you don't like the repetition, I'm alright with that. I just thought it might > be worth stating it explicitly. > > I think your patch is fine and ready to go. Thanks. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Nov 8, 2023 at 01:12:24PM +0100, Laurenz Albe wrote: > > I find the above sentence odd. What is its purpose? > > I cannot count how many times I have seen the complaint "I have run ALTER DEFAULT > PRIVILEGES, and now when some other user creates a table, the permissions are > unchanged". People tend to think that if you omit FOR ROLE, the change applies to > PUBLIC. > > Your improved documentation of "target_role" already covers that somewhat, so if > you don't like the repetition, I'm alright with that. I just thought it might > be worth stating it explicitly. > > I think your patch is fine and ready to go. Patch applied back to PG 16. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Mon, 2023-11-13 at 14:28 -0500, Bruce Momjian wrote: > Patch applied back to PG 16. Great thanks! I am hopeful that that will reduce people's confusion about this feature. Yours, Laurenz Albe
On Mon, Nov 13, 2023 at 08:33:33PM +0100, Laurenz Albe wrote: > On Mon, 2023-11-13 at 14:28 -0500, Bruce Momjian wrote: > > Patch applied back to PG 16. > > Great thanks! > > I am hopeful that that will reduce people's confusion about this feature. Agreed! -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.