- developers - lists.mariadb.org

[Maria-developers] MDEV-8109 unexpected CAST result
by Alexander Barkov 11 Nov '15

11 Nov '15

Hi Serg, Please review my patch for MDEV-8109. Thanks!

2 1

[Maria-developers] Please review MDEV-8140 patch
by chengxiaoz 10 Nov '15

10 Nov '15

Hi kentoku and Sergey, Please review mypatch for MDEV-8140. https://github.com/MariaDB/server/pull/61 Thanks.

3 3

[Maria-developers] Please review MDEV-8140 patch
by chengxiaoz＠189.cn 08 Nov '15

08 Nov '15

1 0

Re: [Maria-developers] [Commits] 7931ca4: MDEV-9024 Build fails with VS2015
by Sergei Golubchik 05 Nov '15

05 Nov '15

Hi, Wlad! Mostly ok, please see below On Nov 03, wlad(a)mariadb.com wrote: > revision-id: 7931ca4f89ad5cd38ea3dc54f1363b7de2fd3414 (mariadb-10.0.22-5-g7931ca4) > parent(s): fa1438cbf4307731a54ea4137d5f7d4b744cdfbc > committer: Vladislav Vaintroub > timestamp: 2015-11-03 11:32:00 +0100 > message: > > MDEV-9024 Build fails with VS2015 > > Fix build failures caused by new C runtime library > - isnan, snprintf, struct timespec are now defined, attempt to redefine them leads > - P_tmpdir, tzname are no more defined > - lfind() and lsearch() in lf_hash.c had to be renamed, declaration > conflicts with some C runtime functions with the same name declared > in a header included by stdlib.h > > Also fix couple of annoying warnings : > - remove #define NOMINMAX from config.h to avoid "redefined" compiler > warnings(NOMINMAX is already in compile flags) > > - disable incremental linker in Debug as well (feature not used much > and compiler crashes often) > > > Also simplify package building with Wix, require Wix 3.9 or later > (VS2015 is not compatible with old Wix 3.5/3.6) > > diff --git a/cmake/os/Windows.cmake b/cmake/os/Windows.cmake > index 4c01bab..fd6e739 100644 > --- a/cmake/os/Windows.cmake > +++ b/cmake/os/Windows.cmake > @@ -181,14 +184,13 @@ CHECK_SYMBOL_REPLACEMENT(S_IROTH _S_IREAD sys/stat.h) > CHECK_SYMBOL_REPLACEMENT(S_IFIFO _S_IFIFO sys/stat.h) > CHECK_SYMBOL_REPLACEMENT(SIGQUIT SIGTERM signal.h) > CHECK_SYMBOL_REPLACEMENT(SIGPIPE SIGINT signal.h) > -CHECK_SYMBOL_REPLACEMENT(isnan _isnan float.h) How can this be wrong? It checks whetherone should use _isnan instead of isnan. That seems like the kind of check that works both with old and new VS. > CHECK_SYMBOL_REPLACEMENT(finite _finite float.h) > diff --git a/config.h.cmake b/config.h.cmake > index 46eed79..a1d505f 100644 > --- a/config.h.cmake > +++ b/config.h.cmake > > -/* We don't want the min/max macros */ > -#ifdef __WIN__ > -#define NOMINMAX > -#endif I'd generally prefer to define symbols in headers, instead of putting everything into ever-growing compiler command line Besides, the minimal fix for the redefinition warning is -#define NOMINMAX +#define NOMINMAX 1 because -DNOMINMAX defines NOMINMAX to be 1, not empty. > /* > MySQL features > */ > diff --git a/mysys/lf_hash.c b/mysys/lf_hash.c > index aa96ca9..fa92971 100644 > --- a/mysys/lf_hash.c > +++ b/mysys/lf_hash.c > @@ -43,7 +43,7 @@ const int LF_HASH_OVERHEAD= sizeof(LF_SLIST); > > /* > a structure to pass the context (pointers two the three successive elements > - in a list) from lfind to linsert/ldelete > + in a list) from my_lfind to my_linsert/my_ldelete > */ > typedef struct { > intptr volatile *prev; > @@ -70,7 +70,7 @@ typedef struct { > cursor is positioned in either case > pins[0..2] are used, they are NOT removed on return > */ > -static int lfind(LF_SLIST * volatile *head, CHARSET_INFO *cs, uint32 hashnr, > +static int my_lfind(LF_SLIST * volatile *head, CHARSET_INFO *cs, uint32 hashnr, er... I don't like that. lfind/lsearch are internal static functions in lf_hash.c, they shouldn't need or use my_ prefix. Two alternatives: 1. #define lfind remap_lfind_for_win #include <my_global.h> #include <my_sys.h> #undef lfind 2. -static int lfind(LF_SLIST * volatile *head, CHARSET_INFO *cs, uint32 hashnr, +static int l_find(LF_SLIST * volatile *head, CHARSET_INFO *cs, uint32 hashnr, > const uchar *key, uint keylen, CURSOR *cursor, LF_PINS *pins) > { > uint32 cur_hashnr; > diff --git a/win/packaging/CMakeLists.txt b/win/packaging/CMakeLists.txt > index 3b9b72f..0535a48 100644 > --- a/win/packaging/CMakeLists.txt > +++ b/win/packaging/CMakeLists.txt > SET(CPACK_WIX_UPGRADE_CODE "49EB7A6A-1CEF-4A1E-9E89-B9A4993963E3") > - SET(CPACK_WIX_PACKAGE_NAME "MariaDB @MAJOR_VERSION@.@MINOR_VERSION@") > + SET(CPACK_WIX_PACKAGE_NAME "MariaDB ${MAJOR_VERSION}.${MINOR_VERSION}") > ELSE() > SET(CPACK_WIX_UPGRADE_CODE "2331E7BD-EE58-431B-9E18-B2B918BCEB1B") > - SET(CPACK_WIX_PACKAGE_NAME "MariaDB @MAJOR_VERSION@.@MINOR_VERSION@ (x64)") > + SET(CPACK_WIX_PACKAGE_NAME "MariaDB ${MAJOR_VERSION}.${MINOR_VERSION} (x64)") Weird. I was sure I've fixed this months ago :-[] Regards, Sergei

