Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix output of createuser --help with --valid-until

  2. createuser: Add support for more clause types through new options

  3. createuser: Cleanup and fix internal option ordering

  4. Add REPLICATION privilege for ROLEs

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

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-04-13T05:51:35Z

    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?

Regards,

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

  2. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-13T06:46:25Z

    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

  3. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-04-13T07:10:01Z

    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

  4. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-13T08:35:02Z

    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

  5. Re: Add --{no-,}bypassrls flags to createuser

    Robert Haas <robertmhaas@gmail.com> — 2022-04-13T13:18:59Z

    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

  6. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-04-14T07:42:39Z

    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

  7. Re: Add --{no-,}bypassrls flags to createuser

    Daniel Gustafsson <daniel@yesql.se> — 2022-04-14T09:57:31Z

    > 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/

  8. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-04-15T05:55:48Z

    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

  9. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-15T06:33:41Z

    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

  10. Re: Add --{no-,}bypassrls flags to createuser

    Robert Haas <robertmhaas@gmail.com> — 2022-04-18T13:59:48Z

    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

  11. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-19T01:50:12Z

    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

  12. Re: Add --{no-,}bypassrls flags to createuser

    Robert Haas <robertmhaas@gmail.com> — 2022-04-19T16:13:51Z

    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

  13. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-20T08:04:21Z

    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

  14. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-04-21T04:29:57Z

    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

  15. Re: Add --{no-,}bypassrls flags to createuser

    Robert Haas <robertmhaas@gmail.com> — 2022-04-21T19:51:05Z

    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

  16. Re: Add --{no-,}bypassrls flags to createuser

    David G. Johnston <david.g.johnston@gmail.com> — 2022-04-21T20:21:57Z

    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.

  17. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-04-25T20:19:47Z

    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

  18. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-04-28T06:06:30Z

    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

  19. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-05-02T17:07:41Z

    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

  20. Re: Re: Add --{no-,}bypassrls flags to createuser

    Przemysław Sztoch <przemyslaw@sztoch.pl> — 2022-05-05T22:08:22Z

    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

  21. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-05-19T01:35:23Z

    Thanks for reviews and comments!

On 2022-05-06 07:08, Przemysław Sztoch wrote:

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

I created a new patch to test the new options!
However, not all option tests exist, so it may be necessary to consider 
whether to actually add this test.


> 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. :-)

Since the createuser command is a wrapper for the CREATE ROLE command, I 
do not think it is appropriate to add options that the CREATE ROLE 
command does not have.


-- 
Regards,

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

  22. Re: Add --{no-,}bypassrls flags to createuser

    David G. Johnston <david.g.johnston@gmail.com> — 2022-05-19T01:46:02Z

    On Wed, May 18, 2022 at 6:35 PM Shinya Kato <Shinya11.Kato@oss.nttdata.com>
wrote:

> > 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. :-)
>
> Since the createuser command is a wrapper for the CREATE ROLE command, I
> do not think it is appropriate to add options that the CREATE ROLE
> command does not have.
>
>
I think that this feature is at least worth considering - but absent an
existing command that does this I would agree that doing so constitutes a
separate feature.

As an aside, I'd rather overcome this particular objection by having the
CREATE object command all accept an optional "COMMENT IS" clause.

David J.

  23. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-05-20T21:45:19Z

    On Thu, May 19, 2022 at 10:35:23AM +0900, Shinya Kato wrote:
> I created a new patch to test the new options!

Thanks for the new patch!  I attached a new version with a few small
changes.  What do you think?

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

  24. Re: Add --{no-,}bypassrls flags to createuser

    Przemysław Sztoch <przemyslaw@sztoch.pl> — 2022-05-22T07:55:37Z

    David G. Johnston wrote on 5/19/2022 3:46 AM:
> I think that this feature is at least worth considering - but absent 
> an existing command that does this I would agree that doing so 
> constitutes a separate feature.
>
> As an aside, I'd rather overcome this particular objection by having 
> the CREATE object command all accept an optional "COMMENT IS" clause.
>
> David J.
The createuser command is typically used by IT personnel unfamiliar with 
SQL and unfamiliar with psql.
They often use this command because software installation procedures 
require it.
Lack of comment then causes more mess in the configuration of larger 
servers.
I still think it's worth adding the --comment argument to execute the 
next SQL statement by createuser.
This will simplify the setup scripts and installation instructions for 
the final software.

