Add --{no-,}bypassrls flags to createuser

At Wed, 13 Apr 2022 14:51:35 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in

Hi,

Add --{no-,}bypassrls flags to createuser.
The following is an example of execution.
--
$ createuser a --bypassrls
$ psql -c "\du a"
List of roles
Role name | Attributes | Member of
-----------+------------+-----------
a | Bypass RLS | {}

--

Do you think?

It is sensible to rig createuser command with full capability of
CREATE ROLE is reasonable.

Only --replication is added by commit 9b8aff8c19 (2010) since
8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
491c029dbc (2014) but it seems to have forgotten to add the
corresponding createuser options.

By a quick search, found a few other CREATE ROLE optinos that are not
supported by createuser.

VALID UNTIL
ROLE (IN ROLE is -g/--role)
ADMIN

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:

It is sensible to rig createuser command with full capability of
CREATE ROLE is reasonable.

Only --replication is added by commit 9b8aff8c19 (2010) since
8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
491c029dbc (2014) but it seems to have forgotten to add the
corresponding createuser options.

By a quick search, found a few other CREATE ROLE optinos that are not
supported by createuser.

My question is: is BYPASSRLS common enough to justify having a switch
to createuser? As the development cycle of 15 has just finished and
that we are in feature freeze, you may want to hold on new patches for
a bit. The next commit fest is planned for July.
--
Michael

At Wed, 13 Apr 2022 16:10:01 +0900, Michael Paquier <michael@paquier.xyz> wrote in

On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:

It is sensible to rig createuser command with full capability of
CREATE ROLE is reasonable.

Only --replication is added by commit 9b8aff8c19 (2010) since
8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
491c029dbc (2014) but it seems to have forgotten to add the
corresponding createuser options.

By a quick search, found a few other CREATE ROLE optinos that are not
supported by createuser.

My question is: is BYPASSRLS common enough to justify having a switch
to createuser? As the development cycle of 15 has just finished and
that we are in feature freeze, you may want to hold on new patches for
a bit. The next commit fest is planned for July.

I don't think there's a definitive criteria (other than feasibility)
for whether each CREATE ROLE option should have the correspondent
option in the createuser command. I don't see a clear reason why
createuser command should not have the option.

As far as schedules are concerned, I don't think this has anything to
do with 15.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Apr 13, 2022 at 4:35 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

I don't think there's a definitive criteria (other than feasibility)
for whether each CREATE ROLE option should have the correspondent
option in the createuser command. I don't see a clear reason why
createuser command should not have the option.

+1.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2022-04-13 17:35, Kyotaro Horiguchi wrote:

At Wed, 13 Apr 2022 16:10:01 +0900, Michael Paquier
<michael@paquier.xyz> wrote in

On Wed, Apr 13, 2022 at 03:46:25PM +0900, Kyotaro Horiguchi wrote:

It is sensible to rig createuser command with full capability of
CREATE ROLE is reasonable.

Only --replication is added by commit 9b8aff8c19 (2010) since
8ae0d476a9 (2005). BYPASSRLS and NOBYPASSRLS were introduced by
491c029dbc (2014) but it seems to have forgotten to add the
corresponding createuser options.

By a quick search, found a few other CREATE ROLE optinos that are not
supported by createuser.

My question is: is BYPASSRLS common enough to justify having a switch
to createuser? As the development cycle of 15 has just finished and
that we are in feature freeze, you may want to hold on new patches for
a bit. The next commit fest is planned for July.

I don't think there's a definitive criteria (other than feasibility)
for whether each CREATE ROLE option should have the correspondent
option in the createuser command. I don't see a clear reason why
createuser command should not have the option.

Thank you for the review!
I created a new patch containing 'VALID UNTIL', 'ADMIN', and 'ROLE'.

To add the ROLE clause, the originally existing --role option
(corresponding to the IN ROLE clause) is changed to the --in-role
option. Would this not be good from a backward compatibility standpoint?

As far as schedules are concerned, I don't think this has anything to
do with 15.

I have registered this patch for the July commit fest.

