Drizzle

Merge lp://staging/~diego-fmpwizard/drizzle/bug-fixes into lp://staging/~drizzle-trunk/drizzle/development

  1. bug-fixes
  2. Merge into development
Proposed by fmpwizard
Status: Superseded
Proposed branch: lp://staging/~diego-fmpwizard/drizzle/bug-fixes
Merge into: lp://staging/~drizzle-trunk/drizzle/development
Diff against target: 187 lines
8 files modified
client/drizzle.cc (+2/-2)
drizzled/error.cc (+10/-1)
drizzled/session.cc (+2/-0)
drizzled/session.h (+4/-1)
drizzled/set_var.cc (+13/-5)
tests/suite/big/r/variables-big.result (+12/-0)
tests/suite/big/t/variables-big.test (+6/-4)
tests/t/variables.test (+7/-2)
To merge this branch: bzr merge lp://staging/~diego-fmpwizard/drizzle/bug-fixes
Reviewer Review Type Date Requested Status
Jay Pipes (community) Needs Fixing
Review via email: mp+12217@code.staging.launchpad.net

This proposal supersedes a proposal from 2009-09-15.

This proposal has been superseded by a proposal from 2009-09-25.

To post a comment you must log in.
Revision history for this message
fmpwizard (diego-fmpwizard) wrote : Posted in a previous version of this proposal

* Fixed bug* #377826 "Failure to detect error in allocating transcation_prealloc_size"

* I converted transaction_prealloc_size to a uint64_t.

That eliminated the strange values that ended up assigned to "transcation_prealloc_size" when you tried to set it to a value of 1024*1024*1024*4 or higher.

* To get a warning on those same cases, I had to add:

throw_bounds_warning(session, true, true, getName(), (int64_t) tmp);

to

set_var.cc:

bool sys_var_session_uint64_t::update(Session *session, set_var *var)

* I also enabled the test suite under big/t/variables-big.test
* Had to modify a test case for
    set @@max_heap_table_size= 4294967296;
  ** Because it now returns a warning about the value being truncated (truncated to 4294966272 ) (This is different than what MySQL does, MySQL accepts a value of 4294967296)

Revision history for this message
Monty Taylor (mordred) wrote : Posted in a previous version of this proposal

Looks great! Merging in to lp:drizzle/mordred

review: Approve
Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

Pulling into my captain branch now....

Revision history for this message
Monty Taylor (mordred) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jay Pipes wrote:
> Pulling into my captain branch now....

There are problems in this, actually. Specifically, you need to do:

=== modified file 'drizzled/session.h'
- --- drizzled/session.h 2009-09-06 06:18:00 +0000
+++ drizzled/session.h 2009-09-07 22:36:14 +0000
@@ -164,7 +164,7 @@
   uint32_t query_alloc_block_size;
   uint32_t query_prealloc_size;
   uint32_t trans_alloc_block_size;
- - uint64_t trans_prealloc_size;
+ uint32_t trans_prealloc_size;
   uint64_t group_concat_max_len;
   /* TODO: change this to my_thread_id - but have to fix set_var first */
   uint64_t pseudo_thread_id;

