maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Sergei" == Sergei Golubchik <serg@xxxxxxxxxxxx> writes:

Sergei> Hi, Michael!
Sergei> On Sep 23, Michael Widenius wrote:
>> === modified file 'mysql-test/r/user_limits.result'
>> --- mysql-test/r/user_limits.result	2005-05-02 18:45:06 +0000
>> +++ mysql-test/r/user_limits.result	2011-09-23 11:50:40 +0000
>> @@ -65,6 +65,16 @@ select * from t1;

>> connect(localhost,mysqltest_1,,test,MYSQL_PORT,MYSQL_SOCK);
>> ERROR 42000: User 'mysqltest_1' has exceeded the 'max_user_connections' resource (current value: 3)
>> +grant usage on *.* to mysqltest_1@localhost with max_user_connections -1;

Sergei> I still think that -1 (and 0 too) as special values look like a very
Sergei> unprofessional hack. I'd rather see keywords there like

Sergei>   with max_user_connections UNLIMITED
Sergei>   with max_user_connections NONE

That would be harder to document and understand when you take into
account how the global variable max_user_connections work.

I prefer to have these work identical.

Sergei> as for the SET, SHOW VARIABLES, and command-line options - we can easily
Sergei> support keywords if we declare max_user_connections to be a string
Sergei> variable, not numeric, and will parse it manually.

That I see would be an even great hack.

The 'proper' way for the future would be to change the default value
of 'max_user_connections' to MAX_INT and in the future threat 0 to
mean zero connections.

>> === modified file 'scripts/mysql_system_tables_fix.sql'
>> --- scripts/mysql_system_tables_fix.sql	2011-05-28 02:11:32 +0000
>> +++ scripts/mysql_system_tables_fix.sql	2011-09-22 21:41:45 +0000
>> @@ -326,8 +326,11 @@ UPDATE host SET Create_routine_priv=Crea
>> 
>> #
>> # Add max_user_connections resource limit
>> +# this is signed in MariaDB so that if one sets it's to -1 then the user
>> +# can't connect anymore.

Sergei> You need one special value (-1) and to get it you cut the number of
Sergei> allowed values in half. Instead, you could've kept max_user_connections
Sergei> as unsigned and only reserve (uint)~0 as "cannot connect".

It's a maxint;  No one will *ever* have 2~31 connections.

This is ok.

>> switch (var->show_type())
>> {
>> -    case SHOW_INT:      get_sys_var_safe (uint);
>> +    case SHOW_INT:      get_sys_var_safe (int);
>> case SHOW_LONG:     get_sys_var_safe (ulong);
>> case SHOW_LONGLONG: get_sys_var_safe (ulonglong);
>> case SHOW_HA_ROWS:  get_sys_var_safe (ha_rows);

Sergei> This is a bit inconsistent. INT is signed, but LONG/LONGLONG are
Sergei> unsigned. Do I understand correctly that there will be no way to create
Sergei> an unsigned INT variable after your patch?

If you want unsigned int, you just use 'long'
(Most of our variables are 'long' anyway;  We have only a handful of
int variables and the range of MAX_INT is good enough for these).

When we ever need unsigned int, we will add a SHOW_UINT.
I wanted to keep the patch minimal and as no-intrusive as possible
and thats why I only fixed SHOW_INT and not SHOW_LONG and SHOW_LONGLONG.

It's actually a bug that SHOW_INT didn't take negative numbers before;
It's supposed to work the same way as GET_INT works in my_getopt.c,
which it didn't do.

>> === modified file 'sql/sql_acl.cc'

>> -    /* Don't allow the user to connect if he has done too many queries */
>> -    if ((acl_user->user_resource.questions || acl_user->user_resource.updates ||
>> -         acl_user->user_resource.conn_per_hour ||
>> -         acl_user->user_resource.user_conn || max_user_connections) &&
>> -        get_or_create_user_conn(thd,
>> -          (opt_old_style_user_limits ? sctx->user : sctx->priv_user),
>> -          (opt_old_style_user_limits ? sctx->host_or_ip : sctx->priv_host),
>> -          &acl_user->user_resource))
>> +    /*
>> +      Don't allow the user to connect if he has done too many queries
>> +      We always have to allocated this as someone may change
>> +      max_user_connections any time.

Sergei> What do you mean?

That the original code was wrong. We can *not* do the allocation based
on the original 'if' as it's assumed that max_user_connections would
never change.

My comments explain that the we have always to call
get_or_create_user_conn() because max_user_connections depends on this
structure to exists.


>> +     */
>> +    if (get_or_create_user_conn(thd,
>> +                                (opt_old_style_user_limits ? sctx->user :
>> +                                 sctx->priv_user),
>> +                                (opt_old_style_user_limits ? sctx->host_or_ip
>> +                                 : sctx->priv_host),
>> +                                &acl_user->user_resource))
>> DBUG_RETURN(1); // The error is set by get_or_create_user_conn()
>> }
>> else
>> 
>> === modified file 'sql/sql_connect.cc'
>> --- sql/sql_connect.cc	2011-09-22 22:13:38 +0000
>> +++ sql/sql_connect.cc	2011-09-22 22:58:07 +0000
>> @@ -113,8 +113,11 @@ int check_for_max_user_connections(THD *
>> DBUG_ENTER("check_for_max_user_connections");
>> 
>> (void) pthread_mutex_lock(&LOCK_user_conn);
>> +
>> +  /* Root is not affected by the value of max_user_connections */

Sergei> It's an incompatible change. Why did you do it? If the point was to
Sergei> allow SUPER connections when max_user_connections==-1, then it would be
Sergei> enough to make only this value (-1) to be ignored by the SUPER user, and
Sergei> any other value limit should still apply.

root users should not be affected by the global variables
max_user_connections or max_connections.

Yes, it's incompatible, but ok and necessary in this case as otherwise
no one could login if we set global.max_user_connections=1 which would
not let you maintain the server at all.

Sergei> SUPER user typically ignores features that prevent him from connecting
Sergei> to the server. E.g incorrect init-connect option may do it. Or a normal
Sergei> user may use up all max_connections limit and prevent SUPER from
Sergei> connecting. That's why SUPER ignores init-connect and is allowed one
Sergei> connection above max_connections.

Sergei> But max_user_connections can not prevent SUPER from connecting (unless
Sergei> it is -1). If it is set to, for example, 5, SUPER can have five
Sergei> simultaneous connections and no normal user can prevent him from doing
Sergei> it. This is why SUPER does *not* ignore max_user_connections.
Sergei> So, by this logic, only the value max_user_connections==-1 should be
Sergei> ignored by SUPER, and not any other value.

I think it's better to not regard any value as special.  It's easier
to document it the current way; Doing it your way would make it hard
to document and understand and that usually means it's a bad idea.
(For example, what happens if you set max_user_connections=-1 and then
later change it to 1; What limits apply to the root user that is now
logged in twice).

If you want to constrain the root user, you can do that with
GRANT root ... max_user_connections=#

Regards,
Monty