--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 14 Apr 2022, at 09:42, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

To add the ROLE clause, the originally existing --role option (corresponding to the IN ROLE clause) is changed to the --in-role option. Would this not be good from a backward compatibility standpoint?

-	printf(_("  -g, --role=ROLE           new role will be a member of this role\n"));
+	printf(_("  -g, --in-role=ROLE        new role will be a member of this role\n"));
+	printf(_("  -G, --role=ROLE           this role will be a member of new role\n"));

Won't this make existing scripts to behave differently after an upgrade? That
seems like something we should avoid at all costs.

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

On 2022-04-14 18:57, Daniel Gustafsson wrote:

On 14 Apr 2022, at 09:42, Shinya Kato <Shinya11.Kato@oss.nttdata.com>
wrote:

To add the ROLE clause, the originally existing --role option
(corresponding to the IN ROLE clause) is changed to the --in-role
option. Would this not be good from a backward compatibility
standpoint?

-	printf(_("  -g, --role=ROLE           new role will be a member of
this role\n"));
+	printf(_("  -g, --in-role=ROLE        new role will be a member of
this role\n"));
+	printf(_("  -G, --role=ROLE           this role will be a member of
new role\n"));

Won't this make existing scripts to behave differently after an
upgrade? That
seems like something we should avoid at all costs.

I understand. For backward compatibility, I left the ROLE clause option
as it is and changed the IN ROLE clause option to --membership option.

--
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Fri, 15 Apr 2022 14:55:48 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in

I understand. For backward compatibility, I left the ROLE clause
option as it is and changed the IN ROLE clause option to --membership
option.

Thanks!

-	printf(_("  -g, --role=ROLE           new role will be a member of this role\n"));
+	printf(_("  -g, --role=ROLE        new role will be a member of this role\n"));

This looks lik an unexpected change. We shoudl preserve it, but *I*
think that we can add a synonym of the old --role for
understandability/memorability. (By the way "-g" looks like coming
from "group", which looks somewhat strange..)

printf(_(" -b, --belongs-to=ROLE new role will be a member of this role\n"));

+ printf(_(" -m, --membership=ROLE this role will be a member of new role\n"));

membership sounds somewhat obscure, it seems *to me* members is clearer

printf(_(" -m, --member=ROLE new role will be a member of this role\n"));

I'd like to hear others' opinions.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Apr 15, 2022 at 2:33 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

printf(_(" -b, --belongs-to=ROLE new role will be a member of this role\n"));

+ printf(_(" -m, --membership=ROLE this role will be a member of new role\n"));

membership sounds somewhat obscure, it seems *to me* members is clearer

printf(_(" -m, --member=ROLE new role will be a member of this role\n"));

I'd like to hear others' opinions.

I think that we need to preserve consistency with the SQL syntax as
much as possible -- and neither MEMBER nor MEMBERSHIP nor BELONGS_TO
appear in that syntax. A lot of the terminology in this area seems
poorly chosen and confusing to me, but having two ways to refer to
something probably won't be an improvement even if the second name is
better-chosen than the first one.

--
Robert Haas
EDB: http://www.enterprisedb.com

Thanks!

At Mon, 18 Apr 2022 09:59:48 -0400, Robert Haas <robertmhaas@gmail.com> wrote in

On Fri, Apr 15, 2022 at 2:33 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

printf(_(" -b, --belongs-to=ROLE new role will be a member of this role\n"));

+ printf(_(" -m, --membership=ROLE this role will be a member of new role\n"));

membership sounds somewhat obscure, it seems *to me* members is clearer

printf(_(" -m, --member=ROLE new role will be a member of this role\n"));

I'd like to hear others' opinions.

I think that we need to preserve consistency with the SQL syntax as
much as possible -- and neither MEMBER nor MEMBERSHIP nor BELONGS_TO
appear in that syntax. A lot of the terminology in this area seems
poorly chosen and confusing to me, but having two ways to refer to
something probably won't be an improvement even if the second name is
better-chosen than the first one.

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go? Or we can give up adding -m for the reason of being hard to
name it..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go? Or we can give up adding -m for the reason of being hard to
name it..

Hmm, yeah, I hadn't quite realized what the problem was when I wrote
that. I honestly don't know what to do about that. Renaming the
existing option is not great, but having the syntax diverge between
SQL and CLI is not great either. Giving up is also not great. Not sure
what is best.

--
Robert Haas
EDB: http://www.enterprisedb.com

At Tue, 19 Apr 2022 12:13:51 -0400, Robert Haas <robertmhaas@gmail.com> wrote in

On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go? Or we can give up adding -m for the reason of being hard to
name it..

Hmm, yeah, I hadn't quite realized what the problem was when I wrote
that. I honestly don't know what to do about that. Renaming the
existing option is not great, but having the syntax diverge between
SQL and CLI is not great either. Giving up is also not great. Not sure
what is best.

Exactly.. So I'm stuckX(

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Apr 19, 2022 at 12:13:51PM -0400, Robert Haas wrote:

On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go? Or we can give up adding -m for the reason of being hard to
name it..

Hmm, yeah, I hadn't quite realized what the problem was when I wrote
that. I honestly don't know what to do about that. Renaming the
existing option is not great, but having the syntax diverge between
SQL and CLI is not great either. Giving up is also not great. Not sure
what is best.

Changing one existing option to mean something entirely different
should be avoided, as this could lead to silent breakages. As the
origin of the problem is that the option --role means "IN ROLE" in the
SQL grammar, we could keep around --role for compatibility while
marking it deprecated, and add two new options whose names would be
more consistent with each other. One choice could be --role-name and
--in-role-name, where --in-role-name maps to the older --role, just to
give an idea.
--
Michael

On Thu, Apr 21, 2022 at 12:30 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Apr 19, 2022 at 12:13:51PM -0400, Robert Haas wrote:

On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go? Or we can give up adding -m for the reason of being hard to
name it..

Hmm, yeah, I hadn't quite realized what the problem was when I wrote
that. I honestly don't know what to do about that. Renaming the
existing option is not great, but having the syntax diverge between
SQL and CLI is not great either. Giving up is also not great. Not sure
what is best.

Changing one existing option to mean something entirely different
should be avoided, as this could lead to silent breakages. As the
origin of the problem is that the option --role means "IN ROLE" in the
SQL grammar, we could keep around --role for compatibility while
marking it deprecated, and add two new options whose names would be
more consistent with each other. One choice could be --role-name and
--in-role-name, where --in-role-name maps to the older --role, just to
give an idea.

I don't think that having both --role and --role-name, doing different
things, is going to be clear at all.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Apr 21, 2022 at 12:51 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Apr 21, 2022 at 12:30 AM Michael Paquier <michael@paquier.xyz>
wrote:

On Tue, Apr 19, 2022 at 12:13:51PM -0400, Robert Haas wrote:

On Mon, Apr 18, 2022 at 9:50 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

Hmm.. So, "-r/--role" and "-m/--member(ship)" is the (least worse) way
to go? Or we can give up adding -m for the reason of being hard to
name it..

Hmm, yeah, I hadn't quite realized what the problem was when I wrote
that. I honestly don't know what to do about that. Renaming the
existing option is not great, but having the syntax diverge between
SQL and CLI is not great either. Giving up is also not great. Not sure
what is best.

Changing one existing option to mean something entirely different
should be avoided, as this could lead to silent breakages. As the
origin of the problem is that the option --role means "IN ROLE" in the
SQL grammar, we could keep around --role for compatibility while
marking it deprecated, and add two new options whose names would be
more consistent with each other. One choice could be --role-name and
--in-role-name, where --in-role-name maps to the older --role, just to
give an idea.

I don't think that having both --role and --role-name, doing different
things, is going to be clear at all.

-g/--role or maybe/additionally (--in-role)?
-m/--role-to or maybe/additionally (--to-role)?

I'm ok with -m/--member as well (like with --role only one role can be
specified per switch instance so member, not membership, the later meaning,
at least for me, the collective).

That -m doesn't match --role-to is no worse than -g not matching --role, a
short option seems worthwhile, and the -m (membership) mnemonic should be
simple to pick-up.

I don't see the addition of "-name" to the option name being beneficial.

Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking that
liberty for consistency here is very appealing and there isn't another SQL
clause that it would be confused with.

David J.

On Thu, Apr 21, 2022 at 01:21:57PM -0700, David G. Johnston wrote:

I'm ok with -m/--member as well (like with --role only one role can be
specified per switch instance so member, not membership, the later meaning,
at least for me, the collective).

That -m doesn't match --role-to is no worse than -g not matching --role, a
short option seems worthwhile, and the -m (membership) mnemonic should be
simple to pick-up.

I don't see the addition of "-name" to the option name being beneficial.

Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking that
liberty for consistency here is very appealing and there isn't another SQL
clause that it would be confused with.

+1 for "member". It might not be perfect, but IMO it's the clearest
option.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Thank you for the reviews!

On 2022-04-26 05:19, Nathan Bossart wrote:

-	printf(_("  -g, --role=ROLE           new role will be a member of 
this role\n"));
+	printf(_("  -g, --role=ROLE        new role will be a member of this 
role\n"));
This looks lik an unexpected change.

I fixed it.

I'm ok with -m/--member as well (like with --role only one role can be
specified per switch instance so member, not membership, the later
meaning,
at least for me, the collective).

That -m doesn't match --role-to is no worse than -g not matching
--role, a
short option seems worthwhile, and the -m (membership) mnemonic should
be
simple to pick-up.

I don't see the addition of "-name" to the option name being
beneficial.

Yes, the standard doesn't use the "TO" prefix for "ROLE" - but taking
that
liberty for consistency here is very appealing and there isn't another
SQL
clause that it would be confused with.

+1 for "member". It might not be perfect, but IMO it's the clearest
option.

Thanks! I changed the option "--membership" to "--member".

For now, I also think "-m / --member" is the best choice, although it is
ambiguous:(
I'd like to hear others' opinions.

regards

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Apr 28, 2022 at 03:06:30PM +0900, Shinya Kato wrote:

On 2022-04-26 05:19, Nathan Bossart wrote:

+1 for "member". It might not be perfect, but IMO it's the clearest
option.

Thanks! I changed the option "--membership" to "--member".

Thanks for the new patch! Would you mind adding some tests for the new
options?

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Dear Shinya,

Too bad there's no --comment parameter to do COMMENT ON ROLE name IS
'Comment';

As you already make such changes in createuser, I would like to ask for
an additional --comment parameter
that will allow sysadmins to set a comment with additional information
about the new DB user.
psql is scary for some. :-)

Overall a very useful patch. I needed bypassrls several times recently.

Shinya Kato wrote on 4/28/2022 8:06 AM:

Thank you for the reviews!

On 2022-04-26 05:19, Nathan Bossart wrote:

-    printf(_("  -g, --role=ROLE           new role will be a member 
of this role\n"));
+    printf(_("  -g, --role=ROLE        new role will be a member of 
this role\n"));
This looks lik an unexpected change.

I fixed it.

I'm ok with -m/--member as well (like with --role only one role can be
specified per switch instance so member, not membership, the later
meaning,
at least for me, the collective).

That -m doesn't match --role-to is no worse than -g not matching
--role, a
short option seems worthwhile, and the -m (membership) mnemonic
should be
simple to pick-up.

I don't see the addition of "-name" to the option name being
beneficial.

Yes, the standard doesn't use the "TO" prefix for "ROLE" - but
taking that
liberty for consistency here is very appealing and there isn't
another SQL
clause that it would be confused with.

+1 for "member".  It might not be perfect, but IMO it's the clearest
option.

Thanks! I changed the option "--membership" to "--member".

For now, I also think "-m / --member" is the best choice, although it
is ambiguous:(
I'd like to hear others' opinions.

regards

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

--
Przemysław Sztoch | Mobile +48 509 99 00 66