- developers - lists.mariadb.org

[Maria-developers] review for SP stack trace
by Alexander Barkov 30 May '17

30 May '17

Hello Jerome, here's Monty's review on your stack trace patch. Thank you very much. We're sorry for delay. > diff --git a/mysql-test/r/commit_1innodb.result b/mysql-test/r/commit_1innodb.result > index 1adba7b..8e40ec0 100644 > <cut> > > --- a/mysql-test/r/get_diagnostics.result > +++ b/mysql-test/r/get_diagnostics.result > @@ -595,7 +595,7 @@ SELECT var, @var; > END| > CALL p1(); > var @var > -1 1 > +2 2 > DROP PROCEDURE p1; > > What is the reason for this going from 1 to 2 ? > I would have understood if there would have been some warnings from before > in the code, but there wasn't. > > Does this come from the error from 'call p1()' earlier that does now have > a new 'note message'. > > If yes, it would be good to make this clear by adding a 'show > warnings' just before the above create procedure p1() line. > > # Setting TABLE_NAME is currently not implemented. > diff --git a/mysql-test/r/signal.result b/mysql-test/r/signal.result > index f05e357..a796cc6 100644 > --- a/mysql-test/r/signal_demo3.result > +++ b/mysql-test/r/signal_demo3.result > @@ -79,14 +79,23 @@ show warnings; > @@ -95,6 +104,4 @@ call proc_1(); > ERROR 45000: Oops in proc_1 > show warnings; > Level Code Message > -Error 1644 Oops in proc_5 > -Error 1644 Oops in proc_4 > -Error 1644 Oops in proc_3 > +Note 4068 At line 4 in demo.proc_3 > > Why has 3 error disappered and why is there a line number note for > demo.proc_3 but no error line? > > I assume this comes from the following line: > SET @@session.max_error_count = 5 > > The question is if max_error_count's should really be counted for notes too, > especially for "At line" notes. > The documentation for the variable says > "Specifies the maximum number of messages stored for display by" > > Still, the question is if we should include the 'At line' or not in counting. > (Not important for this patch, but good to think about) > > > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7484,3 +7484,5 @@ ER_UNKNOWN_SEQUENCES 42S02 > eng "Unknown SEQUENCE: '%-.300s'" > ER_UNKNOWN_VIEW 42S02 > eng "Unknown VIEW: '%-.300s'" > +ER_SP_STACK_TRACE > + eng "At line %d in %s" > > Should be %u as lineno is uint > > > Please add the "SHOW WARNINGS" and fix the error message and it's ok to push. > Thanks for a great addition to MariaDB!

2 2

[Maria-developers] Weekly Progress Report of the Project
by ANKIT KUMAR 29 May '17

29 May '17

Hi all, I am Ankit Kumar and I have been selected to work on allowing multiple authentication methods to same user. These are the few things I did this week- 1. Setted up Development environment on Windows. Resolved few issues which I was facing, some MariaDB members really helped me whenever I needed their help. 2. Read the Testing Framework Documentation of MariaDB in order to see how testing is performed. 3. I have been regularly communicating with the Mentor and other MariaDB Contributors in order in get deep insight of the problems that I would be working on during Coding Period. These are some basic things that I would need during official coding period, so I wont be wasting time on such things. Now I can get Started with my project. IRC Nick- AspiringMind

2 4

Re: [Maria-developers] GSoC NUMA support Project
by Daniel Black 29 May '17

29 May '17

On 29/05/17 06:03, Sumit Lakra wrote: > I think I am near done with dividing the buffer pool into the number of > nodes available in a NUMA machine. There is one thing left to do - to > check for the presence of 'libnuma' library in the server. Based on https://jira.mariadb.org/browse/MDEV-12924 I probably broke it in ab0e5031e41d0aa72dd77a6dced9eacc8be5c548 > As of now I > have explicitly defined HAVE_LIBNUMA in the > CMakeList.txt to check my code. CMakeList.txt in which directory? I'd suggest refering to publicly pushed code/branches for review. A branch on https://github.com/theGodlessLakra/server would be good. > Also, to test whether the control of execution reaches within a function > A, I tried to print out a message on the scree like 'fprintf(stderr, In > function A);' This seems to work in main(), my_init(), sys_var_init() > etc, but not in buf0buf.cc. The function which creates the buffer pool > is called by 'buf_pool_init()', which is called by > 'innobase_start_on_create_for_mysql()', which in turn is called by > 'innobase_init()', but I couldn't find where an d how is this function > called. Its called by function pointer -> see maria_declare_plugin in storage/innobase/handler/ha_innodb.cc which is called by the plugin initialization code. > Seems like the output to bot stdout and stderr is being > redirected somewhere and I can't find where (in code). reopen_fstreams in sql/{mysqld,log}.cc > Having this printing stuff is a quick way of testing the flow of control > and although my code is working, You can also run mysqld in gdb and the use "break" to set a breakpoint at a function/file:line and step through form there. > I would like to know about this > redirections if someone can tell me. add --verbose to the mysqld argument. > Also, which part of code calls the 'innobase_init()' function ? sql/plugin.cc:plugin_initialize in the call to plugin->init() > I have attached a screenshot of the server running on my NUMA emulated > pc with and without --numa-interleave=all. [ PID - 21016 without NUMA, > PID - 21058 with NUMA] > > At present it just ALLOCATES a buffer pool instance on each numa node. > Need to work on specific node binding and stuff.

