Re: bk commit into 5.1 tree (anozdrin:1.2575) BUG#24923

Hi Alik

The following should be addressed before pushing this patch:

1)

The result for the test sp.test is missing, I can't see what the actual result is

2)

The test is incomplete:
- a SHOW CREATE FUNCTION would be great, to verify that the function was stored properly

• invoking a function that returns NULL is not testing enough: returning actual values of the enum (say, 1 call to return the first value, 1 other call to return the last value) would verify that the enum values themselves are not truncated.

3)

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

The database installation scripts have changed for CREATE TABLE ... mysql.proc,
and this will fix the bug for a new 5.1 installation.

What about an upgrade from 5.0 to 5.1 ?
I expect to see at some point, an ALTER TABLE mysql.proc ...

Has the upgrade scenario be considered ?

4)

According to the CSET comments, a truncation does not cause CREATE FUNCTION to fail, it's only a warning. If the function code is not stored properly (hence my request for SHOW CREATE FUNCTION),
I think the CREATE should fail, not produce bad data

5)

This is minor, but I noticed this coding style for test scripts:

+--echo #
+--echo # Bug#20550.
+--echo #
+
+--echo
+
+--echo #
+--echo # - Prepare.
+--echo #
+
+--echo
+
+--disable_warnings
+DROP FUNCTION IF EXISTS f1;
+--enable_warnings

Do you need more help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

Whether this looks good or not in the source code (the .test file), and in the result file (with banners) is a matter of personal taste, personally I don't like it but can live with it.

What concerns me much more is that this style, if propagated to the code base, has a pervert effect on merges:

In the code above, the fragment:

• start
  +--echo #
  +
  +--echo
  +
  +--echo #
  +--echo # - Prepare.
  +--echo #
  +
  +--echo
  +
  +--disable_warnings
  
• end

actually occupy 11 lines in the source code, while not providing any information what would allow a diff / merge tool to make the difference between this fragment in bug A and the same in bug B.

My concern is that, if we code like this, we will start having merge problems where
the wrong code is applied to the wrong place by the merge tool, since the context diffs are generated with far less that 11 lines of context,
and will get confused.

So, please don't use that style.

---

Thanks for looking into this.

Regards,
Marc



Alexander Nozdrin wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of alik. When alik does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see 
http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@1.2575, 2007-10-08 17:12:56+04:00, anozdrin@station. +3 -0
>   Fix for BUG#24923: Functions with ENUM issues.
>   
>   The problem was that the RETURNS column in the mysql.proc was of
>   CHAR(64). That was not enough for storing long-named datatypes.
>   
>   The fix is to change CHAR(64) to LONGBLOB, and to throw warnings
>   at the time a stored routine is created if some data is truncated
>   during writing into mysql.proc.
>
>   mysql-test/t/sp.test@1.260, 2007-10-08 17:12:53+04:00, anozdrin@station. +57 -0
>     Add a test case for BUG#24923.
>
>   scripts/mysql_system_tables.sql@1.16, 2007-10-08 17:12:53+04:00, anozdrin@station. +1 -1
>     Change the data type of column 'returns' from char(64) to longblob.
>
>   sql/sp.cc@1.171, 2007-10-08 17:12:53+04:00, anozdrin@station. +7 -0
>     Produce warnings if any data was truncated during writing
>     into mysql.proc.
>
> diff -Nrup a/mysql-test/t/sp.test b/mysql-test/t/sp.test
> --- a/mysql-test/t/sp.test	2007-10-08 02:02:44 +04:00
> +++ b/mysql-test/t/sp.test	2007-10-08 17:12:53 +04:00
> @@ -7665,4 +7665,61 @@ DROP VIEW v2;
>  
>   


-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com