Merge lp:~zhuzhaoyuan/mysql-proxy/zhuzhaoyuan into lp:mysql-proxy

Proposed by Joshua Zhu
Status: Merged
Merge reported by: Kay Roepke
Merged at revision: not available
Proposed branch: lp:~zhuzhaoyuan/mysql-proxy/zhuzhaoyuan
Merge into: lp:mysql-proxy
To merge this branch: bzr merge lp:~zhuzhaoyuan/mysql-proxy/zhuzhaoyuan
Reviewer Review Type Date Requested Status
Kay Roepke (community) Approve
Review via email: mp+3435@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :

Please review revision 567 (fix compile warnings), and revision 568 (fix bug #42631).

Revision history for this message
Kay Roepke (kay-roepke) wrote :

Rev 567 is fine with me, I'll merge that.

For rev 568 I would like to move the code into a network-mysqld-packet-lua.(hc) wrapper, like the other things we expose to make it reusable, instead of hardcoding it in the proxy plugin.
We also need to figure out which parts make sense to be mutable from Lua (max_packet_size is probably not a good candidate for being mutable, for instance).

review: Needs Fixing
Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :

OK :)
But I think the patch below should be applied, otherwise the program would crash when the read_handshake() hook returning proxy.PROXY_SEND_RESULT
or proxy.PROXY_SEND_QUERY(see proxy_lua_read_handshake() for more details).

=== modified file 'plugins/proxy/proxy-plugin.c'
--- plugins/proxy/proxy-plugin.c 2008-12-04 20:07:39 +0000
+++ plugins/proxy/proxy-plugin.c 2009-02-06 13:19:41 +0000
@@ -469,7 +469,7 @@
  switch (proxy_lua_read_handshake(con)) {
  case PROXY_NO_DECISION:
   break;
- case PROXY_SEND_QUERY:
+ case PROXY_SEND_RESULT:
   /* the client overwrote and wants to send its own packet
    * it is already in the queue */

Revision history for this message
Kay Roepke (kay-roepke) wrote :

On Feb 6, 2009, at 2:39 PM, Joshua Zhu wrote:

> But I think the patch below should be applied, otherwise the program
> would crash when the read_handshake() hook returning
> proxy.PROXY_SEND_RESULT
> or proxy.PROXY_SEND_QUERY(see proxy_lua_read_handshake() for more
> details).

good catch!

thanks,
-k
--
Kay Roepke
Software Engineer, MySQL Enterprise Tools

Sun Microsystems GmbH Sonnenallee 1, DE-85551 Kirchheim-Heimstetten
Geschaeftsfuehrer: Thomas Schroeder, Wolfang Engels, Dr. Roland Boemer
Vorsitz d. Aufs.rat.: Martin Haering HRB MUC 161028

569. By Joshua Zhu <josh@joshua>

merge

570. By Joshua Zhu <josh@joshua>

fix Bug #42783 (http://bugs.mysql.com/bug.php?id=42783)
* rlim_max should be set too.
* According to the latest POSIX standard (IEEE Std 1003.1, 2004 Edition), rlim_t is actually an unsigned integer type. See: http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/resource.h.html

Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :

Please review revision 570 (to fix bug #42783 http://bugs.mysql.com/bug.php?id=42783)

Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :

Any response?

571. By Joshua Zhu <josh@joshua>

fix the warnings (if you are a code neat freak who loves "CFLAGS=-Wall")

Revision history for this message
Kay Roepke (kay-roepke) wrote :

"Any response?"

sorry, been out of town.
There are a couple issues here:
1) rlim_t is different things to different "unixes". On OS X (where I wrote the code) it's __uint64_t. On Linux it seems to be a signed int64... I haven't checked other platforms yet.
In any case, it should be a 64 bit integer, so llu seems appropriate. I'm actually running 32 bit binaries here and output seems to be fine. The only problem is that OS X does not seem to correctly give back 'unlimited', nor can you set NOFILES to 'unlimited' by using the macro for it. Apple seems unwilling to fix this, instead they changed their docs from 10.4 to 10.5 pointing this out (the current maximum value you can use is OPEN_MAX == 10240, btw).
2) Raising the hard limit needs root permissions. Now, you could argue that setting rlim_max = max_open_files will work, because either
   a) max_open_files is lower then rlim_max and if it's lower then we are fine or
   b) max_open_files is larger then rlim_max (and transitively is larger than rlim_cur) thus we will fail anyway.
