February 2017 - developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 13ebc9c: MDEV-11645: archive.archive fails in buildbot with valgrind (Use of uninitialised value)
by Sergey Petrunia 20 Feb '17

20 Feb '17

Hi Varun, On Mon, Feb 13, 2017 at 03:47:57AM +0530, Varun wrote: > revision-id: 13ebc9c826de080e416071b7881585b45922bef9 (mariadb-10.2.3-185-g13ebc9c) > parent(s): 3ae038b732ce503fb839e9095355e05f5c6866f9 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-02-13 03:45:59 +0530 > message: > > MDEV-11645: archive.archive fails in buildbot with valgrind (Use of uninitialised value) > > Set null values to record_buffer->buffer for the VARCHAR fields > > --- > storage/archive/ha_archive.cc | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/storage/archive/ha_archive.cc b/storage/archive/ha_archive.cc > index 9d6d100..329e3d5 100644 > --- a/storage/archive/ha_archive.cc > +++ b/storage/archive/ha_archive.cc > @@ -372,10 +372,31 @@ int Archive_share::write_v1_metafile() > > unsigned int ha_archive::pack_row_v1(uchar *record) > { > + uint offset= 0,length; > uint *blob, *end; > uchar *pos; > DBUG_ENTER("pack_row_v1"); > memcpy(record_buffer->buffer, record, table->s->reclength); > + for(Field** field= table->field; (*field) ; field++) > + { > + > + Field *fld= *field; > + > + if(fld->type() == MYSQL_TYPE_VARCHAR) > + { > + uint start= (fld->ptr - table->record[0]); > + uint index_end= start+fld->field_length; There is no reason to compute the above variables when the field value is an SQL NULL. Please move them into the if. Please also use ptrdiff_t datatype instead of uint. A different question: did you check what happens when one attempts to write an SQL NULL value? > + > + if(!(fld->is_real_null(record - table->record[0]))) > + { > + Field_varstring *const field_var = (Field_varstring *)fld; > + offset = field_var->data_length() + field_var->length_size(); Coding style says there should be no space before the assignment. Please fix the above two lines. > + > + for(uint index= start + offset; index <= index_end ; index++) > + record_buffer->buffer[index]=0; Is there any reason you're not using bzero or memset call here instead? (please check with Serg which one is guaranteed to be available) > + } > + } > + } > pos= record_buffer->buffer + table->s->reclength; > for (blob= table->s->blob_field, end= blob + table->s->blob_fields; > blob != end; blob++) BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 01d5d6db4c3: Fix GCC 6.3.0 warnings.
by Jan Lindström 16 Feb '17

16 Feb '17

ok to push. On Thu, Feb 16, 2017 at 11:17 AM, <marko.makela(a)mariadb.com> wrote: > revision-id: 01d5d6db4c337daae0aa593341e081cc6bfebb8a (mariadb-10.0.29-25- > g01d5d6db4c3) > parent(s): 6011fb6daa5ac9e6cb97a23b877e4d2568c85d94 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2017-02-16 11:16:27 +0200 > message: > > Fix GCC 6.3.0 warnings. > > --- > storage/innobase/fsp/fsp0fsp.cc | 1 + > storage/innobase/row/row0ftsort.cc | 4 ---- > storage/xtradb/fsp/fsp0fsp.cc | 1 + > 3 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/storage/innobase/fsp/fsp0fsp.cc b/storage/innobase/fsp/ > fsp0fsp.cc > index afdd6667711..836ed2de27a 100644 > --- a/storage/innobase/fsp/fsp0fsp.cc > +++ b/storage/innobase/fsp/fsp0fsp.cc > @@ -2801,6 +2801,7 @@ fsp_reserve_free_extents( > } > } else { > ut_a(alloc_type == FSP_CLEANING); > + reserve = 0; > } > > success = fil_space_reserve_free_extents(space, n_free, n_ext); > diff --git a/storage/innobase/row/row0ftsort.cc b/storage/innobase/row/ > row0ftsort.cc > index f662ef6b461..4865c8e850a 100644 > --- a/storage/innobase/row/row0ftsort.cc > +++ b/storage/innobase/row/row0ftsort.cc > @@ -620,7 +620,6 @@ fts_parallel_tokenization( > mem_heap_t* blob_heap = NULL; > fts_doc_t doc; > dict_table_t* table = psort_info->psort_common->new_ > table; > - dict_field_t* idx_field; > fts_tokenize_ctx_t t_ctx; > ulint retried = 0; > dberr_t error = DB_SUCCESS; > @@ -642,9 +641,6 @@ fts_parallel_tokenization( > doc.charset = fts_index_get_charset( > psort_info->psort_common->dup->index); > > - idx_field = dict_index_get_nth_field( > - psort_info->psort_common->dup->index, 0); > - > block = psort_info->merge_block; > zip_size = dict_table_zip_size(table); > > diff --git a/storage/xtradb/fsp/fsp0fsp.cc b/storage/xtradb/fsp/fsp0fsp.cc > index 0bdb2ea0d4c..64576c93545 100644 > --- a/storage/xtradb/fsp/fsp0fsp.cc > +++ b/storage/xtradb/fsp/fsp0fsp.cc > @@ -2810,6 +2810,7 @@ fsp_reserve_free_extents( > } > } else { > ut_a(alloc_type == FSP_CLEANING); > + reserve = 0; > } > > success = fil_space_reserve_free_extents(space, n_free, n_ext); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] ab0943d5b11: Fix gcc 6.3.x compiler warnings.
by Marko Mäkelä 16 Feb '17

16 Feb '17

On Thu, Feb 16, 2017 at 10:26 AM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: ab0943d5b11f01fb3f4638134bd1b8fb6a14a5e0 (mariadb-5.5.54-23- > gab0943d5b11) > parent(s): 2e8fa1c2b2867b99b5cc7f987ad64b666e0af2c4 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-02-16 10:25:27 +0200 > message: > > Fix gcc 6.3.x compiler warnings. > > These are caused by fact that functions are declared with > __attribute__((nonnull)) or left shit like ~0 << macro > when ~0U << macro should be used. > OK to push, except for the following unrelated changes (which are OK in a separate commit): diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/ > trx0trx.h > index 4ade245f03e..94707ecf7cb 100644 > --- a/storage/innobase/include/trx0trx.h > +++ b/storage/innobase/include/trx0trx.h > @@ -481,6 +481,7 @@ struct trx_struct{ > of view of concurrency control: > TRX_ACTIVE, > TRX_COMMITTED_IN_MEMORY, > ... */ > + os_thread_id_t thread_id; > /*------------------------------*/ > /* MySQL has a transaction coordinator to coordinate two phase > commit between multiple storage engines and the binary > log. When > diff --git a/storage/innobase/trx/trx0trx.c b/storage/innobase/trx/ > trx0trx.c > index 2d585b7507f..9a80478f6f1 100644 > --- a/storage/innobase/trx/trx0trx.c > +++ b/storage/innobase/trx/trx0trx.c > @@ -40,6 +40,7 @@ Created 3/26/1996 Heikki Tuuri > #include "srv0srv.h" > #include "btr0sea.h" > #include "os0proc.h" > +#include "os0thread.h" > #include "trx0xa.h" > #include "trx0purge.h" > #include "ha_prototypes.h" > @@ -97,7 +98,6 @@ trx_create( > trx_t* trx; > > ut_ad(mutex_own(&kernel_mutex)); > - ut_ad(sess); > > trx = mem_alloc(sizeof(trx_t)); > > @@ -1770,7 +1770,7 @@ trx_print( > { > ibool newline; > > - fprintf(f, "TRANSACTION " TRX_ID_FMT, (ullint) trx->id); > + fprintf(f, "TRANSACTION " TRX_ID_FMT " OS thread %lu", (ullint) > trx->id, trx->thread_id); > > switch (trx->conc_state) { > case TRX_NOT_STARTED: > Furthermore, I think that in that separate commit for adding these, a macro could be useful instead of "%lu", because pthread_t may be incompatible with "%lu". Marko -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 0

Re: [Maria-developers] [Commits] f380e7b5535: MDEV-12072 Do not unnecessarily construct rec_printer objects in debug builds
by Jan Lindström 15 Feb '17

15 Feb '17

ok to push. On Wed, Feb 15, 2017 at 4:02 PM, <marko.makela(a)mariadb.com> wrote: > revision-id: f380e7b5535358bd3d7caf81d22579ef97f6ca24 (mariadb-10.2.3-271- > gf380e7b5535) > parent(s): 24911cee4e3be988848f6eab4d768849709f5256 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2017-02-15 16:02:00 +0200 > message: > > MDEV-12072 Do not unnecessarily construct rec_printer objects in debug > builds > > I introduced the rec_printer object in MySQL to pretty-print raw InnoDB > records and index tuples in diagnostic messages. These objects are being > constructed unconditionally, even though the DBUG_PRINT is not enabled. > > The unnecessary work would be avoided by simply passing > rec_printer(…).str() to the DBUG_LOG that was introduced in MDEV-11713. > > --- > storage/innobase/btr/btr0cur.cc | 54 ++++++++---------------- > storage/innobase/row/row0log.cc | 58 ++++++++------------------ > storage/innobase/row/row0merge.cc | 88 +++++++++++++++--------------- > --------- > 3 files changed, 68 insertions(+), 132 deletions(-) > > diff --git a/storage/innobase/btr/btr0cur.cc b/storage/innobase/btr/ > btr0cur.cc > index 9033c399ce2..bdf3bd2eca7 100644 > --- a/storage/innobase/btr/btr0cur.cc > +++ b/storage/innobase/btr/btr0cur.cc > @@ -3012,18 +3012,10 @@ btr_cur_optimistic_insert( > > page_cursor = btr_cur_get_page_cur(cursor); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(entry); > - DBUG_PRINT("ib_cur", ("insert %s (" IB_ID_FMT ") by " > IB_ID_FMT " %s", > - index->name(), index->id, > - thr != NULL > - ? trx_get_id_for_print(thr_get_trx(thr)) > - : 0, > - p.str().c_str())); > - } > -#endif > - > + DBUG_LOG("ib_cur", > + "insert " << index->name << " (" << index->id << ") by " > + << ib::hex(thr ? trx_get_id_for_print(thr_get_trx(thr)) > : 0) > + << ' ' << rec_printer(entry).str()); > DBUG_EXECUTE_IF("do_page_reorganize", > btr_page_reorganize(page_cursor, index, mtr);); > > @@ -3635,14 +3627,10 @@ btr_cur_update_in_place( > ut_ad(fil_page_index_page_check(btr_cur_get_page(cursor))); > ut_ad(btr_page_get_index_id(btr_cur_get_page(cursor)) == > index->id); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(rec, offsets); > - DBUG_PRINT("ib_cur", ("update-in-place %s (" IB_ID_FMT ") > by " IB_ID_FMT ": %s", > - index->name(), index->id, trx_id, > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_cur", > + "update-in-place " << index->name << " (" << index->id > + << ") by " << ib::hex(trx_id) << ": " > + << rec_printer(rec, offsets).str()); > > block = btr_cur_get_block(cursor); > page_zip = buf_block_get_page_zip(block); > @@ -3842,14 +3830,10 @@ btr_cur_optimistic_update( > } > } > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(rec, *offsets); > - DBUG_PRINT("ib_cur", ("update %s (" IB_ID_FMT ") by " > IB_ID_FMT ": %s", > - index->name(), index->id, trx_id, > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_cur", > + "update " << index->name << " (" << index->id << ") by " > + << ib::hex(trx_id) << ": " > + << rec_printer(rec, *offsets).str()); > > page_cursor = btr_cur_get_page_cur(cursor); > > @@ -4669,15 +4653,11 @@ btr_cur_del_mark_set_clust_rec( > ut_ad(trx_state_eq(trx, TRX_STATE_ACTIVE)); > ut_ad(!trx->in_rollback); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(rec, offsets); > - DBUG_PRINT("ib_cur", ("delete-mark clust %s (" IB_ID_FMT > ") by " IB_ID_FMT ": %s", > - index->table_name, index->id, > - trx_get_id_for_print(trx), > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_cur", > + "delete-mark clust " << index->table->name > + << " (" << index->id << ") by " > + << ib::hex(trx_get_id_for_print(trx)) << ": " > + << rec_printer(rec, offsets).str()); > > if (dict_index_is_online_ddl(index)) { > row_log_table_delete(rec, entry, index, offsets, NULL); > diff --git a/storage/innobase/row/row0log.cc b/storage/innobase/row/ > row0log.cc > index d43fb333cdc..0fc1dd097cc 100644 > --- a/storage/innobase/row/row0log.cc > +++ b/storage/innobase/row/row0log.cc > @@ -1,6 +1,7 @@ > /*********************************************************** > ****************** > > Copyright (c) 2011, 2016, Oracle and/or its affiliates. All Rights > Reserved. > +Copyright (c) 2017, MariaDB Corporation. > > This program is free software; you can redistribute it and/or modify it > under > the terms of the GNU General Public License as published by the Free > Software > @@ -1641,15 +1642,9 @@ row_log_table_apply_insert_low( > ut_ad(dtuple_validate(row)); > ut_ad(trx_id); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(row); > - DBUG_PRINT("ib_alter_table", > - ("insert table " IB_ID_FMT " (index " IB_ID_FMT > "): %s", > - index->table->id, index->id, > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_alter_table", > + "insert table " << index->table->id << " (index " > + << index->id << "): " << rec_printer(row).str()); > > static const ulint flags > = (BTR_CREATE_FLAG > @@ -1777,15 +1772,10 @@ row_log_table_apply_delete_low( > > ut_ad(dict_index_is_clust(index)); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(btr_pcur_get_rec(pcur), offsets); > - DBUG_PRINT("ib_alter_table", > - ("delete table " IB_ID_FMT " (index " IB_ID_FMT > "): %s", > - index->table->id, index->id, > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_alter_table", > + "delete table " << index->table->id << " (index " > + << index->id << "): " > + << rec_printer(btr_pcur_get_rec(pcur), offsets).str()); > > if (dict_table_get_next_index(index)) { > /* Build a row template for purging secondary index > entries. */ > @@ -2266,17 +2256,11 @@ row_log_table_apply_update( > cur_offsets, NULL, NULL, NULL, &old_ext, heap); > ut_ad(old_row); > > -#ifdef UNIV_DEBUG > - { > - rec_printer old(old_row); > - rec_printer new_row(row); > - DBUG_PRINT("ib_alter_table", > - ("update table " IB_ID_FMT " (index " > IB_ID_FMT "): %s to %s", > - index->table->id, index->id, > - old.str().c_str(), > - new_row.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_alter_table", > + "update table " << index->table->id > + << " (index " << index->id > + << ": " << rec_printer(old_row).str() > + << " to " << rec_printer(row).str()); > } else { > old_row = NULL; > old_ext = NULL; > @@ -3298,17 +3282,11 @@ row_log_apply_op_low( > ut_ad(!dict_index_is_corrupted(index)); > ut_ad(trx_id != 0 || op == ROW_OP_DELETE); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(entry); > - DBUG_PRINT("ib_create_index", > - ("%s %s index " IB_ID_FMT "," IB_ID_FMT ": > %s", > - op == ROW_OP_INSERT ? "insert" : "delete", > - has_index_lock ? "locked" : "unlocked", > - index->id, trx_id, > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_create_index", > + (op == ROW_OP_INSERT ? "insert " : "delete ") > + << (has_index_lock ? "locked index " : "unlocked index ") > + << index->id << ',' << ib::hex(trx_id) << ": " > + << rec_printer(entry).str()); > > mtr_start(&mtr); > mtr.set_named_space(index->space); > diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/ > row0merge.cc > index 57dd8f75e8e..85f1e0b6ecc 100644 > --- a/storage/innobase/row/row0merge.cc > +++ b/storage/innobase/row/row0merge.cc > @@ -1,6 +1,7 @@ > /*********************************************************** > ****************** > > Copyright (c) 2005, 2016, Oracle and/or its affiliates. All Rights > Reserved. > +Copyright (c) 2017, MariaDB Corporation. > > This program is free software; you can redistribute it and/or modify it > under > the terms of the GNU General Public License as published by the Free > Software > @@ -1100,16 +1101,11 @@ row_merge_buf_write( > row_merge_buf_encode(&b, index, entry, n_fields); > ut_ad(b < &block[srv_sort_buf_size]); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(entry->fields, n_fields); > - DBUG_PRINT("ib_merge_sort", > - ("%p,fd=%d,%lu %lu: %s", > - reinterpret_cast<const void*>(b), > of->fd, > - (ulint)of->offset, (ulint)i, > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_merge_sort", > + reinterpret_cast<const void*>(b) << ',' > + << of->fd << ',' << of->offset << ' ' << > + i << ": " << > + rec_printer(entry->fields, n_fields).str()); > } > > /* Write an "end-of-chunk" marker. */ > @@ -1121,10 +1117,9 @@ row_merge_buf_write( > to avoid bogus warnings. */ > memset(b, 0xff, &block[srv_sort_buf_size] - b); > #endif /* UNIV_DEBUG_VALGRIND */ > - DBUG_PRINT("ib_merge_sort", > - ("write %p,%d,%lu EOF", > - reinterpret_cast<const void*>(b), of->fd, > - (ulint)of->offset)); > + DBUG_LOG("ib_merge_sort", > + "write " << b << ',' << of->fd << ',' << of->offset << > + " EOF"); > DBUG_VOID_RETURN; > } > > @@ -1177,7 +1172,7 @@ row_merge_read( > os_offset_t ofs = ((os_offset_t) offset) * srv_sort_buf_size; > > DBUG_ENTER("row_merge_read"); > - DBUG_PRINT("ib_merge_sort", ("fd=%d ofs=" UINT64PF, fd, ofs)); > + DBUG_LOG("ib_merge_sort", "fd=" << fd << " ofs=" << ofs); > DBUG_EXECUTE_IF("row_merge_read_failure", DBUG_RETURN(FALSE);); > > IORequest request; > @@ -1224,7 +1219,7 @@ row_merge_write( > void* out_buf = (void *)buf; > > DBUG_ENTER("row_merge_write"); > - DBUG_PRINT("ib_merge_sort", ("fd=%d ofs=" UINT64PF, fd, ofs)); > + DBUG_LOG("ib_merge_sort", "fd=" << fd << " ofs=" << ofs); > DBUG_EXECUTE_IF("row_merge_write_failure", DBUG_RETURN(FALSE);); > > IORequest request(IORequest::WRITE); > @@ -1296,11 +1291,10 @@ row_merge_read_rec( > if (UNIV_UNLIKELY(!extra_size)) { > /* End of list */ > *mrec = NULL; > - DBUG_PRINT("ib_merge_sort", > - ("read %p,%p,%d,%lu EOF\n", > - reinterpret_cast<const void*>(b), > - reinterpret_cast<const void*>(block), > - fd, ulint(*foffs))); > + DBUG_LOG("ib_merge_sort", > + "read " << reinterpret_cast<const void*>(b) << > ',' << > + reinterpret_cast<const void*>(block) << ',' << > + fd << ',' << *foffs << " EOF"); > DBUG_RETURN(NULL); > } > > @@ -1413,18 +1407,11 @@ row_merge_read_rec( > b += extra_size + data_size - avail_size; > > func_exit: > - > -#ifdef UNIV_DEBUG > - { > - rec_printer p(*mrec, 0, offsets); > - DBUG_PRINT("ib_merge_sort", > - ("%p,%p,fd=%d,%lu: %s", > - reinterpret_cast<const void*>(b), > - reinterpret_cast<const void*>(block), > - fd, ulint(*foffs), > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_merge_sort", > + reinterpret_cast<const void*>(b) << ',' << > + reinterpret_cast<const void*>(block) > + << ",fd=" << fd << ',' << *foffs << ": " > + << rec_printer(*mrec, 0, offsets).str()); > DBUG_RETURN(b); > } > > @@ -1455,15 +1442,9 @@ row_merge_write_rec_low( > #endif /* DBUG_OFF */ > DBUG_ASSERT(e == rec_offs_extra_size(offsets) + 1); > > -#ifdef UNIV_DEBUG > - { > - rec_printer p(mrec, 0, offsets); > - DBUG_PRINT("ib_merge_sort", > - ("%p,fd=%d,%lu: %s", > - reinterpret_cast<const void*>(b), fd, > ulint(foffs), > - p.str().c_str())); > - } > -#endif > + DBUG_LOG("ib_merge_sort", > + reinterpret_cast<const void*>(b) << ",fd=" << fd << ',' > + << foffs << ": " << rec_printer(mrec, 0, offsets).str()); > > if (e < 0x80) { > *b++ = (byte) e; > @@ -1573,11 +1554,10 @@ row_merge_write_eof( > ut_ad(foffs); > > DBUG_ENTER("row_merge_write_eof"); > - DBUG_PRINT("ib_merge_sort", > - ("%p,%p,fd=%d,%lu", > - reinterpret_cast<const void*>(b), > - reinterpret_cast<const void*>(block), > - fd, ulint(*foffs))); > + DBUG_LOG("ib_merge_sort", > + reinterpret_cast<const void*>(b) << ',' << > + reinterpret_cast<const void*>(block) << > + ",fd=" << fd << ',' << *foffs); > > if (b == &block[0]) { > b+= ROW_MERGE_RESERVE_SIZE; > @@ -2937,10 +2917,9 @@ row_merge_blocks( > ulint* offsets1;/* offsets of mrec1 */ > > DBUG_ENTER("row_merge_blocks"); > - DBUG_PRINT("ib_merge_sort", > - ("fd=%d,%lu+%lu to fd=%d,%lu", > - file->fd, ulint(*foffs0), ulint(*foffs1), > - of->fd, ulint(of->offset))); > + DBUG_LOG("ib_merge_sort", > + "fd=" << file->fd << ',' << *foffs0 << '+' << *foffs1 > + << " to fd=" << of->fd << ',' << of->offset); > > heap = row_merge_heap_create(dup->index, &buf, &offsets0, > &offsets1); > > @@ -3050,10 +3029,9 @@ row_merge_blocks_copy( > ulint* offsets1;/* dummy offsets */ > > DBUG_ENTER("row_merge_blocks_copy"); > - DBUG_PRINT("ib_merge_sort", > - ("fd=%d,%lu to fd=%d,%lu", > - file->fd, ulint(foffs0), > - of->fd, ulint(of->offset))); > + DBUG_LOG("ib_merge_sort", > + "fd=" << file->fd << ',' << foffs0 > + << " to fd=" << of->fd << ',' << of->offset); > > heap = row_merge_heap_create(index, &buf, &offsets0, &offsets1); > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] Blackhole, Archive and others only as loadable plugins in 10.1 - intentional change?
by Hartmut Holzgraefe 15 Feb '17

15 Feb '17

In MariaDB 10.1 binary packages several storage engines, e.g BLACKHOLE and ARCHIVE, are no longer compiled in by default but only come as loadable plugins that need to be loaded into the server explicitly. This is mentioned on the knowledge base pages for these engines, e.g. https://mariadb.com/kb/en/mariadb/blackhole/ and https://mariadb.com/kb/en/mariadb/archive/ but is not mentioned in the release notes and the detailed changelog pages for 10.1.x releases at all. I also searched my "maria-developers" and "maria-discuss" mail archives but could not find any discussion or announcement on this topic either. Same for Jira, can't find any ticket that would describe that sort of change there either. Problems with the current situation: * comes as a surprise for installation using these storage engines when upgraded to 10.1 * mysql_upgrade should handle this, but doesn't: https://jira.mariadb.org/browse/MDEV-11942 * it is not even easily possible to identify affected tables after an upgrade to 10.1. For BLACKHOLE tables I_S.TABLES only shows these under specific conditions, and for ARCHIVE it seems to be impossible to identify affected tables at all if the plugin is not loaded https://jira.mariadb.org/browse/MDEV-11943 I have submitted a a fix for MDEV-11942, but without having a fix for MDEV-11943 it only works for BLACKHOLE so far, not for ARCHIVE (haven't tested FEDERATED or other potentially affected engines yet). https://github.com/MariaDB/server/pull/304 -- Hartmut Holzgraefe, Principal Support Engineer (EMEA) MariaDB Corporation | http://www.mariadb.com/

2 1

[Maria-developers] Fwd: 41f885f64fe: MDEV-10859: Wrong result of aggregate window function in query with HAVING and no ORDER BY
by Vicențiu Ciorbaru 14 Feb '17

14 Feb '17

Hi Igor, Sergey! I need a review on this patch that fixes a problem with Window Functions being computed over too many rows. The details are in the commit message. Thank you! Vicențiu ---------- Forwarded message --------- From: vicentiu <vicentiu(a)mariadb.org> Date: Tue, 14 Feb 2017 at 14:08 Subject: 41f885f64fe: MDEV-10859: Wrong result of aggregate window function in query with HAVING and no ORDER BY To: <commits(a)mariadb.org> revision-id: 41f885f64fe4c02e03ec655f225b1dc01e9d5376 (mariadb-10.2.3-252-g41f885f64fe) parent(s): d731ce21a7bc25a49958789d24b3f0eec5845aea author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-02-14 14:02:29 +0200 message: MDEV-10859: Wrong result of aggregate window function in query with HAVING and no ORDER BY Window functions need to be computed after applying the HAVING clause. An optimization that we have for regular, non-window function, cases is to apply having only during sending of the rows to the client. This allows rows that should be filtered from the temporary table used to store aggregation results to be stored there. This behaviour is undesireable for window functions, as we have to compute window functions on the result-set after HAVING is applied. Storing extra rows in the table leads to wrong values as the frame bounds might capture those -to be filtered afterwards- rows. --- mysql-test/r/win.result | 33 +++++++++++++++++++++++++++++++++ mysql-test/t/win.test | 23 +++++++++++++++++++++++ sql/sql_select.cc | 11 ++++++++--- sql/sql_window.cc | 6 ++++++ 4 files changed, 70 insertions(+), 3 deletions(-) diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result index fd3aea80083..b3adf358be6 100644 --- a/mysql-test/r/win.result +++ b/mysql-test/r/win.result @@ -2925,3 +2925,36 @@ WHERE EXISTS ( SELECT * FROM t2 WHERE c IN ( SELECT MAX(d) FROM t3 ) ); a MAX(a) AVG(a) OVER (PARTITION BY b) NULL NULL NULL DROP TABLE t1,t2,t3; +# +# MDEV-MDEV-10859: Wrong result of aggregate window function in query +# with HAVING and no ORDER BY +# +create table empsalary (depname varchar(32), empno smallint primary key, salary int); +insert into empsalary values +('develop', 1, 5000), ('develop', 2, 4000),('sales', 3, '6000'),('sales', 4, 5000); +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary; +depname empno salary avg(salary) OVER (PARTITION BY depname) +develop 1 5000 4500.0000 +develop 2 4000 4500.0000 +sales 3 6000 5500.0000 +sales 4 5000 5500.0000 +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary ORDER BY depname; +depname empno salary avg(salary) OVER (PARTITION BY depname) +develop 1 5000 4500.0000 +develop 2 4000 4500.0000 +sales 3 6000 5500.0000 +sales 4 5000 5500.0000 +# +# These last 2 should have the same row results, ignoring order. +# +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary HAVING empno > 1; +depname empno salary avg(salary) OVER (PARTITION BY depname) +develop 2 4000 4000.0000 +sales 3 6000 5500.0000 +sales 4 5000 5500.0000 +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary HAVING empno > 1 ORDER BY depname; +depname empno salary avg(salary) OVER (PARTITION BY depname) +develop 2 4000 4000.0000 +sales 3 6000 5500.0000 +sales 4 5000 5500.0000 +drop table empsalary; diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test index aa636f7a294..90bbf41688f 100644 --- a/mysql-test/t/win.test +++ b/mysql-test/t/win.test @@ -1711,3 +1711,26 @@ SELECT a, MAX(a), AVG(a) OVER (PARTITION BY b) FROM t1 WHERE EXISTS ( SELECT * FROM t2 WHERE c IN ( SELECT MAX(d) FROM t3 ) ); DROP TABLE t1,t2,t3; + +--echo # +--echo # MDEV-MDEV-10859: Wrong result of aggregate window function in query +--echo # with HAVING and no ORDER BY +--echo # + +create table empsalary (depname varchar(32), empno smallint primary key, salary int); +insert into empsalary values + ('develop', 1, 5000), ('develop', 2, 4000),('sales', 3, '6000'),('sales', 4, 5000); + +--sorted_result +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary; +--sorted_result +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary ORDER BY depname; +--echo # +--echo # These last 2 should have the same row results, ignoring order. +--echo # +--sorted_result +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary HAVING empno > 1; +--sorted_result +SELECT depname, empno, salary, avg(salary) OVER (PARTITION BY depname) FROM empsalary HAVING empno > 1 ORDER BY depname; + +drop table empsalary; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 5c7ae1e88c1..473270f8a69 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -2411,13 +2411,18 @@ bool JOIN::make_aggr_tables_info() If having is not handled here, it will be checked before the row is sent to the client. + + In the case of window functions however, we *must* make sure to not + store any rows which don't match HAVING within the temp table, + as rows will end up being used during their computation. */ if (having && - (sort_and_group || (exec_tmp_table->distinct && !group_list))) + (sort_and_group || (exec_tmp_table->distinct && !group_list) || + select_lex->have_window_funcs())) { - // Attach HAVING to tmp table's condition + /* Attach HAVING to tmp table's condition */ curr_tab->having= having; - having= NULL; // Already done + having= NULL; /* Already done */ } /* Change sum_fields reference to calculated fields in tmp_table */ diff --git a/sql/sql_window.cc b/sql/sql_window.cc index 37095ad78ca..2bdac89f293 100644 --- a/sql/sql_window.cc +++ b/sql/sql_window.cc @@ -2840,6 +2840,12 @@ bool Window_funcs_computation::setup(THD *thd, order_window_funcs_by_window_specs(window_funcs); SQL_SELECT *sel= NULL; + /* + If the tmp table is filtered during sorting + (ex: SELECT with HAVING && ORDER BY), we must make sure to keep the + filtering conditions when we perform sorting for window function + computation. + */ if (tab->filesort && tab->filesort->select) { sel= tab->filesort->select;

1 0

[Maria-developers] bb-10.2-compatibility - MDEV 10585 or 10866
by jerome brauge 14 Feb '17

14 Feb '17

Hello Alexander, Attached script fail on my local bb-10.2-compatibility branch on the following assert : /* If called from a stored procedure, ensure that we won't rollback external changes when cleaning up after validation. */ DBUG_ASSERT(thd->change_list.is_empty()); Stack trace : mysqld.exe!my_sigabrt_handler(int sig) Line 477 C mysqld.exe!raise(int signum) Line 516 C++ mysqld.exe!abort() Line 64 C++ mysqld.exe!common_assert_to_stderr_direct(const wchar_t * const expression, const wchar_t * const file_name, const unsigned int line_number) Line 124 C++ mysqld.exe!common_assert_to_stderr<wchar_t>(const wchar_t * const expression, const wchar_t * const file_name, const unsigned int line_number) Line 138 C++ mysqld.exe!common_assert<wchar_t>(const wchar_t * const expression, const wchar_t * const file_name, const unsigned int line_number, void * const return_address) Line 383 C++ mysqld.exe!_wassert(const wchar_t * expression, const wchar_t * file_name, unsigned int line_number) Line 404 C++ > mysqld.exe!Prepared_statement::prepare(const char * packet, unsigned int packet_len) Line 3848 C++ mysqld.exe!Prepared_statement::execute_immediate(const char * query, unsigned int query_len) Line 4740 C++ mysqld.exe!mysql_sql_stmt_execute_immediate(THD * thd) Line 2863 C++ mysqld.exe!mysql_execute_command(THD * thd) Line 3722 C++ mysqld.exe!sp_instr_stmt::exec_core(THD * thd, unsigned int * nextp) Line 3335 C++ mysqld.exe!sp_lex_keeper::reset_lex_and_exec_core(THD * thd, unsigned int * nextp, bool open_tables, sp_instr * instr) Line 3098 C++ mysqld.exe!sp_instr_stmt::execute(THD * thd, unsigned int * nextp) Line 3251 C++ mysqld.exe!sp_head::execute(THD * thd, bool merge_da_on_success) Line 1265 C++ mysqld.exe!sp_head::execute_procedure(THD * thd, List<Item> * args) Line 2090 C++ mysqld.exe!do_execute_sp(THD * thd, sp_head * sp) Line 2871 C++ mysqld.exe!mysql_execute_command(THD * thd) Line 5900 C++ mysqld.exe!mysql_parse(THD * thd, char * rawbuf, unsigned int length, Parser_state * parser_state, bool is_com_multi, bool is_next_command) Line 7990 C++ mysqld.exe!dispatch_command(enum_server_command command, THD * thd, char * packet, unsigned int packet_length, bool is_com_multi, bool is_next_command) Line 1809 C++ mysqld.exe!do_command(THD * thd) Line 1367 C++ mysqld.exe!threadpool_process_request(THD * thd) Line 346 C++ mysqld.exe!tp_callback(TP_connection * c) Line 192 C++ mysqld.exe!tp_callback(_TP_CALLBACK_INSTANCE * instance, void * context) Line 377 C++ mysqld.exe!work_callback(_TP_CALLBACK_INSTANCE * instance, void * context, _TP_WORK * work) Line 451 C++ Can you reproduce this error ? Regards, Jérôme.

2 1

Re: [Maria-developers] [Commits] 822aa21672b: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir
by Sergei Golubchik 13 Feb '17

13 Feb '17

Hi, Vicentiu! Looks fine, thanks. Few comments below. On Feb 13, vicentiu wrote: > revision-id: 822aa21672ba6650a31a3ec05ab5c1bce7a80c1e (mariadb-10.2.3-198-g822aa21672b) > parent(s): b745ea489f0698c328383d9b8ec20d2f9777b1b8 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-02-13 14:44:00 +0200 > message: > > MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir > > PART 2 of the fix adds the logic of not using password column, unless it > exists. If password column is missing we attempt to use plugin && > authentication_string columns. > > diff --git a/mysql-test/r/no_password_column-mdev-11170.result b/mysql-test/r/no_password_column-mdev-11170.result > new file mode 100644 > index 00000000000..c3920a1b530 > --- /dev/null > +++ b/mysql-test/r/no_password_column-mdev-11170.result > @@ -0,0 +1,170 @@ ... > +# Reset to final original state. > +# > +drop table mysql.user; > +create table mysql.user like backup_user; > +insert into mysql.user select * from backup_user; you could RENAME TABLE backup_user to mysql.user. > +drop table backup_user; > +flush privileges; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 568aff9ac92..7e03253ca98 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2071,7 +2103,7 @@ static bool acl_load(THD *thd, const Grant_tables& tables) > 8, 8, MYF(0)); > > /* check default role, if any */ > - if (!is_role && user_table.num_fields() > DEFAULT_ROLE_COLUMN_IDX) > + if (!is_role && user_table.default_role()) all these changes could've went into the first patch... > { > user.default_rolename.str= > get_field(&acl_memroot, user_table.default_role()); > @@ -4030,11 +4092,16 @@ static int replace_user_table(THD *thd, const User_table &user_table, > > rights= user_table.get_access(); > > - DBUG_PRINT("info",("table fields: %d",user_table.num_fields())); > - if (combo.pwhash.str[0]) > + DBUG_PRINT("info",("table fields: %d", user_table.num_fields())); > + /* If we don't have a password column, we'll use the authentication_string > + column later. */ > + if (combo.pwhash.str[0] && user_table.password()) > user_table.password()->store(combo.pwhash.str, combo.pwhash.length, > system_charset_info); > - if (user_table.num_fields() >= 31) /* From 4.0.0 we have more fields */ > + /* We either have the password column, the plugin column, or both. Otherwise > + we have a corrupt user table. */ > + DBUG_ASSERT(user_table.password() || user_table.plugin()); you cannot assert the table structure. ALTER TABLE shouldn't crash the server. > + if (user_table.ssl_type()) /* From 4.0.0 we have more fields */ > { > /* We write down SSL related ACL stuff */ > switch (lex->ssl_type) { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] b745ea489f0: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir
by Sergei Golubchik 13 Feb '17

13 Feb '17

Hi, Vicentiu! Pretty nice. I like the how it looks now. Thanks! Few small comments below, really minor. On Feb 13, vicentiu wrote: > revision-id: b745ea489f0698c328383d9b8ec20d2f9777b1b8 (mariadb-10.2.3-197-gb745ea489f0) > parent(s): 52e0b585aaedd0f8f7191e0c0768d4be3c1c3496 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-02-13 12:13:36 +0200 > message: > > MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir > > PART 1 of the fix requires a bit of refactoring to not use hard-coded > field indices any more. Create classes that express the grant tables structure, > without exposing the underlying field indices. > > Most of the code is converted to use these classes, except parts which > are not directly affected by the MDEV-11170. These however are TODO > items for subsequent refactoring. > > --- > sql/sql_acl.cc | 1457 ++++++++++++++++++++++++++++++++++++++------------------ > sql/structs.h | 2 +- > 2 files changed, 996 insertions(+), 463 deletions(-) > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index b8a06c22b62..568aff9ac92 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > +#ifdef HAVE_REPLICATION > + if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont) > + { > + /* > + GRANT and REVOKE are applied the slave in/exclusion rules as they are > + some kind of updates to the mysql.% tables. > + */ > + // TODO(remove-after-review) Note: there was a bug here in previous > + // open_grant_tables version. The linking of tables to be opened was > + // done according to a mask, starting from the end of the TABLES_LIST[TABLES_MAX] > + // array. rpl_filter->tables_ok(0, tables) would always start from > + // TABLES_LIST[0] and lookup the chain from there. If we would not open > + // the User table (first table in the array) > + // (as it happened in grant_reload()), the chain would stop there, hence > + // the rpl_filter check would not lookup all the tables opened I see. Right, it's a bug. > + // > + // This is the reason for the first_table_in_list member variable. > + Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; > + if (rpl_filter->is_on() && > + !rpl_filter->tables_ok(0, &first_table_in_list->tl)) > + DBUG_RETURN(1); > + } > +#endif > + if (open_and_lock_tables(thd, &first_table_in_list->tl, FALSE, > + MYSQL_LOCK_IGNORE_TIMEOUT)) > + DBUG_RETURN(-1); > + > + /* The privilge columns vary based on MariaDB version. Figure out > + how many we have after we've opened the table. */ > + if (which_tables & Table_user) > + m_user_table.compute_num_privilege_cols(); > + if (which_tables & Table_db) > + m_db_table.compute_num_privilege_cols(); > + if (which_tables & Table_tables_priv) > + m_tables_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_columns_priv) > + m_columns_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_host) > + m_host_table.compute_num_privilege_cols(); > + if (which_tables & Table_procs_priv) > + m_procs_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_proxies_priv) > + m_proxies_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_roles_mapping) > + m_roles_mapping_table.compute_num_privilege_cols(); couldn't you just walk the list (from first_table_in_list) and do compute_num_privilege_cols() on every table? Like ((Grant_tables*)tl)->compute_num_privilege_cols(); > + DBUG_RETURN(0); > + } > + > + inline const User_table& user_table() const > + { > + return m_user_table; > + } I personally would just make user_table/etc members public, instead of creating accessors like that. > + > + inline const Db_table& db_table() const > + { > + return m_db_table; > + } > + > + > + inline const Tables_priv_table& tables_priv_table() const > + { > + return m_tables_priv_table; > + } > + > + inline const Columns_priv_table& columns_priv_table() const > + { > + return m_columns_priv_table; > + } > + > + inline const Host_table& host_table() const > + { > + return m_host_table; > + } > + > + inline const Procs_priv_table& procs_priv_table() const > + { > + return m_procs_priv_table; > + } > + > + inline const Proxies_priv_table& proxies_priv_table() const > + { > + return m_proxies_priv_table; > + } > + > + inline const Roles_mapping_table& roles_mapping_table() const > + { > + return m_roles_mapping_table; > + } > + > + private: > + User_table m_user_table; > + Db_table m_db_table; > + Tables_priv_table m_tables_priv_table; > + Columns_priv_table m_columns_priv_table; > + Host_table m_host_table; > + Procs_priv_table m_procs_priv_table; > + Proxies_priv_table m_proxies_priv_table; > + Roles_mapping_table m_roles_mapping_table; > + > + /* Bitmap of which tables we want to open. */ > + const int which_tables; you don't need it if you walk the list in open_and_lock() > + /* Type of lock we want to acquire for the tables we are oppening. */ > + const enum thr_lock_type lock_type; not needed here, it's stored in every TABLE_LIST anyway > + /* The grant tables are set-up in a linked list. We keep the head of it. */ > + Grant_table_base *first_table_in_list; > + /** > + Chain two grant tables' TABLE_LIST members. > + */ > + static void link_tables(Grant_table_base *from, Grant_table_base *to) > + { > + DBUG_ASSERT(from); > + if (to) > + from->tl.next_local= from->tl.next_global= &to->tl; > + else > + from->tl.next_local= from->tl.next_global= NULL; > + } > +}; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Server crashes in inline_mysql_mutex_lock
by Michael Widenius 10 Feb '17

10 Feb '17

Hi! > Thread 1 (Thread 0x7fa0f7549700 (LWP 21346)): > #0 0x00007fa0f9511101 in __pthread_kill (threadid=<optimized out>, > signo=11) at ../nptl/sysdeps/unix/sysv/linux/pthread_kill.c:61 > #1 0x00000000013f4d28 in my_write_core (sig=11) at > /data/src/bb-10.0-monty/mysys/stacktrace.c:478 > #2 0x00000000009f6fc3 in handle_fatal_signal (sig=11) at > /data/src/bb-10.0-monty/sql/signal_handler.cc:285 > #3 <signal handler called> > #4 0x000000000093123c in inline_mysql_mutex_lock (that=0x0, > src_file=0x14df108 "/data/src/bb-10.0-monty/sql/rpl_parallel.cc", > src_line=1278) at > /data/src/bb-10.0-monty/include/mysql/psi/mysql_thread.h:662 > #5 0x0000000000936842 in handle_rpl_parallel_thread (arg=0x7fa0df2d3ee8) at > /data/src/bb-10.0-monty/sql/rpl_parallel.cc:1278 > #6 0x00007fa0f950c0a4 in start_thread (arg=0x7fa0f7549700) at > pthread_create.c:309 > #7 0x00007fa0f76c487d in clone () at > ../sysdeps/unix/sysv/linux/x86_64/clone.S:111 The problem is flush tables with read locks combined with a stop slave of the only replication thread that forces a resize of thread pool. This can get the replication thread into a wrong state where it tries to delete itself before waiting for the flush tables, which the thread pool doesn't like. Fixed by adding a check that we are not in flush tables with read lock when we delete ourselves from the pool. Regards, Monty

1 0