=== modified file 'drizzled/set_var.cc'
- --- drizzled/set_var.cc 2009-09-03 23:21:40 +0000
+++ drizzled/set_var.cc 2009-09-07 22:36:14 +0000
@@ -194,7 +194,7 @@
 static sys_var_session_uint32_t sys_trans_alloc_block_size(&vars,
"transaction_alloc_block_size",

&SV::trans_alloc_block_size,
                                                            false,
fix_trans_mem_root);
- -static sys_var_session_uint64_t sys_trans_prealloc_size(&vars,
"transaction_prealloc_size",
+static sys_var_session_uint32_t sys_trans_prealloc_size(&vars,
"transaction_prealloc_size",

&SV::trans_prealloc_size);

 static sys_var_const_str_ptr sys_secure_file_priv(&vars,
"secure_file_priv",
@@ -470,16 +470,22 @@
   return out;
 }

- -static bool get_unsigned32(Session *, set_var *var)
+static bool get_unsigned32(Session *session, set_var *var)
 {
   if (var->value->unsigned_flag)
- - var->save_result.uint32_t_value= (uint32_t) var->value->val_int();
+ var->save_result.uint32_t_value=
+ static_cast<uint32_t>(var->value->val_int());
   else
   {
     int64_t v= var->value->val_int();
- - var->save_result.uint32_t_value= (uint32_t) ((v < 0) ? 0 : v);
+ if (v > UINT32_MAX)
+ throw_bounds_warning(session, true, true,
var->var->getName().c_str(), v);
+
+ var->save_result.uint32_t_value=
+ static_cast<uint32_t>((v > UINT32_MAX) ? UINT32_MAX : (v < 0) ? 0
: v);
+
   }
- - return 0;
+ return false;
 }

 static bool get_unsigned64(Session *, set_var *var)

Otherwise performance on solaris DIES.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqugjUACgkQ2Jv7/VK1RgHNQQCgnp40a/QO7Ewo9qV9UlWXv7jv
SNAAnRWAqOgMFdfhiIxkj1HTMQoUGyut
=2Ipj
-----END PGP SIGNATURE-----

Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

After merging and applying that fix, I get this on Solaris and Linux 64:

main.variables [ fail ] --- ../tests/r/variables.result 2009-09-14 20:17:16.000000000 +0300
+++ ../tests/r/variables.reject 2009-09-14 21:36:31.000000000 +0300
@@ -544,20 +544,14 @@
 set @@global.error_count=1;
 ERROR HY000: Variable 'error_count' is a read only variable
 set @@max_heap_table_size= 4294967296;
-Warnings:
-Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
 select @@max_heap_table_size > 0;
 @@max_heap_table_size > 0
 1
 set global max_heap_table_size= 4294967296;
-Warnings:
-Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
 select @@max_heap_table_size > 0;
 @@max_heap_table_size > 0
 1
 set @@max_heap_table_size= 4294967296;
-Warnings:
-Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
 select @@max_heap_table_size > 0;
 @@max_heap_table_size > 0
 1

drizzletest: Result content mismatch

Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

Going into lp:~jaypipes/drizzle/captain-20090914-01 branch. Tests running now...

review: Approve
Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

:( Still getting error. Only occurs on MacOSX 64-bit:

main.variables [ fail ] --- ../tests/r/variables.result 2009-09-14 21:48:49.000000000 +0300
+++ ../tests/r/variables.reject 2009-09-15 18:08:59.000000000 +0300
@@ -544,14 +544,20 @@
 set @@global.error_count=1;
 ERROR HY000: Variable 'error_count' is a read only variable
 set @@max_heap_table_size= 4294967296;
+Warnings:
+Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
 select @@max_heap_table_size > 0;
 @@max_heap_table_size > 0
 1
 set global max_heap_table_size= 4294967296;
+Warnings:
+Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
 select @@max_heap_table_size > 0;
 @@max_heap_table_size > 0
 1
 set @@max_heap_table_size= 4294967296;
+Warnings:
+Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
 select @@max_heap_table_size > 0;
 @@max_heap_table_size > 0
 1

review: Needs Fixing
Revision history for this message
fmpwizard (diego-fmpwizard) wrote : Posted in a previous version of this proposal

> :( Still getting error. Only occurs on MacOSX 64-bit:

Hi Jay,

I logged in into hades to see what the problem was and I found out that drizzle was being built as 32-bit, instead of 64-bit

I tried to build it as a 64 bit but libdrizzle had to be recompiled as 64-bit as well (I could do this, but wanted to check first)

To force a 64-bit build on hades, you would need to run

$ ./config/autorun.sh && MACOSX_DEPLOYMENT_TARGET=10.5 \
CPPFLAGS="-arch x86_64" \
CFLAGS=" -arch x86_64 " \
CCFLAGS=" -arch x86_64 " \
CXXFLAGS=" -arch x86_64 " \
LDFLAGS=" -arch x86_64 " \
./configure \
--with-debug \
--with-lib-prefix=/opt/local \
--prefix=/Applications/mysql/enterprise/drizzle-local && make -jx

So, I see two solutions:
1) enable the "hack" on the test, so that it detects if drizzle is 32 or 64bit and enable warnings accordingly.
2) force hades to build 64 bit and we tell people that mac 32bit is not going to have all test passing. :(

I'd go for 1), unless anyone sees a better solution.

Thanks

Diego

>
> main.variables [ fail ] --- ../tests/r/variables.result
> 2009-09-14 21:48:49.000000000 +0300
> +++ ../tests/r/variables.reject 2009-09-15 18:08:59.000000000 +0300
> @@ -544,14 +544,20 @@
> set @@global.error_count=1;
> ERROR HY000: Variable 'error_count' is a read only variable
> set @@max_heap_table_size= 4294967296;
> +Warnings:
> +Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
> select @@max_heap_table_size > 0;
> @@max_heap_table_size > 0
> 1
> set global max_heap_table_size= 4294967296;
> +Warnings:
> +Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
> select @@max_heap_table_size > 0;
> @@max_heap_table_size > 0
> 1
> set @@max_heap_table_size= 4294967296;
> +Warnings:
> +Error 1292 Truncated incorrect max_heap_table_size value: '4294967296'
> select @@max_heap_table_size > 0;
> @@max_heap_table_size > 0
> 1

review: Needs Resubmitting
Revision history for this message
Jay Pipes (jaypipes) wrote :

All tests passing on hades, bitters, and linux64. Approved. This is now in my lp:~jaypipes/drizzle/captain-20090924-01 branch and will be proposed for merging shortly.

review: Approve
Revision history for this message
Jay Pipes (jaypipes) wrote :

Grrr, my bad. The variables.test case on hades is still failing.

Looks like you did not add that test hack back into the test case?

-jay

review: Needs Fixing
lp://staging/~diego-fmpwizard/drizzle/bug-fixes updated
1134. By Diego Medina <email address hidden>

* Fixed LP bug#435619
** Protect error_message(code) from retrieving a negative index which causes a crash

* When a new session starts, initialize query_length (this was another
crash on the logging_* plugins)(timing dependant)

1135. By Diego Medina <email address hidden>

Enabled a hack to account for 32-bit vs 64-bit tests results

Revision history for this message
fmpwizard (diego-fmpwizard) wrote :

Hack committed and the new merge proposal includes a fix for another
drizzle crash bug. I think launchpad finally added the option of
resubmitting the merge request all in one step. Let me know if tis was
not the case.

Thanks

  -Diego

On Thu, Sep 24, 2009 at 6:29 PM, Jay Pipes <email address hidden> wrote:
> Review: Needs Fixing
> Grrr, my bad.  The variables.test case on hades is still failing.
>
> Looks like you did not add that test hack back into the test case?
>
> -jay
> --
> https://code.launchpad.net/~diego-fmpwizard/drizzle/bug-fixes/+merge/12217
> You are the owner of lp:~diego-fmpwizard/drizzle/bug-fixes.
>

--
Diego Medina
Web Developer
http://www.fmpwizard.com

lp://staging/~diego-fmpwizard/drizzle/bug-fixes updated
1136. By Diego Medina <email address hidden>

finally fixed the 64-bit vs 32-bit test case

1137. By Diego Medina <email address hidden>

* Fixes bug#438852
** variables test fails as of build 1144 on linux 32 bit machines
** Some linux systems use i686 to indicate they are 32-bit

1138. By Diego Medina <email address hidden>

* On certain UPDATE and DELETE statements, drizzled failed an assert() in
   Diagnostic_area.
** Fixed by resetting m_status after "delete select"
* Fixed LP bug#439719
* Added test case

1139. By Diego Medina <email address hidden>

resolved small merge issue

1140. By Diego Medina <email address hidden>

uncommented a call to reset_diagnostics_area()

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'client/drizzle.cc'
2--- client/drizzle.cc 2009-09-23 18:52:35 +0000
3+++ client/drizzle.cc 2009-09-25 15:04:12 +0000
4@@ -2578,7 +2578,7 @@
5 {
6 put_info(_("No connection. Trying to reconnect..."),INFO_INFO,0,0);
7 (void) com_connect((string *)0, 0);
8- if (opt_rehash)
9+ if (opt_rehash && connected)
10 com_rehash(NULL, NULL);
11 }
12 if (!connected)
13@@ -2602,8 +2602,8 @@
14 drizzle_row_t row= drizzle_row_next(&res);
15 if (row[0])
16 current_db= strdup(row[0]);
17+ drizzle_result_free(&res);
18 }
19- drizzle_result_free(&res);
20 }
21 }
22
23
24=== modified file 'drizzled/error.cc'
25--- drizzled/error.cc 2009-06-26 06:25:57 +0000
26+++ drizzled/error.cc 2009-09-25 15:04:12 +0000
27@@ -1418,7 +1418,16 @@
28
29 const char * error_message(unsigned int code)
30 {
31- return drizzled_error_messages[code-ER_ERROR_FIRST];
32+ /**
33+ if the connection is killed, code is 2
34+ See lp bug# 435619
35+ */
36+ if ((code > ER_ERROR_FIRST) )
37+ {
38+ return drizzled_error_messages[code-ER_ERROR_FIRST];
39+ }
40+ else
41+ return drizzled_error_messages[ER_UNKNOWN_ERROR - ER_ERROR_FIRST];
42 }
43
44
45
46=== modified file 'drizzled/session.cc'
47--- drizzled/session.cc 2009-09-23 21:40:36 +0000
48+++ drizzled/session.cc 2009-09-25 15:04:12 +0000
49@@ -227,6 +227,8 @@
50 thread_id= 0;
51 file_id = 0;
52 query_id= 0;
53+ query= NULL;
54+ query_length= 0;
55 warn_id= 0;
56 memset(ha_data, 0, sizeof(ha_data));
57 replication_data= 0;
58
59=== modified file 'drizzled/session.h'
60--- drizzled/session.h 2009-09-22 07:35:28 +0000
61+++ drizzled/session.h 2009-09-25 15:04:12 +0000
62@@ -786,7 +786,10 @@
63 /** Returns the length of the current query text */
64 inline size_t getQueryLength() const
65 {
66- return strlen(query);
67+ if (query != NULL)
68+ return strlen(query);
69+ else
70+ return 0;
71 }
72
73 /** Accessor method returning the session's ID. */
74
75=== modified file 'drizzled/set_var.cc'
76--- drizzled/set_var.cc 2009-08-13 21:55:13 +0000
77+++ drizzled/set_var.cc 2009-09-25 15:04:12 +0000
78@@ -196,7 +196,7 @@
79 false, fix_trans_mem_root);
80 static sys_var_session_uint32_t sys_trans_prealloc_size(&vars, "transaction_prealloc_size",
81 &SV::trans_prealloc_size,
82- false, fix_trans_mem_root);
83+ false, fix_session_mem_root);
84
85 static sys_var_const_str_ptr sys_secure_file_priv(&vars, "secure_file_priv",
86 &opt_secure_file_priv);
87@@ -471,16 +471,21 @@
88 return out;
89 }
90
91-static bool get_unsigned32(Session *, set_var *var)
92+static bool get_unsigned32(Session *session, set_var *var)
93 {
94 if (var->value->unsigned_flag)
95- var->save_result.uint32_t_value= (uint32_t) var->value->val_int();
96+ var->save_result.uint32_t_value=
97+ static_cast<uint32_t>(var->value->val_int());
98 else
99 {
100 int64_t v= var->value->val_int();
101- var->save_result.uint32_t_value= (uint32_t) ((v < 0) ? 0 : v);
102+ if (v > UINT32_MAX)
103+ throw_bounds_warning(session, true, true,var->var->getName().c_str(), v);
104+
105+ var->save_result.uint32_t_value=
106+ static_cast<uint32_t>((v > UINT32_MAX) ? UINT32_MAX : (v < 0) ? 0 : v);
107 }
108- return 0;
109+ return false;
110 }
111
112 static bool get_unsigned64(Session *, set_var *var)
113@@ -743,7 +748,10 @@
114 uint64_t tmp= var->save_result.uint64_t_value;
115
116 if (tmp > max_system_variables.*offset)
117+ {
118+ throw_bounds_warning(session, true, true, getName(), (int64_t) tmp);
119 tmp= max_system_variables.*offset;
120+ }
121
122 if (option_limits)
123 tmp= fix_unsigned(session, tmp, option_limits);
124
125=== modified file 'tests/suite/big/r/variables-big.result'
126--- tests/suite/big/r/variables-big.result 2009-07-31 15:28:05 +0000
127+++ tests/suite/big/r/variables-big.result 2009-09-25 15:04:12 +0000
128@@ -10,3 +10,15 @@
129 show processlist;
130 Id User Host db Command Time State Info
131 # root 127.0.0.1 test Query 0 NULL show processlist
132+set session transaction_prealloc_size=1024*1024*1024*4;
133+Warnings:
134+Error 1292 Truncated incorrect transaction_prealloc_size value: '4294967296'
135+show processlist;
136+Id User Host db Command Time State Info
137+# root 127.0.0.1 test Query 0 NULL show processlist
138+set session transaction_prealloc_size=1024*1024*1024*5;
139+Warnings:
140+Error 1292 Truncated incorrect transaction_prealloc_size value: '5368709120'
141+show processlist;
142+Id User Host db Command Time State Info
143+# root 127.0.0.1 test Query 0 NULL show processlist
144
145=== modified file 'tests/suite/big/t/variables-big.test'
146--- tests/suite/big/t/variables-big.test 2009-07-31 15:28:05 +0000
147+++ tests/suite/big/t/variables-big.test 2009-09-25 15:04:12 +0000
148@@ -18,7 +18,9 @@
149
150 # Drizzle bug #377826
151 # please uncomment after the bug is fixed
152-#set session transaction_prealloc_size=1024*1024*1024*4;
153-#show processlist;
154-#set session transaction_prealloc_size=1024*1024*1024*5;
155-#show processlist;
156+set session transaction_prealloc_size=1024*1024*1024*4;
157+--replace_column 1 #
158+show processlist;
159+set session transaction_prealloc_size=1024*1024*1024*5;
160+--replace_column 1 #
161+show processlist;
162
163=== modified file 'tests/t/variables.test'
164--- tests/t/variables.test 2009-06-16 00:46:22 +0000
165+++ tests/t/variables.test 2009-09-25 15:04:12 +0000
166@@ -407,14 +407,19 @@
167 #
168 # Bug #10351: Setting ulong variable to > MAX_ULONG fails on 32-bit platform
169 #
170-
171+--disable_query_log
172+select VARIABLE_VALUE from information_schema.GLOBAL_VARIABLES where VARIABLE_NAME like "VERSION_COMPILE_MACHINE" into @arch;
173+--enable_query_log
174+if(`select strcmp(@arch,"i386")`)
175+{
176+ --disable_warnings
177+}
178 set @@max_heap_table_size= 4294967296;
179 select @@max_heap_table_size > 0;
180 set global max_heap_table_size= 4294967296;
181 select @@max_heap_table_size > 0;
182 set @@max_heap_table_size= 4294967296;
183 select @@max_heap_table_size > 0;
184-
185 #
186 # Bug #11775 Variable character_set_system does not exist (sometimes)
187 #