createuser --memeber and PG 16
In writing the PG 16 release notes, I came upon an oddity in our new
createuser syntax, specifically --role and --member. It turns out that
--role matches CREATE ROLE ... ROLE IN (and has prior to PG 16) while
the new --member option matches CREATE ROLE ... ROLE. The PG 16 feature
discussion thread is here:
/messages/by-id/69a9851035cf0f0477bcc5d742b031a3@oss.nttdata.com
This seems like it will be forever confusing to people. I frankly don't
know why --role matching CREATE ROLE ... ROLE IN was not already
confusing in pre-PG 16. Any new ideas for improvement?
At a minium I would like to apply the attached doc patch to PG 16 to
improve awkward wording in CREATE ROLE and createuser.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Embrace your flaws. They make you human, rather than perfect,
which you will never be.
Attachments:
role.difftext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 4a84461b28..69ea9060bf 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -285,10 +285,11 @@ in sync when changing the above synopsis!
<term><literal>IN ROLE</literal> <replaceable class="parameter">role_name</replaceable></term>
<listitem>
<para>
- The <literal>IN ROLE</literal> clause lists one or more existing
- roles to which the new role will be immediately added as a new
- member. (Note that there is no option to add the new role as an
- administrator; use a separate <command>GRANT</command> command to do that.)
+ The <literal>IN ROLE</literal> clause causes the new role to
+ be automatically added as a member of the specified existing
+ roles. (Note that there is no option to add the new role as an
+ administrator; use a separate <command>GRANT</command> command
+ to do that.)
</para>
</listitem>
</varlistentry>
@@ -306,9 +307,9 @@ in sync when changing the above synopsis!
<term><literal>ROLE</literal> <replaceable class="parameter">role_name</replaceable></term>
<listitem>
<para>
- The <literal>ROLE</literal> clause lists one or more existing
- roles which are automatically added as members of the new role.
- (This in effect makes the new role a <quote>group</quote>.)
+ The <literal>ROLE</literal> clause causes one or more specified
+ existing roles to be automatically added as members of the new
+ role. (This in effect makes the new role a <quote>group</quote>.)
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index c5c74b86a2..58ed111642 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -85,11 +85,10 @@ PostgreSQL documentation
<term><option>--admin=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates a role that will be immediately added as a member of the new
+ Indicates an existing role that will be automatically added as a member of the new
role with admin option, giving it the right to grant membership in the
- new role to others. Multiple roles to add as members (with admin
- option) of the new role can be specified by writing multiple
- <option>-a</option> switches.
+ new role to others. Multiple existing roles can be specified by
+ writing multiple <option>-a</option> switches.
</para>
</listitem>
</varlistentry>
@@ -153,11 +152,10 @@ PostgreSQL documentation
<term><option>--role=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates a role to which this role will be added immediately as a new
- member. Multiple roles to which this role will be added as a member
- can be specified by writing multiple
- <option>-g</option> switches.
- </para>
+ Indicates the new role should be automatically added as a member
+ of the specified existing role. Multiple existing roles can be
+ specified by writing multiple <option>-g</option> switches.
+ </para>
</listitem>
</varlistentry>
@@ -227,9 +225,9 @@ PostgreSQL documentation
<term><option>--member=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates role that will be immediately added as a member of the new
- role. Multiple roles to add as members of the new role can be specified
- by writing multiple <option>-m</option> switches.
+ Indicates the specified existing role should be automatically
+ added as a member of the new role. Multiple existing roles can
+ be specified by writing multiple <option>-m</option> switches.
</para>
</listitem>
</varlistentry>
On 10 May 2023, at 19:33, Bruce Momjian <bruce@momjian.us> wrote:
I frankly don't
know why --role matching CREATE ROLE ... ROLE IN was not already
confusing in pre-PG 16. Any new ideas for improvement?
IIRC there were a number of ideas presented in that thread but backwards
compatibility with --role already "taken" made it complicated, so --role and
--member were the least bad options.
At a minium I would like to apply the attached doc patch to PG 16 to
improve awkward wording in CREATE ROLE and createuser.
No objection.
+ role. (This in effect makes the new role a <quote>group</quote>.)
While not introduced here, isn't the latter part interesting enough to warrant
not being inside parenthesis?
--
Daniel Gustafsson
On Thu, May 11, 2023 at 02:21:22PM +0200, Daniel Gustafsson wrote:
On 10 May 2023, at 19:33, Bruce Momjian <bruce@momjian.us> wrote:
I frankly don't
know why --role matching CREATE ROLE ... ROLE IN was not already
confusing in pre-PG 16. Any new ideas for improvement?IIRC there were a number of ideas presented in that thread but backwards
compatibility with --role already "taken" made it complicated, so --role and
--member were the least bad options.At a minimum I would like to apply the attached doc patch to PG 16 to
improve awkward wording in CREATE ROLE and createuser.No objection.
+ role. (This in effect makes the new role a <quote>group</quote>.)
While not introduced here, isn't the latter part interesting enough to warrant
not being inside parenthesis?
The concept of group itself is deprecated, which I think is why the
parenthesis are used.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Embrace your flaws. They make you human, rather than perfect,
which you will never be.
On Wed, May 10, 2023 at 1:33 PM Bruce Momjian <bruce@momjian.us> wrote:
This seems like it will be forever confusing to people. I frankly don't
know why --role matching CREATE ROLE ... ROLE IN was not already
confusing in pre-PG 16. Any new ideas for improvement?
Yeah, it's a bad situation. I think --role is basically misnamed.
Something like --add-to-group would have been clearer, but that also
has the problem of being inconsistent with the SQL command. The whole
ROLE vs. IN ROLE thing is inherently quite confusing, I think. It's
very easy to get confused about which direction the membership arrows
are pointing.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 11.05.23 16:07, Robert Haas wrote:
On Wed, May 10, 2023 at 1:33 PM Bruce Momjian <bruce@momjian.us> wrote:
This seems like it will be forever confusing to people. I frankly don't
know why --role matching CREATE ROLE ... ROLE IN was not already
confusing in pre-PG 16. Any new ideas for improvement?Yeah, it's a bad situation. I think --role is basically misnamed.
Something like --add-to-group would have been clearer, but that also
has the problem of being inconsistent with the SQL command. The whole
ROLE vs. IN ROLE thing is inherently quite confusing, I think. It's
very easy to get confused about which direction the membership arrows
are pointing.
It's hard to tell that for the --member option as well. For
createuser foo --member bar
it's not intuitive whether foo becomes a member of bar or bar becomes a
member of foo. Maybe something more verbose like --member-of would help?
On Fri, May 12, 2023 at 04:35:34PM +0200, Peter Eisentraut wrote:
it's not intuitive whether foo becomes a member of bar or bar becomes a
member of foo. Maybe something more verbose like --member-of would help?
Indeed, presented like that it could be confusing, and --member-of
sounds like it could be a good idea instead of --member.
--
Michael
On Thu, May 11, 2023 at 09:34:42AM -0400, Bruce Momjian wrote:
On Thu, May 11, 2023 at 02:21:22PM +0200, Daniel Gustafsson wrote:
IIRC there were a number of ideas presented in that thread but backwards
compatibility with --role already "taken" made it complicated, so --role and
--member were the least bad options.At a minimum I would like to apply the attached doc patch to PG 16 to
improve awkward wording in CREATE ROLE and createuser.No objection.
None from here as well.
+ role. (This in effect makes the new role a <quote>group</quote>.)
While not introduced here, isn't the latter part interesting enough to warrant
not being inside parenthesis?The concept of group itself is deprecated, which I think is why the
parenthesis are used.
Not sure on this one. The original docs come from 58d214e, and this
sentence was already in there.
--
Michael
On Mon, May 15, 2023 at 04:33:27PM +0900, Michael Paquier wrote:
On Thu, May 11, 2023 at 09:34:42AM -0400, Bruce Momjian wrote:
On Thu, May 11, 2023 at 02:21:22PM +0200, Daniel Gustafsson wrote:
IIRC there were a number of ideas presented in that thread but backwards
compatibility with --role already "taken" made it complicated, so --role and
--member were the least bad options.At a minimum I would like to apply the attached doc patch to PG 16 to
improve awkward wording in CREATE ROLE and createuser.No objection.
None from here as well.
+ role. (This in effect makes the new role a <quote>group</quote>.)
While not introduced here, isn't the latter part interesting enough to warrant
not being inside parenthesis?The concept of group itself is deprecated, which I think is why the
parenthesis are used.Not sure on this one. The original docs come from 58d214e, and this
sentence was already in there.
True. I have removed the parenthesis in this updated patch.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Embrace your flaws. They make you human, rather than perfect,
which you will never be.
Attachments:
role.difftext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 4a84461b28..7249fc7432 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -285,10 +285,11 @@ in sync when changing the above synopsis!
<term><literal>IN ROLE</literal> <replaceable class="parameter">role_name</replaceable></term>
<listitem>
<para>
- The <literal>IN ROLE</literal> clause lists one or more existing
- roles to which the new role will be immediately added as a new
- member. (Note that there is no option to add the new role as an
- administrator; use a separate <command>GRANT</command> command to do that.)
+ The <literal>IN ROLE</literal> clause causes the new role to
+ be automatically added as a member of the specified existing
+ roles. (Note that there is no option to add the new role as an
+ administrator; use a separate <command>GRANT</command> command
+ to do that.)
</para>
</listitem>
</varlistentry>
@@ -306,9 +307,9 @@ in sync when changing the above synopsis!
<term><literal>ROLE</literal> <replaceable class="parameter">role_name</replaceable></term>
<listitem>
<para>
- The <literal>ROLE</literal> clause lists one or more existing
- roles which are automatically added as members of the new role.
- (This in effect makes the new role a <quote>group</quote>.)
+ The <literal>ROLE</literal> clause causes one or more specified
+ existing roles to be automatically added as members of the new
+ role. This in effect makes the new role a <quote>group</quote>.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index c5c74b86a2..58ed111642 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -85,11 +85,10 @@ PostgreSQL documentation
<term><option>--admin=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates a role that will be immediately added as a member of the new
+ Indicates an existing role that will be automatically added as a member of the new
role with admin option, giving it the right to grant membership in the
- new role to others. Multiple roles to add as members (with admin
- option) of the new role can be specified by writing multiple
- <option>-a</option> switches.
+ new role to others. Multiple existing roles can be specified by
+ writing multiple <option>-a</option> switches.
</para>
</listitem>
</varlistentry>
@@ -153,11 +152,10 @@ PostgreSQL documentation
<term><option>--role=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates a role to which this role will be added immediately as a new
- member. Multiple roles to which this role will be added as a member
- can be specified by writing multiple
- <option>-g</option> switches.
- </para>
+ Indicates the new role should be automatically added as a member
+ of the specified existing role. Multiple existing roles can be
+ specified by writing multiple <option>-g</option> switches.
+ </para>
</listitem>
</varlistentry>
@@ -227,9 +225,9 @@ PostgreSQL documentation
<term><option>--member=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates role that will be immediately added as a member of the new
- role. Multiple roles to add as members of the new role can be specified
- by writing multiple <option>-m</option> switches.
+ Indicates the specified existing role should be automatically
+ added as a member of the new role. Multiple existing roles can
+ be specified by writing multiple <option>-m</option> switches.
</para>
</listitem>
</varlistentry>
On Mon, May 15, 2023 at 04:27:04PM +0900, Michael Paquier wrote:
On Fri, May 12, 2023 at 04:35:34PM +0200, Peter Eisentraut wrote:
it's not intuitive whether foo becomes a member of bar or bar becomes a
member of foo. Maybe something more verbose like --member-of would help?Indeed, presented like that it could be confusing, and --member-of
sounds like it could be a good idea instead of --member.
--member specifieѕ an existing role that will be given membership to the
new role (i.e., GRANT newrole TO existingrole). IMO --member-of sounds
like the new role will be given membership to the specified existing role
(i.e., GRANT existingrole TO newrole). IOW a command like
createuser newrole --member-of existingrole
would make existingrole a "member of" newrole according to \du. Perhaps
--role should be --member-of because it makes the new role a member of the
existing role.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, May 10, 2023 at 01:33:26PM -0400, Bruce Momjian wrote:
In writing the PG 16 release notes, I came upon an oddity in our new
createuser syntax, specifically --role and --member. It turns out that
--role matches CREATE ROLE ... ROLE IN (and has prior to PG 16) while
the new --member option matches CREATE ROLE ... ROLE. The PG 16 feature
discussion thread is here:/messages/by-id/69a9851035cf0f0477bcc5d742b031a3@oss.nttdata.com
This seems like it will be forever confusing to people. I frankly don't
know why --role matching CREATE ROLE ... ROLE IN was not already
confusing in pre-PG 16. Any new ideas for improvement?At a minium I would like to apply the attached doc patch to PG 16 to
improve awkward wording in CREATE ROLE and createuser.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
On Thu, May 18, 2023 at 10:22:32PM -0400, Bruce Momjian wrote:
Patch applied.
Thanks, Bruce.
--
Michael
On 15.05.23 22:11, Nathan Bossart wrote:
On Mon, May 15, 2023 at 04:27:04PM +0900, Michael Paquier wrote:
On Fri, May 12, 2023 at 04:35:34PM +0200, Peter Eisentraut wrote:
it's not intuitive whether foo becomes a member of bar or bar becomes a
member of foo. Maybe something more verbose like --member-of would help?Indeed, presented like that it could be confusing, and --member-of
sounds like it could be a good idea instead of --member.--member specifieѕ an existing role that will be given membership to the
new role (i.e., GRANT newrole TO existingrole). IMO --member-of sounds
like the new role will be given membership to the specified existing role
(i.e., GRANT existingrole TO newrole). IOW a command likecreateuser newrole --member-of existingrole
would make existingrole a "member of" newrole according to \du. Perhaps
--role should be --member-of because it makes the new role a member of the
existing role.
Yeah, that's exactly my confusion.
Maybe
createuser --with-members
and
createuser --member-of
would be clearer.
On Sun, May 21, 2023 at 08:00:15AM +0200, Peter Eisentraut wrote:
Maybe
createuser --with-members
and
createuser --member-of
would be clearer.
Those seem like reasonable choices to me. I suspect we'll want to keep
--role around for backward compatibility.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sun, May 21, 2023 at 07:44:49AM -0700, Nathan Bossart wrote:
On Sun, May 21, 2023 at 08:00:15AM +0200, Peter Eisentraut wrote:
Maybe
createuser --with-members
and
createuser --member-of
would be clearer.
Those seem like reasonable choices to me. I suspect we'll want to keep
--role around for backward compatibility.
I've attached a draft patch for this. I also changed --admin to
--with-admin.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
rename_createuser_options.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 58ed111642..062509deb8 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -82,7 +82,7 @@ PostgreSQL documentation
<varlistentry>
<term><option>-a <replaceable class="parameter">role</replaceable></option></term>
- <term><option>--admin=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--with-admin=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
Indicates an existing role that will be automatically added as a member of the new
@@ -149,7 +149,7 @@ PostgreSQL documentation
<varlistentry>
<term><option>-g <replaceable class="parameter">role</replaceable></option></term>
- <term><option>--role=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--member-of=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
Indicates the new role should be automatically added as a member
@@ -222,7 +222,7 @@ PostgreSQL documentation
<varlistentry>
<term><option>-m <replaceable class="parameter">role</replaceable></option></term>
- <term><option>--member=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--with-member=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
Indicates the specified existing role should be automatically
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 0c7f454be5..77cb0bb8e1 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -28,7 +28,7 @@ int
main(int argc, char *argv[])
{
static struct option long_options[] = {
- {"admin", required_argument, NULL, 'a'},
+ {"with-admin", required_argument, NULL, 'a'},
{"connection-limit", required_argument, NULL, 'c'},
{"createdb", no_argument, NULL, 'd'},
{"no-createdb", no_argument, NULL, 'D'},
@@ -40,7 +40,7 @@ main(int argc, char *argv[])
{"no-inherit", no_argument, NULL, 'I'},
{"login", no_argument, NULL, 'l'},
{"no-login", no_argument, NULL, 'L'},
- {"member", required_argument, NULL, 'm'},
+ {"with-member", required_argument, NULL, 'm'},
{"port", required_argument, NULL, 'p'},
{"pwprompt", no_argument, NULL, 'P'},
{"createrole", no_argument, NULL, 'r'},
@@ -56,6 +56,7 @@ main(int argc, char *argv[])
{"interactive", no_argument, NULL, 3},
{"bypassrls", no_argument, NULL, 4},
{"no-bypassrls", no_argument, NULL, 5},
+ {"member-of", required_argument, NULL, 6},
{NULL, 0, NULL, 0}
};
@@ -123,6 +124,7 @@ main(int argc, char *argv[])
case 'E':
/* no-op, accepted for backward compatibility */
break;
+ case 6:
case 'g':
simple_string_list_append(&roles, optarg);
break;
@@ -414,19 +416,19 @@ help(const char *progname)
printf(_("Usage:\n"));
printf(_(" %s [OPTION]... [ROLENAME]\n"), progname);
printf(_("\nOptions:\n"));
- printf(_(" -a, --admin=ROLE this role will be a member of new role with admin\n"
+ printf(_(" -a, --with-admin=ROLE this role will be a member of new role with admin\n"
" option\n"));
printf(_(" -c, --connection-limit=N connection limit for role (default: no limit)\n"));
printf(_(" -d, --createdb role can create new databases\n"));
printf(_(" -D, --no-createdb role cannot create databases (default)\n"));
printf(_(" -e, --echo show the commands being sent to the server\n"));
- printf(_(" -g, --role=ROLE new role will be a member of this role\n"));
+ printf(_(" -g, --member-of=ROLE new role will be a member of this role\n"));
printf(_(" -i, --inherit role inherits privileges of roles it is a\n"
" member of (default)\n"));
printf(_(" -I, --no-inherit role does not inherit privileges\n"));
printf(_(" -l, --login role can login (default)\n"));
printf(_(" -L, --no-login role cannot login\n"));
- printf(_(" -m, --member=ROLE this role will be a member of new role\n"));
+ printf(_(" -m, --with-member=ROLE this role will be a member of new role\n"));
printf(_(" -P, --pwprompt assign a password to new role\n"));
printf(_(" -r, --createrole role can create new roles\n"));
printf(_(" -R, --no-createrole role cannot create roles (default)\n"));
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index da99d0ccb9..7530a9f007 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -64,4 +64,21 @@ $node->issues_sql_like(
$node->command_fails([ 'createuser', 'regress_user1' ],
'fails if role already exists');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user9', '--with-admin', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user9 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ADMIN regress_user1;/,
+ '--with-admin');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user10', '--with-member', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user10 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user1;/,
+ '--with-member');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user11', '--role', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
+ '--role (for backward compatibility)');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
+ '--member-of');
+
done_testing();
On Sun, May 21, 2023 at 08:22:05AM -0700, Nathan Bossart wrote:
On Sun, May 21, 2023 at 07:44:49AM -0700, Nathan Bossart wrote:
On Sun, May 21, 2023 at 08:00:15AM +0200, Peter Eisentraut wrote:
Maybe
createuser --with-members
and
createuser --member-of
would be clearer.
Those seem like reasonable choices to me. I suspect we'll want to keep
--role around for backward compatibility.I've attached a draft patch for this. I also changed --admin to
--with-admin.
If we want to go forward with this, the big question is whether we want
to get this in before beta1. FYI, the release notes don't mention the
option names.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Bruce Momjian <bruce@momjian.us> writes:
On Sun, May 21, 2023 at 08:22:05AM -0700, Nathan Bossart wrote:
I've attached a draft patch for this. I also changed --admin to
--with-admin.
If we want to go forward with this, the big question is whether we want
to get this in before beta1. FYI, the release notes don't mention the
option names.
+1 for doing it before beta1.
A few comments on the patch:
Indicates an existing role that will be automatically added as a member of the new
"Specifies" would be clearer than "indicates" (not your fault, but
let's avoid the passive construction while we are here). Likewise
nearby.
+ {"member-of", required_argument, NULL, 6},
Why didn't you just translate this as 'g' instead of inventing
a new switch case?
- printf(_(" -a, --admin=ROLE this role will be a member of new role with admin\n" + printf(_(" -a, --with-admin=ROLE this role will be a member of new role with admin\n"
I think clearer would be
+ printf(_(" -a, --with-admin=ROLE ROLE will be a member of new role with admin\n"
Likewise
+ printf(_(" -g, --member-of=ROLE new role will be a member of ROLE\n"));
(I assume that's what this should say, it's backwards ATM)
and
+ printf(_(" -m, --with-member=ROLE ROLE will be a member of new role\n"));
regards, tom lane
On Sun, May 21, 2023 at 11:45:24AM -0400, Tom Lane wrote:
A few comments on the patch:
Thanks for taking a look.
Indicates an existing role that will be automatically added as a member of the new
"Specifies" would be clearer than "indicates" (not your fault, but
let's avoid the passive construction while we are here). Likewise
nearby.
Fixed.
+ {"member-of", required_argument, NULL, 6},
Why didn't you just translate this as 'g' instead of inventing
a new switch case?
Fixed. *facepalm*
I think clearer would be
+ printf(_(" -a, --with-admin=ROLE ROLE will be a member of new role with admin\n"
Likewise
+ printf(_(" -g, --member-of=ROLE new role will be a member of ROLE\n"));
(I assume that's what this should say, it's backwards ATM)
and+ printf(_(" -m, --with-member=ROLE ROLE will be a member of new role\n"));
Fixed.
How do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
rename_createuser_options_v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index 58ed111642..448c28a056 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -82,10 +82,10 @@ PostgreSQL documentation
<varlistentry>
<term><option>-a <replaceable class="parameter">role</replaceable></option></term>
- <term><option>--admin=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--with-admin=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates an existing role that will be automatically added as a member of the new
+ Specifies an existing role that will be automatically added as a member of the new
role with admin option, giving it the right to grant membership in the
new role to others. Multiple existing roles can be specified by
writing multiple <option>-a</option> switches.
@@ -149,10 +149,10 @@ PostgreSQL documentation
<varlistentry>
<term><option>-g <replaceable class="parameter">role</replaceable></option></term>
- <term><option>--role=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--member-of=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates the new role should be automatically added as a member
+ Specifies the new role should be automatically added as a member
of the specified existing role. Multiple existing roles can be
specified by writing multiple <option>-g</option> switches.
</para>
@@ -222,10 +222,10 @@ PostgreSQL documentation
<varlistentry>
<term><option>-m <replaceable class="parameter">role</replaceable></option></term>
- <term><option>--member=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--with-member=<replaceable class="parameter">role</replaceable></option></term>
<listitem>
<para>
- Indicates the specified existing role should be automatically
+ Specifies the specified existing role should be automatically
added as a member of the new role. Multiple existing roles can
be specified by writing multiple <option>-m</option> switches.
</para>
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 0c7f454be5..2d5e2452f7 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -28,19 +28,21 @@ int
main(int argc, char *argv[])
{
static struct option long_options[] = {
- {"admin", required_argument, NULL, 'a'},
+ {"with-admin", required_argument, NULL, 'a'},
{"connection-limit", required_argument, NULL, 'c'},
{"createdb", no_argument, NULL, 'd'},
{"no-createdb", no_argument, NULL, 'D'},
{"echo", no_argument, NULL, 'e'},
{"encrypted", no_argument, NULL, 'E'},
- {"role", required_argument, NULL, 'g'},
+ {"role", required_argument, NULL, 'g'}, /* kept for backward
+ * compatibility */
+ {"member-of", required_argument, NULL, 'g'},
{"host", required_argument, NULL, 'h'},
{"inherit", no_argument, NULL, 'i'},
{"no-inherit", no_argument, NULL, 'I'},
{"login", no_argument, NULL, 'l'},
{"no-login", no_argument, NULL, 'L'},
- {"member", required_argument, NULL, 'm'},
+ {"with-member", required_argument, NULL, 'm'},
{"port", required_argument, NULL, 'p'},
{"pwprompt", no_argument, NULL, 'P'},
{"createrole", no_argument, NULL, 'r'},
@@ -414,19 +416,19 @@ help(const char *progname)
printf(_("Usage:\n"));
printf(_(" %s [OPTION]... [ROLENAME]\n"), progname);
printf(_("\nOptions:\n"));
- printf(_(" -a, --admin=ROLE this role will be a member of new role with admin\n"
+ printf(_(" -a, --with-admin=ROLE ROLE will be a member of new role with admin\n"
" option\n"));
printf(_(" -c, --connection-limit=N connection limit for role (default: no limit)\n"));
printf(_(" -d, --createdb role can create new databases\n"));
printf(_(" -D, --no-createdb role cannot create databases (default)\n"));
printf(_(" -e, --echo show the commands being sent to the server\n"));
- printf(_(" -g, --role=ROLE new role will be a member of this role\n"));
+ printf(_(" -g, --member-of=ROLE new role will be a member of ROLE\n"));
printf(_(" -i, --inherit role inherits privileges of roles it is a\n"
" member of (default)\n"));
printf(_(" -I, --no-inherit role does not inherit privileges\n"));
printf(_(" -l, --login role can login (default)\n"));
printf(_(" -L, --no-login role cannot login\n"));
- printf(_(" -m, --member=ROLE this role will be a member of new role\n"));
+ printf(_(" -m, --with-member=ROLE ROLE will be a member of new role\n"));
printf(_(" -P, --pwprompt assign a password to new role\n"));
printf(_(" -r, --createrole role can create new roles\n"));
printf(_(" -R, --no-createrole role cannot create roles (default)\n"));
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index da99d0ccb9..7530a9f007 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -64,4 +64,21 @@ $node->issues_sql_like(
$node->command_fails([ 'createuser', 'regress_user1' ],
'fails if role already exists');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user9', '--with-admin', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user9 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ADMIN regress_user1;/,
+ '--with-admin');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user10', '--with-member', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user10 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS ROLE regress_user1;/,
+ '--with-member');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user11', '--role', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
+ '--role (for backward compatibility)');
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user12', '--member-of', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
+ '--member-of');
+
done_testing();
Nathan Bossart <nathandbossart@gmail.com> writes:
Fixed.
v2 looks good to me, except the documentation wording for --with-role
is needlessly inconsistent with --with-admin. The --with-admin
wording looks better, so I suggest
- Indicates the specified existing role should be automatically
+ Specifies an existing role that will be automatically
added as a member of the new role. Multiple existing roles canHow do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?
I'm okay with leaving it undocumented, but I won't fight about it
if somebody wants to argue for the other.
regards, tom lane
On Sun, May 21, 2023 at 01:20:01PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
Fixed.
v2 looks good to me, except the documentation wording for --with-role
is needlessly inconsistent with --with-admin. The --with-admin
wording looks better, so I suggest- Indicates the specified existing role should be automatically + Specifies an existing role that will be automatically added as a member of the new role. Multiple existing roles can
Will do.
How do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?I'm okay with leaving it undocumented, but I won't fight about it
if somebody wants to argue for the other.
Alright. Barring any additional feedback, I'll commit this tonight.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Sun, May 21, 2023 at 12:16:58PM -0700, Nathan Bossart wrote:
On Sun, May 21, 2023 at 01:20:01PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
How do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?I'm okay with leaving it undocumented, but I won't fight about it
if somebody wants to argue for the other.Alright. Barring any additional feedback, I'll commit this tonight.
v2 passes the eye test, and I am not spotting any references to the
past option names. Thanks!
+$node->issues_sql_like(
+ [ 'createuser', 'regress_user11', '--role', 'regress_user1' ],
+ qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
+ '--role (for backward compatibility)');Not sure I would have kept this test, still that's cheap enough to
test.
--
Michael
On Mon, May 22, 2023 at 09:11:18AM +0900, Michael Paquier wrote:
On Sun, May 21, 2023 at 12:16:58PM -0700, Nathan Bossart wrote:
Alright. Barring any additional feedback, I'll commit this tonight.
v2 passes the eye test, and I am not spotting any references to the
past option names. Thanks!
Committed. Thanks for taking a look. I'll keep an eye on the buildfarm
for a few.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 21.05.23 19:07, Nathan Bossart wrote:
How do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?
We made a point in this release to document deprecated options
consistently. See commit 2f80c95740.
On Mon, May 22, 2023 at 08:42:28AM +0200, Peter Eisentraut wrote:
On 21.05.23 19:07, Nathan Bossart wrote:
How do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?We made a point in this release to document deprecated options consistently.
See commit 2f80c95740.
Alright. Does the attached patch suffice?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
doc_role.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index ba7ed1f853..5c34c62342 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -150,6 +150,7 @@ PostgreSQL documentation
<varlistentry>
<term><option>-g <replaceable class="parameter">role</replaceable></option></term>
<term><option>--member-of=<replaceable class="parameter">role</replaceable></option></term>
+ <term><option>--role=<replaceable class="parameter">role</replaceable></option> (deprecated)</term>
<listitem>
<para>
Specifies the new role should be automatically added as a member
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 2d5e2452f7..0709491185 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -34,8 +34,7 @@ main(int argc, char *argv[])
{"no-createdb", no_argument, NULL, 'D'},
{"echo", no_argument, NULL, 'e'},
{"encrypted", no_argument, NULL, 'E'},
- {"role", required_argument, NULL, 'g'}, /* kept for backward
- * compatibility */
+ {"role", required_argument, NULL, 'g'},
{"member-of", required_argument, NULL, 'g'},
{"host", required_argument, NULL, 'h'},
{"inherit", no_argument, NULL, 'i'},
@@ -423,6 +422,7 @@ help(const char *progname)
printf(_(" -D, --no-createdb role cannot create databases (default)\n"));
printf(_(" -e, --echo show the commands being sent to the server\n"));
printf(_(" -g, --member-of=ROLE new role will be a member of ROLE\n"));
+ printf(_(" --role=ROLE (same as --member-of, deprecated)\n"));
printf(_(" -i, --inherit role inherits privileges of roles it is a\n"
" member of (default)\n"));
printf(_(" -I, --no-inherit role does not inherit privileges\n"));
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 40452fcae3..9ca282181d 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -71,7 +71,7 @@ $node->issues_sql_like(
$node->issues_sql_like(
[ 'createuser', '--role', 'regress_user1', 'regress_user11' ],
qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
- '--role (for backward compatibility)');
+ '--role');
$node->issues_sql_like(
[ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
On Mon, May 22, 2023 at 05:11:14AM -0700, Nathan Bossart wrote:
On Mon, May 22, 2023 at 08:42:28AM +0200, Peter Eisentraut wrote:
On 21.05.23 19:07, Nathan Bossart wrote:
How do folks feel about keeping --role undocumented? Should we give it a
mention in the docs for --member-of?We made a point in this release to document deprecated options consistently.
See commit 2f80c95740.Alright. Does the attached patch suffice?
Seeing the precedent with --no-blobs and --blobs, yes, that should be
enough. You may want to wait until beta1 is stamped to apply
something, though, as the period between the stamp and the tag is used
to check the state of the tarballs to-be-released.
--
Michael
On Tue, May 23, 2023 at 07:50:36AM +0900, Michael Paquier wrote:
Seeing the precedent with --no-blobs and --blobs, yes, that should be
enough. You may want to wait until beta1 is stamped to apply
something, though, as the period between the stamp and the tag is used
to check the state of the tarballs to-be-released.
Thanks, committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com