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
=== modified file 'client/drizzle.cc'
--- client/drizzle.cc 2009-09-23 18:52:35 +0000
+++ client/drizzle.cc 2009-09-25 15:04:12 +0000
@@ -2578,7 +2578,7 @@
2578 {2578 {
2579 put_info(_("No connection. Trying to reconnect..."),INFO_INFO,0,0);2579 put_info(_("No connection. Trying to reconnect..."),INFO_INFO,0,0);
2580 (void) com_connect((string *)0, 0);2580 (void) com_connect((string *)0, 0);
2581 if (opt_rehash)2581 if (opt_rehash && connected)
2582 com_rehash(NULL, NULL);2582 com_rehash(NULL, NULL);
2583 }2583 }
2584 if (!connected)2584 if (!connected)
@@ -2602,8 +2602,8 @@
2602 drizzle_row_t row= drizzle_row_next(&res);2602 drizzle_row_t row= drizzle_row_next(&res);
2603 if (row[0])2603 if (row[0])
2604 current_db= strdup(row[0]);2604 current_db= strdup(row[0]);
2605 drizzle_result_free(&res);
2605 }2606 }
2606 drizzle_result_free(&res);
2607 }2607 }
2608}2608}
26092609
26102610
=== modified file 'drizzled/error.cc'
--- drizzled/error.cc 2009-06-26 06:25:57 +0000
+++ drizzled/error.cc 2009-09-25 15:04:12 +0000
@@ -1418,7 +1418,16 @@
14181418
1419const char * error_message(unsigned int code)1419const char * error_message(unsigned int code)
1420{1420{
1421 return drizzled_error_messages[code-ER_ERROR_FIRST];1421 /**
1422 if the connection is killed, code is 2
1423 See lp bug# 435619
1424 */
1425 if ((code > ER_ERROR_FIRST) )
1426 {
1427 return drizzled_error_messages[code-ER_ERROR_FIRST];
1428 }
1429 else
1430 return drizzled_error_messages[ER_UNKNOWN_ERROR - ER_ERROR_FIRST];
1422}1431}
14231432
14241433
14251434
=== modified file 'drizzled/session.cc'
--- drizzled/session.cc 2009-09-23 21:40:36 +0000
+++ drizzled/session.cc 2009-09-25 15:04:12 +0000
@@ -227,6 +227,8 @@
227 thread_id= 0;227 thread_id= 0;
228 file_id = 0;228 file_id = 0;
229 query_id= 0;229 query_id= 0;
230 query= NULL;
231 query_length= 0;
230 warn_id= 0;232 warn_id= 0;
231 memset(ha_data, 0, sizeof(ha_data));233 memset(ha_data, 0, sizeof(ha_data));
232 replication_data= 0;234 replication_data= 0;
233235
=== modified file 'drizzled/session.h'
--- drizzled/session.h 2009-09-22 07:35:28 +0000
+++ drizzled/session.h 2009-09-25 15:04:12 +0000
@@ -786,7 +786,10 @@
786 /** Returns the length of the current query text */786 /** Returns the length of the current query text */
787 inline size_t getQueryLength() const787 inline size_t getQueryLength() const
788 {788 {
789 return strlen(query);789 if (query != NULL)
790 return strlen(query);
791 else
792 return 0;
790 }793 }
791794
792 /** Accessor method returning the session's ID. */795 /** Accessor method returning the session's ID. */
793796
=== modified file 'drizzled/set_var.cc'
--- drizzled/set_var.cc 2009-08-13 21:55:13 +0000
+++ drizzled/set_var.cc 2009-09-25 15:04:12 +0000
@@ -196,7 +196,7 @@
196 false, fix_trans_mem_root);196 false, fix_trans_mem_root);
197static sys_var_session_uint32_t sys_trans_prealloc_size(&vars, "transaction_prealloc_size",197static sys_var_session_uint32_t sys_trans_prealloc_size(&vars, "transaction_prealloc_size",
198 &SV::trans_prealloc_size,198 &SV::trans_prealloc_size,
199 false, fix_trans_mem_root);199 false, fix_session_mem_root);
200200
201static sys_var_const_str_ptr sys_secure_file_priv(&vars, "secure_file_priv",201static sys_var_const_str_ptr sys_secure_file_priv(&vars, "secure_file_priv",
202 &opt_secure_file_priv);202 &opt_secure_file_priv);
@@ -471,16 +471,21 @@
471 return out;471 return out;
472}472}
473473
474static bool get_unsigned32(Session *, set_var *var)474static bool get_unsigned32(Session *session, set_var *var)
475{475{
476 if (var->value->unsigned_flag)476 if (var->value->unsigned_flag)
477 var->save_result.uint32_t_value= (uint32_t) var->value->val_int();477 var->save_result.uint32_t_value=
478 static_cast<uint32_t>(var->value->val_int());
478 else479 else
479 {480 {
480 int64_t v= var->value->val_int();481 int64_t v= var->value->val_int();
481 var->save_result.uint32_t_value= (uint32_t) ((v < 0) ? 0 : v);482 if (v > UINT32_MAX)
483 throw_bounds_warning(session, true, true,var->var->getName().c_str(), v);
484
485 var->save_result.uint32_t_value=
486 static_cast<uint32_t>((v > UINT32_MAX) ? UINT32_MAX : (v < 0) ? 0 : v);
482 }487 }
483 return 0;488 return false;
484}489}
485490
486static bool get_unsigned64(Session *, set_var *var)491static bool get_unsigned64(Session *, set_var *var)
@@ -743,7 +748,10 @@
743 uint64_t tmp= var->save_result.uint64_t_value;748 uint64_t tmp= var->save_result.uint64_t_value;
744749
745 if (tmp > max_system_variables.*offset)750 if (tmp > max_system_variables.*offset)
751 {
752 throw_bounds_warning(session, true, true, getName(), (int64_t) tmp);
746 tmp= max_system_variables.*offset;753 tmp= max_system_variables.*offset;
754 }
747755
748 if (option_limits)756 if (option_limits)
749 tmp= fix_unsigned(session, tmp, option_limits);757 tmp= fix_unsigned(session, tmp, option_limits);
750758
=== modified file 'tests/suite/big/r/variables-big.result'
--- tests/suite/big/r/variables-big.result 2009-07-31 15:28:05 +0000
+++ tests/suite/big/r/variables-big.result 2009-09-25 15:04:12 +0000
@@ -10,3 +10,15 @@
10show processlist;10show processlist;
11Id User Host db Command Time State Info11Id User Host db Command Time State Info
12# root 127.0.0.1 test Query 0 NULL show processlist12# root 127.0.0.1 test Query 0 NULL show processlist
13set session transaction_prealloc_size=1024*1024*1024*4;
14Warnings:
15Error 1292 Truncated incorrect transaction_prealloc_size value: '4294967296'
16show processlist;
17Id User Host db Command Time State Info
18# root 127.0.0.1 test Query 0 NULL show processlist
19set session transaction_prealloc_size=1024*1024*1024*5;
20Warnings:
21Error 1292 Truncated incorrect transaction_prealloc_size value: '5368709120'
22show processlist;
23Id User Host db Command Time State Info
24# root 127.0.0.1 test Query 0 NULL show processlist
1325
=== modified file 'tests/suite/big/t/variables-big.test'
--- tests/suite/big/t/variables-big.test 2009-07-31 15:28:05 +0000
+++ tests/suite/big/t/variables-big.test 2009-09-25 15:04:12 +0000
@@ -18,7 +18,9 @@
1818
19# Drizzle bug #37782619# Drizzle bug #377826
20# please uncomment after the bug is fixed20# please uncomment after the bug is fixed
21#set session transaction_prealloc_size=1024*1024*1024*4;21set session transaction_prealloc_size=1024*1024*1024*4;
22#show processlist;22--replace_column 1 #
23#set session transaction_prealloc_size=1024*1024*1024*5;23show processlist;
24#show processlist;24set session transaction_prealloc_size=1024*1024*1024*5;
25--replace_column 1 #
26show processlist;
2527
=== modified file 'tests/t/variables.test'
--- tests/t/variables.test 2009-06-16 00:46:22 +0000
+++ tests/t/variables.test 2009-09-25 15:04:12 +0000
@@ -407,14 +407,19 @@
407#407#
408# Bug #10351: Setting ulong variable to > MAX_ULONG fails on 32-bit platform408# Bug #10351: Setting ulong variable to > MAX_ULONG fails on 32-bit platform
409#409#
410410--disable_query_log
411select VARIABLE_VALUE from information_schema.GLOBAL_VARIABLES where VARIABLE_NAME like "VERSION_COMPILE_MACHINE" into @arch;
412--enable_query_log
413if(`select strcmp(@arch,"i386")`)
414{
415 --disable_warnings
416}
411set @@max_heap_table_size= 4294967296;417set @@max_heap_table_size= 4294967296;
412select @@max_heap_table_size > 0;418select @@max_heap_table_size > 0;
413set global max_heap_table_size= 4294967296;419set global max_heap_table_size= 4294967296;
414select @@max_heap_table_size > 0;420select @@max_heap_table_size > 0;
415set @@max_heap_table_size= 4294967296;421set @@max_heap_table_size= 4294967296;
416select @@max_heap_table_size > 0;422select @@max_heap_table_size > 0;
417
418#423#
419# Bug #11775 Variable character_set_system does not exist (sometimes)424# Bug #11775 Variable character_set_system does not exist (sometimes)
420#425#