My only objection is that there's no real value in modifying the hard limit, because it would not make a difference. I doubt that we would get a signal for exceeding the soft limit of NOFILES, but I might be wrong. If we do, we could try to further increase the soft limit, but if we lowered the hard limit that would not be an option any more. Of course, none of that code has been written yet.

Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :
Download full text (4.7 KiB)

Sorry, maybe I should not believe the POSIX specification, which cheated me :(
(
http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/resource.h.html
rlim_t
    Unsigned integer type used for limit values.
)

> ...
> On Linux it seems to be a signed int64...

Not exactly, I'm afraid. It's an _unsigned long int_ on Linux. And all my Linux systems on hand proved this.

#include <sys/resource.h>
#include <stdio.h>

/**
 * see:
 * /usr/include/sys/resource.h
 * /usr/include/bits/resource.h
 * /usr/include/bits/typesizes.h
 * /usr/include/bits/types.h
 */

int main(int argc, char *argv[])
{
    printf("sizeof(rlim_t) = %d\n", sizeof(rlim_t));
    return 0;
}

$ cat /etc/issue
Ubuntu 8.04.2 \n \l
$ uname -a
Linux joshua 2.6.24-23-generic #1 SMP Mon Jan 26 00:13:11 UTC 2009 i686 GNU/Linux
$ ./a.out
sizeof(rlim_t) = 4

$ cat /etc/redhat-release
CentOS release 5.2 (Final)
$ uname -a
Linux test98.com 2.6.27.4 #1 SMP Thu Nov 6 13:33:18 CST 2008 i686 i686 i386 GNU/Linux
$ ./a.out
sizeof(rlim_t) = 4

$ cat /etc/redhat-release
CentOS release 5 (Final)
$ uname -a
Linux test64 2.6.23.12 #1 SMP Wed Jun 4 13:09:38 CST 2008 x86_64 x86_64 x86_64 GNU/Linux
$ ./a.out
sizeof(rlim_t) = 8

$ cat /etc/gentoo-release
Gentoo Base System release 1.12.11.1
$ uname -a
Linux Test-DB-Server 2.6.27-gentoo-r8 #1 SMP Mon Feb 9 16:22:38 CST 2009 x86_64 AMD Athlon(tm) 64 X2 Dual Core Processor 5200+ AuthenticAMD GNU/Linux
$ ./a.out
sizeof(rlim_t) = 8

Sadly, I have no other unix systems available. But I think treating rlim_t as unsigned long on other platforms should be fine as well (e.g. unsigned long is still 64 bits long on your Mac).
This is a trivial problem though (just debug message).

> 2) Raising the hard limit needs root permissions.

Sure.

> Now, you could argue that setting rlim_max = max_open_files will work, because either
> a) max_open_files is lower then rlim_max and if it's lower then we are fine or
> b) max_open_files is larger then rlim_max (and transitively is larger than rlim_cur) thus we will fail anyway.
> My only objection is that there's no real value in modifying the hard limit, because it would not make a difference.
> I doubt that we would get a signal for exceeding the soft limit of NOFILES, but I might be wrong. If we do, we could try to further increase the soft limit, but if we lowered the hard limit that would not be an option any more. Of course, none of that code has been written yet.

Imagine I'm the root user and rlim_max now is 1024 (actually default value on most systems). When I raise the max_open_files to any number greater than 1024, say 8192, it won't work at all. Is it too confusing, since I'm already root??
By the way, IMHO, setting both the values of rlim_cur and rlim_max is a conventional way in many server programs.
So, how about this?