1 0

Re: [Maria-developers] [Commits] 01bd373: MDEV-8437 - plugin variables conflict with bootstrap
by Sergei Golubchik 05 Nov '15

05 Nov '15

Hi, Sergey! On Nov 03, Sergey Vojtovich wrote: > revision-id: 01bd3737777f8f0cf2e1737d01a48033a7615b2a (mariadb-10.1.8-23-g01bd373) > parent(s): dc40d737d4af3cb0e272c58507ad54e4eb24be28 > committer: Sergey Vojtovich > timestamp: 2015-11-03 14:02:49 +0400 > message: > > MDEV-8437 - plugin variables conflict with bootstrap ok to push > There is no strong need to change password column: the only side effect is > that 4.0 -> 10.1 upgrades may get root/debian-sys-maint passwords stored in old > format. This should be perfectly acceptable, since all password at this point > are stored in old format. Regards, Sergei

2 3

Re: [Maria-developers] [Commits] 178cddd: MDEV-9082 - Debian: mysql_install_db is called on upgrade
by Sergei Golubchik 05 Nov '15

05 Nov '15

Hi, Sergey! On Nov 05, Sergey Vojtovich wrote: > revision-id: 178cddd664cb4ce2c6ccbe76c33e4fd2c1fc1b3d (mariadb-10.1.8-25-g178cddd) > parent(s): 2ef562d2c74297b0b92c4322697dad98767cc873 > committer: Sergey Vojtovich > timestamp: 2015-11-05 13:57:24 +0400 > message: > > MDEV-9082 - Debian: mysql_install_db is called on upgrade looks ok to me Regards, Sergei

