- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] ee8755e3c51: MDEV-15012: ASAN: numerous test failures in PS
by Sergei Golubchik 26 Jan '18

26 Jan '18

Hi, Oleksandr! Nice one! Ok to push! On Jan 24, Oleksandr Byelkin wrote: > revision-id: ee8755e3c51a1da8fcf108ad0257a7e62fc94347 (mariadb-5.5.59-32-gee8755e3c51) > parent(s): 76577e1e2602f3c30859a176808c433a263e1b0a > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-24 14:42:52 +0100 > message: > > MDEV-15012: ASAN: numerous test failures in PS > > First roll back changes, then free Items which can lead to memory freeing. > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Patch for MDEV-13417 - UPDATE produces wrong values if an updated column is later used as an update source
by jerome brauge 24 Jan '18

24 Jan '18

Hello Sergei, Here is a new version of the patch following your comments. I let you decide if SET_CONSISTENCY must also be part of TRADITIONAL and ANSI. Regards, Jérôme. > -----Message d'origine----- > De : Sergei Golubchik [mailto:serg@mariadb.org] > Envoyé : jeudi 11 janvier 2018 23:17 > À : jerome brauge > Cc : bar(a)mariadb.org > Objet : Re: Patch for MDEV-13417 - UPDATE produces wrong values if an > updated column is later used as an update source > > Hi, Jérôme! > > Happy New Year! > > Sorry for the delay. > > But I've finally got around to reviewing your MDEV-13417 > > The patch was pretty good and with good tests too! > > I had only a few minor comments, see below. > If you'd like, I can apply your patch and do these changes myself. > > See the review below. > > On Dec 28, jerome brauge wrote: > > Hello Sergei, > > Do you have time to review my patch for this mdev ? > > This is very important for us because I don't see any workaround > > (without unique key and cursor). > > > diff --git a/sql/sql_class.h b/sql/sql_class.h index f30b442..a5e2104 > > 100644 > > --- a/sql/sql_class.h > > +++ b/sql/sql_class.h > > @@ -139,6 +139,7 @@ enum enum_binlog_row_image { > > #define MODE_HIGH_NOT_PRECEDENCE (1ULL << 29) > > #define MODE_NO_ENGINE_SUBSTITUTION (1ULL << 30) > > #define MODE_PAD_CHAR_TO_FULL_LENGTH (1ULL << 31) > > +#define MODE_SET_CONSISTENCY (1ULL << 32) > > I don't particularly like "SET_CONSISTENCY" name, but cannot come up with a > good alternative. It doesn't matter, renaming the mode is a trivial change > that doesn't affect anything else. > So, unless you have a strong preference for some specific mode name - just > ignore this naming issue completely. > And I'll ask our documentation guys if they could suggest something. > > > > > /* Bits for different old style modes */ > > #define OLD_MODE_NO_DUP_KEY_WARNINGS_WITH_IGNORE (1 << 0) > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 60247c8..debb81a > > 100644 > > --- a/sql/sql_base.cc > > +++ b/sql/sql_base.cc > > @@ -7952,15 +7954,41 @@ fill_record(THD *thd, TABLE *table_arg, > List<Item> &fields, List<Item> &values, > > ER_THD(thd, > ER_WARNING_NON_DEFAULT_VALUE_FOR_VIRTUAL_COLUMN), > > rfield->field_name.str, table->s->table_name.str); > > } > > - if (rfield->stored_in_db() && > > - (value->save_in_field(rfield, 0)) < 0 && !ignore_errors) > > + if (rfield->stored_in_db()) > > { > > - my_message(ER_UNKNOWN_ERROR, ER_THD(thd, > ER_UNKNOWN_ERROR), MYF(0)); > > - goto err; > > + if (value->save_in_field(rfield, 0) < 0 && !ignore_errors) > > + { > > + my_message(ER_UNKNOWN_ERROR, ER_THD(thd, > ER_UNKNOWN_ERROR), MYF(0)); > > + goto err; > > + } > > + /* > > + ** In sql MODE_SET_CONSISTENCY, > > + ** move field pointer on value stored in record[1] > > + ** which contains row before update (see MDEV-13417) > > + */ > > + if (update && thd->variables.sql_mode & MODE_SET_CONSISTENCY) > > + rfield->move_field_offset((my_ptrdiff_t) (table->record[1] - > > + table->record[0])); > > Cool! A simple and nice solution :) > > > } > > rfield->set_explicit_default(value); > > } > > > > + if (update && thd->variables.sql_mode & MODE_SET_CONSISTENCY) { > > + // restore fields pointers on record[0] > > + f.rewind(); > > + while ((fld= f++)) > > + { > > + rfield= fld->field_for_view_update()->field; > > + if (rfield->stored_in_db()) > > + { > > + table= rfield->table; > > + rfield->move_field_offset((my_ptrdiff_t) (table->record[0] - > > + table->record[1])); > > + } > > + } > > + } > > + > > if (!update && table_arg->default_field && > > table_arg->update_default_fields(0, ignore_errors)) > > goto err; > > diff --git a/sql/sql_update.cc b/sql/sql_update.cc index > > 568dd40..5796b96 100644 > > --- a/sql/sql_update.cc > > +++ b/sql/sql_update.cc > > @@ -148,7 +148,25 @@ bool compare_record(const TABLE *table) > > we make temporary copy of Item_field, to avoid influence of changing > > result_field on Item_ref which refer on this field > > */ > > thd->change_item_tree(it.ref(), new (thd->mem_root) Item_field(thd, > field)); > > } > > + } > > + > > + if (thd->variables.sql_mode & MODE_SET_CONSISTENCY) { > > + // Make sure that a column is updated only once > > + List_iterator_fast<Item> it(items); > > + List<Item> ul; > > + ul.empty(); > > + while ((item= it++) && !ul.add_unique(item, &cmp_items)) ; > > Dunno, it's O(N²) and item->eq() is not trivial either. > perhaps it'd be faster, say, mark fields some way, like > > while ((item= it++)) > { > Field *f= item->field_for_view_update()->field; > if (f->has_explicit_value()) > { > my_error(ER_UPDATED_COLUMN_ONLY_ONCE, MYF(0), > f->table_name, f->field_name.str); > return TRUE; > } > f->set_has_explicit_value(); > } > > note that this assumes the change in the error message, %s -> %`s.%`s > > > + > > + if (item) > > + { > > + // same field appear more than once in a single update statement. > > + my_error(ER_UPDATED_COLUMN_ONLY_ONCE, MYF(0), item- > >full_name()); > > + return TRUE; > > + } > > + } > > return FALSE; > > } > > diff --git a/mysql-test/r/set_consistency.result > > b/mysql-test/r/set_consistency.result > > new file mode 100644 > > index 0000000..747b6b3 > > --- /dev/null > > +++ b/mysql-test/r/set_consistency.result > > @@ -0,0 +1,185 @@ > > +SET > > > +sql_mode='STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_A > UTO_CREA > > +TE_USER,NO_ENGINE_SUBSTITUTION,SET_CONSISTENCY'; > > shouldn't SET_CONSISTENCY be part of TRADITIONAL, ORACLE, and ANSI? > > > +# > > +# MDEV-13417 UPDATE produces wrong values if an UPDATEd column is > > +later used as an UPDATE source # MDEV-13418 Compatibility: The order > > +of evaluation of SELECT..INTO assignments # CREATE TABLE t1 (c1 > > +INTEGER, c2 INTEGER, c3 INTEGER) ENGINE=InnoDb; INSERT INTO > > +t1(c1,c2,c3) VALUES (1,1,1); CREATE TABLE t2 (c1 INTEGER, c2 > > +INTEGER, c3 INTEGER) ENGINE=InnoDb; INSERT INTO t2(c1,c2,c3) VALUES > > +(1,1,1); > > + > > +Check that a column is only updated once. > > + > > please, comment out these messages, just like the MDEV numbers above > > > +UPDATE t1 > > +SET c1 = 1, > > +c1 = 2; > > +ERROR HY000: The column test.t1.c1 cannot be changed more than once > > +in a single UPDATE statement > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org

2 1

Re: [Maria-developers] [Commits] 16298d9e82e: MDEV-7533: COLUMN_JSON() doesn't escape control characters in string values
by Sergei Golubchik 22 Jan '18

22 Jan '18

Hi, Oleksandr! On Jan 22, Oleksandr Byelkin wrote: > revision-id: 16298d9e82eb507c5e0ca6d097e6a7016582ccbc (mariadb-10.0.33-47-g16298d9e82e) > parent(s): 5e87f49a9927bf3fd0bd9af789bbdd329e4b4416 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-22 16:53:10 +0100 > message: > > MDEV-7533: COLUMN_JSON() doesn't escape control characters in string values > > escape all charecters less or equal 0x1F (control symbols) > (shorter sequence are not used to make code simple, long encoding is > always legal according to the rfc4627) ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-14603 signal 11 with short stacktrace
by Alexander Barkov 22 Jan '18

22 Jan '18

Hi Sergei, can you please review a patch for MDEV-14603? Thanks!

2 5

Re: [Maria-developers] [JIRA] (MDEV-14185) CREATE TEMPORARY TABLE AS SELECT causes error 1290 with read_only and InnoDB
by Sergei Golubchik 22 Jan '18

22 Jan '18

Hi, Vesa! On Jan 16, Vesa Pentti (JIRA) wrote: > > Vesa Pentti updated MDEV-14185: > ------------------------------- > Status: In Review (was: In Progress) > Assignee: Sergei Golubchik (was: Vesa Pentti) > > commit d4366c4a2c9b41901b11ca180d3ddbf79e992490 Please send commit emails next time. This time I had to go and download the patch from github and paste it in an email. > diff --git a/sql/handler.cc b/sql/handler.cc > index 174034a7eb55..85b73719f0e8 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -4387,8 +4390,11 @@ handler::ha_create_partitioning_metadata(const char *name, const char *old_name, > partitions. > */ > DBUG_ASSERT(m_lock_type == F_UNLCK || > - (!old_name && strcmp(name, table_share->path.str))); > - mark_trx_read_write(); > + (!old_name && table_share && strcmp(name, table_share->path.str))); > + if (should_mark_rw) > + { > + mark_trx_read_write(); > + } Just remove mark_trx_read_write() from ha_create_partitioning_metadata(). It's a non-transactional operation anyway. It's always a part of DDL, so will have the same protection as DDL does. > > return create_partitioning_metadata(name, old_name, action_flag); > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 47bb2dcbcc8: MDEV-14786: Server crashes in Item_cond::transform on 2nd execution of SP querying from a view
by Sergei Golubchik 22 Jan '18

22 Jan '18

Hi, Oleksandr! On Jan 21, Oleksandr Byelkin wrote: > revision-id: 47bb2dcbcc8c3cdca43baf6259f2434a746c7b03 (mariadb-5.5.58-42-g47bb2dcbcc8) > parent(s): c0e964dcf1787a101f6e9c115751c0d02b0fac1b > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-01-21 23:48:32 +0100 > message: > > MDEV-14786: Server crashes in Item_cond::transform on 2nd execution of SP querying from a view > > MDEV-14957: JOIN::prepare gets unusable "conds" as argument > > Do not touch merged derived (it is irreversible) > > Fix first argument of in_optimizer for calls possible before fix_fields() > > diff --git a/sql/item.cc b/sql/item.cc > index 332e027adf1..562c0bec78d 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -10010,7 +10010,7 @@ const char *dbug_print_item(Item *item) > if (!item) > return "(Item*)NULL"; > item->print(&str ,QT_ORDINARY); > - if (str.c_ptr() == buf) > + if (str.ptr() == buf) c_ptr_safe() here. It's MDEV-14981. > return buf; > else > return "Couldn't fit into buffer"; > diff --git a/sql/item.h b/sql/item.h > index 4d0860af547..add408602fc 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -4478,4 +4478,11 @@ class Item_iterator_row: public Item_iterator > void close() {} > }; > > +#ifdef DBUG_OFF > +static inline const char *dbug_print_item(Item *item) { return NULL; } > +#else > +extern const char *dbug_print_item(Item *item); > +#endif Put it not at the end, but at the beginning - above 'class Protocol;' That's where someone had it in 10.2, it'll be easier to merge if you add it in the same place. And no 'extern' in the #else branch, just as in 10.2. > #endif /* SQL_ITEM_INCLUDED */ > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 62e76922c0e..a40340baae9 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -1443,6 +1443,33 @@ bool Item_in_optimizer::eval_not_null_tables(uchar *opt_arg) > } > > > +void Item_in_optimizer::print(String *str, enum_query_type query_type) > +{ > + restore_first_argumet(); > + Item_func::print(str, query_type); > +} > + > + > +/** > + "Restore" first argument before fix_fields() call (after it is harmless). > + > + @Note: Main pointer to left part of IN/ALL/ANY subselect is subselect's > + lest_expr (see Item_in_optimizer::fix_left) so changes made during > + fix_fields will be rolled back there which can make > + Item_in_optimizer::args[0] unusable on second execution before fix_left() > + call. This call fix the pointer. > +*/ > + > +void Item_in_optimizer::restore_first_argumet() > +{ > + if (args[1]->type() == Item::SUBSELECT_ITEM && > + ((Item_subselect *)args[1])->is_in_predicate()) > + { > + args[0]= ((Item_in_subselect *)args[1])->left_expr; > + } > +} Any way to trigger the bug without dbug_print_item() ? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Mdev-14586 Assertion `0' failed in retrieve_auto_increment upon attempt to drop PK
by Sergei Golubchik 19 Jan '18

19 Jan '18

Hi, Sachin! I agree with the patch, I think it's correct. Good tests too. I wasn't able to understand the bug from the commit comment though, see below. Ok to push (preferrably with the improved commit message). On Jan 19, Sachin Setiya wrote: > Hi serg , please review the latest patch > > commit d9641478bb8f6045039a3d942f2925ada334907e > Author: Sachin Setiya <sachin.setiya(a)maridb.com> > Date: Fri Jan 19 19:33:45 2018 +0530 > > MDEV-14586 Assertion `0' failed in retrieve_auto_increment ... > > Problem:- > If we create table using myisam/aria then this crashes the server. > CREATE TABLE t1(a bit(1), b int auto_increment , index(a,b)); > insert into t1 values(1,1); > Or this query > CREATE TABLE t1 (b BIT(1), pk INTEGER AUTO_INCREMENT PRIMARY KEY); > ALTER TABLE t1 ADD INDEX(b,pk); > INSERT INTO t1 VALUES (1,b'1'); > ALTER TABLE t1 DROP PRIMARY KEY; > > Reason:- > The reason for this is > 1st- Since bit field is stored in record null bits itself , so > find_ref_key in open_binary_frm will wrongly set share->next_number_key_offset > and share->next_number_keypart (both set to 0). I would say that find_ref_key() finds what key an auto_increment field belongs to by comparing key_part->offset and field->ptr. But BIT fields might have zero length in the record, so a key might have many key parts with the same offset. That is, comparing offsets cannot uniquely identify the correct key part. > 2nd- Since next_number_key_offset is zero it myisam/aria will think that > auto_increment is in first part of key. > 3nd- myisam/aria will call retrieve_auto_key which will see first key_part > field as a bit field and call assert(0) > > Solution:- Many key parts might have the same offset, but BIT fields do not support auto_increment. So, we can skip all key parts over BIT fields, and then comparing offsets will be unambiguous. > Although the bit field is stored in record null_byte but in key file(.MYI) > this does not hold true. Bit(1-7) will take one byte of memory. So the correct > approach will be to never call retrieve_auto_increment in the case when > bit field is in the starting of index. So I have changed the find_ref_key > function to take account that if first N fields is Bit field then simply go > to next field which is not Bit field. > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 60d1803ec64: MDEV-6707: Wrong result (extra row) with group by, multi-part key
by Sergey Petrunia 18 Jan '18

18 Jan '18

On Thu, Jan 11, 2018 at 02:35:55AM +0530, Varun wrote: > revision-id: 60d1803ec645b6d388aac7125ac6381ecee4d548 (mariadb-5.5.56-115-g60d1803ec64) > parent(s): 9c9cf556a15af84f9133ef859b50596085f7ae8e > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-01-11 01:51:23 +0530 > message: > > MDEV-6707: Wrong result (extra row) with group by, multi-part key > > Range optimizer creates ranges according to the where conditions. So this case involves using a composite key, > few parts of which are involved in GROUP BY and few in the MIN/MAX functions in the select list. > So during the execution of such queries, we try to find a prefix using the fields involved in GROUP BY > and we then check if this newly returned prefix lies within the range we had calculated earlier. > After we find a row that satisfies the partial range(involving fields of GROUP BY), then we look at the other fields > of the where clause. We look in all the ranges if we can satisfy the above retured row. > > The issue is while calculating these prefixes we might encounter same partial ranges for multiple ranges. > An example would be > where condition is f2 ='c' AND f1 <> 9 > so the ranges would be > (NULL,c <= f1,f2 <= c,9) > (c,9 <= f1,f2 <= c,+infinity) > > In this case for calculating prefixes with the group by field we take up the > partial ranges involving field f2 those would be > c<= f2 <=c > c<= f2 <=c > So we lookup rows with the same prefix in all such ranges and then we check for the other > part(in this case f1) in all the ranges. So lets take a row of the table to be (4,'c'), that would have > a prefix 'c' and value for f1 to be 4, so it would lie in the first range mentioned above. But we would > get this same record again when we read the partial range and this would again satisfy the first range. > This issue can be fixed if we compare such partial ranges and don't lookup if we see the same prefix again. This is a huge and long comment. Please make it smaller, and move at least part of it into the code. Good places would be: - The function comment for QUICK_RANGE_SELECT::get_next_prefix - the new lines in QUICK_RANGE_SELECT::get_next_prefix which use/update the value of save_last_range > > --- > mysql-test/r/range.result | 14 ++++++++++++++ > mysql-test/r/range_mrr_icp.result | 14 ++++++++++++++ > mysql-test/t/range.test | 13 +++++++++++++ > sql/opt_range.cc | 20 +++++++++++++++++--- > sql/opt_range.h | 6 ++++-- > 5 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/r/range.result b/mysql-test/r/range.result > index 630a692cef6..72b31de4df4 100644 > --- a/mysql-test/r/range.result > +++ b/mysql-test/r/range.result > @@ -2144,3 +2144,17 @@ value1 1000685 12345 > value1 1003560 12345 > value1 1004807 12345 > drop table t1; > +# > +# MDEV-6707: Wrong result (extra row) with group by, multi-part key > +# > +CREATE TABLE t1 (f1 INT, f2 VARCHAR(1), KEY(f2,f1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES > +(7,'v'),(0,'s'),(9,'l'),(4,'c'); > +explain > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 range f2 f2 9 NULL 2 Using where; Using index for group-by > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +MAX(f1) f2 > +4 c > +DROP TABLE t1; > diff --git a/mysql-test/r/range_mrr_icp.result b/mysql-test/r/range_mrr_icp.result > index 3f5de5b0189..1050bfcd887 100644 > --- a/mysql-test/r/range_mrr_icp.result > +++ b/mysql-test/r/range_mrr_icp.result > @@ -2146,4 +2146,18 @@ value1 1000685 12345 > value1 1003560 12345 > value1 1004807 12345 > drop table t1; > +# > +# MDEV-6707: Wrong result (extra row) with group by, multi-part key > +# > +CREATE TABLE t1 (f1 INT, f2 VARCHAR(1), KEY(f2,f1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES > +(7,'v'),(0,'s'),(9,'l'),(4,'c'); > +explain > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 range f2 f2 9 NULL 2 Using where; Using index for group-by > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +MAX(f1) f2 > +4 c > +DROP TABLE t1; > set optimizer_switch=@mrr_icp_extra_tmp; > diff --git a/mysql-test/t/range.test b/mysql-test/t/range.test > index 393ca68e945..62e907b7b4a 100644 > --- a/mysql-test/t/range.test > +++ b/mysql-test/t/range.test > @@ -1718,3 +1718,16 @@ where (key1varchar='value1' AND (key2int <=1 OR key2int > 1)); > --echo # The following must show col1=12345 for all rows: > select * from t1; > drop table t1; > + > +--echo # > +--echo # MDEV-6707: Wrong result (extra row) with group by, multi-part key > +--echo # > + > +CREATE TABLE t1 (f1 INT, f2 VARCHAR(1), KEY(f2,f1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES > +(7,'v'),(0,'s'),(9,'l'),(4,'c'); > + > +explain > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +DROP TABLE t1; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 25a9e729a8b..e985aba1140 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -11249,6 +11249,7 @@ int QUICK_RANGE_SELECT::get_next() > @param prefix_length length of cur_prefix > @param group_key_parts The number of key parts in the group prefix > @param cur_prefix prefix of a key to be searched for > + @save_last_range Saving the last range we encountered You've meant "@param save_last_range" ? Also please use " INOUT " to indicate this parameter is used to pass info both from the caller and to the caller. > > Each subsequent call to the method retrieves the first record that has a > prefix with length prefix_length and which is different from cur_prefix, > @@ -11271,7 +11272,8 @@ int QUICK_RANGE_SELECT::get_next() > > int QUICK_RANGE_SELECT::get_next_prefix(uint prefix_length, > uint group_key_parts, > - uchar *cur_prefix) > + uchar *cur_prefix, > + QUICK_RANGE **save_last_range) > { > DBUG_ENTER("QUICK_RANGE_SELECT::get_next_prefix"); > const key_part_map keypart_map= make_prev_keypart_map(group_key_parts); > @@ -11301,8 +11303,19 @@ int QUICK_RANGE_SELECT::get_next_prefix(uint prefix_length, > last_range= 0; > DBUG_RETURN(HA_ERR_END_OF_FILE); > } > + > last_range= *(cur_range++); > > + if (*save_last_range) > + { > + if (!key_tuple_cmp(key_part_info, (*save_last_range)->min_key, > + last_range->min_key, prefix_length)) > + { > + last_range=NULL; > + continue; > + } > + } > + *save_last_range= last_range; > key_range start_key, end_key; > last_range->make_min_endpoint(&start_key, prefix_length, keypart_map); > last_range->make_max_endpoint(&end_key, prefix_length, keypart_map); > @@ -13315,7 +13328,7 @@ QUICK_GROUP_MIN_MAX_SELECT(TABLE *table, JOIN *join_arg, bool have_min_arg, > seen_first_key(FALSE), doing_key_read(FALSE), min_max_arg_part(min_max_arg_part_arg), > key_infix(key_infix_arg), key_infix_len(key_infix_len_arg), > min_functions_it(NULL), max_functions_it(NULL), > - is_index_scan(is_index_scan_arg) > + is_index_scan(is_index_scan_arg), save_last_range(NULL) > { > head= table; > index= use_index; > @@ -13968,7 +13981,8 @@ int QUICK_GROUP_MIN_MAX_SELECT::next_prefix() > uchar *cur_prefix= seen_first_key ? group_prefix : NULL; > if ((result= quick_prefix_select->get_next_prefix(group_prefix_len, > group_key_parts, > - cur_prefix))) > + cur_prefix, > + &save_last_range))) > DBUG_RETURN(result); > seen_first_key= TRUE; > } > diff --git a/sql/opt_range.h b/sql/opt_range.h > index b8b46ae5ab1..f4fb0642590 100644 > --- a/sql/opt_range.h > +++ b/sql/opt_range.h > @@ -455,6 +455,7 @@ class QUICK_RANGE_SELECT : public QUICK_SELECT_I > QUICK_RANGE **cur_range; /* current element in ranges */ > > QUICK_RANGE *last_range; > + QUICK_RANGE *save_last_range; So, now both QUICK_RANGE_SELECT and QUICK_GROUP_MIN_MAX_SELECT have 'save_last_range'. Does QUICK_RANGE_SELECT need one? > > KEY_PART *key_parts; > KEY_PART_INFO *key_part_info; > @@ -477,7 +478,7 @@ class QUICK_RANGE_SELECT : public QUICK_SELECT_I > int get_next(); > void range_end(); > int get_next_prefix(uint prefix_length, uint group_key_parts, > - uchar *cur_prefix); > + uchar *cur_prefix, QUICK_RANGE **save_last_range); > bool reverse_sorted() { return 0; } > bool unique_key_range(); > int init_ror_merged_scan(bool reuse_handler, MEM_ROOT *alloc); > @@ -916,7 +917,8 @@ class QUICK_GROUP_MIN_MAX_SELECT : public QUICK_SELECT_I > Use index scan to get the next different key instead of jumping into it > through index read > */ > - bool is_index_scan; > + bool is_index_scan; > + QUICK_RANGE *save_last_range; > public: > /* > The following two members are public to allow easy access from BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 6ffd535e415: MDEV-14229: Stack trace is not resolved for shared objects
by Sergei Golubchik 18 Jan '18

18 Jan '18

Hi, Vicentiu! Looks mostly ok. Does it mean, it will replace my changes in 10.0+ related to calculating the offset? struct link_map *lm = (struct link_map*) dlopen(0, RTLD_NOW); etc As far as LIBDL and dependencies are concerned... 1. Don't add LIBDL to the link list. The server has it and clients should not link with my_addr_resove (and it's good to get a linker error, if by some mistake they do) 2. Check for dladdr in configure.cmake: CHECK_FUNCTION_EXISTS (dladdr HAVE_DLADDR) 3. handle dladdr in my_global.h (search for dlopen or dlerror) But, I think, you won't need any ifdefs in my_addr_resolve.c, because my_global.h will take care of that. Ok to push after that, thanks! On Jan 16, vicentiu(a)mariadb.org wrote: > revision-id: 6ffd535e415028f414b9d1c9e9efbfb81bf25798 (mariadb-5.5.58-42-g6ffd535e415) > parent(s): 6267be460ab5147e3bc0fffd03e690b3650f2fe2 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2018-01-16 22:57:52 +0200 > message: > > MDEV-14229: Stack trace is not resolved for shared objects > > Resolving a stacktrace including functions in dynamic libraries requires > us to look inside the libraries for the symbols. Addr2line needs to be > started with the correct binary for each address on the stack. To do this, > figure out which library it is using dladdr, then if the addr2line > binary was started with a different binary, fork it again with the > correct one. > > We only have one addr2line process running at any point during the > stacktrace resolving step. The maximum number of forks for addr2line should > generally be around 6. > > One for server stacktrace code, one for plugin code, one when going back > into server code, one for pthread library, one for libc, one for the > _start function in the server. More can come up if plugin calls server > function which goes back to a plugin, etc. > > --- > mysys/CMakeLists.txt | 2 +- > mysys/my_addr_resolve.c | 97 +++++++++++++++++++++++++++++++++---------------- > 2 files changed, 67 insertions(+), 32 deletions(-) > > diff --git a/mysys/CMakeLists.txt b/mysys/CMakeLists.txt > index cb86850c2de..15f3fbad9bf 100644 > --- a/mysys/CMakeLists.txt > +++ b/mysys/CMakeLists.txt > @@ -67,7 +67,7 @@ ENDIF() > > ADD_CONVENIENCE_LIBRARY(mysys ${MYSYS_SOURCES}) > TARGET_LINK_LIBRARIES(mysys dbug strings ${ZLIB_LIBRARY} > - ${LIBNSL} ${LIBM} ${LIBRT} ${LIBSOCKET} ${LIBEXECINFO}) > + ${LIBNSL} ${LIBM} ${LIBRT} ${LIBSOCKET} ${LIBEXECINFO} ${LIBDL}) > DTRACE_INSTRUMENT(mysys) > > IF(HAVE_BFD_H) > diff --git a/mysys/my_addr_resolve.c b/mysys/my_addr_resolve.c > index 90e6f43f390..f831ad5121f 100644 > --- a/mysys/my_addr_resolve.c > +++ b/mysys/my_addr_resolve.c > @@ -132,15 +132,79 @@ const char *my_addr_resolve_init() > > #include <m_string.h> > #include <ctype.h> > + > +#include <sys/wait.h> > + > static int in[2], out[2]; > -static int initialized= 0; > +static pid_t pid; > +static char addr2line_binary[1024]; > static char output[1024]; > + > +int start_addr2line_fork(const char *binary_path) > +{ > + > + if (pid > 0) > + { > + /* Don't leak FDs */ > + close(in[1]); > + close(out[0]); > + /* Don't create zombie processes. */ > + waitpid(pid, NULL, 0); > + } > + > + if (pipe(in) < 0) > + return 1; > + if (pipe(out) < 0) > + return 1; > + > + pid = fork(); > + if (pid == -1) > + return 1; > + > + if (!pid) /* child */ > + { > + dup2(in[0], 0); > + dup2(out[1], 1); > + close(in[0]); > + close(in[1]); > + close(out[0]); > + close(out[1]); > + execlp("addr2line", "addr2line", "-C", "-f", "-e", binary_path, NULL); > + exit(1); > + } > + > + close(in[0]); > + close(out[1]); > + > + return 0; > +} > + > int my_addr_resolve(void *ptr, my_addr_loc *loc) > { > char input[32], *s; > size_t len; > > - len= my_snprintf(input, sizeof(input), "%p\n", ptr); > + Dl_info info; > + void *offset; > + > + if (!dladdr(ptr, &info)) > + return 1; > + > + if (strcmp(addr2line_binary, info.dli_fname)) > + { > + /* We use dli_fname in case the path is longer than the length of our static > + string. We don't want to allocate anything dynamicaly here as we are in > + a "crashed" state. */ > + if (start_addr2line_fork(info.dli_fname)) > + { > + addr2line_binary[0] = '\0'; > + return 1; > + } > + /* Save result for future comparisons. */ > + strnmov(addr2line_binary, info.dli_fname, sizeof(addr2line_binary)); > + } > + offset = info.dli_fbase; > + len= my_snprintf(input, sizeof(input), "%p\n", ptr - offset); > if (write(in[1], input, len) <= 0) > return 1; > if (read(out[0], output, sizeof(output)) <= 0) > @@ -168,35 +232,6 @@ int my_addr_resolve(void *ptr, my_addr_loc *loc) > > const char *my_addr_resolve_init() > { > - if (!initialized) > - { > - pid_t pid; > - > - if (pipe(in) < 0) > - return "pipe(in)"; > - if (pipe(out) < 0) > - return "pipe(out)"; > - > - pid = fork(); > - if (pid == -1) > - return "fork"; > - > - if (!pid) /* child */ > - { > - dup2(in[0], 0); > - dup2(out[1], 1); > - close(in[0]); > - close(in[1]); > - close(out[0]); > - close(out[1]); > - execlp("addr2line", "addr2line", "-C", "-f", "-e", my_progname, NULL); > - exit(1); > - } > - > - close(in[0]); > - close(out[1]); > - initialized= 1; > - } > return 0; > } > #endif Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] a doubt about locks on supremum
by hongxiang jiang 18 Jan '18

18 Jan '18

2 1