Hi Pete. Leaving aside for a moment the architectural decisions
(which I'll ask about internally), here's my review of the patch. I
have a few notes below, and an embarrassingly few of them are about
your code.
On 9 Mar 2007, at 14:14, Pete Harlan wrote:
> diff -ur bk.orig/sql/mysql_priv.h bk.pete/sql/mysql_priv.h
> --- bk.orig/sql/mysql_priv.h 2007-03-05 02:50:56.000000000 -0800
> +++ bk.pete/sql/mysql_priv.h 2007-03-08 15:14:47.000000000 -0800
> @@ -410,6 +410,7 @@
> #define MODE_NO_AUTO_CREATE_USER (MODE_TRADITIONAL*2)
> #define MODE_HIGH_NOT_PRECEDENCE (MODE_NO_AUTO_CREATE_USER*2)
> #define MODE_NO_ENGINE_SUBSTITUTION (MODE_HIGH_NOT_PRECEDENCE*2)
> +#define MODE_EMPTY_DEFAULT_FOR_BLOB (MODE_NO_ENGINE_SUBSTITUTION*2)
Grr, I'm not happy with those definitions, but that's not your fault,
Pete. Just don't take that as an example of what we think is good!
I'd like to change all those to (ULL(1) << number) eventually.
> /*
> Replication uses 8 bytes to store SQL_MODE in the binary log.
> The day you
> use strictly more than 64 bits by adding one more define above,
> you should
> diff -ur bk.orig/sql/mysqld.cc bk.pete/sql/mysqld.cc
> --- bk.orig/sql/mysqld.cc 2007-03-08 04:27:37.000000000 -0800
> +++ bk.pete/sql/mysqld.cc 2007-03-08 15:14:47.000000000 -0800
> @@ -226,7 +226,7 @@
> "NO_ZERO_IN_DATE", "NO_ZERO_DATE", "ALLOW_INVALID_DATES",
> "ERROR_FOR_DIVISION_BY_ZERO",
> "TRADITIONAL", "NO_AUTO_CREATE_USER", "HIGH_NOT_PRECEDENCE",
> - "NO_ENGINE_SUBSTITUTION",
> + "NO_ENGINE_SUBSTITUTION", "EMPTY_DEFAULT_FOR_BLOB",
This and the next are very minor, but if you can avoid removing a
line and adding a line, then we're sure to avoid merging conflicts.
If the order matters not at all, then you can add
"EMPTY_DEFAULT_FOR_BLOB" on a line by itself, before the end. Thus,
no "-" lines in the patch.
> NullS
> };
> static const unsigned int sql_mode_names_len[]=
> @@ -261,7 +261,8 @@
> /*TRADITIONAL*/ 11,
> /*NO_AUTO_CREATE_USER*/ 19,
> /*HIGH_NOT_PRECEDENCE*/ 19,
> - /*NO_ENGINE_SUBSTITUTION*/ 22
> + /*NO_ENGINE_SUBSTITUTION*/ 22,
> + /*EMPTY_DEFAULT_FOR_BLOB*/ 22
(again)
Also not your fault, but it's terrible to have two different places
that rely on each other's order. Another place for me to add a "TODO
FIXME" note. :\
> };
> TYPELIB sql_mode_typelib= { array_elements(sql_mode_names)-1,"",
> sql_mode_names,
>
> diff -ur bk.orig/sql/sql_insert.cc bk.pete/sql/sql_insert.cc
> --- bk.orig/sql/sql_insert.cc 2007-02-28 23:47:46.000000000 -0800
> +++ bk.pete/sql/sql_insert.cc 2007-03-08 15:42:23.000000000 -0800
> @@ -1338,12 +1338,17 @@
> TABLE_LIST *table_list)
> {
> int err= 0;
> + bool allow_blob_default =
> + (thd->variables.sql_mode & MODE_EMPTY_DEFAULT_FOR_BLOB);
> for (Field **field=entry->field ; *field ; field++)
> {
> if ((*field)->query_id != thd->query_id &&
> ((*field)->flags & NO_DEFAULT_VALUE_FLAG) &&
> ((*field)->real_type() != FIELD_TYPE_ENUM))
> {
> + if (((*field)->real_type() == FIELD_TYPE_BLOB) &&
> allow_blob_default)
> + continue; /* Mode enabled that allows blobs to default to '' */
It's hard to tell from the diff output, but the indentation doesn't
look right. Use two space characters per level, according to our
standards.
> +
> bool view= FALSE;
> if (table_list)
> {
You're missing test cases that prove that it works (and that keep us
from accidentally breaking the feature in the future). "mysql-test/t/
type_blob.test" is probably a good place for it. Show behavior with
the mode off, and contrast it with the mode on. Be creative with how
to test it. E.g., insert NULLs into one table, and "INSERT INTO t1
SELECT null_fields FROM t2" -- does that bypass the your checks?
Show it.
We run "make test" at every update of the code, to prove that
everything works as we expect. You should run it also, to make sure
that all is well.
Please create a bug report (severity 4) with "patch" at the start of
the description and attach the updated patch. That's how we track
contributions and make sure that we don't lose them.
- chad
--
Chad Miller, Software Developer chad@stripped
MySQL Inc., www.mysql.com
Orlando, Florida, USA 13-20z, UTC-0500
Office: +1 408 213 6740 sip:6740@stripped
Attachment: [application/pgp-signature] This is a digitally signed message part PGP.sig
