- developers - lists.mariadb.org

Re: [Maria-developers] 4b142536a24: MDEV-20604: Duplicate key value is silently truncated to 64 characters in print_keydup_error
by Sergei Golubchik 23 Mar '20

23 Mar '20

Hi, Oleksandr! On Mar 22, Oleksandr Byelkin wrote: > revision-id: 4b142536a24 (mariadb-10.2.31-56-g4b142536a24) > parent(s): ed21202a14e > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-03-18 08:25:13 +0100 > message: > > MDEV-20604: Duplicate key value is silently truncated to 64 characters in print_keydup_error > > Added indication of truncated string with special width/precission suffix ':'. The comment looks wrong :) > diff --git a/client/mysql_upgrade.c b/client/mysql_upgrade.c > index 0bbb8a149ff..003701514cf 100644 > --- a/client/mysql_upgrade.c > +++ b/client/mysql_upgrade.c > @@ -501,10 +499,10 @@ static void find_tool(char *tool_executable_name, const char *tool_name, > last_fn_libchar -= 6; > } > > - len= (int)(last_fn_libchar - self_name); > - > - my_snprintf(tool_executable_name, FN_REFLEN, "%.*s%c%s", > - len, self_name, FN_LIBCHAR, tool_name); > + last_fn_libchar[0]= 0; > + my_snprintf(tool_executable_name, FN_REFLEN, "%s%c%s", > + self_name, FN_LIBCHAR, tool_name); > + last_fn_libchar[0]= FN_LIBCHAR; Okay, but why not to use %b here? > } > > if (opt_verbose) > diff --git a/client/mysqltest.cc b/client/mysqltest.cc > index 60a203ccedd..045ab566fdb 100644 > --- a/client/mysqltest.cc > +++ b/client/mysqltest.cc > @@ -9534,6 +9533,7 @@ int main(int argc, char **argv) > case Q_LET: do_let(command); break; > case Q_EVAL_RESULT: > die("'eval_result' command is deprecated"); > + break; // never called but keep compiler calm wouldn't it be better to specify __attribute__ ((noreturn)) for die()? > case Q_EVAL: > case Q_EVALP: > case Q_QUERY_VERTICAL: > diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c > index ad3517e4252..9ae10f337cc 100644 > --- a/strings/my_vsnprintf.c > +++ b/strings/my_vsnprintf.c > @@ -28,7 +28,9 @@ > #define LENGTH_ARG 1 > #define WIDTH_ARG 2 > #define PREZERO_ARG 4 > -#define ESCAPED_ARG 8 > +#define ESCAPED_ARG 8 > +// Show truncation of string argument > +#define SH_TRUNC_ARG 16 you don't really need a flag, because process_str_arg should always do it. By the way, in your code it doesn't. It doesn't print dots for %M and for %1$s. > > typedef struct pos_arg_info ARGS_INFO; > typedef struct print_info PRINT_INFO; > @@ -198,18 +199,42 @@ static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end, > size_t width, char *par, uint print_type) > { > int well_formed_error; > - size_t plen, left_len= (size_t) (end - to) + 1; > + uint dots= 0; > + size_t plen, left_len= (size_t) (end - to) + 1, slen=0; > if (!par) > par = (char*) "(null)"; > > - plen= strnlen(par, width); > + plen= slen= strnlen(par, width + 1); > + if (plen > width) > + plen= width; > if (left_len <= plen) > plen = left_len - 1; > + if ((slen > plen) && (print_type & SH_TRUNC_ARG)) > + { > + if (plen < 3) > + { > + dots= plen; > + plen= 0; > + } > + else > + { > + plen-= 3; > + dots= 3; > + } > + } > + > plen= my_well_formed_length(cs, par, par + plen, width, &well_formed_error); > if (print_type & ESCAPED_ARG) > to= backtick_string(cs, to, end, par, plen, '`'); > else > to= strnmov(to,par,plen); > + > + if (dots) > + { > + for (; dots; dots--) > + *(to++)= '.'; > + *(to)= 0; > + } This should look strange when truncating a quoted string. See the test case below > return to; > } > > diff --git a/unittest/mysys/my_vsnprintf-t.c b/unittest/mysys/my_vsnprintf-t.c > index 6ba0a42cf7e..e8831f7c1f4 100644 > --- a/unittest/mysys/my_vsnprintf-t.c > +++ b/unittest/mysys/my_vsnprintf-t.c > @@ -99,12 +99,12 @@ int main(void) > test1("Width is ignored for strings <x> <y>", > "Width is ignored for strings <%04s> <%5s>", "x", "y"); > > - test1("Precision works for strings <abcde>", > + test1("Precision works for strings <ab...>", > "Precision works for strings <%.5s>", "abcdef!"); > > - test1("Flag '`' (backtick) works: `abcd` `op``q` (mysql extension)", This > - "Flag '`' (backtick) works: %`s %`.4s (mysql extension)", This > - "abcd", "op`qrst"); > + test1("Flag '`' (backtick) works: `abcd` `op``q`... (mysql extension)", > + "Flag '`' (backtick) works: %`s %`.7s (mysql extension)", > + "abcd", "op`qrstuuuuuuuuu"); Here, the test case with a quoted string. First, you print 10 characters instead of 7. Second, you put dots after the closing backtick, which looks confusing, the truncation happens before the backtick. Third, as you can see, you cannot use strlen() for a string length because there can be backticks inside the string. perhaps it would be simpler to move ... inside backtick_string() > > test1("Length modifiers work: 1 * -1 * 2 * 3", > "Length modifiers work: %d * %ld * %lld * %zd", 1, -1L, 2LL, (size_t)3); > @@ -125,7 +125,7 @@ int main(void) > test1("Asterisk '*' as a width works: < 4>", > "Asterisk '*' as a width works: <%*d>", 5, 4); > > - test1("Asterisk '*' as a precision works: <qwerty>", > + test1("Asterisk '*' as a precision works: <qwe...>", > "Asterisk '*' as a precision works: <%.*s>", 6, "qwertyuiop"); please add tests for ...truncation in %M and %1$s > > test1("Positional arguments for a width: < 4>", > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] cf70893ac9d: MDEV-21303 Make executables MariaDB named
by Sergei Golubchik 19 Mar '20

19 Mar '20