I believe that it is not worth dividing it into a separate program.

The same --comment argument is needed for the createdb command.
-- 
Przemysław Sztoch | Mobile +48 509 99 00 66

  25. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-05-23T01:32:40Z

    At Sun, 22 May 2022 09:55:37 +0200, Przemysław Sztoch <przemyslaw@sztoch.pl> wrote in 
> David G. Johnston wrote on 5/19/2022 3:46 AM:
> > As an aside, I'd rather overcome this particular objection by having
> > the CREATE object command all accept an optional "COMMENT IS" clause.
> >
> I believe that it is not worth dividing it into a separate program.
> 
> The same --comment argument is needed for the createdb command.

David didn't say that it should be another "program", but said it
should be another "patch/development", because how we implement the
--comment feature is apparently controversial.

It doesn't seem to be explicity mentioned that "createuser is mere a
shell-substitute for the SQL CREATE ROLE", but I feel the same with
Shinya that it is. We could directly invoke "COMMENT ON" from
createuser command, but I think it is not the way to go in that light.

We can either add COMMENT clause only to "CREATE ROLE" , or "COMMENT
IS" clause to all (or most of) "CREATE object" commands, or something
others. (Perhaps "COMMETN IS" requires "ALTER object" handle comments,
and I'm not sure how we think about the difference of it from "comment
on" command.)  We might return to "comment on" in the end..

Anyway, after fixing that issue we will modify the createrole command
so that it uses the new SQL feature. I find no hard obstacles in
reaching there in the 16 cycle.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  26. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-05-23T06:18:48Z

    On 2022-05-21 06:45, Nathan Bossart wrote:
> On Thu, May 19, 2022 at 10:35:23AM +0900, Shinya Kato wrote:
>> I created a new patch to test the new options!
> 
> Thanks for the new patch!  I attached a new version with a few small
> changes.  What do you think?

Thanks for updating the patch!
It looks good to me.


On 2022-05-23 10:32, Kyotaro Horiguchi wrote:
> Anyway, after fixing that issue we will modify the createrole command
> so that it uses the new SQL feature. I find no hard obstacles in
> reaching there in the 16 cycle.

+1.


-- 
Regards,

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

  27. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-05-23T07:29:50Z

    On Fri, May 20, 2022 at 02:45:19PM -0700, Nathan Bossart wrote:
> Thanks for the new patch!  I attached a new version with a few small
> changes.  What do you think?

So you have settled down to --member to emulate the clause ROLE.
Well, this choice is fine by me at the end.

> +$node->issues_sql_like(
> +	[ 'createuser', 'regress_role2', '-a', 'regress_user1' ],
> +	qr/statement: CREATE ROLE regress_role2 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ADMIN regress_user1;/,
> +	'add a role as a member with admin option of the newly created role');
> +$node->issues_sql_like(
> +	[ 'createuser', 'regress_role3', '-m', 'regress_user1' ],
> +	qr/statement: CREATE ROLE regress_role3 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN ROLE regress_user1;/,
> +	'add a role as a member of the newly created role');

May I ask for the addition of tests when one specifies multiple
switches for --admin and --member?  This would check the code path
where you build a list of role names.  You could check fancier string
patterns, while on it, to look after the use of fmtId(), say with
role names that include whitespaces or such.
--
Michael

  28. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-05-23T14:55:43Z

    On 2022-05-23 16:29, Michael Paquier wrote:
>> +$node->issues_sql_like(
>> +	[ 'createuser', 'regress_role2', '-a', 'regress_user1' ],
>> +	qr/statement: CREATE ROLE regress_role2 NOSUPERUSER NOCREATEDB 
>> NOCREATEROLE INHERIT LOGIN ADMIN regress_user1;/,
>> +	'add a role as a member with admin option of the newly created 
>> role');
>> +$node->issues_sql_like(
>> +	[ 'createuser', 'regress_role3', '-m', 'regress_user1' ],
>> +	qr/statement: CREATE ROLE regress_role3 NOSUPERUSER NOCREATEDB 
>> NOCREATEROLE INHERIT LOGIN ROLE regress_user1;/,
>> +	'add a role as a member of the newly created role');
> 
> May I ask for the addition of tests when one specifies multiple
> switches for --admin and --member?  This would check the code path
> where you build a list of role names.  You could check fancier string
> patterns, while on it, to look after the use of fmtId(), say with
> role names that include whitespaces or such.

Thanks!
I changed to the test that describes multiple "-m".
It seems to be working without any problems, how about it?


-- 
Regards,

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

  29. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-05-23T16:37:35Z

    On Mon, May 23, 2022 at 11:55:43PM +0900, Shinya Kato wrote:
> On 2022-05-23 16:29, Michael Paquier wrote:
>> May I ask for the addition of tests when one specifies multiple
>> switches for --admin and --member?  This would check the code path
>> where you build a list of role names.  You could check fancier string
>> patterns, while on it, to look after the use of fmtId(), say with
>> role names that include whitespaces or such.
> 
> Thanks!
> I changed to the test that describes multiple "-m".
> It seems to be working without any problems, how about it?

Michael also requested a test for multiple -a switches and for fancier
string patterns.  Once that is taken care of, I think this can be marked as
ready-for-committer.

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

  30. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-05-24T02:09:01Z

    On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
> Michael also requested a test for multiple -a switches and for fancier
> string patterns.  Once that is taken care of, I think this can be marked as
> ready-for-committer.

Looking at v7, this means to extend the tests to process lists for
--admin with more name patterns.  And while on it, we could do the
same for the existing command for --role, but this one is on me, being
overly-pedantic while looking at the patch :)
--
Michael

  31. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-05-24T11:07:31Z

    On 2022-05-24 11:09, Michael Paquier wrote:
> On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
>> Michael also requested a test for multiple -a switches and for fancier
>> string patterns.  Once that is taken care of, I think this can be 
>> marked as
>> ready-for-committer.
> 
> Looking at v7, this means to extend the tests to process lists for
> --admin with more name patterns.  And while on it, we could do the
> same for the existing command for --role, but this one is on me, being
> overly-pedantic while looking at the patch :)

