Re: [Maria-developers] CloudLinux patches for MariaDB

Hi!

...

...

...

...

"Sergei" == Sergei Golubchik 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