1 0

Re: [Maria-developers] [Commits] 2ef562d: MDEV-9080 - Debian: incorrect empty password check in postinst
by Sergei Golubchik 05 Nov '15

05 Nov '15

Hi, Sergey! ok to push On Nov 04, Sergey Vojtovich wrote: > revision-id: 2ef562d2c74297b0b92c4322697dad98767cc873 (mariadb-10.1.8-24-g2ef562d) > parent(s): 01bd3737777f8f0cf2e1737d01a48033a7615b2a > committer: Sergey Vojtovich > timestamp: 2015-11-04 20:37:16 +0400 > message: > > MDEV-9080 - Debian: incorrect empty password check in postinst > > There was code that was supposed to "catch upgrades from previous versions where > the root password wasn't set". But it is wrong in many regards: > - it is supposed to be executed against running server, but at this point server > should be down, which makes this code no-op > - if the above is fixed, root password will be requested twice (initial root > password request + this one) > - it asks for a password only once, while "initial root password request" asks > twice (password + password verification) > - it may give false positive if unix socket based authentication is in effect > > Removed this code since it didn't work for quite a while (at least since > mysql-5.1) and nobody cared about it. Regards, Sergei

1 0

Re: [Maria-developers] [Commits] dc40d73: MDEV-8437 - plugin variables conflict with bootstrap
by Sergei Golubchik 05 Nov '15

05 Nov '15

Hi, Sergey! On Nov 03, Sergey Vojtovich wrote: > revision-id: dc40d737d4af3cb0e272c58507ad54e4eb24be28 (mariadb-10.1.8-22-gdc40d73) > parent(s): a574407444fc3ea93ca88afa93dc18154251bf74 > committer: Sergey Vojtovich > timestamp: 2015-11-03 12:08:23 +0400 > message: > > MDEV-8437 - plugin variables conflict with bootstrap ok to push > Removed redundant attempt to create mysql.plugin table: > - original code was supposed to INSTALL some plugins: > INSERT INTO plugin VALUES ('innodb', 'ha_innodb.so'), > ('federated', 'ha_federated.so'), ('blackhole', 'ha_blackhole.so'), > ('archive', 'ha_archive.so'); I didn't see any INSERT's in the postinst script. > - original code was supposed to fail if mysql.plugin exists: > The query sequence is supposed to be aborted if the CREATE TABLE fails due > to an already existent table in which case the admin might already have > chosen to remove one or more plugins. > - mysql.plugin must've been created by preceeding mysql_install_db anyway > Regards, Sergei

2 1

Re: [Maria-developers] [Commits] 4685c90: mariadb@.service.in should be configured for all systemd platforms.
by Sergey Vojtovich 04 Nov '15

04 Nov '15

