- developers - lists.mariadb.org

Re: [Maria-developers] MDEV-12803 Improve function parameter data type control
by Alexander Barkov 26 May '17

26 May '17

Hello Alexey, Thanks for your feedback. Sending a new version, as discussed in this email and on IRC. Please find attached. - Removed Item_geometry_func_importer - Renamed Item_xxx_geometry_property to Item_xxx_args_geometry - Renamed Item_xxx_dyadic_operation to Item_xxx_args_geometry_geometry. This is the new hierarchy: Item_str_func Item_geometry_func Item_geometry_func_args_geometry Item_func_centroid Item_func_envelope Item_func_boundary Item_func_spatial_decomp Item_func_spatial_decomp_n Item_func_buffer Item_func_pointonsurface Item_func_geometry_from_text Item_func_geometry_from_wkb Item_func_geometry_from_json Item_func_point Item_func_spatial_collection Item_func_spatial_operation Item_binary_func_args_geometry Item_func_as_wkb Item_real_func Item_real_func_args_geometry Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_args_geometry_geometry Item_func_distance Item_long_func Item_long_func_args_geometry Item_func_issimple Item_func_isring Item_func_isclosed Item_func_dimension Item_func_numgeometries Item_func_numinteriorring Item_func_numpoints Item_func_srid Item_bool_func Item_bool_func_args_geometry Item_func_isempty Item_bool_func_args_geometry_geometry Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_args_geometry Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Thanks! On 05/26/2017 11:50 AM, Alexey Botchkov wrote: >>1. It will look like a C fragment in a C++ code. >> Why is it more natural than a virtual method? > > The Item hierarchy and 'check_type' hierarchy are parallel. > So it seems natural to me if we can assign an already-prepared > parameter-validation function to the type. > >> 2. It will make the Item size bigger. > But it will make Item** virtual tables smaller. > >> 4. It will need more CPU for m_check_arguments_function initialization. > Must be really small overhead. > >> Are you afraid that I'll have to modify class hierarchy? > Yes. My only concern is that the class hierarchy becomes more complicated > and with repeating code. > > But after a while i sort of used to it so now i'm fine with the virtual > method. > > >> We have a convention that functions return true on error and false on >> success. > For the check_something function returning TRUE or FALSE doesn't mean an > 'error'. > It's just the condition fits or doesn't. > But fine, our code has many examples of 'check' functions returning > results this way. > > Typenames discussed. > So otherwise i'm ok with the patch. > > Best regards. > HF > > On Wed, May 24, 2017 at 4:22 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hi Alexey, > > Thank you very much for your review! > > Please see my comments below: > > > On 05/23/2017 12:16 AM, Alexey Botchkov wrote: > > Firstly I'd say i don't like the idea of the 'virtual check_arguments()' > > function. > > I belive it's more natural to have a member 'm_check_arguments_function' > > that would be set in constructor (or be an argument to the constructor) > > and the non-virtual 'check_arguments()' that would just call that member. > > This would make Item class structure much nicer and would even save us > > memory. > > > > If you have good arguments for your approach, I have some > > comments/questions to your patch :) > > I'm not sure that a function member is better. > > 1. It will look like a C fragment in a C++ code. > Why is it more natural than a virtual method? > > 2. It will make the Item size bigger. > > 3. It will need more coding. > Now we have to declare only virtual functions. > With a function member, we'll have to do: > - declare function anyway > - add a code into constructors > > 4. It will need more CPU for m_check_arguments_function initialization. > On the contrary, having a virtual methods costs nothing, > except a small initialization in VMTs during mysqld startup. > > > What is your main concern? > Are you afraid that I'll have to modify class hierarchy > for other Item_xxx_func? It should not be necessary. > We'll just have a set of protected methods on the Item_func level, > for most typical cases. > > > > > > Why this change? > > +++ b/mysql-test/t/gis-rtree.test > > @@ -61,7 +61,7 @@ while ($1) > > let $2=10; > > while ($2) > > { > > - eval DELETE FROM t2 WHERE Within(g, > > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), Point($1 * 10, > > $2 * 10)))); > > + eval DELETE FROM t2 WHERE Within(g, > > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), 0))); > > SELECT count(*) FROM t2; > > dec $2; > > > It passed POINT() as the second argiment to GeometryFromWKB. > The second argument is for SRID. After my changes it's not allowed. > > > > } > > > > > > It looks weird when the 'check_something' functions returns FALSE if > > the argument fits the check. I'd make them returning TRUE in this case > > as people normally expect. > > We have a convention that functions return true on error and false on > success. > > Please have a look into: > > check_db_name() > check_for_broken_triggers() > check_unique_table() > > They all return true on error. > I did the same. > > > > > > +bool Item_func::check_argument_types_scalar(uint start, uint end) const > > +{ > > + for (uint i= start; i < end; i++) > > + { > > + DBUG_ASSERT(i < arg_count); > > > > In code like this i'd rather do > > bool Item_func::check_argument_types_scalar(uint start, uint end) const > > { > > DBUG_ASSERT(end < arg_count); > > > > for (uint i= start; i < end; i++) > > { > > // no DBUG_ASSERT here > > ... > > Agree. > > > > > > > > > Some typenames feel misleading. I'd suggest changes > > Item_real_func_geometry_property -> Item_real_func_arg_geometry > > Item_int_func_geometry_property -> Item_int_func_arg_geometry > > Item_bool_func_geometry_property -> Item_bool_func_arg_geometry > > Item_str_ascii_func_geometry_property -> Item_ascii_func_arg_geometry > > Item_geometry_func_geometry_property -> Item_geometry_func_arg_geometry > > This looks like a property. > The first argument is an object of some complex type (GEOMETRY in > our case). > The other arguments are simple type additional optional parameters. > > In C++ this would look like: > > geom->IsSimple(); > geom->Buffer(10); > > > OpenGIS also calls these methods "properties", > so do we in the documentation: > > https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html > <https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html> > > > > > > > Item_binary_func_geometry_property - do we really need this type? > > Right, there is only one binary string property: AsWKB(). > > I added Item_xxx_geometry_property for regularity > (to have a symmetric class hierarchy with other "properties" ). > > Here's the class structure: > > > Item_str_func > Item_geometry_func > Item_geometry_func_geometry_property > Item_func_centroid > Item_func_envelope > Item_func_boundary > Item_func_spatial_decomp > Item_func_spatial_decomp_n > Item_func_buffer > Item_func_pointonsurface > Item_geometry_func_importer > Item_func_geometry_from_text > Item_func_geometry_from_wkb > Item_func_geometry_from_json > Item_func_point > Item_func_spatial_collection > Item_func_spatial_operation > Item_binary_func_geometry_property > Item_func_as_wkb > Item_real_func > Item_real_func_geometry_property > Item_func_x > Item_func_y > Item_func_area > Item_func_glength > Item_real_func_geometry_dyadic_operation > Item_func_distance > Item_int_func > Item_int_func_geometry_property > Item_func_issimple > Item_func_isring > Item_func_isclosed > Item_func_dimension > Item_func_numgeometries > Item_func_numinteriorring > Item_func_numpoints > Item_func_srid > Item_func_gis_debug > Item_bool_func > Item_bool_func_geometry_property > Item_func_isempty > Item_bool_func_geometry_dyadic_operation > Item_func_spatial_relate > Item_str_ascii_func > Item_str_ascii_func_geometry_property > Item_func_as_wkt > Item_func_as_geojson > Item_func_geometry_type > > Btw, do you need a program to print this hierarchy? :) > I wrote it a few years ago, and use it almost every day. > > > > > Item_geometry_func_importer - do we need this type? > > I can remove this. It does not have shared methods indeed. > > > > > Item_real_func_geometry_dyadic_operation -> Item_real_func_arg_2_geometries > > Item_bool_func_geometry_dyadic_operation -> Item_bool_func_arg_2_geometries > > It's an operation on two geometries. > My names is quite self-descriptive. > Also, it tells that: > - there is no any other GEOMETRY arguments > - there can be optional non-GEOMETRY additional arguments > > > > > > > + bool check_arguments() const > > + { > > + DBUG_ASSERT(arg_count >= 2); > > + return check_argument_types_or_binary(&type_handler_geometry, 0, 2); > > > > the check_argument_types_or_binary() does the check of the arg_count so > > no need for the DBUG_ASSERT(). > > But i don't insist on removing this line :) > > I can remove it :) > > > > > > > On Mon, May 15, 2017 at 5:56 PM, Alexander Barkov <bar(a)mariadb.org <mailto:bar@mariadb.org> > > <mailto:bar@mariadb.org <mailto:bar@mariadb.org>>> wrote: > > > > Hello Alexey, > > > > can you please review a patch for MDEV-12803? > > > > Thanks! > > > > > >

1 0

Re: [Maria-developers] [Commits] 5ac7ee3: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 25 May '17

25 May '17

Hi Varun, Good to see that the feedback from the last time was addressed. There is another issue, though. Consider a case where some (but not necessarily all) PK columns are present in the secondary key: create table t1 ( pk1 VARCHAR(1000) not null, pk2 INT not null, col2 INT not null, KEY key1(pk1, col2), PRIMARY KEY(pk1, pk2) ) CHARSET utf8 engine=innodb; insert into t1 select a,a,a from ten; In this case, key1 still has a suffix, but the suffix only includes columns that are not already present in the key. That is, the "real" definition of key1 is (pk1, col2, pk2) and not (pk1, col2, pk1, pk2) . Let's try that. explain select * from t1 force index(key1) where pk1 < 'zz' and col2 <=2 and pk2 <=4; Without the patch, I have all 3 key parts used: | { "query_block": { "select_id": 1, "table": { "table_name": "t1", "access_type": "range", "possible_keys": ["key1"], "key": "key1", "key_length": "3010", "used_key_parts": ["pk1", "col2", "pk2"], "rows": 1, "filtered": 100, "attached_condition": "t1.pk1 <= '0' and t1.col2 <= 2 and t1.pk2 <= 4", "using_index": true } } } | With the patch, I only see the explicitly defined key parts used: | { "query_block": { "select_id": 1, "table": { "table_name": "t1", "access_type": "range", "possible_keys": ["key1"], "key": "key1", "key_length": "3006", "used_key_parts": ["pk1", "col2"], "rows": 1, "filtered": 100, "attached_condition": "t1.pk1 <= '0' and t1.col2 <= 2 and t1.pk2 <= 4", "using_index": true } } } | Which I think is wrong, because the key length never exceeds MAX_KEY_LEN. This can be easily fixed by appyling this patch over your patch: --- table.cc.varun 2017-05-24 18:30:39.076341082 +0300 +++ table.cc 2017-05-24 19:04:25.637740027 +0300 @@ -2164,10 +2164,13 @@ for (i= 0; i < add_keyparts_for_this_key; i++) { uint pk_part_length= key_first_info->key_part[i].store_length; - if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) + if (keyinfo->ext_key_part_map & 1<<i) { - add_keyparts_for_this_key= i; - break; + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) + { + add_keyparts_for_this_key= i; + break; + } } ext_key_length+= pk_part_length; } Do you agree with this change? Any amendments? If it's ok, please add this (together with the testcase) into your patch. I've checked MySQL 5.7 also, and they do something weird: ... insert into t1 select A.a + 1000*B.a, A.a + 1000*B.a, A.a + 1000*B.a from one_k A, ten B; explain format=json select * from t1 force index(key1) where pk1 <= '0' and col2 <=2 and pk2 <=4; EXPLAIN { "query_block": { "select_id": 1, "cost_info": { "query_cost": "1.41" }, "table": { "table_name": "t1", "access_type": "range", "possible_keys": [ "key1" ], "key": "key1", "used_key_parts": [ "pk1" ], "key_length": "3010", "rows_examined_per_scan": 1, "rows_produced_per_join": 0, "filtered": "11.11", "using_index": true, "cost_info": { "read_cost": "1.39", "eval_cost": "0.02", "prefix_cost": "1.41", "data_read_per_join": "335" }, "used_columns": [ "pk1", "pk2", "col2" ], "attached_condition": "((`test`.`t1`.`pk1` <= '0') and (`test`.`t1`.`col2` <= 2) and (`test`.`t1`.`pk2` <= 4))" } } } key_length is 3010, but used_key_parts only includes "pk1"... The numbers do not match. On Tue, May 23, 2017 at 07:08:48PM +0530, Varun wrote: > revision-id: 5ac7ee32a8e373d989bf9d665b5e01f770ac583f (mariadb-10.1.20-282-g5ac7ee3) > parent(s): a1b6128dedb4419db9fadaf94c356d3477d4e06f > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-23 19:00:15 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > mysql-test/r/innodb_ext_key.result | 51 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 38 ++++++++++++++++++++++++++++ > sql/table.cc | 36 ++++++++++++++++++++++++++- > 3 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..c76660c 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,56 @@ where index_date_updated= 10 and index_id < 800; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 range index_date_updated index_date_updated 13 NULL # Using index condition > drop table t0,t1,t2; > +# > +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +# was corrupted, server crashes in opt_sum_query > +set @save_innodb_file_format= @@innodb_file_format; > +set @save_innodb_large_prefix= @@innodb_large_prefix; > +set global innodb_file_format = BARRACUDA; > +set global innodb_large_prefix = ON; > +CREATE TABLE t1 ( > +pk INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1024), > +PRIMARY KEY (pk), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +MAX(t2.pk) > +2 > +drop table t1; > +CREATE TABLE t1 ( > +pk1 INT, > +pk2 INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1021), > +PRIMARY KEY (pk1,pk2), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["f2"], > + "key": "f2", > + "key_length": "3070", > + "used_key_parts": ["f2", "pk1"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "((t1.pk1 <= 5) and (t1.pk2 <= 5) and (t1.f2 = 'abc'))", > + "attached_condition": "(t1.f1 <= '3')" > + } > + } > +} > +drop table t1; > set optimizer_switch=@save_ext_key_optimizer_switch; > +set global innodb_file_format = @save_innodb_file_format; > +set global innodb_large_prefix = @save_innodb_large_prefix; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/innodb_ext_key.test b/mysql-test/t/innodb_ext_key.test > index bf94b7d..adb032f 100644 > --- a/mysql-test/t/innodb_ext_key.test > +++ b/mysql-test/t/innodb_ext_key.test > @@ -778,5 +778,43 @@ where index_date_updated= 10 and index_id < 800; > > drop table t0,t1,t2; > > + > +--echo # > +--echo # MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +--echo # was corrupted, server crashes in opt_sum_query > + > +set @save_innodb_file_format= @@innodb_file_format; > +set @save_innodb_large_prefix= @@innodb_large_prefix; > +set global innodb_file_format = BARRACUDA; > +set global innodb_large_prefix = ON; > + > +CREATE TABLE t1 ( > + pk INT, > + f1 VARCHAR(3), > + f2 VARCHAR(1024), > + PRIMARY KEY (pk), > + KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > + > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +drop table t1; > + > +CREATE TABLE t1 ( > + pk1 INT, > + pk2 INT, > + f1 VARCHAR(3), > + f2 VARCHAR(1021), > + PRIMARY KEY (pk1,pk2), > + KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > + > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +drop table t1; > + > set optimizer_switch=@save_ext_key_optimizer_switch; > +set global innodb_file_format = @save_innodb_file_format; > +set global innodb_large_prefix = @save_innodb_large_prefix; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/sql/table.cc b/sql/table.cc > index 37c0b63..c8c9696 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1721,6 +1721,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo= share->key_info; > uint primary_key= my_strcasecmp(system_charset_info, share->keynames.type_names[0], > primary_key_name) ? MAX_KEY : 0; > + KEY* key_first_info; > > if (primary_key >= MAX_KEY && keyinfo->flags & HA_NOSAME) > { > @@ -1800,19 +1801,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo->name_length+1); > } > > + if (!key) > + key_first_info= keyinfo; > + > if (ext_key_parts > share->key_parts && key) > { > KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part + > (keyinfo-1)->ext_key_parts; > uint add_keyparts_for_this_key= add_first_key_parts; > + uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0; > + Field *field; > > /* > Do not extend the key that contains a component > defined over the beginning of a field. > */ > for (i= 0; i < keyinfo->user_defined_key_parts; i++) > - { > + { > uint fieldnr= keyinfo->key_part[i].fieldnr; > + field= share->field[keyinfo->key_part[i].fieldnr-1]; > + > + if (field->null_ptr) > + len_null_byte= HA_KEY_NULL_LENGTH; > + > + if (field->type() == MYSQL_TYPE_BLOB || > + field->real_type() == MYSQL_TYPE_VARCHAR || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + length_bytes= HA_KEY_BLOB_LENGTH; > + } > + > + ext_key_length+= keyinfo->key_part[i].length + len_null_byte > + + length_bytes; > if (share->field[fieldnr-1]->key_length() != > keyinfo->key_part[i].length) > { > @@ -1821,6 +1841,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > } > } > > + if (add_keyparts_for_this_key) > + { > + for (i= 0; i < add_keyparts_for_this_key; i++) > + { > + uint pk_part_length= key_first_info->key_part[i].store_length; > + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + { > + add_keyparts_for_this_key= i; > + break; > + } > + ext_key_length+= pk_part_length; > + } > + } > + > if (add_keyparts_for_this_key < (keyinfo->ext_key_parts - > keyinfo->user_defined_key_parts)) > { > _______________________________________________ > 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

2 1

Re: [Maria-developers] MDEV-12803 Improve function parameter data type control
by Alexander Barkov 24 May '17

24 May '17

Hi Alexey, Thank you very much for your review! Please see my comments below: On 05/23/2017 12:16 AM, Alexey Botchkov wrote: > Firstly I'd say i don't like the idea of the 'virtual check_arguments()' > function. > I belive it's more natural to have a member 'm_check_arguments_function' > that would be set in constructor (or be an argument to the constructor) > and the non-virtual 'check_arguments()' that would just call that member. > This would make Item class structure much nicer and would even save us > memory. > > If you have good arguments for your approach, I have some > comments/questions to your patch :) I'm not sure that a function member is better. 1. It will look like a C fragment in a C++ code. Why is it more natural than a virtual method? 2. It will make the Item size bigger. 3. It will need more coding. Now we have to declare only virtual functions. With a function member, we'll have to do: - declare function anyway - add a code into constructors 4. It will need more CPU for m_check_arguments_function initialization. On the contrary, having a virtual methods costs nothing, except a small initialization in VMTs during mysqld startup. What is your main concern? Are you afraid that I'll have to modify class hierarchy for other Item_xxx_func? It should not be necessary. We'll just have a set of protected methods on the Item_func level, for most typical cases. > > Why this change? > +++ b/mysql-test/t/gis-rtree.test > @@ -61,7 +61,7 @@ while ($1) > let $2=10; > while ($2) > { > - eval DELETE FROM t2 WHERE Within(g, > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), Point($1 * 10, > $2 * 10)))); > + eval DELETE FROM t2 WHERE Within(g, > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), 0))); > SELECT count(*) FROM t2; > dec $2; It passed POINT() as the second argiment to GeometryFromWKB. The second argument is for SRID. After my changes it's not allowed. > } > > > It looks weird when the 'check_something' functions returns FALSE if > the argument fits the check. I'd make them returning TRUE in this case > as people normally expect. We have a convention that functions return true on error and false on success. Please have a look into: check_db_name() check_for_broken_triggers() check_unique_table() They all return true on error. I did the same. > > +bool Item_func::check_argument_types_scalar(uint start, uint end) const > +{ > + for (uint i= start; i < end; i++) > + { > + DBUG_ASSERT(i < arg_count); > > In code like this i'd rather do > bool Item_func::check_argument_types_scalar(uint start, uint end) const > { > DBUG_ASSERT(end < arg_count); > > for (uint i= start; i < end; i++) > { > // no DBUG_ASSERT here > ... Agree. > > > > Some typenames feel misleading. I'd suggest changes > Item_real_func_geometry_property -> Item_real_func_arg_geometry > Item_int_func_geometry_property -> Item_int_func_arg_geometry > Item_bool_func_geometry_property -> Item_bool_func_arg_geometry > Item_str_ascii_func_geometry_property -> Item_ascii_func_arg_geometry > Item_geometry_func_geometry_property -> Item_geometry_func_arg_geometry This looks like a property. The first argument is an object of some complex type (GEOMETRY in our case). The other arguments are simple type additional optional parameters. In C++ this would look like: geom->IsSimple(); geom->Buffer(10); OpenGIS also calls these methods "properties", so do we in the documentation: https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html > > Item_binary_func_geometry_property - do we really need this type? Right, there is only one binary string property: AsWKB(). I added Item_xxx_geometry_property for regularity (to have a symmetric class hierarchy with other "properties" ). Here's the class structure: Item_str_func Item_geometry_func Item_geometry_func_geometry_property Item_func_centroid Item_func_envelope Item_func_boundary Item_func_spatial_decomp Item_func_spatial_decomp_n Item_func_buffer Item_func_pointonsurface Item_geometry_func_importer Item_func_geometry_from_text Item_func_geometry_from_wkb Item_func_geometry_from_json Item_func_point Item_func_spatial_collection Item_func_spatial_operation Item_binary_func_geometry_property Item_func_as_wkb Item_real_func Item_real_func_geometry_property Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_geometry_dyadic_operation Item_func_distance Item_int_func Item_int_func_geometry_property Item_func_issimple Item_func_isring Item_func_isclosed Item_func_dimension Item_func_numgeometries Item_func_numinteriorring Item_func_numpoints Item_func_srid Item_func_gis_debug Item_bool_func Item_bool_func_geometry_property Item_func_isempty Item_bool_func_geometry_dyadic_operation Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_geometry_property Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Btw, do you need a program to print this hierarchy? :) I wrote it a few years ago, and use it almost every day. > > Item_geometry_func_importer - do we need this type? I can remove this. It does not have shared methods indeed. > > Item_real_func_geometry_dyadic_operation -> Item_real_func_arg_2_geometries > Item_bool_func_geometry_dyadic_operation -> Item_bool_func_arg_2_geometries It's an operation on two geometries. My names is quite self-descriptive. Also, it tells that: - there is no any other GEOMETRY arguments - there can be optional non-GEOMETRY additional arguments > > > + bool check_arguments() const > + { > + DBUG_ASSERT(arg_count >= 2); > + return check_argument_types_or_binary(&type_handler_geometry, 0, 2); > > the check_argument_types_or_binary() does the check of the arg_count so > no need for the DBUG_ASSERT(). > But i don't insist on removing this line :) I can remove it :) > > > On Mon, May 15, 2017 at 5:56 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Alexey, > > can you please review a patch for MDEV-12803? > > Thanks! > >

1 0

[Maria-developers] Oracle compatibility : Update with same source and target
by jerome brauge 23 May '17

23 May '17

Hello Alexander, In the same spirit of MDEV-12137, can you create a task for update with same source and target ? Regard, Jérôme.

2 3

[Maria-developers] MDEV-10142 - Replace function
by jerome brauge 23 May '17

23 May '17

Hello Alexander, Here is patch for replace function in oracle mode. Do I have to add a test for replication ? Regards, Jérôme.

2 4

Re: [Maria-developers] [Commits] 55a4858fe6f: MDEV-12666: CURRENT_ROLE() does not work in a view
by Alexander Barkov 23 May '17

23 May '17

Hi Vicentiu, It seems that Item_func_sysconst abuses const_charset_converter(). I have the following proposals: - Instead of adding a new code into Item::const_charset_converter(), can you please override Item_func_sysconst::safe_charset_converter()? The direction proposed by Sergei's looks good: Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { + if (thd->lex->context_analysis_only & CONTEXT_ANALYSIS_ONLY_VIEW) + return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } But it probably needs to be extended to handle prepared statement. At PREPARE-time it should not call const_charset_converter(). At EXECUTE-time it should be Ok to call const_charset_converter(). So can you please change Item_func_sysconst::const_item() to report false when in views and during PREPARE-time? And then change safe_charset_converter() as follows: Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { if (!const_item()) return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } - Please add tests for DATABASE() and CURRENT_USER - Please add tests for PS with changing context between PREPARE and EXECUTE. Thanks. On 05/22/2017 06:15 PM, vicentiu wrote: > revision-id: 55a4858fe6f3fdc63a072b93b9bc3d8e51dc5e94 (mariadb-10.0.30-76-g55a4858fe6f) > parent(s): 8d1827f5540a0def82b1e2cbc557245c3e5e14ca > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-05-22 17:06:01 +0300 > message: > > MDEV-12666: CURRENT_ROLE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The solution proposed is to not replace any function with an Item_null, > but instead use an Item_static_string_func. > > --- > .../suite/roles/current_role_view-12666.result | 48 ++++++++++++++++++++ > .../suite/roles/current_role_view-12666.test | 52 ++++++++++++++++++++++ > sql/item.cc | 13 +++++- > 3 files changed, 112 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/roles/current_role_view-12666.result b/mysql-test/suite/roles/current_role_view-12666.result > new file mode 100644 > index 00000000000..525748b4875 > --- /dev/null > +++ b/mysql-test/suite/roles/current_role_view-12666.result > @@ -0,0 +1,48 @@ > +CREATE USER has_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO has_role@'localhost'; > +CREATE ROLE test_role; > +GRANT test_role TO has_role@'localhost'; > +CREATE USER no_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO no_role@'localhost'; > +CREATE TABLE view_role_test ( > +id int primary key, > +role_name varchar(50) > +); > +INSERT INTO view_role_test VALUES (1, 'test_role'); > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +show create view v_view_role_test; > +View Create View character_set_client collation_connection > +v_view_role_test CREATE ALGORITHM=UNDEFINED DEFINER=`no_role`@`localhost` SQL SECURITY INVOKER VIEW `v_view_role_test` AS select `view_role_test`.`id` AS `id`,`view_role_test`.`role_name` AS `role_name` from `view_role_test` where (`view_role_test`.`role_name` = current_role()) latin1 latin1_swedish_ci > +SET ROLE test_role; > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test_2 > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +show create view v_view_role_test_2; > +View Create View character_set_client collation_connection > +v_view_role_test_2 CREATE ALGORITHM=UNDEFINED DEFINER=`no_role`@`localhost` SQL SECURITY INVOKER VIEW `v_view_role_test_2` AS select `view_role_test`.`id` AS `id`,`view_role_test`.`role_name` AS `role_name` from `view_role_test` where (`view_role_test`.`role_name` = current_role()) latin1 latin1_swedish_ci > +SELECT CURRENT_USER(); > +CURRENT_USER() > +root@localhost > +SELECT CURRENT_ROLE(); > +CURRENT_ROLE() > +test_role > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +id role_name > +1 test_role > +SELECT * FROM v_view_role_test; > +id role_name > +1 test_role > +drop user has_role@'localhost'; > +drop user no_role@'localhost'; > +drop role test_role; > +drop table view_role_test; > +drop view v_view_role_test; > +drop view v_view_role_test_2; > diff --git a/mysql-test/suite/roles/current_role_view-12666.test b/mysql-test/suite/roles/current_role_view-12666.test > new file mode 100644 > index 00000000000..86c74260b6e > --- /dev/null > +++ b/mysql-test/suite/roles/current_role_view-12666.test > @@ -0,0 +1,52 @@ > +CREATE USER has_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO has_role@'localhost'; > + > +CREATE ROLE test_role; > +GRANT test_role TO has_role@'localhost'; > + > +CREATE USER no_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO no_role@'localhost'; > + > +CREATE TABLE view_role_test ( > + id int primary key, > + role_name varchar(50) > + ); > + > +INSERT INTO view_role_test VALUES (1, 'test_role'); > + > + > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > + > +show create view v_view_role_test; > + > +SET ROLE test_role; > + > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test_2 > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > + > +show create view v_view_role_test_2; > + > + > +SELECT CURRENT_USER(); > +SELECT CURRENT_ROLE(); > + > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +SELECT * FROM v_view_role_test; > + > + > +drop user has_role@'localhost'; > +drop user no_role@'localhost'; > +drop role test_role; > + > +drop table view_role_test; > +drop view v_view_role_test; > +drop view v_view_role_test_2; > diff --git a/sql/item.cc b/sql/item.cc > index 4ce8396f71e..2ee3a6d61dc 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -1263,9 +1263,20 @@ Item *Item::const_charset_converter(CHARSET_INFO *tocs, > DBUG_ASSERT(fixed); > StringBuffer<64>tmp; > String *s= val_str(&tmp); > - if (!s) > + > + /* Functions that return NULL during prepare must not be replaced with > + Item_null as they may return non-null values during execution. > + Ex: CURRENT_ROLE() during view creation time can be NULL if no role > + is set. */ > + if (!s && !func_name) > return new Item_null((char *) func_name, tocs); > > + if (!s) > + { > + tmp.set_ascii("", 0); > + s = &tmp; > + } > + > if (!needs_charset_converter(s->length(), tocs)) > { > if (collation.collation == &my_charset_bin && tocs != &my_charset_bin && > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits >

1 0

Re: [Maria-developers] [Commits] b400696: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 22 May '17

22 May '17

Hi Varun, Please find my input below. I would like again to voice my dissatisfaction that I have to point out coding style violations in pretty much every patch I get. The other error in the patch is also pretty basic, ok to have it once but I think it is reasonable to expect more polished patches to be submitted for code review. On Sat, May 20, 2017 at 01:50:42AM +0530, Varun wrote: > revision-id: b400696386529fa63e79f723bbb06c05f7bae820 (mariadb-10.2.2-416-gb400696) > parent(s): fff61e31eca83a0a9af7c30e2bcda8309e9c695a > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-20 01:49:34 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > mysql-test/r/innodb_ext_key.result | 45 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 31 ++++++++++++++++++++++++++ > sql/table.cc | 36 +++++++++++++++++++++++++++++- > 3 files changed, 111 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..66391d6 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,50 @@ where index_date_updated= 10 and index_id < 800; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 range index_date_updated index_date_updated 13 NULL # Using index condition > drop table t0,t1,t2; > +# > +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +# was corrupted, server crashes in opt_sum_query > +CREATE TABLE t1 ( > +pk INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1024), > +PRIMARY KEY (pk), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8; > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +MAX(t2.pk) > +2 > +drop table t1; > +CREATE TABLE t1 ( > +pk1 INT, > +pk2 INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1021), > +PRIMARY KEY (pk1,pk2), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["f2"], > + "key": "f2", > + "key_length": "3070", > + "used_key_parts": ["f2", "pk1"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'", > + "attached_condition": "t1.f1 <= '3'" > + } > + } > +} > +drop table t1; > set optimizer_switch=@save_ext_key_optimizer_switch; > SET SESSION STORAGE_ENGINE=DEFAULT; ... > diff --git a/sql/table.cc b/sql/table.cc > index 398383e..7d88eba 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2104,6 +2104,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > for (uint key=0 ; key < keys ; key++,keyinfo++) > { > uint usable_parts= 0; > + KEY* key_first_info; This variable may be used un-initialized. If I change the above line to be KEY* key_first_info=(KEY*)0x1; and run this statement: CREATE TABLE t1a ( pk INT, f1 VARCHAR(3), f2 VARCHAR(1024), f2a VARCHAR(1024), PRIMARY KEY (pk), KEY(f2), KEY(f2a) ) ENGINE=InnoDB CHARSET utf8; Then I get a crash. > keyinfo->name=(char*) share->keynames.type_names[key]; > keyinfo->name_length= strlen(keyinfo->name); > keyinfo->cache_name= > @@ -2118,19 +2119,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo->name_length+1); > } > > + if(!key) Coding style: 'if (' ... > + key_first_info= keyinfo; > + > if (ext_key_parts > share->key_parts && key) > { > KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part + > (keyinfo-1)->ext_key_parts; > uint add_keyparts_for_this_key= add_first_key_parts; > + uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0; > + Field *field; > > /* > Do not extend the key that contains a component > defined over the beginning of a field. > */ > for (i= 0; i < keyinfo->user_defined_key_parts; i++) > - { > + { Coding style: wrong identation. > uint fieldnr= keyinfo->key_part[i].fieldnr; > + field= share->field[keyinfo->key_part[i].fieldnr-1]; > + > + if (field->null_ptr) > + len_null_byte= HA_KEY_NULL_LENGTH; > + > + if (field->type() == MYSQL_TYPE_BLOB || > + field->real_type() == MYSQL_TYPE_VARCHAR || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + length_bytes= HA_KEY_BLOB_LENGTH; > + } > + > + ext_key_length+= keyinfo->key_part[i].length + len_null_byte > + + length_bytes; > if (share->field[fieldnr-1]->key_length() != > keyinfo->key_part[i].length) > { > @@ -2139,6 +2159,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > } > } > > + if (add_keyparts_for_this_key) > + { > + for (i= 0; i < add_keyparts_for_this_key; i++) > + { > + uint pk_part_length= key_first_info->key_part[i].store_length; > + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + { > + add_keyparts_for_this_key= i; > + break; > + } > + ext_key_length+= pk_part_length; > + } > + } > + > if (add_keyparts_for_this_key < (keyinfo->ext_key_parts - > keyinfo->user_defined_key_parts)) > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 55a4858fe6f: MDEV-12666: CURRENT_ROLE() does not work in a view
by Sergei Golubchik 22 May '17

22 May '17

Hi, Vicentiu! Another possible approach (that doesn't necessarily penalizes other const functions that happen to return NULL) would be for Item_func_sysconst not to behave as const during view creation. Because they really aren't, sysconst functions can change between view creation and a select from the view. The fix would be, then Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { + if (thd->lex->context_analysis_only & CONTEXT_ANALYSIS_ONLY_VIEW) + return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } And possibly (not sure) other values of context_analysis_only would need a similar treatment here. On May 22, vicentiu wrote: > revision-id: 55a4858fe6f3fdc63a072b93b9bc3d8e51dc5e94 (mariadb-10.0.30-76-g55a4858fe6f) > parent(s): 8d1827f5540a0def82b1e2cbc557245c3e5e14ca > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-05-22 17:06:01 +0300 > message: > > MDEV-12666: CURRENT_ROLE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The solution proposed is to not replace any function with an Item_null, > but instead use an Item_static_string_func. > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MariaDB License Clarification
by Ryan Coe 19 May '17

19 May '17

Hello, My name is Ryan Coe and I've been maintaining the MariaDB package for Buildroot (https://buildroot.org) In commit 577915def8d39223aa23a06f898e11e82e72365a, COPYING.LESSER was removed. Our packaging currently states the MariaDB license as GPL-2.0 (server), GPL-2.0 with FLOSS exception (GPL client library), LGPL-2.0 (LGPL client library). Could I please get clarification on how the different components are licensed? Also, the GitHub page is still referencing COPYING.LESSER in the README. Thank you.

2 1

Re: [Maria-developers] strange IF in translog_flush_buffers()
by Sergei Golubchik 18 May '17

18 May '17

Hi, Sanja! On May 17, Oleksandr Byelkin wrote: > Hi! > > Actually code should work as it is, but it check unnecessary condition and > also very dificult to understand. I propose following fix (comment in it > explains why it work now and why condition is always TRUE) I cannot review it, this is between you and Monty. If you think it's ok, please push it (into 10.0). Or I can push it together with all other similar fixes. > diff --git a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c > index 86e51cc3526..e680e830703 100644 > --- a/storage/maria/ma_loghandler.c > +++ b/storage/maria/ma_loghandler.c > @@ -7789,8 +7789,23 @@ void translog_flush_buffers(TRANSLOG_ADDRESS *lsn, > translog_force_current_buffer_to_finish(); > translog_buffer_unlock(buffer); > } > - else if (log_descriptor.bc.buffer->prev_last_lsn != LSN_IMPOSSIBLE) > + else > { > + if (log_descriptor.bc.buffer->last_lsn == LSN_IMPOSSIBLE) > + { > + /* > + In this case both last_lsn & prev_last_lsn are LSN_IMPOSSIBLE > + otherwise it will go in the first IF because LSN_IMPOSSIBLE less > + then any real LSN and cmp_translog_addr(*lsn, > + log_descriptor.bc.buffer->prev_last_lsn) will be TRUE > + */ > + DBUG_ASSERT(log_descriptor.bc.buffer->prev_last_lsn == > + LSN_IMPOSSIBLE); > + DBUG_PRINT("info", ("There is no LSNs yet generated => do nothing")); > + translog_unlock(); > + DBUG_VOID_RETURN; > + } > + > /* fix lsn if it was horizon */ > *lsn= log_descriptor.bc.buffer->prev_last_lsn; > DBUG_PRINT("info", ("LSN to flush fixed to prev last lsn: (%lu,0x%lx)", > @@ -7799,13 +7814,6 @@ void translog_flush_buffers(TRANSLOG_ADDRESS *lsn, > TRANSLOG_BUFFERS_NO); > translog_unlock(); > } > - else if (log_descriptor.bc.buffer->last_lsn == LSN_IMPOSSIBLE) > - { > - DBUG_PRINT("info", ("There is no LSNs yet generated => do nothing")); > - translog_unlock(); > - DBUG_VOID_RETURN; > - } > - > /* flush buffers */ > *sent_to_disk= translog_get_sent_to_disk(); > if (cmp_translog_addr(*lsn, *sent_to_disk) > 0) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0