- developers - lists.mariadb.org

Review of MDEV-6017 Add support for Indexes on Expressions
by Michael Widenius 03 Dec '24

03 Dec '24

Hi! Here is the review for MDEV-6017 Add support for Indexes on Expressions commit c1e7574c799163f5d8a26ca8e0f91af6c91f7077 Instead of adding bool function virtual bool vcol_subst_analyzer(uchar **) { return false; } Why not add Item_base_t flag: IS_SIMPLE_BOOL_FUNC This takes one bit instead of increasing ALL created item base structures with 8 bytes. As we have 100+ items, adding a virtual function increases code space with 1K or more + the functions. You can also replace vcol_subst_analyzer() with a top level function: bool Item::vcol_subst_analyzer/( { return (type() == Item::FUNC_ITEM && (((Item_func*) this)->bitmap_bit() & (BITMAP_EQ | BITMAP_EQUAL | BITMAP_LT | BITMAP_GT | BITMAP_LE | BITMAP_GE | BITMAP_BETWEEN | BITMAP_IN | BITMAP_ISNULL | BITMAP_ISNOTNULL ) } For this to work, you would have to add the last two bitmaps to enum Bitmap in item_func.h @@ -3272,6 +3292,7 @@ class Item_cond :public Item_bool_func bool excl_dep_on_table(table_map tab_map) override; bool excl_dep_on_grouping_fields(st_select_lex *sel) override; + bool vcol_subst_analyzer(uchar **) override { return true; } I assume you need the above to be able to traverse AND and OR expressions for WHERE. Please add a comment that this is the intent. If you decide to use the bitmaps, add an explanation why AND and OR is in the vcol_subst_analyzer list. ---- +void collect_indexed_vcols_for_table(TABLE *tbl, List<Field> *vcol_fields) +{ Please use table instead of tbl. We use table as a varible much more than tbl: ((/my/maria-10.5/sql)) grep tbl *.* | wc 1143 5371 73297 ((/my/maria-10.5/sql)) grep table *.* | wc 40886 236754 2832202 + for (uint i=0; i < tbl->s->keys; i++) + { + // note: we could also support histograms here + if (!tbl->keys_in_use_for_query.is_set(i)) + continue; I wonder if it would be a good idea to extend bitmap to have an iterator like we have for tables where we only loop over the set bits. From sql_bitmap.h: class Table_map_iterator { ulonglong bmp; public: Table_map_iterator(ulonglong t): bmp(t){} uint next_bit() { if (!bmp) return BITMAP_END; uint bit= my_find_first_bit(bmp); bmp &= ~(1ULL << bit); return bit; } int operator++(int) { return next_bit(); } enum { BITMAP_END= 64 }; }; Would be quite trival to do also for keys. This would help tables with many keys and also would allows us to remove the 'call' to if (!tbl->keys_in_use_for_query.is_set(i)) We could also do an optimized version where we implement bitmap_get_next_set() where we do not have to reset the previous bits for each call. --------- + if (field->vcol_info) + vcol_fields->push_back(field); You have to take care of error handling here. Please change the functions collect_indexed_vcols_for_table(), collect_indexed_vcols_for_join() and other functions that calls push_back() to bool and return 1 in case of error. + item->top_level_compile(ctx->thd, + &Item::vcol_subst_analyzer, &yes, + &Item::vcol_subst_transformer, (uchar*)ctx); You also have to check the return of the above for errors. Another option is to just check thd->fatal_error() as this will be set in case of OOM. .... + { + if (auto nested_join= table->nested_join) + subst_vcols_in_join_list(ctx, &nested_join->join_list); PLEASE do not use auto. I want to know the type while reading and debugging the code! auto can also introduce bugs if a type changes. + @todo: this cannot handle VIEW columns currently. View columns are wrapped + in Item_direct_view_ref objects which do not comsider themselves equal to + Item_field objects they're wrapping. Please add this also to the commit comment. + /* + Do not do the substitution if the datatypes do not match. + Virtual column's datatype can be different from the expression, for + example: + + col3 INT AS (CONCAT(col1, col2)) + + Do not allow substitutions that change the semantics of comparison. + At the moment, we require that datatypes are the same. + This probably could be relaxed. + + For strings: we allow two cases: + - vcol_expr and vcol_field have the same collation. + - vcol_field has the same collation as the comparison collation. + + (Note: MySQL calls resolve_type() after it has done the substitution. + This can potentially update the comparator. The idea is that this + shouldn't be necessary as we do not want to change the comparator. + Changing the comparator will change the semantics of the condition, + our point is that this must not happen) + */ Move to a function comment. It is hard to read read the function code with this long comment in the middle. + if (vcol_expr->type_handler_for_comparison() != + vcol_field->type_handler_for_comparison() || + vcol_expr->maybe_null() != vcol_field->maybe_null()) + fail_cause="type mismatch"; Why cannot we handle this case if vcol_expr->maybe_null is 0 and vcol_field->maybe_null() is set? In this case we know that the vcol_field's we are going to encounter can never be null, which should satisfy our requirements. + Item_field *itf= new (thd->mem_root) Item_field(thd, vcol_field); + if (!itf) + return false; Better to return true in case of errors, like we do in most of the other code. Remove and a blank line instead + //Item *const_expr; ... remove + //const_expr= args[0]; ----- +Item* Item_func_in::vcol_subst_transformer(THD *thd, uchar *arg) +{ + Vcol_subst_context *ctx= (Vcol_subst_context*)arg; + if (!compatible_types_scalar_bisection_possible()) Add comment /* Check that all arguments inside IN() are constants */ Regards, Monty

1 0

Re: 5b6b23563bf: MDEV-34979 generate SBOM from server builds
by Sergei Golubchik 02 Dec '24

02 Dec '24

Hi, Vladislav, Thanks! See comments below On Dec 02, Vladislav Vaintroub wrote: > revision-id: 5b6b23563bf (mariadb-11.7.1-5-g5b6b23563bf) > parent(s): eff9c198e32 > author: Vladislav Vaintroub > committer: Vladislav Vaintroub > timestamp: 2024-11-25 19:04:18 +0100 > message: > > MDEV-34979 generate SBOM from server builds > > This commit adds the capability to generate a Software Bill of Materials > (SBOM) from server builds. > > It introduces a new WITH_SBOM variable, which defaults to ON for package > builds (i.e if BUILD_CONFIG is used) and to OFF otherwise. > > When enabled, the build process will produce an sbom.json document in > CycloneDX format, capturing information about various dependencies, > which is gathered from various sources. > > We use git submodule information and CMake external projects properties > to gather version information for 3rd party code, but also handle > dependencies if external code is part of our repository > (zlib, or Connect storage engine's minizip) > > The SBOM document is stored in the root build directory in sbom.json file, > but is not currently installed. > > diff --git a/CMakeLists.txt b/CMakeLists.txt > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -581,6 +581,13 @@ ENDIF() > > INCLUDE(build_depends) > > +INCLUDE(CMakeDependentOption) > +CMAKE_DEPENDENT_OPTION(WITH_SBOM "Generate Software Bill of Materials (SBOM)" OFF "BUILD_CONFIG" ON) > +IF(WITH_SBOM) > + INCLUDE(generate_sbom) > + GENERATE_SBOM() > +ENDIF() This doesn't work for me, I mean, cmake -DBUILD_CONFIG=mysql_release . does not generate the file, but cmake -DBUILD_CONFIG=mysql_release . -DWITH_SBOM=ON does. May be it'd be more robust to use OPTION() and set it from cmake/build_configuration/mysql_release.cmake? Like it's done for other options. > + > INCLUDE(CPack) > > IF(WIN32 AND SIGNCODE) > diff --git a/cmake/generate_submodule_info.cmake b/cmake/generate_submodule_info.cmake > --- /dev/null > +++ b/cmake/generate_submodule_info.cmake > @@ -0,0 +1,73 @@ ... > + FOREACH(s ${out_list}) > + IF(${counter} EQUAL 0) > + IF (NOT("${s}" MATCHES "Entering '")) does it depend on the locale? > + MESSAGE(FATAL "Unexpected output ${outvar}") > + ENDIF() > + STRING(LENGTH "${s}" slen) > + MATH(EXPR substr_len "${slen} - 11") ... > diff --git a/cmake/generate_sbom.cmake b/cmake/generate_sbom.cmake > --- /dev/null > +++ b/cmake/generate_sbom.cmake > @@ -0,0 +1,240 @@ > +INCLUDE(generate_submodule_info) > +INCLUDE(ExternalProject) > + > +# Remove all matches from a cmake list > +FUNCTION (REMOVE_ALL_MATCHES list_var_name match_str) > + SET(new_list) > + FOREACH(el ${${list_var_name}}) > + IF(NOT("${el}" MATCHES "${match_str}")) > + LIST(APPEND new_list ${el}) > + ENDIF() > + ENDFOREACH() > + SET(${list_var_name} ${new_list} PARENT_SCOPE) > +ENDFUNCTION() you could've used LIST(FILTER ...) instead. (but not LIST(REMOVE_ITEM ...) which is 3.16+) > + > +# Extract user name and repository name from a github URL. > +FUNCTION (EXTRACT_REPO_NAME_AND_USER repo_url repo_name_var repo_user_var) > + IF(repo_url MATCHES "^git@") > + # normalize to https-style URLs > + STRING(REGEX REPLACE "^git@([^:]+):(.*)$" "https://\\1/\\2" repo_url "${repo_url}") I don't think we have that, is it even tested? :) note, I'm not saying it isn't needed, better to have it than not. > + ENDIF() > + # Extract the repository user > + STRING(REGEX REPLACE "https://([^/]+)/([^/]+)/.*" "\\2" repo_user "${repo_url}") > + > + STRING(REGEX REPLACE ".*/([^/]*)$" "\\1" repo_name "${repo_url}") > + STRING(REGEX REPLACE "\\.git$" "" repo_name "${repo_name}") > + > + SET(${repo_name_var} ${repo_name} PARENT_SCOPE) > + SET(${repo_user_var} ${repo_user} PARENT_SCOPE) > +ENDFUNCTION() > + > +# Add a known 3rd party dependency for SBOM generation > +# Currently used for "vendored" (part of our repository) source code we know about > +# such as zlib, as well ExternalProject_Add() projects > +MACRO(ADD_THIRD_PARTY_DEPENDENCY name url tag rev version description) > + LIST(FIND ALL_THIRD_PARTY ${name} idx) > + IF (idx GREATER -1) > + MESSAGE(FATAL_ERROR "${name} is already in ALL_THIRD_PARTY") > + ENDIF() > + SET(${name}_URL ${url}) > + SET(${name}_TAG ${tag}) > + SET(${name}_REVISION ${rev}) > + SET(${name}_DESCRIPTION "${description}") > + SET(${name}_VERSION "${version}") > + LIST(APPEND ALL_THIRD_PARTY ${name}) > +ENDMACRO() > + > +# Match third party component with supplier > +# CyclonDX documentation says it is > +# "The organization that supplied the component. > +# The supplier may often be the manufacturer, but may also be a distributor or repackager." > +# > +# Perhaps it can always be "MariaDB", but security team recommendation is different > +# more towards "author" > +FUNCTION (sbom_get_supplier repo_name repo_user varname) why a function? just curious > + IF("${repo_name_SUPPLIER}") > + SET(${varname} "${repo_name_SUPPLIER}" PARENT_SCOPE) ... > + # It is possible to provide EXTRA_SBOM_DEPENDENCIES > + # and accompanying per-dependency data, to extend generared sbom > + # document. > + # Example below injects an extra "ncurses" dependency using several > + # command line parameters for CMake. > + # -DEXTRA_SBOM_DEPENDENCIES=ncurses > + # -Dncurses_URL=https://github.com/mirror/ncurses > + # -Dncurses_TAG=v6.4 > + # -Dncurses_VERSION=6.4 > + # -Dncurses_DESCRIPTION="A fake extra dependency" > + SET(ALL_THIRD_PARTY ${ALL_SUBMODULES} ${EXTRA_SBOM_DEPENDENCIES}) okay. Though I personally wouldn't use ALL_SUBMODULES and EXTRA_SBOM_DEPENDENCIES and instead would scan GET_CMAKE_PROPERTY(VARS VARIABLES) for *_TAG or something > + > + IF(TARGET ha_connect OR TARGET connect) > + ADD_THIRD_PARTY_DEPENDENCY(minizip > + "https://github.com/zlib-ng/minizip-ng" > + "" 252588f "" > + "Vendored minizip, inside connect storage engine, storage/connect/zip.c et al") did you verify that our file matches the content of the file from that repo at that commit? also, a though - how can we prevent our minigzip.c from being modified without updaing the dependency? Suggestion, check the commit hash, like git log -n1 --format=%h storage/connect/zip.c > + ENDIF() > + > + IF(TARGET libfmt) > + ExternalProject_GET_PROPERTY(libfmt URL) > + STRING(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" libmt_TAG "${URL}") > + ADD_THIRD_PARTY_DEPENDENCY(libfmt > + "https://github.com/fmtlib/fmt" > + "${libmt_TAG}" "${libmt_TAG}" "${libmt_TAG}" > + "header only library, used in server") > + ENDIF() I wonder, wouldn't it be simpler and more robust to overwrite or wrap ExternalProject() to do the above automatically? (using the same name or a different one like MyExternalProject) ... Also, I noticed that the server entry gets "purl": "pkg:github//@5b6b23563bf" perhaps it should have a repo name here? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 534dda1234b: MDEV-35522: MariaDB Audit does not detect all DCLs forms when masking password
by Sergei Golubchik 02 Dec '24

02 Dec '24

Hi, Oleksandr, Ok to push. But do you no longer delete server_audit.log after the test? On Dec 02, Oleksandr Byelkin wrote: > revision-id: 534dda1234b (mariadb-10.5.27-64-g534dda1234b) > parent(s): fdb6db6b47f > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2024-11-29 11:45:01 +0100 > message: > > MDEV-35522: MariaDB Audit does not detect all DCLs forms when masking password > > 1. skip OR REPLACE > (to make it filter_query_type made recursive) > > 2. skip SET STATEMENT ... FOR before checking > statements with passwords > > diff --git a/mysql-test/suite/plugins/t/server_audit_pwd_mask.test b/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > --- a/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > +++ b/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > --echo # cleaunup > +DROP SERVER s1; > DROP USER u1; > DROP USER u2; > set global server_audit_logging=off; > ---remove_file $SEARCH_FILE > +##--remove_file $SEARCH_FILE why? > --disable_warnings > UNINSTALL PLUGIN ed25519; > UNINSTALL PLUGIN server_audit; > --enable_warnings Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: ff3d06f6601: MDEV-22217 Make OS character sets "utf8" and "utf-8" map to MariaDB character set "utf8mb4"
by Sergei Golubchik 16 Nov '24

16 Nov '24

Hi, Alexander, Of course, there can be no comments about the code change here. But the question is, is it something we want to do? 1. Is utf8/utf-8 locale indeed 4-byte utf8? 2. Why is hard-coding to utf8mb4 better than having it dependent on UTF8_IS_UTF8MB3 ? On Nov 16, Alexander Barkov wrote: > revision-id: ff3d06f6601 (mariadb-11.6.1-44-gff3d06f6601) > parent(s): 4b6922a315f > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-10-28 14:11:40 +0400 > message: > > MDEV-22217 Make OS character sets "utf8" and "utf-8" map to MariaDB character set "utf8mb4" > > diff --git a/mysql-test/main/mysql_locale_posix.test b/mysql-test/main/mysql_locale_posix.test > index c047b7b1ae9..b37d04baca2 100644 > --- a/mysql-test/main/mysql_locale_posix.test > +++ b/mysql-test/main/mysql_locale_posix.test > @@ -203,3 +203,20 @@ DROP DATABASE `Ã³`; > + > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_client" > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_connection" > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_results" may be do it in one SELECT? or in SHOW VARIABLES LIKE 'character_set%' ? to have it a tiny bit faster? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

2024 Q4 release part 2 (11.6.2, 11.7,1)
by Daniel Black 12 Nov '24

12 Nov '24

All, The 11.6.2 and 11.7.1 releases are now built: * 11.6.2 d8dad8c3b54cd09fefce7bc3b9749f427eed9709 (GA) * 11.7.1 74331a48dd3dadfe5041f3440b81a7183e9fd4a8 release notes still work in progress: * https://mariadb.com/kb/en/mariadb-11-6-2-release-notes/ * 11.7.1 still being prepared but will be 11.6.2 like + the features of 11.7.0 release notes + Vector search, + bugs fixes since 11.7.0. Both are rolling releases and once GA, like 11.6.2, will be the last of the release series. containers for testing are available: * quay.io/mariadb-foundation/mariadb-devel:11.6 * quay.io/mariadb-foundation/mariadb-devel:11.7 Build packages for RPM/Deb distros repo/source files: https://ci.mariadb.org/ * 11.6.2 - bb-11.6-release-latest* * 11.7.1 - bb-11.7-release-latest* Sources: * https://ci.mariadb.org/51755/ (11.6.2) * https://ci.mariadb.org/51770/ (11.7.1) Now begins the process of loading packages to mirrors and Docker Official Images releases.

1 0

Contribute to MariaDB compatibility in open source projects?
by Robert Silén 06 Nov '24

06 Nov '24

All, We have been working on community outreach as described in an earlier blog Improving MariaDB support in open source projects <https://mariadb.org/improving-mariadb-support-in-open-source-projects/>. Many projects have made efforts to improve their MariaDB compatibility with or without our help, and also documented compatibility more visibly in READMEs and documentation. However, we at the Foundation don't have the knowhow or resources to help out everybody, so I thought I'd share a list of some issues. Would somebody like to contribute to any of them? Any comment on the issues, or code contributions to them would be much appreciated. Thank you. Do share any thoughts or actions on this. Robert Silén, community advocacy at MariaDB Foundation robert.silen(a)mariadb.org - https://github.com/SOCI/soci/issues/1150: add MariaDB to README - https://github.com/tinode/chat/pull/924: Help set up CI for MariaDB (and MySQL) - https://github.com/inspircd/inspircd-docs/issues/239: tls and srv not available in MariaDB - https://github.com/umami-software/umami/discussions/2943: BIN_TO_UUID() not available in MariaDB (MDEV-15854 <https://jira.mariadb.org/browse/MDEV-15854>) - https://github.com/diesel-rs/diesel/issues/1882: Create a distinct backend for MariaDB - https://github.com/grafana/grafana: Create a dedicated DataStore for MariaDB - https://github.com/drizzle-team/drizzle-orm/pull/1692: WIP: Add support for Maria DB (core API implementation) - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1079753: Create mariadb specific dbconfig-mariadb - https://core.trac.wordpress.org/ticket/60037: Fix version checking in next release - https://gitlab.com/zoph-project/zoph/-/issues/267: give a thumbs up to their own addition of MariaDB to documentation (preferably recommend MariaDB)

2 1

Re: e8021aaf28e: auth: refactor MYSQL_AUTHENTICATION_INTERFACE_VERSION naming
by Sergei Golubchik 28 Oct '24

28 Oct '24

Hi, Nikita, This looks very confusing, this is not how versions work. What are you trying to achieve by that? On Oct 07, Nikita Malyavin wrote: > revision-id: e8021aaf28e (mariadb-11.6.1-9-ge8021aaf28e) > parent(s): a5b80531fbb > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-10-04 00:16:17 +0200 > message: > > auth: refactor MYSQL_AUTHENTICATION_INTERFACE_VERSION naming > > Rename MYSQL_AUTHENTICATION_INTERFACE_VERSION to > MYSQL_AUTH_INTERFACE_VERSION_2_02. > This is the current latest version, and it will be fixed to all auth plugins > for current implementation. > MYSQL_AUTHENTICATION_INTERFACE_VERSION will stay for compatibility with 3-rd > party auth plugins and will be fixed to V. 2.02. > Add MYSQL_AUTHENTICATION_INTERFACE_VERSION_LAST that will be updated as needed. > Add MYSQL_AUTH_INTERFACE_MAJOR_MASK to identify major version. > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 5

Re: b17308e254a: MDEV-34854 Parsec sends garbage when using an empty password
by Sergei Golubchik 28 Oct '24

28 Oct '24

Hi, Nikita, On Oct 07, Nikita Malyavin wrote: > revision-id: b17308e254a (mariadb-11.6.1-10-gb17308e254a) > parent(s): e8021aaf28e > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-10-04 01:33:47 +0200 > message: > > MDEV-34854 Parsec sends garbage when using an empty password > > When an empty password is set, the server doesn't call > st_mysql_auth::hash_password and leaves > MYSQL_SERVER_AUTH_INFO::auth_string empty. > > Fix: > generate hashes for empty passwords as well. This breaks some auth > plugins, so we increment interface version and do it only from Auth V. > 2.03. > > Some empty passwords could be already stored with no though. The user "with no though" ? > will have to call SET PASSWORD once again, anyway the authentication > wouldn't have worked for such password. ok, I presume you mean ed25519 only, because mysql_native_password worked with an empty password and it generates an empty hash for it. > diff --git a/mysql-test/suite/plugins/r/parsec.result b/mysql-test/suite/plugins/r/parsec.result > index 512c066e2d7..b7e3537af29 100644 > --- a/mysql-test/suite/plugins/r/parsec.result > +++ b/mysql-test/suite/plugins/r/parsec.result ... let's add ed25519 test too, for completeness. > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 2722ea2ea19..ba05a5656c5 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2402,7 +2402,10 @@ static int set_user_auth(THD *thd, const LEX_CSTRING &user, > res= ER_NOT_VALID_PASSWORD; > goto end; > } > - if (pwtext.length) > + > + // Starting from version 2.03 we also generate hash for empty passwords. > + if ((info->interface_version >= MYSQL_AUTH_INTERFACE_VERSION_2_03 I don't understand this MYSQL_AUTH_INTERFACE_VERSION_2_03 thing. First, again, that's not how a version is supposed to work. Second, this empty-password change isn't a change in the API. You can simply start calling hash_password() for empty passwords and it'll just work. I've tried :) > + && pwtext.str) || pwtext.length) > { > if (info->hash_password) > { Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

2024 Q4 release
by Daniel Black 27 Oct '24

27 Oct '24

All, Just letting you know that we're in the progress of getting the releases ready. The issue where this is tracked is: * https://jira.mariadb.org/browse/MDEV-35030 Branches being released are at the tips of these branches: bb-10.5-release (LTS) bb-10.6-release (LTS) bb-10.11-release (LTS) bb-11.2-release bb-11.4-release (LTS) bb-11.6-release bb-11.7-release So far the code is merged up to bb-11.4-release. Later branches will be updated, check history before using. There are two blocking bugs on bb-11.2-release onwards affecting compilation of the columnstore and connect storage engine. In addition to compiling the source yourselves; Containers: quay.ion/mariadb-foundation/mariadb-devel:{release} list: https://quay.io/repository/mariadb-foundation/mariadb-devel?tab=tags note: * 11.4 is from the 11.4 branch rather than bb-11.4-release, * 11.2 didn't build due to blocker bugs). Repos for testing: https://ci.mariadb.org/ contains for deb and rpm distros a repo file in the form: bb-{version}-release-latest-{arch}-{distro}-{version}-{deb|rpm}-autobake.{sources|repo} Please report any serious regressions with a Blocker priority bug, and mark this issue blocking MDEV-35030.

1 0

Re: 692ca003f8c: MDEV-33658 2/2 Cannot add a foreign key on a table with a matching long UNIQUE
by Sergei Golubchik 23 Oct '24

23 Oct '24

Hi, Nikita, On Oct 18, Nikita Malyavin wrote: > revision-id: 692ca003f8c (mariadb-10.5.26-169-g692ca003f8c) > parent(s): ba988513ec7 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-10-13 19:33:14 +0200 > message: > > MDEV-33658 2/2 Cannot add a foreign key on a table with a matching long UNIQUE > > Cannot add a foreign key on a table with a long UNIQUE multi-column index, that > contains a foreign key as a prefix. > > Check that index algorithms match during the "generated" keys duplicate removal. > > diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test > index 2f85328bda3..08b49b6c5da 100644 > --- a/mysql-test/main/long_unique_bugs.test > +++ b/mysql-test/main/long_unique_bugs.test > @@ -706,4 +706,19 @@ alter table t1 enable keys; > insert into t1 values (2); > drop table t1; > > +--echo # > +--echo # Cannot add a foreign key on a table with a long UNIQUE multi-column > +--echo # index, that contains a foreign key as a prefix. > +--echo # > +create table a (id int(19) primary key) engine = innodb; > +create table b (id int primary key, > + long_text varchar(1000), > + constraint unique_b unique key (id, long_text) > + ) engine = innodb default charset utf8mb4; > + > +alter table b add constraint b_fk_ida foreign key (id) references a (id); > +show create table b; > +drop table b; > +drop table a; Two problems with this test case. A minor one - it doesn't start with the --echo # MDEV-33658 Cannot add a foreign key on a table with a matching long UNIQUE line. Please add it. A more serious problem - it doesn't fail on a vanilla 10.5. The original set of SQL commands from MDEV-33658 fails with "Foreign key constraint is incorrectly formed", but your test doesn't. You might've oversimplified it. Please, fix the test case to fail on vanilla 10.5 with the above error. > + > --echo # End of 10.5 tests > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 99ba9e3587b..3d01b9d5584 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -203,7 +203,7 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root) > > /* > Test if a foreign key (= generated key) is a prefix of the given key > - (ignoring key name, key type and order of columns) > + (ignoring key name, key type and order of columns, but minding the algorithm) it doesn't ignore the order of columns and "key type" is basically algorithm, so the above can simply be "(ignoring key name)" > > NOTES: > This is only used to test if an index for a FOREIGN KEY exists > @@ -218,6 +218,17 @@ Foreign_key::Foreign_key(const Foreign_key &rhs, MEM_ROOT *mem_root) > > bool is_foreign_key_prefix(Key *a, Key *b) > { > + ha_key_alg a_alg= a->key_create_info.algorithm; > + ha_key_alg b_alg= b->key_create_info.algorithm; > + > + bool is_a_btree= a_alg == HA_KEY_ALG_BTREE || a_alg == HA_KEY_ALG_UNDEF; > + bool is_b_btree= b_alg == HA_KEY_ALG_BTREE || b_alg == HA_KEY_ALG_UNDEF; > + > + // Either the algorithms are total match, or they both are btree > + // in order to be able to prefix > + if (a_alg != b_alg && !(is_a_btree && is_b_btree)) > + return false; I don't see why algorithms can be not b-tree, if they match. What use case did you have in mind? > + > /* Ensure that 'a' is the generated key */ > if (a->generated) > { > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1