1 0

[Maria-developers] GSoC NUMA support Project
by Sumit Lakra 28 May '17

28 May '17

I think I am near done with dividing the buffer pool into the number of nodes available in a NUMA machine. There is one thing left to do - to check for the presence of 'libnuma' library in the server. As of now I have explicitly defined HAVE_LIBNUMA in the CMakeList.txt to check my code. Also, to test whether the control of execution reaches within a function A, I tried to print out a message on the scree like 'fprintf(stderr, In function A);' This seems to work in main(), my_init(), sys_var_init() etc, but not in buf0buf.cc. The function which creates the buffer pool is called by 'buf_pool_init()', which is called by 'innobase_start_on_create_for_mysql()', which in turn is called by 'innobase_init()', but I couldn't find where an d how is this function called. Seems like the output to bot stdout and stderr is being redirected somewhere and I can't find where (in code). Having this printing stuff is a quick way of testing the flow of control and although my code is working, I would like to know about this redirections if someone can tell me. Also, which part of code calls the 'innobase_init()' function ? I have attached a screenshot of the server running on my NUMA emulated pc with and without --numa-interleave=all. [ PID - 21016 without NUMA, PID - 21058 with NUMA] At present it just ALLOCATES a buffer pool instance on each numa node. Need to work on specific node binding and stuff.

1 0

[Maria-developers] Building LibraOffice w/ MariaDB installed fails apt-get build-dep
by Howard Johnson 27 May '17

27 May '17

Hi, I'm having trouble with MariaDB as is described here: https://ask.libreoffice.org/en/question/96902/building-lo-w-mariadb-install… I suspect that there might be an issue with the virtual package or something like that. Any idea of what is going wrong or how to fix it? Thanks -- howard

2 1

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

26 May '17

Hi Varun, Ok to push. On Thu, May 25, 2017 at 07:41:50PM +0530, Varun wrote: > revision-id: df944293721a7ad93d115a58f2b574067d1dc79f (mariadb-10.1.20-282-gdf94429) > parent(s): a1b6128dedb4419db9fadaf94c356d3477d4e06f > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-25 19:40:08 +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 | 81 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 51 ++++++++++++++++++++++++ > sql/table.cc | 39 +++++++++++++++++- > 3 files changed, 170 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..de1323e 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,86 @@ 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; > +CREATE TABLE t1 ( > +f2 INT, > +pk2 INT, > +f1 VARCHAR(3), > +pk1 VARCHAR(1000), > +PRIMARY KEY (pk1,pk2), > +KEY k1(pk1,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(k1) where f2 <= 5 and pk2 <=5 and pk1 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["k1"], > + "key": "k1", > + "key_length": "3011", > + "used_key_parts": ["pk1", "f2", "pk2"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "((t1.f2 <= 5) and (t1.pk2 <= 5) and (t1.pk1 = '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..c8acfc5 100644 > --- a/mysql-test/t/innodb_ext_key.test > +++ b/mysql-test/t/innodb_ext_key.test > @@ -778,5 +778,56 @@ 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; > + > +CREATE TABLE t1 ( > +f2 INT, > +pk2 INT, > +f1 VARCHAR(3), > +pk1 VARCHAR(1000), > +PRIMARY KEY (pk1,pk2), > +KEY k1(pk1,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(k1) where f2 <= 5 and pk2 <=5 and pk1 = '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..a1e9ebf 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,23 @@ 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 (keyinfo->ext_key_part_map & 1<<i) > + { > + 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

1 0

[Maria-developers] Cannot push branch to github
by Valentin Rakush 26 May '17

26 May '17

Hi all, I am trying to do first pull request to https://github.com/MariaDB/server but I cannot push my new branch to github. Git says "ERROR: Permission to MariaDB/server.git denied to alpes214. fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists." Should I talk to somebody to get access rights first? -- Kind regards, Valentin Rakush

2 3

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