Hi, Rasmus! See my comments/questions below: On Mar 16, Rasmus Johansson wrote: > revision-id: cf70893ac9d (mariadb-10.5.0-391-gcf70893ac9d) > parent(s): 56402e84b5b > author: Rasmus Johansson <razze(a)iki.fi> > committer: Rasmus Johansson <razze(a)iki.fi> > timestamp: 2020-03-16 11:10:25 +0000 > message: > > MDEV-21303 Make executables MariaDB named > > To change all executables to have a mariadb name I had to: > - Do name changes in every CMakeLists.txt that produces executables > - CREATE_MARIADB_SYMLINK was removed and GET_SYMLINK added by Wlad to reuse the function in other places also > - The scripts/CMakeLists.txt could make use of GET_SYMLINK instead of introducing redundant code, but I thought I'll leave that for next release > - A lot of changes to debian/.install and debian/.links files due to swapping of real executable and symlink. I did not however change the name of the manpages, so the real name is still mysql there and mariadb are symlinks. > - The Windows part needed a change now when we made the executables mariadb -named. MSI (and ZIP) do not support symlinks and to not break backward compatibility we had to include mysql named binaries also. Done by Wlad > > diff --git a/cmake/mysql_add_executable.cmake b/cmake/mysql_add_executable.cmake > index eec370d51af..f4d71ae9cef 100644 > --- a/cmake/mysql_add_executable.cmake > +++ b/cmake/mysql_add_executable.cmake > @@ -48,7 +48,7 @@ FUNCTION (MYSQL_ADD_EXECUTABLE) > ENDIF() > > IF (ARG_WIN32) > - SET(WIN32 WIN32) > + SET(WIN32 ARG_WIN32) This looks wrong, see below > ELSE() > UNSET(WIN32) > ENDIF() > @@ -62,6 +62,7 @@ FUNCTION (MYSQL_ADD_EXECUTABLE) > ELSE() > UNSET(EXCLUDE_FROM_ALL) > ENDIF() > + > ADD_EXECUTABLE(${target} ${WIN32} ${MACOSX_BUNDLE} ${EXCLUDE_FROM_ALL} ${sources}) here it'll pass ARG_WIN32 to ADD_EXECUTABLE. But ADD_EXECUTABLE takes an optional WIN32 keyword, not ARG_WIN32. > > # tell CPack where to install > @@ -79,16 +80,49 @@ FUNCTION (MYSQL_ADD_EXECUTABLE) > IF (COMP MATCHES ${SKIP_COMPONENTS}) > RETURN() > ENDIF() > + > IF (WITH_STRIPPED_CLIENT AND NOT target STREQUAL mysqld) this should be `STREQUAL mariadbd` now, I believe. > INSTALL(CODE "SET(CMAKE_INSTALL_DO_STRIP 1)" COMPONENT ${COMP}) > SET(reset_strip ON) > ENDIF() > - MYSQL_INSTALL_TARGETS(${target} DESTINATION ${ARG_DESTINATION} COMPONENT ${COMP}) > + > + IF(NOT ${mariadbname} STREQUAL "") where is mariadbname set? > + MYSQL_INSTALL_TARGETS(${mariadbname} DESTINATION ${ARG_DESTINATION} COMPONENT ${COMP}) > + ELSE() > + MYSQL_INSTALL_TARGETS(${target} DESTINATION ${ARG_DESTINATION} COMPONENT ${COMP}) > + ENDIF() > + > IF (reset_strip) > INSTALL(CODE "SET(CMAKE_INSTALL_DO_STRIP 0)" COMPONENT ${COMP}) > ENDIF() > ENDIF() > > - # create mariadb named symlink > - CREATE_MARIADB_SYMLINK(${target} ${ARG_DESTINATION} ${COMP}) > + # create MySQL named "legacy links" > + > + # Windows note: > + # Here, hardlinks are used, because cmake can't install symlinks. > + # In packages, there are won't be links, just copies. better to put the comment below in the ELSE branch, not here, far from the code that it is supposed to explain. > + GET_SYMLINK(${target} link) > + IF(link) > + IF(UNIX) > + ADD_CUSTOM_COMMAND(TARGET ${target} POST_BUILD > + COMMAND ${CMAKE_COMMAND} -E create_symlink > + ${target} ${link} > + COMMENT "Creating ${link} link" > + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) > + INSTALL(PROGRAMS > + ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${link} > + DESTINATION > + ${ARG_DESTINATION} > + COMPONENT ${COMP}) > + ELSE() > + SET(link ${link}.exe) > + ADD_CUSTOM_COMMAND(TARGET ${target} POST_BUILD > + COMMAND cmake -E remove -f ${link} > + COMMAND mklink /H ${link} $<TARGET_FILE_NAME:${target}> > + COMMENT "Creating ${link} link" > + WORKING_DIRECTORY $<TARGET_FILE_DIR:${target}>) > + INSTALL(PROGRAMS $<TARGET_FILE_DIR:${target}>/${link} DESTINATION ${ARG_DESTINATION} COMPONENT ${COMP}) > + ENDIF() > + ENDIF() > ENDFUNCTION() > diff --git a/cmake/symlinks.cmake b/cmake/symlinks.cmake > index ec638bc82de..e040ff19f77 100644 > --- a/cmake/symlinks.cmake > +++ b/cmake/symlinks.cmake > @@ -9,68 +9,46 @@ macro(REGISTER_SYMLINK from to) > endmacro() > > # MariaDB names for executables > -REGISTER_SYMLINK("mysql_client_test_embedded" "mariadb-client-test-embedded") > -REGISTER_SYMLINK("mysql_client_test" "mariadb-client-test") > +REGISTER_SYMLINK("mariadb-client-test-embedded" "mysql_client_test_embedded") > +REGISTER_SYMLINK("mariadb-client-test" "mysql_client_test") ... I suppose you need to make sure that everything works with new names, without symlinks. That is all internal tools and scripts invoke other tools and scripts by their mariadb* names. A way to do it could be to comment out all REGISTER_SYMLINK lines and fix whatever that will break. grep is also a useful tool here, but with rather low signal-to-noise ratio. > + > +MACRO(GET_SYMLINK name out) > + set(${out}) > + list(FIND MARIADB_SYMLINK_FROMS ${name} _index) > if (${_index} GREATER -1) > - list(GET MARIADB_SYMLINK_TOS ${_index} mariadbname) > - endif() > - > - if (mariadbname) > - CREATE_MARIADB_SYMLINK_IN_DIR(${src} ${mariadbname} ${dir} ${comp}) > - endif() > -endmacro(CREATE_MARIADB_SYMLINK) > - > -# Add MariaDB symlinks in directory > -macro(CREATE_MARIADB_SYMLINK_IN_DIR src dest dir comp) > - if(UNIX) > - add_custom_target( > - SYM_${dest} ALL > - DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/${dest} > - ) > - > - add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${dest} POST_BUILD > - COMMAND ${CMAKE_COMMAND} -E create_symlink ${src} ${dest} > - COMMENT "mklink ${src} -> ${dest}") > - > - install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${dest} DESTINATION ${dir} COMPONENT ${comp}) > + list(GET MARIADB_SYMLINK_TOS ${_index} ${out}) > endif() > -endmacro(CREATE_MARIADB_SYMLINK_IN_DIR) > +ENDMACRO() > diff --git a/debian/mariadb-backup.install b/debian/mariadb-backup.install > index 2bc61c12079..e1967495b00 100644 > --- a/debian/mariadb-backup.install > +++ b/debian/mariadb-backup.install > @@ -1,5 +1,4 @@ > -usr/bin/mariabackup > +usr/bin/mariadb-backup > usr/bin/mbstream > usr/share/man/man1/mariabackup.1 > -usr/share/man/man1/mariadb-backup.1 this is strange. Why did you remove mariadb-backup.1 and not mariabackup.1 ? > usr/share/man/man1/mbstream.1 > diff --git a/debian/mariadb-client-10.5.install b/debian/mariadb-client-10.5.install > index 67c0c2619c3..216e4015851 100644 > --- a/debian/mariadb-client-10.5.install > +++ b/debian/mariadb-client-10.5.install > @@ -1,29 +1,17 @@ > debian/additions/innotop/innotop usr/bin/ > debian/additions/mysqlreport usr/bin/ > +usr/bin/mariadb-access > +usr/bin/mariadb-admin > usr/bin/mariadb-conv > -usr/bin/mysql_find_rows > -usr/bin/mysql_fix_extensions > -usr/bin/mysql_waitpid > -usr/bin/mysqlaccess > -usr/bin/mysqladmin > -usr/bin/mysqldump > -usr/bin/mysqldumpslow > -usr/bin/mysqlimport > -usr/bin/mysqlshow you removed mysqlshow here, and added mariadb-show to the server package. Intentional? UPD: I see that before your patch mysqlshow was in the client package, while mariadb-show symlink was in the server package. It's a bug, both should be in the client package. > +usr/bin/mariadb-dump > +usr/bin/mariadb-dumpslow > +usr/bin/mariadb-find-rows > +usr/bin/mariadb-fix-extensions > +usr/bin/mariadb-import > +usr/bin/mariadb-slap > +usr/bin/mariadb-waitpid > usr/bin/mysqlslap you did not remove mysqlslap, but added mariadb-slap > usr/bin/mytop > -usr/share/man/man1/mariadb-access.1 > -usr/share/man/man1/mariadb-admin.1 > -usr/share/man/man1/mariadb-binlog.1 > -usr/share/man/man1/mariadb-conv.1 > -usr/share/man/man1/mariadb-dump.1 > -usr/share/man/man1/mariadb-dumpslow.1 > -usr/share/man/man1/mariadb-find-rows.1 > -usr/share/man/man1/mariadb-fix-extensions.1 > -usr/share/man/man1/mariadb-import.1 > -usr/share/man/man1/mariadb-plugin.1 > -usr/share/man/man1/mariadb-slap.1 > -usr/share/man/man1/mariadb-waitpid.1 same weirdness as elsewhere, mariadb*.1 man pages are symlinks to mysql*.1 manpages. Should be vice versa. > usr/share/man/man1/mysql_find_rows.1 > usr/share/man/man1/mysql_fix_extensions.1 > usr/share/man/man1/mysql_waitpid.1 > diff --git a/debian/mariadb-client-10.5.links b/debian/mariadb-client-10.5.links > index 5d966575b76..d1bdfcfa3c8 100644 > --- a/debian/mariadb-client-10.5.links > +++ b/debian/mariadb-client-10.5.links > @@ -1,21 +1,19 @@ > -usr/bin/mysql_find_rows usr/bin/mariadb-find-rows > -usr/bin/mysql_fix_extensions usr/bin/mariadb-fix-extensions > -usr/bin/mysql_plugin usr/bin/mariadb-plugin > -usr/bin/mysql_waitpid usr/bin/mariadb-waitpid > -usr/bin/mysqlaccess usr/bin/mariadb-access > -usr/bin/mysqladmin usr/bin/mariadb-admin > -usr/bin/mysqlbinlog usr/bin/mariadb-binlog you removed mysqlbinlog, but added mariadb-binlog to the server package. Intentional? same for mysql_plugin/mariadb-plugin. UPD: mysqlbinlog was in the server package, while mariadb-binlog symlink was in the client package. So your change is correct, both will be in the server package now. Good. same for mysql_plugin/mariadb-plugin. > -usr/bin/mysqlcheck usr/bin/mariadb-analyze > -usr/bin/mysqlcheck usr/bin/mariadb-optimize > diff --git a/debian/mariadb-plugin-rocksdb.install b/debian/mariadb-plugin-rocksdb.install > index 80987612c30..0fc868a0721 100644 > --- a/debian/mariadb-plugin-rocksdb.install > +++ b/debian/mariadb-plugin-rocksdb.install > @@ -2,6 +2,5 @@ etc/mysql/conf.d/rocksdb.cnf etc/mysql/mariadb.conf.d > usr/bin/myrocks_hotbackup > usr/bin/mysql_ldb you didn't replace mysql_ldb with mariadb-ldb? > usr/lib/mysql/plugin/ha_rocksdb.so > -usr/share/man/man1/mariadb-ldb.1 > usr/share/man/man1/myrocks_hotbackup.1 > usr/share/man/man1/mysql_ldb.1 > diff --git a/debian/mariadb-server-10.5.install b/debian/mariadb-server-10.5.install > index 4a860ff57df..38d9f984276 100644 > --- a/debian/mariadb-server-10.5.install > +++ b/debian/mariadb-server-10.5.install > @@ -14,22 +14,23 @@ usr/bin/aria_pack > usr/bin/aria_read_log > usr/bin/galera_new_cluster > usr/bin/galera_recovery > +usr/bin/mariadb-binlog > +usr/bin/mariadb-convert-table-format > +usr/bin/mariadb-hotcopy > +usr/bin/mariadb-plugin > +usr/bin/mariadb-secure-installation > usr/bin/mariadb-service-convert > +usr/bin/mariadb-setpermission > +usr/bin/mariadb-show > +usr/bin/mariadb-tzinfo-to-sql > +usr/bin/mariadbd-multi > +usr/bin/mariadbd-safe > +usr/bin/mariadbd-safe duplicate line ^^^ > usr/bin/msql2mysql > usr/bin/myisam_ftdump > usr/bin/myisamchk > usr/bin/myisamlog > usr/bin/myisampack > -usr/bin/mysql_convert_table_format > -usr/bin/mysql_plugin > -usr/bin/mysql_secure_installation > -usr/bin/mysql_setpermission > -usr/bin/mysql_tzinfo_to_sql > -usr/bin/mysqlbinlog > -usr/bin/mysqld_multi > -usr/bin/mysqld_safe > -usr/bin/mysqld_safe_helper you don't install mariadbd-safe-helper. Perhaps that's where your duplicate line comes from, it's not a duplicate, it's truncated :) > -usr/bin/mysqlhotcopy > usr/bin/perror > usr/bin/replace > usr/bin/resolve_stack_dump > diff --git a/debian/mariadb-server-10.5.links b/debian/mariadb-server-10.5.links > index f2d97460371..e3a2d68541a 100644 > --- a/debian/mariadb-server-10.5.links > +++ b/debian/mariadb-server-10.5.links > @@ -1,9 +1,20 @@ > -usr/bin/mysql_convert_table_format usr/bin/mariadb-convert-table-format > -usr/bin/mysql_secure_installation usr/bin/mariadb-secure-installation > -usr/bin/mysql_setpermission usr/bin/mariadb-setpermission > -usr/bin/mysql_tzinfo_to_sql usr/bin/mariadb-tzinfo-to-sql > -usr/bin/mysqld_multi usr/bin/mariadbd-multi > -usr/bin/mysqld_safe usr/bin/mariadbd-safe > -usr/bin/mysqld_safe_helper usr/bin/mariadbd-safe-helper > -usr/bin/mysqlhotcopy usr/bin/mariadb-hotcopy > -usr/bin/mysqlshow usr/bin/mariadb-show > +usr/bin/mariadb-binlog usr/bin/mysqlbinlog > +usr/bin/mariadb-convert-table-format usr/bin/mysql_convert_table_format > +usr/bin/mariadb-hotcopy usr/bin/mysqlhotcopy > +usr/bin/mariadb-plugin usr/bin/mysql_plugin > +usr/bin/mariadb-secure-installation usr/bin/mysql_secure_installation > +usr/bin/mariadb-setpermission usr/bin/mysql_setpermission > +usr/bin/mariadb-show /usr/bin/mysqlshow this is the only line in your whole patch where you symlink by the absolute path. Looks like a typo. > +usr/bin/mariadb-tzinfo-to-sql usr/bin/mysql_tzinfo_to_sql > +usr/bin/mariadbd-multi usr/bin/mysqld_multi > +usr/bin/mariadbd-safe usr/bin/mysqld_safe > +usr/bin/mariadbd-safe-helper usr/bin/mysqld_safe_helper > diff --git a/libmariadb b/libmariadb > index 3be5897c334..ca68b114ba7 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit 3be5897c3346639fa6d7195480d93108798c4917 > +Subproject commit ca68b114ba7e882e8abe8888204d675c4d8751f8 This is clearly wrong, please make sure you've removed it from your commit before pushing! > diff --git a/scripts/CMakeLists.txt b/scripts/CMakeLists.txt > index 7be46ac1985..350fbd14cf6 100644 > --- a/scripts/CMakeLists.txt > +++ b/scripts/CMakeLists.txt > @@ -244,8 +232,20 @@ SET(mysqlaccess_COMPONENT COMPONENT Client) > SET(mysql_find_rows_COMPONENT COMPONENT Client) > SET(mytop_COMPONENT Mytop) > > +MACRO(INSTALL_LINK old new destination component) > + EXECUTE_PROCESS( > + COMMAND ${CMAKE_COMMAND} -E create_symlink ${old} ${new} > + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} > + ) > + INSTALL( > + PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${new} > + DESTINATION ${destination} > + COMPONENT ${component} > + ) > +ENDMACRO() > + > IF(WIN32) > - # On Windows, some .sh and some .pl.in files are configured > + # On Windows, some .sh and some .pl.in files are configured > # The resulting files will have .pl extension (those are perl scripts) > > # Input files with pl.in extension > @@ -296,60 +296,73 @@ ELSE() > # On Unix, most of the files end up in the bin directory > SET(BIN_SCRIPTS > msql2mysql > - mysql_config > - mysql_setpermission > - mysql_secure_installation > - mysqlaccess > - mysql_convert_table_format > - mysql_find_rows > + mariadb-setpermission > + mariadb-secure-installation > + mariadb-access > + mariadb-convert-table-format > + mariadb-find-rows > mytop > - mysqlhotcopy > + mariadb-hotcopy > + mariadb-install-db > ${SERVER_SCRIPTS} > ${WSREP_SCRIPTS} > ${SYSTEMD_SCRIPTS} > ) > + > FOREACH(file ${BIN_SCRIPTS}) > - IF(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${file}.sh) > - CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/${file}.sh > - ${CMAKE_CURRENT_BINARY_DIR}/${file} ESCAPE_QUOTES @ONLY) > - ELSEIF(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${file}) > - CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/${file} > - ${CMAKE_CURRENT_BINARY_DIR}/${file} COPYONLY) > - ELSE() > - MESSAGE(FATAL_ERROR "Can not find ${file}.sh or ${file} in " > - "${CMAKE_CURRENT_SOURCE_DIR}" ) > - ENDIF() > - # TODO: The following EXECUTE could be redundant as INSTALL_SCRIPT > - # macro does an INSTALL(PROGRAMS ..) that automatically sets +x on > - # the executable. > - EXECUTE_PROCESS(COMMAND chmod +x ${CMAKE_CURRENT_BINARY_DIR}/${file}) > - IF(NOT ${file}_COMPONENT) > - SET(${file}_COMPONENT Server) > - ENDIF() > - INSTALL_SCRIPT( > - ${CMAKE_CURRENT_BINARY_DIR}/${file} > - DESTINATION ${INSTALL_BINDIR} > - COMPONENT ${${file}_COMPONENT} > - ) > + # set name of executable > + list(FIND MARIADB_SYMLINK_FROMS ${file} _index) > + > + if(${_index} GREATER -1) > + list(GET MARIADB_SYMLINK_TOS ${_index} binname) > + else() > + set(binname ${file}) > + endif() 1. why are you not using GET_SYMLINK ? 2. Why INSTALL_SYMLINK here instead of the old CREATE_MARIADB_SYMLINK ? You avoid creating new targets for script symlinks, but who cares about that? > + > + if(NOT "${binname}" STREQUAL "") how can ${binname} be "" ? > + IF(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${binname}.sh) > + CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/${binname}.sh > + ${CMAKE_CURRENT_BINARY_DIR}/${file} ESCAPE_QUOTES @ONLY) > + ELSEIF(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${binname}) > + CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/${binname} > + ${CMAKE_CURRENT_BINARY_DIR}/${file} COPYONLY) > + ELSE() > + MESSAGE(FATAL_ERROR "Can not find ${binname}.sh or ${binname} in " > + "${CMAKE_CURRENT_SOURCE_DIR}" ) > + ENDIF() > + > + IF(NOT ${file}_COMPONENT) > + SET(${file}_COMPONENT Server) > + ENDIF() > + INSTALL_SCRIPT( > + ${CMAKE_CURRENT_BINARY_DIR}/${file} > + DESTINATION ${INSTALL_BINDIR} > + COMPONENT ${${file}_COMPONENT} > + ) > + > + # Create symlink > + if (NOT ${binname} STREQUAL ${file}) > + INSTALL_LINK(${file} ${binname} ${INSTALL_BINDIR} ${${file}_COMPONENT}) > + > + # Place mysql_install_db symlink also in scripts > + if (${binname} STREQUAL "mysql_install_db") > + SET(bindir ${bindir1}) > + SET(prefix ${prefix1}) > + SET(sbindir ${sbindir1}) > + SET(scriptdir ${scriptdir1}) > + SET(libexecdir ${libexecdir1}) > + SET(pkgdatadir ${pkgdatadir1}) > + SET(pkgplugindir ${pkgplugindir1}) > + SET(localstatedir ${localstatedir1}) > + CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/mysql_install_db.sh ${CMAKE_CURRENT_BINARY_DIR}/mariadb-install-db ESCAPE_QUOTES @ONLY) > + INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${file} DESTINATION ${INSTALL_SCRIPTDIR} COMPONENT Server) > + INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${binname} DESTINATION ${INSTALL_SCRIPTDIR} COMPONENT Server) I don't understand that at all. 1. What does the comment "Place mysql_install_db symlink also in scripts" even mean? 2. you CONFIGURE_FILE mariadb-install-db twice. 3. Why two INSTALL(PROGRAM ? 4. You set all bindir/etc variables for mysql_install_db and it will affect CONFIGURE_FILE for all following scripts 5. why do you even compare ${binname} with mysql_install_db and not ${file} with mariadb-install-db ? > + endif() > + endif() > + endif() we customary always use uppercase in all cmake keywords. > ENDFOREACH() > > - SET (wsrep_sst_rsync_wan ${CMAKE_CURRENT_BINARY_DIR}/wsrep_sst_rsync_wan) > - ADD_CUSTOM_COMMAND( > - OUTPUT ${wsrep_sst_rsync_wan} > - COMMAND ${CMAKE_COMMAND} ARGS -E create_symlink > - wsrep_sst_rsync > - wsrep_sst_rsync_wan > - ) > - ADD_CUSTOM_TARGET(symlink_wsrep_sst_rsync > - ALL > - DEPENDS ${wsrep_sst_rsync_wan} > - ) > - INSTALL( > - FILES ${wsrep_sst_rsync_wan} > - DESTINATION ${INSTALL_BINDIR} > - COMPONENT Server > - ) > - > + INSTALL_LINK(wsrep_sst_rsync wsrep_sst_rsync_wan ${INSTALL_BINDIR} Server) > FOREACH(file ${WSREP_SOURCE}) > CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/${file}.sh > ${CMAKE_CURRENT_BINARY_DIR}/${file} ESCAPE_QUOTES @ONLY) > diff --git a/win/packaging/ca/CMakeLists.txt b/win/packaging/ca/CMakeLists.txt > index 99dc7da01fb..326bab47de4 100644 > --- a/win/packaging/ca/CMakeLists.txt > +++ b/win/packaging/ca/CMakeLists.txt I didn't review windows packaging, it's between you and wlad. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 5365db70cb8: Improve update handler (long unique keys on blobs)
by Sergei Golubchik 17 Mar '20