Thanks! I fixed it.


-- 
Regards,

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

  32. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-05-24T17:09:10Z

    On Tue, May 24, 2022 at 08:07:31PM +0900, Shinya Kato wrote:
> On 2022-05-24 11:09, Michael Paquier wrote:
>> On Mon, May 23, 2022 at 09:37:35AM -0700, Nathan Bossart wrote:
>> > Michael also requested a test for multiple -a switches and for fancier
>> > string patterns.  Once that is taken care of, I think this can be
>> > marked as
>> > ready-for-committer.
>> 
>> Looking at v7, this means to extend the tests to process lists for
>> --admin with more name patterns.  And while on it, we could do the
>> same for the existing command for --role, but this one is on me, being
>> overly-pedantic while looking at the patch :)
> 
> Thanks! I fixed it.

We're still missing some "fancier" string patterns in the tests, but we
might just be nitpicking at this point.

I noticed that the cfbot tests for this are failing for Windows.  I've
looked at the relevant logs a bit, and I'm not sure what is going on.  The
expected log messages are indeed missing, but I haven't found any clues for
why those test cases are skipped.

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

  33. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-05-25T02:07:52Z

    At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> We're still missing some "fancier" string patterns in the tests, but we
> might just be nitpicking at this point.

Such "fancier" strings should be properly handled by FmtId() and
appendStringLiteralConn.  If this is a privilege escalating command,
we should have ones but this is not.

> I noticed that the cfbot tests for this are failing for Windows.  I've
> looked at the relevant logs a bit, and I'm not sure what is going on.  The
> expected log messages are indeed missing, but I haven't found any clues for
> why those test cases are skipped.

createuser command complains like this.

> # Running: createuser regress_user4 -a regress_user1 -a regress_user2
> createuser: error: too many command-line arguments (first is "-a")
> hint: Try "createuser --help" for more information.

It seems like '-a' is not recognised as an option parameter.

