Re: Extra functionality to createuser

On Sat, Nov 16, 2013 at 4:57 AM, Christopher Browne <cbbrowne(at)gmail(dot)com> wrote:
> On Fri, Nov 15, 2013 at 3:14 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> On 11/14/13, 4:35 PM, Christopher Browne wrote:> On Thu, Nov 14, 2013 at
>> 5:41 AM, Sameer Thakur <samthakur74(at)gmail(dot)com> wrote:
>>>> So i think -g option is failing
>>>
>>> Right you are.
>>>

This patch adds useful option '-g' to createuser utility which will
allow user to make new roles as member of existing roles and the same
is already possible by Create Role/User syntax.

Few comments:

1.
+ <term><option>-g</></term>
+ <term><option>--roles</></term>

All other options which require argument are of form:
<term><option>-c <replaceable class="parameter">number</replaceable></></term>
<term><option>--connection-limit=<replaceable
class="parameter">number</replaceable></></term>

So I think it is better to have this new option which require argument
in similar form.

2.
+ Indicates roles to associate with this role.

I think word associate is not very clear, wouldn't it be better to
mention that this new role will be member of roles specified.
For example:
Indicates roles to which the new role will be immediately added as a new member.

3.
+ case 'g':
+ roles = pg_strdup(optarg);
+ break;

If we see most of other options in case handling are ordered as per
their order in long_options array. For example

static struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
..

Now the order of handling for both is same in switch case or while get
opt_long() function call. I think this makes code easy to understand
and modify.
However there is no functionality issue here, so you can keep the code
as per your existing patch as well, this is just a suggestion.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com