17 Mar '20

Hi, Michael! On Feb 29, Michael Widenius wrote: > commit 5365db70cb8 > Author: Michael Widenius <monty(a)mariadb.com> > Date: Mon Jan 13 18:30:13 2020 +0200 > > Improve update handler (long unique keys on blobs) > > MDEV-21606 Improve update handler (long unique keys on blobs) > MDEV-21470 MyISAM and Aria start_bulk_insert doesn't work with long unique > MDEV-21606 Bug fix for previous version of this code > MDEV-21819 2 Assertion `inited == NONE || update_handler != this' > > - Move update_handler from TABLE to handler > - Move out initialization of update handler from ha_write_row() to > prepare_for_insert() > - Fixed that INSERT DELAYED works with update handler > - Give an error if using long unique with an autoincrement column > - Added handler function to check if table has long unique hash indexes > - Disable write cache in MyISAM and Aria when using update_handler as > if cache is used, the row will not be inserted until end of statement > and update_handler would not find conflicting rows. > - Removed not used handler argument from > check_duplicate_long_entries_update() > - Syntax cleanups > - Indentation fixes > - Don't use single character indentifiers for arguments > > squash! 1ca2f0871ecfb550268ffc1a5cc23d7043d6b855 you may want to remove this line from the commit comment > > diff --git a/mysql-test/main/long_unique.result b/mysql-test/main/long_unique.result > index a4955b3e7b5..fee8e7721bf 100644 > --- a/mysql-test/main/long_unique.result > +++ b/mysql-test/main/long_unique.result > @@ -1477,4 +1477,28 @@ id select_type table type possible_keys key key_len ref rows Extra > SELECT t2.b FROM t1 JOIN t2 ON t1.d = t2.f WHERE t2.pk >= 20; > b > drop table t1,t2; > +# > +# MDEV-21470 MyISAM start_bulk_insert doesn't work with long unique > +# > +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MyISAM; > +INSERT INTO t1 VALUES (1,'foo'),(2,'bar'); > +CREATE TABLE t2 (c BIT, d BLOB, UNIQUE(d)) ENGINE=MyISAM; > +INSERT INTO t2 SELECT * FROM t1; > +Warnings: > +Warning 1264 Out of range value for column 'c' at row 2 > +DROP TABLE t1, t2; > +# > +# MDEV-19338 Using AUTO_INCREMENT with long unique > +# > +CREATE TABLE t1 (pk INT, a TEXT NOT NULL DEFAULT '', PRIMARY KEY (pk), b INT AUTO_INCREMENT, UNIQUE(b), UNIQUE (a,b)) ENGINE=myisam; > +ERROR HY000: Function or expression 'AUTO_INCREMENT' cannot be used in the UNIQUE clause of `a` Quite confusing error message :( I understand where it comes from, but for a user, I'm afraid, it just won't make any sense. A reasonable message could be, for example, AUTO_INCREMENT column %`s cannot be used in the UNIQUE index %`s > +# > +# MDEV-21819 Assertion `inited == NONE || update_handler != this' > +# failed in handler::ha_write_row > +# > +CREATE OR REPLACE TABLE t1 (a INT, b BLOB, s DATE, e DATE, PERIOD FOR app(s,e), UNIQUE(b)) ENGINE=MyISAM PARTITION BY HASH(a) PARTITIONS 2; > +INSERT INTO t1 VALUES (1,'foo','2022-01-01', '2025-01-01'); > +DELETE FROM t1 FOR PORTION OF app FROM '2023-01-01' TO '2024-01-01'; > +ERROR 23000: Duplicate entry 'foo' for key 'b' > +DROP TABLE t1; > set @@GLOBAL.max_allowed_packet= @allowed_packet; > diff --git a/mysql-test/suite/versioning/r/long_unique.result b/mysql-test/suite/versioning/r/long_unique.result > new file mode 100644 > index 00000000000..da07bc66e22 > --- /dev/null > +++ b/mysql-test/suite/versioning/r/long_unique.result > @@ -0,0 +1,8 @@ > +# > +# Assertion `inited == NONE || update_handler != this' failed in > +# handler::ha_write_row > +# > +CREATE TABLE t1 (f VARCHAR(4096), s DATE, e DATE, PERIOD FOR app(s,e), UNIQUE(f)) ENGINE=MyISAM; > +INSERT INTO t1 VALUES ('foo', '2023-08-30', '2025-07-09'),('bar', '2021-01-01', '2021-12-31'); > +DELETE FROM t1 FOR PORTION OF app FROM '2023-08-29' TO '2025-07-01'; > +DROP TABLE t1; you've had almost the same test (MDEV-21819) in main/long_unique.test it's a bit strange to have to very similar tests in different suites I suggest to move MDEV-21819 test from main/long_unique.test to this file. and then move this whole file from versioning suite to the period suite. > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 73191907aac..c7e61864366 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -4340,6 +4339,10 @@ int ha_partition::write_row(const uchar * buf) > } > m_last_part= part_id; > DBUG_PRINT("info", ("Insert in partition %u", part_id)); > + if (table->s->long_unique_table && > + m_file[part_id]->update_handler == m_file[part_id] && inited == RND) > + m_file[part_id]->prepare_for_insert(0); This looks quite incomprehensible. If the table has a long unique key and the update_handler is the handler itself you _prepare for insert_ ??? if it's an insert it should've been prepared earlier, like, normally for an insert, and it don't have to do anything with long uniques. What you actually do here (as I've found out looking firther in the patch) you prepare update_handler. That is "prepare_for_insert" is VERY confusing here, it sounds like it prepares for a normal insert and should be called at the beginning of mysql_insert(). And it's nothing like that. It would remove half of the questions if you'd call it prepare_update_handler() or, better, prepare_auxiliary_handler() (because it'll be used not only for long uniques) or something like that. Still, some questions are left: 1. why partitioning needs special code for that? 2. when a handler itself is its update_handler AND inited==RND? the handler itself can be the update_handler on INSERT, but then inited is NONE Is it UPDATE with system versioning? > + > start_part_bulk_insert(thd, part_id); > > tmp_disable_binlog(thd); /* Do not replicate the low-level changes. */ > diff --git a/sql/handler.cc b/sql/handler.cc > index 0700b1571e5..1e3f987b4e5 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -2648,6 +2649,39 @@ handler *handler::clone(const char *name, MEM_ROOT *mem_root) > return NULL; > } > > +/** > + @brief clone of current handler. you generally don't have to write @brief in these cases. Just /** clone of current handler. Creates a clone of handler used in update for unique hash key. */ > + Creates a clone of handler used in update for > + unique hash key. > +*/ > +bool handler::clone_handler_for_update() > +{ > + handler *tmp; > + DBUG_ASSERT(table->s->long_unique_table); > + > + if (update_handler != this) > + return 0; // Already done > + if (!(tmp= clone(table->s->normalized_path.str, table->in_use->mem_root))) > + return 1; > + update_handler= tmp; > + update_handler->ha_external_lock(table->in_use, F_RDLCK); Looks strange. Why F_RDLCK if it's an *update* handler? This definitely needs a comment, why you're using read lock for updates. > + return 0; > +} > + > +/** > + @brief Deletes update handler object > +*/ > +void handler::delete_update_handler() > +{ > + if (update_handler != this) > + { > + update_handler->ha_external_lock(table->in_use, F_UNLCK); > + update_handler->ha_close(); > + delete update_handler; > + } > + update_handler= this; > +} > + > LEX_CSTRING *handler::engine_name() > { > return hton_name(ht); > @@ -6481,6 +6515,8 @@ int handler::ha_reset() > DBUG_ASSERT(inited == NONE); > /* reset the bitmaps to point to defaults */ > table->default_column_bitmaps(); > + if (update_handler != this) > + delete_update_handler(); you already have an if() inside the delete_update_handler(). Either remove it from here or from there. > pushed_cond= NULL; > tracker= NULL; > mark_trx_read_write_done= 0; > @@ -6671,10 +6717,35 @@ static int check_duplicate_long_entries_update(TABLE *table, handler *h, uchar * > } > } > } > - exit: > - return error; > + return 0; > +} > + > + > +/** > + Do all initialization needed for insert > + > + @param force_update_handler Set to TRUE if we should always create an > + update handler. Needed if we don't know if we > + are going to do inserted while a scan is in "going to do inserts" > + progress. > +*/ > + > +int handler::prepare_for_insert(bool force_update_handler) > +{ > + /* Preparation for unique of blob's */ > + if (table->s->long_unique_table && (inited == RND || force_update_handler)) > + { > + /* > + When doing a scan we can't use the same handler to check > + duplicate rows. Create a new temporary one > + */ > + if (clone_handler_for_update()) > + return 1; > + } > + return 0; > } > > + > int handler::ha_write_row(const uchar *buf) > { > int error; > @@ -6977,6 +7044,11 @@ void handler::set_lock_type(enum thr_lock_type lock) > table->reginfo.lock_type= lock; > } > > +bool handler::has_long_unique() > +{ > + return table->s->long_unique_table; > +} I don't see why you need it for Aria. It can always check table->s->long_unique_table without an extra call > + > #ifdef WITH_WSREP > /** > @details > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > index 6a4ce266af2..e55ae4f97e6 100644 > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -752,6 +752,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > && !table->versioned() > && table->file->has_transactions(); > > + if (table->versioned(VERS_TIMESTAMP) || > + (table_list->has_period() && !portion_of_time_through_update)) > + table->file->prepare_for_insert(1); Ok, now I think that your idea of the default update_handler=this is rather fragile. If the default is NULL, than we can put in many places DBUG_ASSERT(update_handler != NULL); to make sure it was initialized when needed. Now we cannot do it. And there are many places in the code and non-trivial conditions when the update_handler has to be initialized and these places and conditions will change in the future. An assert would be helpful here. > + > THD_STAGE_INFO(thd, stage_updating); > while (likely(!(error=info.read_record())) && likely(!thd->killed) && > likely(!thd->is_error())) > diff --git a/sql/table.h b/sql/table.h > index 6ce92ee048e..1a0b9514d23 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -1182,6 +1181,7 @@ struct TABLE > /* Map of keys dependent on some constraint */ > key_map constraint_dependent_keys; > KEY *key_info; /* data of keys in database */ > + KEY_PART_INFO *base_key_part; /* Where key parts are stored */ again, this is *only* needed for INSERT DELAYED that very few people ever use. It should be in some INSERT DELAYED specific data structure, not in the common TABLE. By the way, why cannot you get the list of keyparts from table->key_info->key_part ? > > Field **field; /* Pointer to fields */ > Field **vfield; /* Pointer to virtual fields*/ > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index 38091dae0ba..39147396359 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -1740,7 +1740,7 @@ void ha_myisam::start_bulk_insert(ha_rows rows, uint flags) > enable_indexes() failed, thus it's essential that indexes are > disabled ONLY for an empty table. > */ > - if (file->state->records == 0 && can_enable_indexes && > + if (file->state->records == 0 && can_enable_indexes && !has_long_unique() && > (!rows || rows >= MI_MIN_ROWS_TO_DISABLE_INDEXES)) why do you disable bulk inserts for long uniques? > { > if (file->open_flag & HA_OPEN_INTERNAL_TABLE) > diff --git a/sql/table.cc b/sql/table.cc > index 718efa5767c..18a942964c3 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1241,31 +1241,46 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, > Item *list_item; > KEY *key= 0; > uint key_index, parts= 0; > + KEY_PART_INFO *key_part= table->base_key_part; > + > for (key_index= 0; key_index < table->s->keys; key_index++) > { > - key=table->key_info + key_index; > - parts= key->user_defined_key_parts; > - if (key->key_part[parts].fieldnr == field->field_index + 1) > - break; > + /* > + We have to use key from share as this function may have changed > + table->key_info if it was ever invoked before. This could happen > + in case of INSERT DELAYED. > + */ > + key= table->s->key_info + key_index; > + if (key->algorithm == HA_KEY_ALG_LONG_HASH) > + { > + parts= key->user_defined_key_parts; > + if (key_part[parts].fieldnr == field->field_index + 1) > + break; > + key_part++; > + } Here, again, you've basically duplicated the logic of how long uniques are represented in the keys and keyparts. All for the sake of INSERT DELAYED. I'd really prefer INSERT DELAYED problems to be solved inside the Delayed_insert class. And it can use setup_keyinfo_hash() and re_setup_keyinfo_hash() functions to avoid duplicating the logic. > + key_part+= key->ext_key_parts; > } > - if (!key || key->algorithm != HA_KEY_ALG_LONG_HASH) > + if (key_index == table->s->keys) > goto end; > - KEY_PART_INFO *keypart; > - for (uint i=0; i < parts; i++) > + > + /* Correct the key & key_parts if this function has been called before */ > + key= table->key_info + key_index; > + key->key_part= key_part; > + > + for (uint i=0; i < parts; i++, key_part++) > { > - keypart= key->key_part + i; > - if (keypart->key_part_flag & HA_PART_KEY_SEG) > + if (key_part->key_part_flag & HA_PART_KEY_SEG) > { > - int length= keypart->length/keypart->field->charset()->mbmaxlen; > + int length= key_part->length/key_part->field->charset()->mbmaxlen; > list_item= new (mem_root) Item_func_left(thd, > - new (mem_root) Item_field(thd, keypart->field), > + new (mem_root) Item_field(thd, key_part->field), > new (mem_root) Item_int(thd, length)); > list_item->fix_fields(thd, NULL); > - keypart->field->vcol_info= > - table->field[keypart->field->field_index]->vcol_info; > + key_part->field->vcol_info= > + table->field[key_part->field->field_index]->vcol_info; > } > else > - list_item= new (mem_root) Item_field(thd, keypart->field); > + list_item= new (mem_root) Item_field(thd, key_part->field); > field_list->push_back(list_item, mem_root); > } > Item_func_hash *hash_item= new(mem_root)Item_func_hash(thd, *field_list); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] [Commits] 70b0c838fad: MDEV-21922: Allow packing addon fields even if they don't honour max_length_for_sort_data
by Sergey Petrunia 13 Mar '20