(Fortunately, the ActiveState installer looks like having been fixed,
 but something's still wrong..)

I reproduced the same failure at my hand and identified the
cause. Windows' version of getopt_long seems to dislike that
non-optional parameters precedes options.

> createuser <user name to create> <options>

The test succeeded if I moved the <user name to create> to the end of
command line.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  34. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-05-25T03:47:56Z

    On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
> I reproduced the same failure at my hand and identified the
> cause. Windows' version of getopt_long seems to dislike that
> non-optional parameters precedes options.

Tweaking the list of arguments in some commands kicked by the TAP
tests to satisfy our implementation of getopt_long() has been the
origin of a couple of portability fixes, like ffd3980.
--
Michael

  35. Re: Add --{no-,}bypassrls flags to createuser

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2022-05-26T05:16:37Z

    On 2022-05-25 12:47, Michael Paquier wrote:
> On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
>> I reproduced the same failure at my hand and identified the
>> cause. Windows' version of getopt_long seems to dislike that
>> non-optional parameters precedes options.
> 
> Tweaking the list of arguments in some commands kicked by the TAP
> tests to satisfy our implementation of getopt_long() has been the
> origin of a couple of portability fixes, like ffd3980.

Thanks! I fixed it.


On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
> At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
> <nathandbossart@gmail.com> wrote in
>> We're still missing some "fancier" string patterns in the tests, but 
>> we
>> might just be nitpicking at this point.
> 
> Such "fancier" strings should be properly handled by FmtId() and
> appendStringLiteralConn.  If this is a privilege escalating command,
> we should have ones but this is not.

Sorry, I didn't quite understand the "fancier" pattern. Is a string like 
this patch correct?


-- 
Regards,

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

  36. Re: Add --{no-,}bypassrls flags to createuser

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-05-26T07:47:46Z

    At Thu, 26 May 2022 14:16:37 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
> On 2022-05-25 12:47, Michael Paquier wrote:
> > On Wed, May 25, 2022 at 11:07:52AM +0900, Kyotaro Horiguchi wrote:
> >> I reproduced the same failure at my hand and identified the
> >> cause. Windows' version of getopt_long seems to dislike that
> >> non-optional parameters precedes options.
> > Tweaking the list of arguments in some commands kicked by the TAP
> > tests to satisfy our implementation of getopt_long() has been the
> > origin of a couple of portability fixes, like ffd3980.
> 
> Thanks! I fixed it.
> 
> 
> On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
> > At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
> > <nathandbossart@gmail.com> wrote in
> >> We're still missing some "fancier" string patterns in the tests, but
> >> we
> >> might just be nitpicking at this point.
> > Such "fancier" strings should be properly handled by FmtId() and
> > appendStringLiteralConn.  If this is a privilege escalating command,
> > we should have ones but this is not.
> 
> Sorry, I didn't quite understand the "fancier" pattern. Is a string
> like this patch correct?

FWIW, the "fancy" here causes me to think about something likely to
cause syntax breakage of the query to be sent.

createuser -a 'user"1' -a 'user"2' 'user"3'
createuser -v "2023-1-1'; DROP TABLE public.x; select '" hoge

BUT, thses should be prevented by the functions enumerated above. So,
I don't think we need them.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  37. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-05-26T16:20:37Z

    On Thu, May 26, 2022 at 02:16:37PM +0900, Shinya Kato wrote:
> On 2022-05-25 11:07, Kyotaro Horiguchi wrote:
>> At Tue, 24 May 2022 10:09:10 -0700, Nathan Bossart
>> <nathandbossart@gmail.com> wrote in
>> > We're still missing some "fancier" string patterns in the tests, but
>> > we
>> > might just be nitpicking at this point.
>> 
>> Such "fancier" strings should be properly handled by FmtId() and
>> appendStringLiteralConn.  If this is a privilege escalating command,
>> we should have ones but this is not.
> 
> Sorry, I didn't quite understand the "fancier" pattern. Is a string like
> this patch correct?

Yes, thanks.  I'm marking this as ready-for-committer.

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

  38. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-07-13T03:25:18Z

    On Thu, May 26, 2022 at 04:47:46PM +0900, Kyotaro Horiguchi wrote:
> FWIW, the "fancy" here causes me to think about something likely to
> cause syntax breakage of the query to be sent.
> 
> createuser -a 'user"1' -a 'user"2' 'user"3'
> createuser -v "2023-1-1'; DROP TABLE public.x; select '" hoge

That would be mostly using spaces here, to make sure that quoting is
correctly applied.

> BUT, thses should be prevented by the functions enumerated above. So,
> I don't think we need them.

Mostly.  For example, the test for --valid-until can use a timestamp
with spaces to validate the use of appendStringLiteralConn().  A
second thing is that --member was checked, but not --admin, so I have
renamed regress_user2 to "regress user2" for that to apply a maximum
of coverage, and applied the patch.

One thing that I found annoying is that this made the list of options
of createuser much harder to follow.  That's not something caused by
this patch as many options have accumulated across the years and there
is a kind pattern where the connection options were listed first, but
I have cleaned up that while on it.  A second area where this could be
done is createdb, as it could be easily expanded if the backend query
gains support for more stuff, but that can happen when it makes more
sense.
--
Michael

  39. RE: Add --{no-,}bypassrls flags to createuser

    Shinoda, Noriyoshi <noriyoshi.shinoda@hpe.com> — 2022-07-13T08:14:28Z

    Hi,
Thanks to the developers and reviewers.
The attached small patch fixes the message in "createuser --help" command. The patch has changed to specify a time stamp for the --valid-for option. I don't think the SGML description needs to be modified.

Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Michael Paquier <michael@paquier.xyz> 
Sent: Wednesday, July 13, 2022 12:25 PM
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Shinya11.Kato@oss.nttdata.com; nathandbossart@gmail.com; przemyslaw@sztoch.pl; david.g.johnston@gmail.com; robertmhaas@gmail.com; daniel@yesql.se; pgsql-hackers@postgresql.org
Subject: Re: Add --{no-,}bypassrls flags to createuser

On Thu, May 26, 2022 at 04:47:46PM +0900, Kyotaro Horiguchi wrote:
> FWIW, the "fancy" here causes me to think about something likely to 
> cause syntax breakage of the query to be sent.
> 
> createuser -a 'user"1' -a 'user"2' 'user"3'
> createuser -v "2023-1-1'; DROP TABLE public.x; select '" hoge

That would be mostly using spaces here, to make sure that quoting is correctly applied.

> BUT, thses should be prevented by the functions enumerated above. So, 
> I don't think we need them.

Mostly.  For example, the test for --valid-until can use a timestamp with spaces to validate the use of appendStringLiteralConn().  A second thing is that --member was checked, but not --admin, so I have renamed regress_user2 to "regress user2" for that to apply a maximum of coverage, and applied the patch.

One thing that I found annoying is that this made the list of options of createuser much harder to follow.  That's not something caused by this patch as many options have accumulated across the years and there is a kind pattern where the connection options were listed first, but I have cleaned up that while on it.  A second area where this could be done is createdb, as it could be easily expanded if the backend query gains support for more stuff, but that can happen when it makes more sense.
--
Michael

  40. Re: Add --{no-,}bypassrls flags to createuser

    Nathan Bossart <nathandbossart@gmail.com> — 2022-07-13T18:38:03Z

    On Wed, Jul 13, 2022 at 08:14:28AM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
> The attached small patch fixes the message in "createuser --help" command. The patch has changed to specify a time stamp for the --valid-for option. I don't think the SGML description needs to be modified.

Good catch.  Apart from a nitpick about the indentation, your patch looks
reasonable to me.

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

  41. Re: Add --{no-,}bypassrls flags to createuser

    Michael Paquier <michael@paquier.xyz> — 2022-07-13T23:39:58Z

    On Wed, Jul 13, 2022 at 11:38:03AM -0700, Nathan Bossart wrote:
> On Wed, Jul 13, 2022 at 08:14:28AM +0000, Shinoda, Noriyoshi (PN Japan FSIP) wrote:
>> The attached small patch fixes the message in "createuser --help"
>> command. The patch has changed to specify a time stamp for the
>> --valid-for option. I don't think the SGML description needs to be
>> modified.

Thanks, Shinoda-san.  Fixed.

> Good catch.  Apart from a nitpick about the indentation, your patch looks
> reasonable to me.

FWIW, one can check that with a simple `git diff --check` or similar
to see what was going wrong here.  This simple trick allows me to find
quickly formatting issues in any patch posted.
--
Michael