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)
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
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@stripped, 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@stripped, 2007-10-08 17:12:53+04:00, anozdrin@station. +57 -0
> Add a test case for BUG#24923.
>
> scripts/mysql_system_tables.sql@stripped, 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@stripped, 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;
>
>