13 Mar '20

Hi Varun, * The patch needs a testcase * Please check the comment at https://jira.mariadb.org/browse/MDEV-21922 , and add a comment in filesort_use_addons() about the use of upper bound for computations. * Ok to push after the above is addressed. On Thu, Mar 12, 2020 at 05:11:45PM +0530, Varun wrote: > revision-id: 70b0c838fada8e02d4ce9525611d6f4aeadf56d4 (mariadb-10.5.0-367-g70b0c838fad) > parent(s): 9d7ed94f6a526748eff29dae2939a3fd341f118b > author: Varun Gupta > committer: Varun Gupta > timestamp: 2020-03-12 14:41:03 +0530 > message: > > MDEV-21922: Allow packing addon fields even if they don't honour max_length_for_sort_data > > Addon fields will be packed if the length of addon fields is greater > than max_length_for_sort_data. > > --- > sql/filesort.cc | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/sql/filesort.cc b/sql/filesort.cc > index 1e19d492220..7b4e0bc31cd 100644 > --- a/sql/filesort.cc > +++ b/sql/filesort.cc > @@ -139,8 +139,6 @@ void Sort_param::try_to_pack_addons(ulong max_length_for_sort_data) > return; > > const uint sz= Addon_fields::size_of_length_field; > - if (rec_length + sz > max_length_for_sort_data) > - return; > > // Heuristic: skip packing if potential savings are less than 10 bytes. > if (m_packable_length < (10 + sz)) > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 5d0e4ce: MySQL support added
by andrei.elkin＠pp.inet.fi 12 Mar '20

12 Mar '20

