maria-developers team mailing list archive

[Marko Mäkelä <marko.makela@xxxxxxxxxxx>]

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