=== modified file 'src/chassis.c'
--- src/chassis.c 2009-01-15 17:45:43 +0000
+++ src/chassis.c 2009-02-21 07:47:52 +0000
@@ -980,19 +980,20 @@
       G_STRLOC, g_strerror(errno), errno);
  } else {
   rlim_t soft_limit = max_files_rlimit.rlim_cur;
- g_debug("%s: current RLIMIT_NOFILE = %llu (hard: %llu)", G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
+ g_deb...

Read more...

Revision history for this message
Kay Roepke (kay-roepke) wrote :
Download full text (5.1 KiB)

On Feb 21, 2009, at 9:28 AM, Joshua Zhu wrote:

> Sorry, maybe I should not believe the POSIX specification, which
> cheated me :(
> (
> http://www.opengroup.org/onlinepubs/009695399/basedefs/sys/resource.h.html
> rlim_t
> Unsigned integer type used for limit values.
> )

well, it _is_ an integer type, just not int, so the implementations
seem to be in accordance with the spec. :)

>> ...
>> On Linux it seems to be a signed int64...
>
> Not exactly, I'm afraid. It's an _unsigned long int_ on Linux. And
> all my Linux systems on hand proved this.

'k, i scraped header files from the net and saw a signed long int.
maybe that used to be a bug (many UNIXes in fact only use 63 bits of
rlim_t because of old buggy implementations that used signed longs for
this purpose).

> Sadly, I have no other unix systems available. But I think treating
> rlim_t as unsigned long on other platforms should be fine as well
> (e.g. unsigned long is still 64 bits long on your Mac).
> This is a trivial problem though (just debug message).

i'll be working to on an automatic build process over the weekend.
that will give us results on all platforms MySQL itself is compiled
on, including some really arcane systems. then we should know more.

>> Now, you could argue that setting rlim_max = max_open_files will
>> work, because either
>> a) max_open_files is lower then rlim_max and if it's lower then we
>> are fine or
>> b) max_open_files is larger then rlim_max (and transitively is
>> larger than rlim_cur) thus we will fail anyway.
>> My only objection is that there's no real value in modifying the
>> hard limit, because it would not make a difference.
>> I doubt that we would get a signal for exceeding the soft limit of
>> NOFILES, but I might be wrong. If we do, we could try to further
>> increase the soft limit, but if we lowered the hard limit that
>> would not be an option any more. Of course, none of that code has
>> been written yet.
>
> Imagine I'm the root user and rlim_max now is 1024 (actually default
> value on most systems). When I raise the max_open_files to any
> number greater than 1024, say 8192, it won't work at all. Is it too
> confusing, since I'm already root??
> By the way, IMHO, setting both the values of rlim_cur and rlim_max
> is a conventional way in many server programs.
> So, how about this?

this looks like the right thing to do, thank you!
i'll take another look at the format string to use on the different
platforms. we should be able to hinge that on sizeof(rlim_t) and use a
simple macro if necessary.

> === modified file 'src/chassis.c'
> --- src/chassis.c 2009-01-15 17:45:43 +0000
> +++ src/chassis.c 2009-02-21 07:47:52 +0000
> @@ -980,19 +980,20 @@
> G_STRLOC, g_strerror(errno), errno);
> } else {
> rlim_t soft_limit = max_files_rlimit.rlim_cur;
> - g_debug("%s: current RLIMIT_NOFILE = %llu (hard: %llu)",
> G_STRLOC, max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
> + g_debug("%s: current RLIMIT_NOFILE = %lu (hard: %lu)", G_STRLOC,
> max_files_rlimit.rlim_cur, max_files_rlimit.rlim_max);
>
> max_files_rlimit.rlim_cur = max_files_number;
> + max_files_rlimit.rli...

Read more...

Revision history for this message
Joshua Zhu (zhuzhaoyuan) wrote :

Thanks for your reply and encouragement, I'll keep going :)

Revision history for this message
Kay Roepke (kay-roepke) wrote :

This request does not merge for me anymore, due to a bzr error message.
Also I believe this was also fixed in trunk (the corresponding bug is marked "closed" now).
Marking this as approved, since it should be in.

review: Approve

Subscribers

People subscribed via source and target branches