Skip to content

MDEV-31053 UUID(size) should be disallowed #4287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DaveGosselin-MariaDB wants to merge 1 commit into
base: 10.11
Choose a base branch
from
Open

MDEV-31053 UUID(size) should be disallowed #4287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions plugin/type_inet/mysql-test/type_inet/type_inet4.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2017,3 +2017,12 @@ DROP TABLE t1;
#
# End of 10.10 tests
#
#
# Begin 10.11 tests
#
#
# MDEV-31053 UUID(size) should be disallowed
#
CREATE TABLE t1 (a INET4(24));
ERROR HY000: Type 'inet4' does not accept length nor precision values
# End 10.11 tests
12 changes: 12 additions & 0 deletions plugin/type_inet/mysql-test/type_inet/type_inet4.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1479,3 +1479,15 @@ DROP TABLE t1;
--echo #
--echo # End of 10.10 tests
--echo #

--echo #
--echo # Begin 10.11 tests
--echo #

--echo #
--echo # MDEV-31053 UUID(size) should be disallowed
--echo #
--error ER_NO_LENGTH_PRECISION_ALLOWED
CREATE TABLE t1 (a INET4(24));

--echo # End 10.11 tests
19 changes: 19 additions & 0 deletions plugin/type_inet/sql_type_inet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@ class Inet6: public FixedBinTypeStorage<IN6_ADDR_SIZE, IN6_ADDR_MAX_CHAR_LENGTH>
return !only_zero_bytes(m_buffer, sizeof(m_buffer));
}
static const Name &default_value();
static bool allowed(const Lex_field_type_st &attr,
const Name& name)
{
/*
Inet6 support allows length at least, for the case

@vuvova vuvova Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? what does it mean?

@DaveGosselin-MariaDB DaveGosselin-MariaDB Sep 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

CREATE TABLE t1 (id int NOT NULL PRIMARY KEY, a INET6(6) DEFAULT '::10');
*/
return false; // no error, is allowed
}
};


Expand All @@ -59,6 +68,16 @@ class Inet4: public FixedBinTypeStorage<IN_ADDR_SIZE, IN_ADDR_MAX_CHAR_LENGTH>
bool ascii_to_fbt(const char *str, size_t str_length);
size_t to_string(char *dst, size_t dstsize) const;
static const Name &default_value();
static bool allowed(const Lex_field_type_st &attr,
const Name& name)
{
if (attr.length() || attr.dec()) {
my_error(ER_NO_LENGTH_PRECISION_ALLOWED, MYF(0), name.ptr());
return true;
}

return false; // no error, is allowed
}
};

typedef Type_handler_fbt<Inet4> Type_handler_inet4;
Expand Down
9 changes: 9 additions & 0 deletions plugin/type_uuid/mysql-test/type_uuid/type_uuid.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3202,3 +3202,12 @@ Warnings:
Warning 1292 Incorrect uuid value: ''
Warning 1292 Incorrect uuid value: ''
DROP TABLE t1;
#
# Begin 10.11 tests
#
#
# MDEV-31053 UUID(size) should be disallowed
#
CREATE TABLE t1 (a UUID(24));
ERROR HY000: Type 'uuid' does not accept length nor precision values
# End 10.11 tests
12 changes: 12 additions & 0 deletions plugin/type_uuid/mysql-test/type_uuid/type_uuid.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1697,3 +1697,15 @@ INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000');
SELECT * FROM t1 WHERE a IN ('','00000000-0000-0000-0000-000000000001');
SELECT * FROM t1 WHERE a='';
DROP TABLE t1;

--echo #
--echo # Begin 10.11 tests
--echo #

--echo #
--echo # MDEV-31053 UUID(size) should be disallowed
--echo #
--error ER_NO_LENGTH_PRECISION_ALLOWED
CREATE TABLE t1 (a UUID(24));

--echo # End 10.11 tests
10 changes: 10 additions & 0 deletions plugin/type_uuid/sql_type_uuid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,16 @@ class UUID: public FixedBinTypeStorage<MY_UUID_SIZE, MY_UUID_STRING_LENGTH>
return rc;
}

static bool allowed(const Lex_field_type_st &attr,
const Name& name)
{
if (attr.length() || attr.dec()) {
my_error(ER_NO_LENGTH_PRECISION_ALLOWED, MYF(0), name.ptr());
return true;
}

return false; // no error, is allowed
}
};

class Type_collection_uuid: public Type_collection
Expand Down
2 changes: 2 additions & 0 deletions sql/share/errmsg-utf8.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10758,3 +10758,5 @@ ER_CM_OPTION_MISSING_REQUIREMENT
eng "CHANGE MASTER TO option '%s=%s' is missing requirement %s"
ER_SLAVE_STATEMENT_TIMEOUT 70100
eng "Slave log event execution was interrupted (slave_max_statement_time exceeded)"
ER_NO_LENGTH_PRECISION_ALLOWED
eng "Type '%s' does not accept length nor precision values"

@vuvova vuvova Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 DaveGosselin-MariaDB Sep 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try it out 👍

12 changes: 12 additions & 0 deletions sql/sql_type_fixedbin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,18 @@ class Type_handler_fbt: public Type_handler
const Record_addr tmp(NULL, Bit_addr(true));
return new (table->in_use->mem_root) Field_fbt(&empty_clex_str, tmp);
}

bool Column_definition_set_attributes(THD *thd,
Column_definition *def,
const Lex_field_type_st &attr,
column_definition_type_t type) const override
{
if (FbtImpl::allowed(attr, singleton()->name()))
return true;
return Type_handler::Column_definition_set_attributes(thd, def,
attr, type);

@vuvova vuvova Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DaveGosselin-MariaDB DaveGosselin-MariaDB Sep 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Fix attributes after the parser
bool Column_definition_fix_attributes(Column_definition *c) const override
{
Expand Down