MDEV-31053 UUID(size) should be disallowed

[DaveGosselin-MariaDB]

Emits an error when creating a table with either a UUID or ipv4 column having a nonzero length. Specifying lengths has no effect for these types:

  CREATE TABLE t1 (id UUID(24), ..., ip ipv4(4), ... );

ipv6 columns having nonzero lengths are permitted, for example in the case of

  CREATE TABLE t1 (a INET6(6) DEFAULT '::10');

Specifying length zero for uuid, ipv4 has the same behavior as specifying no explicit length. Since these types are plugins, the lengths cannot be rejected by the grammar at lexing; rather they are rejected at a later parsing step if they are nonzero.

[vuvova]

why? what does it mean?

[DaveGosselin-MariaDB]

From the code and our documentation, it has no meaning. Perhaps it did at one point in the past, for compatibility? The MySQL docs don't mention a specific inet6 type. Searching around online, it seems that varbinary(16) is recommended for storing ipv6 in MySQL. So I wonder if we allowed a length here to ease a transition from MySQL to MariaDB (i.e. only change the type, not the values in parens)?

[vuvova]

why do you need a new allowed() method when you can put the check into Column_definition_set_attributes() ?

[DaveGosselin-MariaDB]

I could've changed descendant types to implement their own Column_definition_set_attributes overrides, but the Type_handler_fbt class implements, and heavily depends on, a static singleton() method which always returns type Type_handler_fbt. Because that method, internally, declares a static Type_handler_fbt instance, any descendant types from Type_handler_fbt are not instantiated.

I tried changing this to be a non-static virtual method instead, but there's a lot that assumes it is a static method and that an object isn't needed. I attempted to supply a different implementation for the internals of the static singleton() method via template parameters and std::enable_if magic, but the simplest approach is to have an implied allowed method (via template contracts) which the compiler can check. Unfortunately, at least at this point in time and like many template classes, there isn't a way to implicitly document in the language that allowed should be implemented. This is true of the STL, too. We could document with a comment, but by "in the language" I mean in a way that the compiler can verify (like it does with virtual methods and their overrides).

With this approach, we don't need to care about singleton() and we can ask the FbtImpl instance if the length/precision should be allowed. In the cases of this patch, FbtImpl can be Inet4, Inet6, or UUID.

[vuvova]

dunno. A dedicated error, different from built-in types. Ideally plugin types should behave exactly the same as builtin types.

Perhaps you can move the check into the parser? Like

field_type_all_with_typedefs:
          field_type_all_builtin
          {
            Lex->map_data_type(Lex_ident_sys(), &($$= $1));
          }
        | udt_name
          {
          /* copied from the known_storage_engines rule */
             plugin_ref plugin;
            if (!(plugin= ha_resolve_by_name(thd, &$1, false)))
              my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str));
           /* check the options */
           if (plugin->...->check_options(0))
           {
              thd->parse_error(ER_SYNTAX_ERROR, ...);
              MYSQL_YYABORT;
            }
            if (unlikely(Lex->set_field_type_udt_or_typedef(&$$, $1, $2)))
              MYSQL_YYABORT;
          }
        | udt_name field_length_str
          {
          /* copied from the known_storage_engines rule */
             plugin_ref plugin;
            if (!(plugin= ha_resolve_by_name(thd, &$1, false)))
              my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str));
           /* check the options */
           if (plugin->...->check_options(1))
           {
              thd->parse_error(ER_SYNTAX_ERROR, ...);
              MYSQL_YYABORT;
            }
            if (unlikely(Lex->set_field_type_udt_or_typedef(&$$, $1, $2)))
              MYSQL_YYABORT;
          }
        | udt_name precision
          {
          /* copied from the known_storage_engines rule */
             plugin_ref plugin;
            if (!(plugin= ha_resolve_by_name(thd, &$1, false)))
              my_yyabort_error((ER_UNKNOWN_STORAGE_ENGINE, MYF(0), $1.str));
           /* check the options */
           if (plugin->...->check_options(2))
           {
              thd->parse_error(ER_SYNTAX_ERROR, ...);
              MYSQL_YYABORT;
            }
            if (unlikely(Lex->set_field_type_udt_or_typedef(&$$, $1, $2)))
              MYSQL_YYABORT;
          }
        ;

but with less code duplication

[DaveGosselin-MariaDB]

I'll try it out 👍