Hi Bar, The logic of the fix looks sound to me. Before you push, I would like you to address one comment. This first suggestion is optional: diff --git a/mysql-test/suite/innodb/t/innodb-online-alter-gis.test
b/mysql-test/suite/innodb/t/innodb-online-alter-gis.test index 64d07ba..3f39750 100644 --- a/mysql-test/suite/innodb/t/innodb-online-alter-gis.test +++ b/mysql-test/suite/innodb/t/innodb-online-alter-gis.test @@ -19,3 +19,13 @@ ALTER ONLINE TABLE t1 ADD PRIMARY KEY(a),DROP INDEX d, LOCK=SHARED; show warnings; show errors; drop table t1; + +--echo # +--echo # MDEV-14038 ALTER TABLE does not exit on error with InnoDB + bad default function +--echo # + +CREATE OR REPLACE TABLE t1 (a INT) ENGINE=InnoDB; +--error ER_TRUNCATED_WRONG_VALUE_FOR_FIELD +ALTER TABLE t1 ADD COLUMN b LINESTRING DEFAULT POINT(1,1); +DESCRIBE t1; +DROP TABLE t1;
Consider using CREATE TABLE instead of CREATE OR REPLACE TABLE. The table will not pre-exist here because of the preceding DROP TABLE t1. diff --git a/storage/innobase/handler/handler0alter.cc
b/storage/innobase/handler/handler0alter.cc index 084fc80..6a6295e 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -1069,8 +1069,9 @@ ha_innobase::check_if_supported_inplace_alter(
/* Compute the DEFAULT values of non-constant columns (VCOL_SESSION_FUNC | VCOL_TIME_FUNC). */ - (*af)->set_default(); - goto next_column; + int rc= (*af)->set_default(); + if (rc == 0 || rc == 3) + goto next_column; }
DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
In the InnoDB coding style, {} braces are mandatory after "if" and "while". Would it be possible to change the return type of Field::set_default() to an enum, and to use switch here? It is not clear what the other possible error return values are, or what the code 3 means. I tried to search, and I did not find any symbolic name that could be used in place of the 3. It looks like some date or time conversions could return 3 when the data is truncated. Maybe this truncation is considered minor compared to the original issue (clamping 1000 to 127). I would suggest the following: switch ((*af)->set_default()) { case 0: /* No error */ case 3: /* Date or time precision loss */ goto next_column; } My suggested comment after "case 3:" is a guess. Feel free to adjust it. Best regards, Marko