Hi Nirbhay, taking into account that CPackRPM won't be able to install it, what's the purpose of having this configured for all systemd platforms? Regards, Sergey On Tue, Nov 03, 2015 at 05:38:12PM -0500, Nirbhay Choubey wrote: > revision-id: 4685c903bc822596bcf12cbfdb5bcc4460ed0105 > parent(s): 95289e5b665d6a3e37f938d0db978d0d0166e493 > committer: Nirbhay Choubey > branch nick: 10.1 > timestamp: 2015-11-03 17:38:11 -0500 > message: > > mariadb@.service.in should be configured for all systemd platforms. > > --- > support-files/CMakeLists.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/support-files/CMakeLists.txt b/support-files/CMakeLists.txt > index 2ab4350..2929db1 100644 > --- a/support-files/CMakeLists.txt > +++ b/support-files/CMakeLists.txt > @@ -79,9 +79,9 @@ IF(UNIX) > IF(HAVE_SYSTEMD) > CONFIGURE_FILE(mariadb.service.in > ${CMAKE_CURRENT_BINARY_DIR}/mariadb.service @ONLY) > + CONFIGURE_FILE(mariadb@.service.in > + ${CMAKE_CURRENT_BINARY_DIR}/mariadb@.service @ONLY) > IF(NOT RPM) > - CONFIGURE_FILE(mariadb@.service.in > - ${CMAKE_CURRENT_BINARY_DIR}/mariadb@.service @ONLY) > INSTALL(FILES use_galera_new_cluster.conf > ${CMAKE_CURRENT_BINARY_DIR}/mariadb.service > ${CMAKE_CURRENT_BINARY_DIR}/mariadb@.service > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 2

[Maria-developers] write_set.diff review
by Sergei Golubchik 04 Nov '15

04 Nov '15

Hi, Summary: 1. Three questions (are you sure, what guarantee, why) 2. A couple of suggestions (spilt a commit, move function call out of DBUG_RETURN) 3. Two suggestions for unrelated optimizations (don't allocate unneded bitmaps, don't modify read_set) > commit 2734eca05d499bcbcaf428d04d4f1acbb298589f > Author: Monty <monty(a)mariadb.org> > Date: Mon Nov 2 11:52:06 2015 +0200 > > diff --git a/sql/table.h b/sql/table.h > index b150e4f..6ecfdef 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -1066,9 +1066,11 @@ struct TABLE > String alias; /* alias or table name */ > uchar *null_flags; > MY_BITMAP def_read_set, def_write_set, def_vcol_set, tmp_set; > + MY_BITMAP def_rpl_write_set; I would rather not allocate more bitmaps directly in the TABLE. They are not always needed, and only increase the TABLE size. The same is true for def_vcol_set, in fact, it's *even more true* for vcols. Instead I'd allocate them in the TABLE's memroot as needed. > MY_BITMAP eq_join_set; /* used to mark equi-joined fields */ > MY_BITMAP cond_set; /* used to mark fields from sargable conditions*/ > - MY_BITMAP *read_set, *write_set, *vcol_set; /* Active column sets */ > + /* Active column sets */ > + MY_BITMAP *read_set, *write_set, *vcol_set, *rpl_write_set; > /* > The ID of the query that opened and is using this table. Has different > meanings depending on the table type. > diff --git a/sql/table.cc b/sql/table.cc > index 76cb4b8..8816ee0 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2884,20 +2884,28 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share, > /* Allocate bitmaps */ > > bitmap_size= share->column_bitmap_size; > - if (!(bitmaps= (uchar*) alloc_root(&outparam->mem_root, bitmap_size*6))) > + if (!(bitmaps= (uchar*) alloc_root(&outparam->mem_root, bitmap_size*7))) > goto err; > my_bitmap_init(&outparam->def_read_set, > (my_bitmap_map*) bitmaps, share->fields, FALSE); > my_bitmap_init(&outparam->def_write_set, > - (my_bitmap_map*) (bitmaps+bitmap_size), share->fields, FALSE); > + (my_bitmap_map*) (bitmaps+bitmap_size), share->fields, > + FALSE); > my_bitmap_init(&outparam->def_vcol_set, > - (my_bitmap_map*) (bitmaps+bitmap_size*2), share->fields, FALSE); > + (my_bitmap_map*) (bitmaps+bitmap_size*2), share->fields, > + FALSE); This is totally wasted memory if the table has no virtual columns. (that's what I meant by *even more true* - rpl bitmap and eq_join_set, cond_set bitmaps may or may not be used for this table, one never knows. But for vcols it's always known in advance whether the table has them or not). Still, I'd allocate other bitmaps on demand too. > my_bitmap_init(&outparam->tmp_set, > - (my_bitmap_map*) (bitmaps+bitmap_size*3), share->fields, FALSE); > + (my_bitmap_map*) (bitmaps+bitmap_size*3), share->fields, > + FALSE); > my_bitmap_init(&outparam->eq_join_set, > - (my_bitmap_map*) (bitmaps+bitmap_size*4), share->fields, FALSE); > + (my_bitmap_map*) (bitmaps+bitmap_size*4), share->fields, > + FALSE); > my_bitmap_init(&outparam->cond_set, > - (my_bitmap_map*) (bitmaps+bitmap_size*5), share->fields, FALSE); > + (my_bitmap_map*) (bitmaps+bitmap_size*5), share->fields, > + FALSE); > + my_bitmap_init(&outparam->def_rpl_write_set, > + (my_bitmap_map*) (bitmaps+bitmap_size*6), share->fields, > + FALSE); > outparam->default_column_bitmaps(); > > outparam->cond_selectivity= 1.0; > @@ -5986,36 +5997,48 @@ void TABLE::mark_columns_needed_for_insert() > we only want to log a PK and we needed other fields for > execution). > */ > + > void TABLE::mark_columns_per_binlog_row_image() > { > + THD *thd= in_use; > DBUG_ENTER("mark_columns_per_binlog_row_image"); > DBUG_ASSERT(read_set->bitmap); > DBUG_ASSERT(write_set->bitmap); > > - THD *thd= current_thd; Hmm. Are you sure that TABLE::in_use is *always* set at this point? You've basically replaced - THD *thd=current_thd; - if (in_use && in_use->is_current_stmt_binlog_format_row) + THD *thd=in_use; + if (thd->is_current_stmt_binlog_format_row) And, btw, where's your MDEV for caching rbr checks per statement? > + /* If not using row format */ > + rpl_write_set= write_set; > > /** > If in RBR we may need to mark some extra columns, > depending on the binlog-row-image command line argument. > */ > if ((WSREP_EMULATE_BINLOG(thd) || mysql_bin_log.is_open()) && > - in_use && > - in_use->is_current_stmt_binlog_format_row() && > + thd->is_current_stmt_binlog_format_row() && > !ha_check_storage_engine_flag(s->db_type(), HTON_NO_BINLOG_ROW_OPT)) > { > /* if there is no PK, then mark all columns for the BI. */ > if (s->primary_key >= MAX_KEY) > + { > bitmap_set_all(read_set); > + rpl_write_set= read_set; > + } > + else > + { > switch (thd->variables.binlog_row_image) { > case BINLOG_ROW_IMAGE_FULL: > - if (s->primary_key < MAX_KEY) > bitmap_set_all(read_set); > - bitmap_set_all(write_set); > + /* Set of columns that should be written (all) */ > + rpl_write_set= read_set; > break; > case BINLOG_ROW_IMAGE_NOBLOB: > - /* for every field that is not set, mark it unless it is a blob */ > + /* Only write changed columns + not blobs */ > + rpl_write_set= &def_rpl_write_set; > + bitmap_copy(rpl_write_set, write_set); > + > + /* > + for every field that is not set, mark it unless it is a blob or > + part of a primary key > + */ > for (Field **ptr=field ; *ptr ; ptr++) > { > Field *my_field= *ptr; > @@ -6027,23 +6050,24 @@ void TABLE::mark_columns_per_binlog_row_image() > If set in the AI, then the blob is really needed, there is > nothing we can do about it. > */ > - if ((s->primary_key < MAX_KEY) && > - ((my_field->flags & PRI_KEY_FLAG) || > - (my_field->type() != MYSQL_TYPE_BLOB))) > + if ((my_field->flags & PRI_KEY_FLAG) || > + (my_field->type() != MYSQL_TYPE_BLOB)) > + { > bitmap_set_bit(read_set, my_field->field_index); > - > - if (my_field->type() != MYSQL_TYPE_BLOB) > - bitmap_set_bit(write_set, my_field->field_index); > + bitmap_set_bit(rpl_write_set, my_field->field_index); > } > + } > break; > case BINLOG_ROW_IMAGE_MINIMAL: > - /* mark the primary key if available in the read_set */ > - if (s->primary_key < MAX_KEY) > + /* mark the primary key */ > mark_columns_used_by_index_no_reset(s->primary_key, read_set); > + /* Only write columns that have changed */ > + rpl_write_set= write_set; Where's the guarantee that the primary key is in the write set? > break; > > default: > DBUG_ASSERT(FALSE); > + } > } > file->column_bitmaps_signal(); > } > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 860426c..6201065 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -11140,26 +11138,21 @@ int QUICK_RANGE_SELECT::reset() > int QUICK_RANGE_SELECT::get_next() > { > range_id_t dummy; > + int result; > + DBUG_ENTER("QUICK_RANGE_SELECT::get_next"); > + > + if (!in_ror_merged_scan) > + DBUG_RETURN(file->multi_range_read_next(&dummy)); please don't put function calls inside DBUG_RETURN, always use a variable to store the return value first > + > MY_BITMAP * const save_read_set= head->read_set; > MY_BITMAP * const save_write_set= head->write_set; > - > - DBUG_ENTER("QUICK_RANGE_SELECT::get_next"); > - if (in_ror_merged_scan) > - { > /* > We don't need to signal the bitmap change as the bitmap is always the > same for this head->file > */ > head->column_bitmaps_set_no_signal(&column_bitmap, &column_bitmap); > - } > - > - int result= file->multi_range_read_next(&dummy); > + result= file->multi_range_read_next(&dummy); > - > - if (in_ror_merged_scan) > - { > - /* Restore bitmaps set on entry */ > head->column_bitmaps_set_no_signal(save_read_set, save_write_set); > - } > DBUG_RETURN(result); > } Ok, all changes in opt_range.cc are unrelated to your rpl_write_set patch, please commit them separately > diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc > index ee222f0..208b2b6 100644 > --- a/sql/rpl_record.cc > +++ b/sql/rpl_record.cc > @@ -434,7 +433,7 @@ unpack_row(rpl_group_info *rgi, > *master_reclength = table->s->reclength; > } > > - DBUG_RETURN(error); > + DBUG_RETURN(0); > } > > /** also unrelated to rpl_write_set. If you'd like you can put it in the same commit with opt_range.cc changes, with the sybject "cleanups" or something > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 5054357..b238e89 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -6259,11 +6248,11 @@ int THD::binlog_delete_row(TABLE* table, bool is_trans, > DBUG_ASSERT(is_current_stmt_binlog_format_row() && > ((WSREP(this) && wsrep_emulate_bin_log) || mysql_bin_log.is_open())); > /** > - Save a reference to the original read and write set bitmaps. > - We will need this to restore the bitmaps at the end. > + Save a reference to the original read bitmaps > + We will need this to restore the bitmaps at the end as > + binlog_prepare_row_images() may changetable->read_set. > */ This doesn't make much sense. Here, the code is like that THD::binlog_delete_row: save table->read_set THD::binlog_prepare_row_images calculate table->tmp_set table->read_set=table->tmp_set pack_row(table->read_set) for every set bit in the set_arg call field->pack() restore table->read_set So, two thoughts here: 1. tmp_set must be a strict subset of read_set! other fields may not be present in the record[0] and there're no row reads between binlog_prepare_row_images and field->pack 2. One can pass table->tmp_set directly into pack_row() as an argument which means there's no need for binlog_prepare_row_images to modify table->read_set at all > MY_BITMAP *old_read_set= table->read_set; > - MY_BITMAP *old_write_set= table->write_set; > > /** > This will remove spurious fields required during execution but > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 488826a..0661e70 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2867,5 +2867,7 @@ pthread_handler_t handle_delayed_insert(void *arg) > /* Tell client that the thread is initialized */ > mysql_cond_signal(&di->cond_client); > > + di->table->mark_columns_needed_for_insert(); why? > + > /* Now wait until we get an insert or lock to handle */ > /* We will not abort as long as a client thread uses this thread */ Regards, Sergei

1 0