Привет, Сергей! О чем этот "суппорт", не обьяснишь одной фразой? Спасибо! Андрей psergey <sergey(a)mariadb.com> writes: > revision-id: 5d0e4ce291ae940feaa8398435158dc56a3db3c4 () > parent(s): d504887718112e211544beca0e6651d5477466e1 > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2020-03-12 10:26:06 +0300 > message: > > MySQL support added > > --- > .gitignore | 1 + > filesort-bench1/06-make-varchar-bench.sh | 30 +++++++++++++++-- > prepare-server.sh | 11 ++++-- > setup-server/setup-mariadb-current.sh | 6 ++-- > setup-server/setup-mysql-8.0.sh | 58 ++++++++++++++++++++++++++++++++ > 5 files changed, 98 insertions(+), 8 deletions(-) > > diff --git a/.gitignore b/.gitignore > index fcd5175..3bccc18 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -23,3 +23,4 @@ my-10.5-old.cnf > mysql-boost > mysql-8.0 > mysql8-data > +mysql-8.0-data.clean/ > diff --git a/filesort-bench1/06-make-varchar-bench.sh b/filesort-bench1/06-make-varchar-bench.sh > index 85afcfd..21da087 100644 > --- a/filesort-bench1/06-make-varchar-bench.sh > +++ b/filesort-bench1/06-make-varchar-bench.sh > @@ -29,6 +29,17 @@ create table test_run_queries ( > test_time_ms bigint, > sort_merge_passes int > ); > + > +drop view if exists session_status; > + > +set @var= IF(version() like '%8.0%', > + 'create view session_status as select * from performance_schema.session_status', > + 'create view session_status as select * from information_schema.session_status'); > + > +prepare s from @var; > +execute s; > + > + > END > > ### > @@ -55,7 +66,18 @@ create table $test_table_name ( > char_field varchar($varchar_size) character set utf8, b int > ) engine=myisam; > > -insert into $rand_table_name select 1+floor(rand() * @n_countries) from seq_1_to_$table_size; > +drop table if exists ten, one_k; > +create table ten(a int); > +insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > + > +create table one_k(a int); > +insert into one_k select A.a + B.a* 10 + C.a * 100 from ten A, ten B, ten C; > + > +set @a=0; > +insert into $rand_table_name > +select 1+floor(rand() * @n_countries) > +from > + (select @a:=@a+1 from one_k A, one_k B, one_k C limit $table_size) T; > insert into $test_table_name > select > (select Name from Country where id=T.a), 1234 > @@ -63,13 +85,15 @@ from $rand_table_name T ; > > drop table $rand_table_name; > analyze table $test_table_name; > +select count(*) from $test_table_name; > +show create table $test_table_name; > END > > for i in 1 2 3 4 5 6 7 8 9 10 ; do > > ### query_start.sql here: > cat <<END > -select variable_value into @query_start_smp from information_schema.session_status where variable_name like 'sort_merge_passes'; > +select variable_value into @query_start_smp from session_status where variable_name like 'sort_merge_passes'; > select current_timestamp(6) into @query_start_time; > END > ### > @@ -87,7 +111,7 @@ echo $QUERY > cat << END > set @test_name='$TEST_NAME'; > set @query_time_ms= timestampdiff(microsecond, @query_start_time, current_timestamp(6))/1000; > -select variable_value into @query_end_smp from information_schema.session_status where variable_name like 'sort_merge_passes'; > +select variable_value into @query_end_smp from session_status where variable_name like 'sort_merge_passes'; > set @query_merge_passes = @query_end_smp - @query_start_smp; > insert into test_run_queries > (table_size, varchar_size, test_ts, test_time_ms, sort_merge_passes) > diff --git a/prepare-server.sh b/prepare-server.sh > index 01dbeba..d4da8db 100755 > --- a/prepare-server.sh > +++ b/prepare-server.sh > @@ -38,6 +38,11 @@ if [ ! -d $SERVERNAME ]; then > exit 1 > fi > > +if [ ! -f $SERVERNAME-vars.sh ]; then > + echo "Can't find settings file $SERVERNAME-vars.sh." > + exit 1 > +fi > + > if [[ $USE_RAMDISK ]] ; then > echo " Using /dev/shm for data dir" > fi > @@ -49,6 +54,8 @@ sleep 5 > > DATA_DIR=$SERVERNAME-data > > +source ${SERVERNAME}-vars.sh > + > if [[ $RECOVER ]] ; then > echo "Recovering the existing datadir" > else > @@ -64,7 +71,7 @@ else > fi > > #exit 0; > -./$SERVERNAME/sql/mysqld --defaults-file=./my-${SERVERNAME}.cnf & > +$MYSQLD --defaults-file=./my-${SERVERNAME}.cnf & > > > server_attempts=0 > @@ -72,7 +79,7 @@ server_attempts=0 > while true ; do > client_attempts=0 > while true ; do > - ./$SERVERNAME/client/mysql --defaults-file=./my-${SERVERNAME}.cnf -uroot -e "create database sbtest" > + $MYSQL $MYSQL_ARGS -e "select 1" > > if [ $? -eq 0 ]; then > break > diff --git a/setup-server/setup-mariadb-current.sh b/setup-server/setup-mariadb-current.sh > index 1b49ef0..b6bc417 100755 > --- a/setup-server/setup-mariadb-current.sh > +++ b/setup-server/setup-mariadb-current.sh > @@ -85,16 +85,16 @@ innodb_buffer_pool_size=8G > > EOF > > -cat > mysql-vars.sh <<EOF > +cat > $DIRNAME-vars.sh <<EOF > MYSQL="`pwd`/$DIRNAME/client/mysql" > +MYSQLD="`pwd`/$DIRNAME/sql/mysqld" > MYSQLSLAP="`pwd`/$DIRNAME/client/mysqlslap" > MYSQL_SOCKET="--socket=$SOCKETNAME" > MYSQL_USER="-uroot" > MYSQL_ARGS="\$MYSQL_USER \$MYSQL_SOCKET" > EOF > > -source mysql-vars.sh > -cp mysql-vars.sh $DIRNAME-vars.sh > +source $DIRNAME-vars.sh > > ( > cd $HOMEDIR/$DIRNAME/sql > diff --git a/setup-server/setup-mysql-8.0.sh b/setup-server/setup-mysql-8.0.sh > new file mode 100644 > index 0000000..ffd9987 > --- /dev/null > +++ b/setup-server/setup-mysql-8.0.sh > @@ -0,0 +1,58 @@ > +#!/bin/bash > + > +HOMEDIR=`pwd` > + > +BRANCH=8.0 > +SERVER_VERSION=8.0 > +DIRNAME="mysql-$SERVER_VERSION" > + > +git clone --branch $BRANCH --depth 1 https://github.com/mysql/mysql-server.git $DIRNAME > + > +cd mysql-$SERVER_VERSION > +cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DDOWNLOAD_BOOST=1 -DWITH_BOOST=$HOMEDIR/mysql-boost \ > + -DENABLE_DOWNLOADS=1 -DFORCE_INSOURCE_BUILD=1 -DWITH_UNIT_TESTS=0 > + > +make -j8 > + > +cd mysql-test > +perl ./mysql-test-run alias > +cp -r var/data $HOMEDIR/$DIRNAME-data > +cp -r var/data $HOMEDIR/$DIRNAME-data.clean > +cd .. > + > + > +source_dir=`pwd` > +socket_name="`basename $source_dir`.sock" > +SOCKETNAME="/tmp/$socket_name" > + > +cat > $HOMEDIR/my-$DIRNAME.cnf <<EOF > + > +[mysqld] > +datadir=$HOMEDIR/$DIRNAME-data > + > +tmpdir=/tmp > +port=3320 > +socket=$SOCKETNAME > +#binlog-format=row > +gdb > +lc_messages_dir=../share > +server-id=12 > +bind-address=0.0.0.0 > +log-error > +secure_file_priv= > +innodb_buffer_pool_size=4G > +EOF > + > +cat > $DIRNAME-vars.sh <<EOF > +MYSQL="`pwd`/$DIRNAME/bin/mysql" > +MYSQLD="`pwd`/$DIRNAME/bin/mysqld" > +MYSQLSLAP="`pwd`/$DIRNAME/bin/mysqlslap" > +MYSQL_SOCKET="--socket=$SOCKETNAME" > +MYSQL_USER="-uroot" > +MYSQL_ARGS="\$MYSQL_USER \$MYSQL_SOCKET" > +EOF > + > +source $DIRNAME-vars.sh > + > +$MYSQLD --defaults-file=$HOMEDIR/my-$DIRNAME.cnf & > + > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] (no subject)
by Badrul Chowdhury 12 Mar '20

12 Mar '20

-- Cheers, Badrul

1 0

Re: [Maria-developers] bbe056ac3fa: support NULL fields in key
by Sergei Golubchik 11 Mar '20

11 Mar '20

Hi, Nikita! On Mar 10, Nikita Malyavin wrote: > revision-id: bbe056ac3fa (mariadb-10.5.0-273-gbbe056ac3fa) > parent(s): 7a5d3316805 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2020-03-11 00:46:24 +1000 > message: > > support NULL fields in key > > --- > mysql-test/suite/period/r/overlaps.result | 18 ++++++++++++++++-- > mysql-test/suite/period/t/overlaps.test | 16 +++++++++++++++- > sql/handler.cc | 18 ++++++++++++++---- > sql/sql_table.cc | 29 +++++++---------------------- > 4 files changed, 52 insertions(+), 29 deletions(-) > > diff --git a/mysql-test/suite/period/r/overlaps.result b/mysql-test/suite/period/r/overlaps.result > index cf980afd7f0..e52b21496b5 100644 > --- a/mysql-test/suite/period/r/overlaps.result > +++ b/mysql-test/suite/period/r/overlaps.result > @@ -104,16 +104,30 @@ show create table t; > Table Create Table > t CREATE TABLE `t` ( > `id` int(11) NOT NULL, > - `u` int(11) NOT NULL, > + `u` int(11) DEFAULT NULL, > `s` date NOT NULL, > `e` date NOT NULL, > PERIOD FOR `p` (`s`, `e`), > PRIMARY KEY (`id`,`p` WITHOUT OVERLAPS), > UNIQUE KEY `u` (`u`,`p` WITHOUT OVERLAPS) > ) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 > +insert into t values (2, NULL, '2003-03-01', '2003-05-01'); > +insert into t values (2, NULL, '2003-03-01', '2003-05-01'); > +ERROR 23000: Duplicate entry '2-2003-05-01-2003-03-01' for key 'PRIMARY' This is wrong. Should be no duplicate key error above. > +insert into t values (3, NULL, '2003-03-01', '2003-05-01'); > insert into t values (1, 1, '2003-03-01', '2003-05-01'); > insert into t values (1, 2, '2003-05-01', '2003-07-01'); > -insert into t values (2, 1, '2003-05-01', '2003-07-01'); > +insert into t values (4, NULL, '2003-03-01', '2003-05-01'); > +create sequence seq start=5; > +update t set id= nextval(seq), u= nextval(seq), s='2003-05-01', e='2003-07-01' > + where u is NULL; > +select * from t; > +id u s e > +1 1 2003-03-01 2003-05-01 > +1 2 2003-05-01 2003-07-01 > +5 6 2003-05-01 2003-07-01 > +7 8 2003-05-01 2003-07-01 > +9 10 2003-05-01 2003-07-01 > create or replace table t(id int, s date, e date, > period for p(s,e)); > insert into t values (1, '2003-01-01', '2003-03-01'), > diff --git a/sql/handler.cc b/sql/handler.cc > index 917386f4392..16f57533f27 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7015,7 +7017,8 @@ int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data) > bool key_used= false; > for (uint k= 0; k < key_parts && !key_used; k++) > key_used= bitmap_is_set(table->write_set, > - key_info.key_part[k].fieldnr - 1); > + key_info.key_part[k].fieldnr - 1) > + && !key_info.key_part[k].field->is_null_in_record(new_data); Why is that? > if (!key_used) > continue; > } > @@ -7064,8 +7067,15 @@ int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data) > error= handler->ha_index_next(record_buffer); > } > > - if (!error && table->check_period_overlaps(key_info, key_info, > - new_data, record_buffer) == 0) > + bool null_in_key= false; > + for (uint k= 0; k < key_parts && !null_in_key; k++) > + { > + null_in_key= key_info.key_part[k].field->is_null_in_record(record_buffer); > + } > + > + if (!null_in_key && !error > + && table->check_period_overlaps(key_info, key_info, > + new_data, record_buffer) == 0) That's strange. You compare keys in key_period_compare_bases(), why do you do NULL values here? > error= HA_ERR_FOUND_DUPP_KEY; > > if (error == HA_ERR_KEY_NOT_FOUND || error == HA_ERR_END_OF_FILE) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] eea71e8b05a: MDEV-16978 Application-time periods: WITHOUT OVERLAPS
by Sergei Golubchik 11 Mar '20

11 Mar '20

