maria-developers team mailing list archive
Mailing list archive
Re: CloudLinux patches for MariaDB
>>>>> "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;
>> 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
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()
>> === 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 *
>> (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=#