Hi, Nikita! Despite what the subject says, it's the review for eea71e8b05a..7a5d3316805 That is everything in bb-10.5-MDEV-16978-without-overlaps minus the last commit (that came too late and I'll review it separately) In general it looks pretty good, just few minor comments below: On Mar 10, Nikita Malyavin wrote: > revision-id: eea71e8b05a (mariadb-10.5.0-246-geea71e8b05a) > parent(s): 6618fc29749 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2020-02-21 02:38:57 +1000 > message: > > MDEV-16978 Application-time periods: WITHOUT OVERLAPS > > * The overlaps check is implemented on a handler level per row command. > It creates a separate cursor (actually, another handler instance) and > caches it inside the original handler, when ha_update_row or > ha_insert_row is issued. Cursor closes on unlocking the handler. > > * Containing the same key in index means unique constraint violation > even in usual terms. So we fetch left and right neighbours and check > that they have same key prefix, excluding from the key only the period part. > If it doesnt match, then there's no such neighbour, and the check passes. > Otherwise, we check if this neighbour intersects with the considered key. > > * The check does introduce new error and fails with ER_DUPP_KEY error. "does not introduce new error" you mean? > This might break REPLACE workflow and should be fixed separately > > diff --git a/sql/field.h b/sql/field.h > index 4a8eec35b05..e187ffeb331 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -1444,8 +1444,9 @@ class Field: public Value_source > if (null_ptr) > null_ptr=ADD_TO_PTR(null_ptr,ptr_diff,uchar*); > } > - virtual void get_image(uchar *buff, uint length, CHARSET_INFO *cs) > - { memcpy(buff,ptr,length); } > + virtual void get_image(uchar *buff, uint length, > + const uchar *ptr_arg, CHARSET_INFO *cs) const > + { memcpy(buff,ptr_arg,length); } please, add a convenience method. void get_image(uchar *buff, uint length, CHARSET_INFO *cs) { get_image(buff, length, ptr, cs); } and the same below, where you add ptr_arg > virtual void set_image(const uchar *buff,uint length, CHARSET_INFO *cs) > { memcpy(ptr,buff,length); } > > @@ -4056,7 +4066,8 @@ class Field_varstring :public Field_longstr { > using Field_str::store; > double val_real() override; > longlong val_int() override; > - String *val_str(String *, String *) override; > + String *val_str(String *, String *) final; > + virtual String *val_str(String*,String *, const uchar*) const; This means that for the sake of indexes WITHOUT OVERLAPS (that very few people will use) and compressed blobs (that are used even less) you've added a new virtual call to Field_varstring::val_str (that is used awfully a lot) Try to avoid it, please > my_decimal *val_decimal(my_decimal *) override; > int cmp_max(const uchar *, const uchar *, uint max_length) const override; > int cmp(const uchar *a,const uchar *b) const override > diff --git a/sql/field.cc b/sql/field.cc > index 1ce49b0bdfa..82df2784057 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -9638,11 +9645,12 @@ int Field_bit::cmp_offset(my_ptrdiff_t row_offset) > } > > > -uint Field_bit::get_key_image(uchar *buff, uint length, imagetype type_arg) > +uint Field_bit::get_key_image(uchar *buff, uint length, const uchar *ptr_arg, imagetype type_arg) const > { > if (bit_len) > { > - uchar bits= get_rec_bits(bit_ptr, bit_ofs, bit_len); > + auto *bit_ptr_for_arg= ptr_arg + (bit_ptr - ptr); please, don't use auto in trivial cases like this one. it might be easier to type, but then the reviewer and whoever will in the future will edit this code will have to do type derivation in the head. > + uchar bits= get_rec_bits(bit_ptr_for_arg, bit_ofs, bit_len); > *buff++= bits; > length--; > } > diff --git a/sql/item_buff.cc b/sql/item_buff.cc > index 81949bcdae0..514ac740697 100644 > --- a/sql/item_buff.cc > +++ b/sql/item_buff.cc > @@ -195,7 +195,7 @@ bool Cached_item_field::cmp(void) > becasue of value change), then copy the new value to buffer. > */ > if (! null_value && (tmp || (tmp= (field->cmp(buff) != 0)))) > - field->get_image(buff,length,field->charset()); > + field->get_image(buff,length,field->ptr,field->charset()); not needed if you add convenience methods > return tmp; > } > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 13b2659789d..2f1b1431dc0 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -357,13 +357,15 @@ class Key :public Sql_alloc, public DDL_options { > engine_option_value *option_list; > bool generated; > bool invisible; > + bool without_overlaps; > + Lex_ident period; strictly speaking, you don't need without_overlaps property, if the period name is set it *has to be* without overlaps. but ok, whatever you prefer > > Key(enum Keytype type_par, const LEX_CSTRING *name_arg, > ha_key_alg algorithm_arg, bool generated_arg, DDL_options_st ddl_options) > :DDL_options(ddl_options), > type(type_par), key_create_info(default_key_create_info), > name(*name_arg), option_list(NULL), generated(generated_arg), > - invisible(false) > + invisible(false), without_overlaps(false) > { > key_create_info.algorithm= algorithm_arg; > } > diff --git a/sql/table.h b/sql/table.h > index 6ce92ee048e..09f03690a8c 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -1635,13 +1635,12 @@ struct TABLE > int insert_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, > ha_rows *rows_inserted); > bool vers_check_update(List<Item> &items); > - > + static int check_period_overlaps(const KEY &lhs_key, const KEY &rhs_key, > + const uchar *lhs, const uchar *rhs); why did you make it a static method of TABLE? > int delete_row(); > void vers_update_fields(); > void vers_update_end(); > void find_constraint_correlated_indexes(); > - void clone_handler_for_update(); > - void delete_update_handler(); > > /** Number of additional fields used in versioned tables */ > #define VERSIONING_FIELDS 2 > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 240f001f7de..74d28ede25f 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -3959,6 +3959,28 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, > DBUG_RETURN(TRUE); > } > > + switch (key->type) { > + case Key::UNIQUE: > + if (!key->period) > + break; > + /* Fall through: > + WITHOUT OVERLAPS forces fields to be NOT NULL > + */ Why is that? > + case Key::PRIMARY: > + /* Implicitly set primary key fields to NOT NULL for ISO conf. */ > + if (!(sql_field->flags & NOT_NULL_FLAG)) > + { > + /* Implicitly set primary key fields to NOT NULL for ISO conf. */ duplicated comment > + sql_field->flags|= NOT_NULL_FLAG; > + sql_field->pack_flag&= ~FIELDFLAG_MAYBE_NULL; > + null_fields--; > + } > + break; > + default: > + // Fall through > + break; > + } > + > cols2.rewind(); > switch(key->type) { > > @@ -4536,15 +4556,13 @@ bool Column_definition::sp_prepare_create_field(THD *thd, MEM_ROOT *mem_root) > } > > > -static bool vers_prepare_keys(THD *thd, HA_CREATE_INFO *create_info, > - Alter_info *alter_info, KEY **key_info, uint key_count) > +static bool append_system_key_parts(THD *thd, HA_CREATE_INFO *create_info, > + Alter_info *alter_info, KEY **key_info, > + uint key_count) > { > - DBUG_ASSERT(create_info->versioned()); > - > - const char *row_start_field= create_info->vers_info.as_row.start; > - DBUG_ASSERT(row_start_field); > - const char *row_end_field= create_info->vers_info.as_row.end; > - DBUG_ASSERT(row_end_field); > + const auto &row_start_field= create_info->vers_info.as_row.start; > + const auto &row_end_field= create_info->vers_info.as_row.end; please, don't use auto in trivial cases like this one. it might be easier to type, but then the reviewer and whoever will in the future will edit this code will have to do type derivation in the head. > + DBUG_ASSERT(!create_info->versioned() || (row_start_field && row_end_field)); > > List_iterator<Key> key_it(alter_info->key_list); > Key *key= NULL; > @@ -4553,25 +4571,61 @@ static bool vers_prepare_keys(THD *thd, HA_CREATE_INFO *create_info, > if (key->type != Key::PRIMARY && key->type != Key::UNIQUE) > continue; > > + if (create_info->versioned()) > + { > Key_part_spec *key_part=NULL; > List_iterator<Key_part_spec> part_it(key->columns); > while ((key_part=part_it++)) > { > - if (!my_strcasecmp(system_charset_info, > - row_start_field, > - key_part->field_name.str) || > - > - !my_strcasecmp(system_charset_info, > - row_end_field, > - key_part->field_name.str)) > + if (row_start_field.streq(key_part->field_name) || > + row_end_field.streq(key_part->field_name)) > break; > } > - if (key_part) > - continue; // Key already contains Sys_start or Sys_end > + if (!key_part) > + key->columns.push_back(new Key_part_spec(&row_end_field, 0)); > + } > + } > > - Key_part_spec *key_part_sys_end_col= > - new (thd->mem_root) Key_part_spec(&create_info->vers_info.as_row.end, 0); > - key->columns.push_back(key_part_sys_end_col); > + key_it.rewind(); > + while ((key=key_it++)) please skip this loop if there's no PERIOD and skip the loop above if there's no system versioning. > + { > + if (key->without_overlaps) > + { > + if (key->type != Key::PRIMARY && key->type != Key::UNIQUE) > + { > + my_error(ER_PERIOD_WITHOUT_OVERLAPS_NON_UNIQUE, MYF(0), key->period.str); > + return true; I think this can even be a syntax error in the parser. there's no need to postpone it till here, is there? > + } > + if (!create_info->period_info.is_set() > + || !key->period.streq(create_info->period_info.name)) > + { > + my_error(ER_PERIOD_NOT_FOUND, MYF(0), key->period.str); > + return true; > + } > + if (thd->work_part_info) > + { > + // Unfortunately partitions do not support searching upper/lower bounds > + // (i.e. ha_index_read_map with KEY_OR_PREV, KEY_OR_NEXT) > + my_error(ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING, MYF(0), > + "WITHOUT OVERLAPS"); > + return true; > + } > + const auto &period_start= create_info->period_info.period.start; > + const auto &period_end= create_info->period_info.period.end; > + List_iterator<Key_part_spec> part_it(key->columns); > + while (Key_part_spec *key_part= part_it++) > + { > + if (period_start.streq(key_part->field_name) > + || period_end.streq(key_part->field_name)) > + { > + my_error(ER_KEY_CONTAINS_PERIOD_FIELDS, MYF(0), key->name.str, > + key_part->field_name); > + return true; > + } > + } > + key->columns.push_back(new Key_part_spec(&period_end, 0)); > + key->columns.push_back(new Key_part_spec(&period_start, 0)); > + } > } > > return false; > diff --git a/sql/handler.cc b/sql/handler.cc > index 7d61252eea6..917386f4392 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -4173,6 +4173,11 @@ uint handler::get_dup_key(int error) > if (table->s->long_unique_table && table->file->errkey < table->s->keys) > DBUG_RETURN(table->file->errkey); > table->file->errkey = (uint) -1; > + if (overlaps_error_key != -1) > + { > + table->file->errkey= (uint)overlaps_error_key; > + DBUG_RETURN(table->file->errkey); > + } Why do you need overlaps_error_key? it looks like you can store the conflicting key number directly in errkey and it's somewhat confusing that this method uses table->file->errkey instead of just errkey. It's totally not clear why it does that. Looks like some historical thing from 2000. > if (error == HA_ERR_FOUND_DUPP_KEY || > error == HA_ERR_FOREIGN_DUPLICATE_KEY || > error == HA_ERR_FOUND_DUPP_UNIQUE || error == HA_ERR_NULL_IN_SPATIAL || > @@ -6563,10 +6576,12 @@ static int check_duplicate_long_entry_key(TABLE *table, handler *h, > unique constraint on long columns. > @returns 0 if no duplicate else returns error > */ > -static int check_duplicate_long_entries(TABLE *table, handler *h, > - const uchar *new_rec) > +int handler::check_duplicate_long_entries(const uchar *new_rec) > { > - table->file->errkey= -1; > + if (this->inited == RND) > + create_lookup_handler(); 1. and if inited==INDEX ? 2. why 'this->' ? 3. generally it's not a good idea to check inited==RND and lookup_handler==NULL inside a loop per row, because these values cannot change in the middle of a scan. Better to check them once, when inited is being set to RND. > + handler *h= lookup_handler ? lookup_handler : table->file; why table->file and not this? > + errkey= -1; > int result; > for (uint i= 0; i < table->s->keys; i++) > { > @@ -6935,6 +6956,142 @@ void handler::set_lock_type(enum thr_lock_type lock) > table->reginfo.lock_type= lock; > } > > +/** > + @brief clone of current handler. > + Creates a clone of handler used for unique hash key and WITHOUT OVERLAPS. > + @return error code > +*/ > +int handler::create_lookup_handler() > +{ > + if (lookup_handler) > + return 0; > + lookup_handler= clone(table_share->normalized_path.str, > + table->in_use->mem_root); > + int error= lookup_handler->ha_external_lock(table->in_use, F_RDLCK); > + return error; > +} > + > +int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data) > +{ > + DBUG_ASSERT(new_data); > + if (!table_share->period.unique_keys) > + return 0; > + if (table->versioned() && !table->vers_end_field()->is_max()) > + return 0; > + > + bool is_update= old_data != NULL; > + if (!check_overlaps_buffer) > + check_overlaps_buffer= (uchar*)alloc_root(&table_share->mem_root, > + table_share->max_unique_length > + + table_share->reclength); check_overlaps_buffer is per handler. It should be allocated in the TABLE::mem_root, not TABLE_SHARE::mem_root Also, it should probably be called lookup_buffer and check_duplicate_long_entry_key() should use it too. > + auto *record_buffer= check_overlaps_buffer + table_share->max_unique_length; > + auto *handler= this; > + // handler->inited can be NONE on INSERT > + if (handler->inited != NONE) > + { > + create_lookup_handler(); > + handler= lookup_handler; > + > + // Needs to compare record refs later is old_row_found() > + if (is_update) > + position(old_data); > + } > + > + // Save and later restore this handler's keyread > + int old_this_keyread= this->keyread; Again, why do you save/restore this->keyread? If you're using lookup_handler below, then this->keyread doesn't matter. And if handler==this below, then this->inited==NONE, which implies no keyread. Either way, handler->keyread_enabled() should always be false here. What about DBUG_ASSERT(!handler->keyread_enabled()) ? > + DBUG_ASSERT(this->ha_end_keyread() == 0); please fix all cases where you used a function with side effects inside an assert. not just the one I've commented about > + > + int error= 0; > + > + for (uint key_nr= 0; key_nr < table_share->keys && !error; key_nr++) > + { > + const KEY &key_info= table->key_info[key_nr]; > + const uint key_parts= key_info.user_defined_key_parts; > + if (!key_info.without_overlaps) > + continue; > + > + if (is_update) > + { > + bool key_used= false; > + for (uint k= 0; k < key_parts && !key_used; k++) > + key_used= bitmap_is_set(table->write_set, > + key_info.key_part[k].fieldnr - 1); > + if (!key_used) > + continue; > + } > + > + error= handler->ha_index_init(key_nr, 0); > + if (error) > + return error; we should try to minimize number of index_init/index_end, it doesn't look like a cheap operation, at least in InnoDB. but in a separate commit, because it should cover long uniques too. > + > + error= handler->ha_start_keyread(key_nr); > + DBUG_ASSERT(!error); > + > + const uint period_field_length= key_info.key_part[key_parts - 1].length; > + const uint key_base_length= key_info.key_length - 2 * period_field_length; > + > + key_copy(check_overlaps_buffer, new_data, &key_info, 0); > + > + /* Copy period_start to period_end. > + the value in period_start field is not significant, but anyway let's leave > + it defined to avoid uninitialized memory access > + */ > + memcpy(check_overlaps_buffer + key_base_length, > + check_overlaps_buffer + key_base_length + period_field_length, > + period_field_length); > + > + /* Find row with period_end > (period_start of new_data) */ > + error = handler->ha_index_read_map(record_buffer, > + check_overlaps_buffer, > + key_part_map((1 << (key_parts - 1)) - 1), > + HA_READ_AFTER_KEY); > + > + if (!error && is_update) > + { > + /* In case of update it could happen that the nearest neighbour is > + a record we are updating. It means, that there are no overlaps > + from this side. > + > + An assumption is made that during update we always have the last > + fetched row in old_data. Therefore, comparing ref's is enough > + */ > + DBUG_ASSERT(handler != this); > + DBUG_ASSERT(inited != NONE); > + DBUG_ASSERT(ref_length == handler->ref_length); > + > + handler->position(record_buffer); > + if (memcmp(ref, handler->ref, ref_length) == 0) > + error= handler->ha_index_next(record_buffer); > + } > + > + if (!error && table->check_period_overlaps(key_info, key_info, > + new_data, record_buffer) == 0) > + error= HA_ERR_FOUND_DUPP_KEY; > + > + if (error == HA_ERR_KEY_NOT_FOUND || error == HA_ERR_END_OF_FILE) > + error= 0; > + > + if (error == HA_ERR_FOUND_DUPP_KEY) > + overlaps_error_key= key_nr; > + > + int end_error= handler->ha_end_keyread(); > + DBUG_ASSERT(!end_error); > + > + end_error= handler->ha_index_end(); > + if (!error && end_error) > + error= end_error; > + } > + > + // Restore keyread of this handler, if it was enabled > + if (old_this_keyread < MAX_KEY) > + { > + error= this->ha_start_keyread(old_this_keyread); > + DBUG_ASSERT(error == 0); > + } > + > + return error; > +} > + > #ifdef WITH_WSREP > /** > @details > diff --git a/sql/table.cc b/sql/table.cc > index 718efa5767c..65fc44458f4 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1499,6 +1500,14 @@ static size_t extra2_read_len(const uchar **extra2, const uchar *extra2_end) > return length; > } > > +static > +bool fill_unique_extra2(const uchar *extra2, size_t len, LEX_CUSTRING *section) thanks, good point. a bit confusing name, I thought it's something about UNIQUE particularly as this is what the whole patch is about :) What about read_extra2_section_once() or something like that? Or get_ or consume_ or store_ ? > +{ > + if (section->str) > + return true; > + *section= {extra2, len}; > + return false; > +} > > static > bool read_extra2(const uchar *frm_image, size_t len, extra2_fields *fields) > @@ -1725,11 +1725,26 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo= &first_keyinfo; > thd->mem_root= &share->mem_root; > > + auto err= [thd, share, &handler_file, &se_plugin, old_root](){ > + share->db_plugin= NULL; > + share->error= OPEN_FRM_CORRUPTED; > + share->open_errno= my_errno; > + delete handler_file; > + plugin_unlock(0, se_plugin); > + my_hash_free(&share->name_hash); > + > + if (!thd->is_error()) > + open_table_error(share, OPEN_FRM_CORRUPTED, share->open_errno); > + > + thd->mem_root= old_root; > + return HA_ERR_NOT_A_TABLE; > + }; > + Okay. I kind of see your point, but let's postpone this refactoring until after 10.5.2. I still need some time to think about it. You'll still be able to push it later. > if (write && write_frm_image(frm_image, frm_length)) > - goto err; > + DBUG_RETURN(err()); > > if (frm_length < FRM_HEADER_SIZE + FRM_FORMINFO_SIZE) > - goto err; > + DBUG_RETURN(err()); > > share->frm_version= frm_image[2]; > /* > @@ -8603,6 +8626,21 @@ void TABLE::evaluate_update_default_function() > DBUG_VOID_RETURN; > } > > +/** > + Compare two keys with periods > + @return -1, lhs precedes rhs > + 0, lhs overlaps rhs > + 1, lhs succeeds rhs > + */ > +int TABLE::check_period_overlaps(const KEY &lhs_key, const KEY &rhs_key, > + const uchar *lhs, const uchar *rhs) > +{ > + int cmp_res= key_period_compare_bases(lhs_key, rhs_key, lhs, rhs); you only use key_period_compare_bases here. I just wouldn't create a separate small function for that, but rather inlined it here. > + if (cmp_res) > + return cmp_res; > + > + return key_period_compare_periods(lhs_key, rhs_key, lhs, rhs); same for key_period_compare_periods > +} > > void TABLE::vers_update_fields() > { > diff --git a/sql/key.cc b/sql/key.cc > index 9dbb7a15726..49e97faea22 100644 > --- a/sql/key.cc > +++ b/sql/key.cc > @@ -896,3 +897,51 @@ bool key_buf_cmp(KEY *key_info, uint used_key_parts, > } > return FALSE; > } > + > + > +/** > + Compare base parts (not including the period) of keys with period > + @return -1, lhs less than rhs > + 0, lhs equals rhs > + 1, lhs more than rhs > + */ > +int key_period_compare_bases(const KEY &lhs_key, const KEY &rhs_key, > + const uchar *lhs, const uchar *rhs) > +{ > + uint base_part_nr= lhs_key.user_defined_key_parts - 2; may be, give this '2' a name, like 'period_key_parts' ? and use them everywhere, of course, not just here > + int cmp_res= 0; > + for (uint part_nr= 0; !cmp_res && part_nr < base_part_nr; part_nr++) > + { > + Field *f= lhs_key.key_part[part_nr].field; > + cmp_res= f->cmp(f->ptr_in_record(lhs), > + rhs_key.key_part[part_nr].field->ptr_in_record(rhs)); This doesn't seem to be handling NULLs (see the next review) > + } > + > + return cmp_res; > +} > + > +/** > + Compare periods of two keys > + @return -1, lhs preceeds rhs > + 0, lhs overlaps rhs > + 1, lhs succeeds rhs > + */ > +int key_period_compare_periods(const KEY &lhs_key, const KEY &rhs_key, > + const uchar *lhs, const uchar *rhs) > +{ > + uint period_start= lhs_key.user_defined_key_parts - 1; > + uint period_end= lhs_key.user_defined_key_parts - 2; > + > + const auto *f= lhs_key.key_part[period_start].field; > + const uchar *l[]= {lhs_key.key_part[period_start].field->ptr_in_record(lhs), > + rhs_key.key_part[period_start].field->ptr_in_record(rhs)}; > + > + const uchar *r[]= {lhs_key.key_part[period_end].field->ptr_in_record(lhs), > + rhs_key.key_part[period_end].field->ptr_in_record(rhs)}; I'd still prefer names like 'ls', 'le', 'rs', 're'. Now I need to look up and keep in mind that 'r[0] is left key end period' etc > + > + if (f->cmp(r[0], l[1]) <= 0) > + return -1; > + if (f->cmp(l[0], r[1]) >= 0) > + return 1; > + return 0; > +} Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] 9ae015878f1: MDEV-10047: table-based master info repository
by sujatha 10 Mar '20

10 Mar '20

revision-id: 9ae015878f11be3e3033fd1b35357ea5927c6c51 (mariadb-10.5.0-329-g9ae015878f1) parent(s): b753ac066bc26acda9deb707a31c112f1bbf9ec2 author: Sujatha committer: Sujatha timestamp: 2020-03-10 15:55:50 +0530 message: MDEV-10047: table-based master info repository Problem: ======= When we upgrade from "mysql" to "mariadb" if slave is using repositories as tables their data is completely ignored and no warning is issued in error log. Fix: === "mysql_upgrade" test should check for the presence of data in "mysql.slave_master_info" and "mysql.slave_relay_log_info" tables. When tables have some data the upgrade script should report a warning which hints users that the data in repository tables will be ignored. --- client/mysql_upgrade.c | 61 +++++++++- .../main/rpl_mysql_upgrade_slave_repo_check.result | 33 ++++++ .../main/rpl_mysql_upgrade_slave_repo_check.test | 127 +++++++++++++++++++++ 3 files changed, 220 insertions(+), 1 deletion(-) diff --git a/client/mysql_upgrade.c b/client/mysql_upgrade.c index 4e17089593f..bea82c2a112 100644 --- a/client/mysql_upgrade.c +++ b/client/mysql_upgrade.c @@ -1014,6 +1014,64 @@ static int install_used_engines(void) return 0; } +static int check_slave_repositories(void) +{ + DYNAMIC_STRING ds_result; + int row_count= 0; + int error= 0; + const char *query = "SELECT COUNT(*) AS c1 FROM mysql.slave_master_info"; + + if (init_dynamic_string(&ds_result, "", 512, 512)) + die("Out of memory"); + + run_query(query, &ds_result, TRUE); + + if (ds_result.length) + { + row_count= atoi((char *)ds_result.str); + if (row_count) + { + fprintf(stderr,"Slave info repository compatibility check:" + " Found data in `mysql`.`slave_master_info` table.\n"); + fprintf(stderr,"Warning: Content of `mysql`.`slave_master_info` table" + " will be ignored as MariaDB supports file based info " + "repository.\n"); + error= 1; + } + } + dynstr_free(&ds_result); + + query = "SELECT COUNT(*) AS c1 FROM mysql.slave_relay_log_info"; + + if (init_dynamic_string(&ds_result, "", 512, 512)) + die("Out of memory"); + + run_query(query, &ds_result, TRUE); + + if (ds_result.length) + { + row_count= atoi((char *)ds_result.str); + if (row_count) + { + fprintf(stderr, "Slave info repository compatibility check:" + " Found data in `mysql`.`slave_relay_log_info` table.\n"); + fprintf(stderr, "Warning: Content of `mysql`.`slave_relay_log_info` " + "table will be ignored as MariaDB supports file based " + "repository.\n"); + error= 1; + } + } + dynstr_free(&ds_result); + if (error) + { + fprintf(stderr,"Slave server may not possess the correct replication " + "metadata.\n"); + fprintf(stderr, "Execution of CHANGE MASTER as per " + "`mysql`.`slave_master_info` and `mysql`.`slave_relay_log_info` " + "table content is recommended.\n"); + } + return 0; +} /* Update all system tables in MySQL Server to current @@ -1225,7 +1283,8 @@ int main(int argc, char **argv) run_mysqlcheck_views() || run_sql_fix_privilege_tables() || run_mysqlcheck_fixnames() || - run_mysqlcheck_upgrade(FALSE)) + run_mysqlcheck_upgrade(FALSE) || + check_slave_repositories()) die("Upgrade failed" ); verbose("Phase %d/%d: Running 'FLUSH PRIVILEGES'", ++phase, phases_total); diff --git a/mysql-test/main/rpl_mysql_upgrade_slave_repo_check.result b/mysql-test/main/rpl_mysql_upgrade_slave_repo_check.result new file mode 100644 index 00000000000..87cc9ab5a24 --- /dev/null +++ b/mysql-test/main/rpl_mysql_upgrade_slave_repo_check.result @@ -0,0 +1,33 @@ +include/master-slave.inc +[connection master] +******************************************************************** +* Test case1: Upgrade when repository tables have data. * +* mysql_upgrade script should report warnings. * +******************************************************************** +connection master; +Slave info repository compatibility check: Found data in `mysql`.`slave_master_info` table. +Warning: Content of `mysql`.`slave_master_info` table will be ignored as MariaDB supports file based info repository. +Slave info repository compatibility check: Found data in `mysql`.`slave_relay_log_info` table. +Warning: Content of `mysql`.`slave_relay_log_info` table will be ignored as MariaDB supports file based repository. +Slave server may not possess the correct replication metadata. +Execution of CHANGE MASTER as per `mysql`.`slave_master_info` and `mysql`.`slave_relay_log_info` table content is recommended. +connection slave; +Slave info repository compatibility check: Found data in `mysql`.`slave_master_info` table. +Warning: Content of `mysql`.`slave_master_info` table will be ignored as MariaDB supports file based info repository. +Slave info repository compatibility check: Found data in `mysql`.`slave_relay_log_info` table. +Warning: Content of `mysql`.`slave_relay_log_info` table will be ignored as MariaDB supports file based repository. +Slave server may not possess the correct replication metadata. +Execution of CHANGE MASTER as per `mysql`.`slave_master_info` and `mysql`.`slave_relay_log_info` table content is recommended. +connection master; +TRUNCATE TABLE `mysql`.`slave_master_info`; +TRUNCATE TABLE `mysql`.`slave_relay_log_info`; +******************************************************************** +* Test case2: Upgrade when repository tables are empty. * +* mysql_upgrade script should not report any warning. * +******************************************************************** +connection master; +connection slave; +"====== Clean up ======" +connection master; +DROP TABLE `mysql`.`slave_master_info`, `mysql`.`slave_relay_log_info`; +include/rpl_end.inc diff --git a/mysql-test/main/rpl_mysql_upgrade_slave_repo_check.test b/mysql-test/main/rpl_mysql_upgrade_slave_repo_check.test new file mode 100644 index 00000000000..24b5f029e8d --- /dev/null +++ b/mysql-test/main/rpl_mysql_upgrade_slave_repo_check.test @@ -0,0 +1,127 @@ +# ==== Purpose ==== +# +# While upgrading from "mysql" to "mariadb" if slave info repositories are +# configured to be tables then appropriate warnings should be reported. +# +# ==== Implementation ==== +# +# Steps: +# 1 - On MariaDB server create `mysql`.`slave_master_info` and +# `mysql.slave_relay_log_info` tables to simulate upgrade from "mysql" +# to "mariadb" server. Insert data into these tables. +# 2 - Execute "mysql_upgrade" script and verify that appropriate warning +# is reported. i.e Warning is to alert user that the data present in +# repository tables will be ignored. +# 3 - Truncate these tables. This simulates repositories being file and +# the tables are empty. +# 4 - Execute "mysql_upgrade" script and verify that no warnings are +# reported. +# +# ==== References ==== +# +# MDEV-10047: table-based master info repository +# + +--source include/have_innodb.inc +--source include/mysql_upgrade_preparation.inc +--source include/have_binlog_format_mixed.inc +--source include/master-slave.inc + +--write_file $MYSQLTEST_VARDIR/tmp/slave_table_repo_init.sql +--disable_query_log +--disable_result_log +SET SQL_LOG_BIN=0; +# Table structure extracted from MySQL-5.6.47 +CREATE TABLE `mysql`.`slave_master_info` ( + `Number_of_lines` int(10) unsigned NOT NULL COMMENT 'Number of lines in the file.', + `Master_log_name` text CHARACTER SET utf8 COLLATE utf8_bin NOT NULL COMMENT 'The name of the master binary log currently being read from the master.', + `Master_log_pos` bigint(20) unsigned NOT NULL COMMENT 'The master log position of the last read event.', + `Host` char(64) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT '' COMMENT 'The host name of the master.', + `User_name` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The user name used to connect to the master.', + `User_password` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The password used to connect to the master.', + `Port` int(10) unsigned NOT NULL COMMENT 'The network port used to connect to the master.', + `Connect_retry` int(10) unsigned NOT NULL COMMENT 'The period (in seconds) that the slave will wait before trying to reconnect to the master.', + `Enabled_ssl` tinyint(1) NOT NULL COMMENT 'Indicates whether the server supports SSL connections.', + `Ssl_ca` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The file used for the Certificate Authority (CA) certificate.', + `Ssl_capath` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The path to the Certificate Authority (CA) certificates.', + `Ssl_cert` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The name of the SSL certificate file.', + `Ssl_cipher` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The name of the cipher in use for the SSL connection.', + `Ssl_key` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The name of the SSL key file.', + `Ssl_verify_server_cert` tinyint(1) NOT NULL COMMENT 'Whether to verify the server certificate.', + `Heartbeat` float NOT NULL, + `Bind` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'Displays which interface is employed when connecting to the MySQL server', + `Ignored_server_ids` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The number of server IDs to be ignored, followed by the actual server IDs', + `Uuid` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The master server uuid.', + `Retry_count` bigint(20) unsigned NOT NULL COMMENT 'Number of reconnect attempts, to the master, before giving up.', + `Ssl_crl` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The file used for the Certificate Revocation List (CRL)', + `Ssl_crlpath` text CHARACTER SET utf8 COLLATE utf8_bin COMMENT 'The path used for Certificate Revocation List (CRL) files', + `Enabled_auto_position` tinyint(1) NOT NULL COMMENT 'Indicates whether GTIDs will be used to retrieve events from the master.', + PRIMARY KEY (`Host`,`Port`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8 STATS_PERSISTENT=0 COMMENT='Master Information'; + +INSERT INTO `mysql`.`slave_master_info` VALUES (23,'master-bin.000001', 120, 'localhost', 'root'," ", 13000, 60, 0," "," "," "," "," ",0 , 60," ", " ", '28e10fdd-6289-11ea-aab9-207918567a34',10," "," ", 0 ); + +# Table structure extracted from MySQL-5.6.47 +CREATE TABLE `mysql`.`slave_relay_log_info` ( + `Number_of_lines` int(10) unsigned NOT NULL COMMENT 'Number of lines in the file or rows in the table. Used to version table definitions.', + `Relay_log_name` text CHARACTER SET utf8 COLLATE utf8_bin NOT NULL COMMENT 'The name of the current relay log file.', + `Relay_log_pos` bigint(20) unsigned NOT NULL COMMENT 'The relay log position of the last executed event.', + `Master_log_name` text CHARACTER SET utf8 COLLATE utf8_bin NOT NULL COMMENT 'The name of the master binary log file from which the events in the relay log file were read.', + `Master_log_pos` bigint(20) unsigned NOT NULL COMMENT 'The master log position of the last executed event.', + `Sql_delay` int(11) NOT NULL COMMENT 'The number of seconds that the slave must lag behind the master.', + `Number_of_workers` int(10) unsigned NOT NULL, + `Id` int(10) unsigned NOT NULL COMMENT 'Internal Id that uniquely identifies this record.', + PRIMARY KEY (`Id`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8 STATS_PERSISTENT=0 COMMENT='Relay Log Information'; + +INSERT INTO `mysql`.`slave_relay_log_info` VALUES (7,'./slave-relay-bin.000001',4 ," ",0, 0 ,0 , 1); +SET SQL_LOG_BIN=1; +--enable_query_log +--enable_result_log +EOF + +--echo ******************************************************************** +--echo * Test case1: Upgrade when repository tables have data. * +--echo * mysql_upgrade script should report warnings. * +--echo ******************************************************************** +--connection master +--source $MYSQLTEST_VARDIR/tmp/slave_table_repo_init.sql +--exec $MYSQL_UPGRADE --skip-verbose --force --user=root > $MYSQLTEST_VARDIR/log/mysql_upgrade_master.log 2>&1 +--cat_file $MYSQLTEST_VARDIR/log/mysql_upgrade_master.log + +--connection slave +--source $MYSQLTEST_VARDIR/tmp/slave_table_repo_init.sql +--exec $MYSQL_UPGRADE --skip-verbose --force --user=root > $MYSQLTEST_VARDIR/log/mysql_upgrade_slave.log 2>&1 +--cat_file $MYSQLTEST_VARDIR/log/mysql_upgrade_slave.log + +--connection master +let $datadir= `select @@datadir`; +remove_file $datadir/mysql_upgrade_info; +TRUNCATE TABLE `mysql`.`slave_master_info`; +TRUNCATE TABLE `mysql`.`slave_relay_log_info`; +--remove_file $MYSQLTEST_VARDIR/log/mysql_upgrade_master.log +--remove_file $MYSQLTEST_VARDIR/log/mysql_upgrade_slave.log + +--echo ******************************************************************** +--echo * Test case2: Upgrade when repository tables are empty. * +--echo * mysql_upgrade script should not report any warning. * +--echo ******************************************************************** +--connection master +--exec $MYSQL_UPGRADE --skip-verbose --force --user=root > $MYSQLTEST_VARDIR/log/mysql_upgrade_master.log 2>&1 +--cat_file $MYSQLTEST_VARDIR/log/mysql_upgrade_master.log + +--connection slave +--exec $MYSQL_UPGRADE --skip-verbose --force --user=root > $MYSQLTEST_VARDIR/log/mysql_upgrade_slave.log 2>&1 +--cat_file $MYSQLTEST_VARDIR/log/mysql_upgrade_slave.log + +--echo "====== Clean up ======" +--connection master +let $datadir= `select @@datadir`; +remove_file $datadir/mysql_upgrade_info; +DROP TABLE `mysql`.`slave_master_info`, `mysql`.`slave_relay_log_info`; + +--remove_file $MYSQLTEST_VARDIR/tmp/slave_table_repo_init.sql +--remove_file $MYSQLTEST_VARDIR/log/mysql_upgrade_master.log +--remove_file $MYSQLTEST_VARDIR/log/mysql_upgrade_slave.log + +--source include/rpl_end.inc

1 0

[Maria-developers] Please review MDEV-17832 Protocol: extensions for Pluggable types and JSON, GEOMETRY
by Alexander Barkov 06 Mar '20

06 Mar '20

Hi, Sergei, Georg, Please review a fixed version of the patch for MDEV-17832. There are two files attached: - mdev-17821.v18.diff (server changes) - mdev-17821-cli.v06.diff (libmariadb changes) Comparing to the previous version, this version: 1. Adds a new structure MA_FIELD_EXTENSION 2. Moves extended data type information from MYSQL_FIELD to MYSQL_FIELD::extension in the client-server implementation. Note, in case of embedded server, the extended metadata is stored directly to MYSQL_FIELD. 3. Adds a new API function mariadb_field_metadata_attr(), to extract metadata from MYSQL_FIELD. 4. Changes the way how the metadata is packed on the wire from "easily human readable" to "easily parse-able", which: - makes the things faster - allows to transfer arbitrary binary data in the future, if needed. Every metadata chunk is now encoded as: a. chunk type (1 byte) b. chunk data length (1 byte) c. chunk data (according to #b) For now, two chunk types are implemented: - data type name (used for GEOMETRY sub-types, and for INET6) - format name (for JSON) Thanks!

3 4