Bug#1634932: Assertion failure in thread x in file fts0que.cc by robgolebiowski · Pull Request #1297 · percona/percona-server

robgolebiowski · 2017-01-19T09:16:37Z

No test case for this bug as it is very time and resource consuming.

laurynas-biveinis · 2017-01-20T04:16:23Z

Same comment re. Launchpad bug handling as in the other PRs

laurynas-biveinis · 2017-01-20T04:31:14Z

The commit message should be slightly more descriptive ("foo and bar allocated but did not free baz in the case of error")

laurynas-biveinis · 2017-01-20T04:31:49Z

Can you tell why the testcase is time and resource consuming? Is it possible to reduce it?

laurynas-biveinis · 2017-01-20T04:20:22Z

 }

+
+/*******************************************************************//**


New InnoDB code should use the new InnoDB function header comment style (see e.g. fts_ast_string_create, innobase_mysql_tmpfile, fil_rename_tablespace_check for examples)

laurynas-biveinis · 2017-01-20T04:23:15Z

+static
+void
+fts_query_free_intersection(
+	fts_query_t*	query)		/*!< in: query instance */


Either here, either in the function body the indentation is wrong (should be tabs everywhere)

laurynas-biveinis · 2017-01-20T04:24:56Z

 		/* error is passed by 'query->error' */
 		if (query->error != DB_SUCCESS) {
 			ut_ad(query->error == DB_FTS_EXCEED_RESULT_CACHE_LIMIT);
+                        fts_query_free_intersection(query);


Indentation (should be tabs)

laurynas-biveinis · 2017-01-20T04:28:42Z

 				query, ranking->doc_id, ranking->rank);

 		if (query->error != DB_SUCCESS) {
+                        if (query->oper == FTS_EXIST && query->intersection != NULL)


The query->oper == FTS_EXIST check looks redundant: the function asserts query->intersection == NULL at the start and only sets it to something if that check passes

Can replace with an ut_ad(query->oper == FTS_EXIST) if the branch is taken

robgolebiowski · 2017-01-26T16:00:55Z

Jenkin's link: http://jenkins.percona.com/job/percona-server-5.6-param/1594/

laurynas-biveinis · 2017-01-27T04:06:31Z

@@ -0,0 +1,50 @@
+--echo #


Please use upstream bug number for upstream bugs (ie. bug83648.test). After the Percona Server merge the next step is to send the bugfix to Oracle (get your Percona OCA sorted out meanwhile)

laurynas-biveinis · 2017-01-27T04:06:52Z

+
+-- source include/have_innodb.inc
+
+# Must have debug code to use SET SESSION debug


Redundant comment

laurynas-biveinis · 2017-01-27T04:09:38Z

+delimiter ;//
+
+call populate_t1;
+SET autocommit=1;


not needed, from documentation: "If autocommit is 0 and you change it to 1, MySQL performs an automatic COMMIT of any open transaction"

laurynas-biveinis · 2017-01-27T04:10:20Z

+call populate_t1;
+SET autocommit=1;
+
+let @saved_session_debug= @@session.debug;


No need to save and restore session variables - MTR will disconnect the session at the end of the testcase. Only global variables have to be saved and restored

laurynas-biveinis · 2017-01-27T04:11:23Z

+SET autocommit=1;
+
+let @saved_session_debug= @@session.debug;
+SET SESSION debug="d,fts_instrument_result_cache_limit";


But here you want to enable some debug setting and later disable it, so for that here use "+d,fts_...", and below "-d,fts_..."

Done, removed.

laurynas-biveinis · 2017-01-27T04:12:30Z

+# Must have debug code to use SET SESSION debug
+--source include/have_debug.inc
+
+create table `t1` (


Consistent case for SQL keywords, suggesting uppercase

laurynas-biveinis · 2017-01-27T04:22:04Z

      !select_lex->materialized_table_count)
  {
-    init_ftfuncs(thd, select_lex, order);
+    if (init_ftfuncs(thd, select_lex, order))


All the other callers of init_ftfuncs do not check its return code - are they affected by this bug?

They were. This was fixed in 5.7 by bug fix:21140111. I have ported this bug into 5.6. The bug fix is not completely correct - needed to move check for errors from init_ftfuncs bit higher in sql_delete.cc, otherwise error from init_ftfuncs caused assertion problem.

The problem here is missing error check in Item_func_match::init_search() after the handler call ft_init_ext_with_hints(), and missing error propagation in the call stack. The fix also includes a new inlined query block property function has_ft_funcs() that is used to avoid an unnecessary and costly function call to optimize full-text searches. (cherry picked from commit e78f3cb)

robgolebiowski · 2017-02-01T08:46:32Z

Jenkins run: http://jenkins.percona.com/job/percona-server-5.6-param/1637/

laurynas-biveinis

Er, no, please check Jenkins results - the debug build crashes constantly

robgolebiowski · 2017-02-02T09:42:17Z

Jenkins run => http://jenkins.percona.com/job/percona-server-5.6-param/1641/ There are some tests failing, but I have run them locally and all pass fine.

percona-ysorokin · 2017-02-07T12:30:14Z

@@ -0,0 +1,55 @@
+--echo #


Shouldn't this test be put into innod_fts test suite rather than innodb?

percona-ysorokin · 2017-02-07T12:31:29Z

+	`text_content` MEDIUMTEXT, PRIMARY KEY (`FTS_DOC_ID`)
+) ENGINE=InnoDB DEFAULT CHARSET=latin1;
+
+CREATE UNIQUE INDEX FTS_DOC_ID_INDEX ON t1(FTS_DOC_ID);


Is additional index really needed? FTS_DOC_ID is already the primary key.

This is taken from here: https://dev.mysql.com/doc/refman/5.6/en/optimizing-innodb-bulk-data-loading.html . To speed up the inserts.

percona-ysorokin · 2017-02-07T12:38:04Z

+DELETE FROM t1
+WHERE MATCH text_content AGAINST ('+some_text_1234' IN BOOLEAN MODE);
+
+SET GLOBAL innodb_ft_result_cache_limit=default;


Store @@global.innodb_ft_result_cache_limit at the beginning of the test and restore it here.

percona-ysorokin · 2017-02-07T12:39:18Z

 			     && rbt_size(query->doc_ids) <= n_doc_ids));
-		}
+		} else if (query->intersection != NULL) {
+                        fts_query_free_intersection(query);


percona-ysorokin · 2017-02-07T12:39:36Z

-		}
+		} else if (query->intersection != NULL) {
+                        fts_query_free_intersection(query);
+                }


percona-ysorokin · 2017-02-08T14:56:58Z

+--source include/have_innodb.inc
+--source include/have_debug.inc
+
+SET @saved_innodb_ft_result_cache_limit= @@innodb_ft_result_cache_limit;


You need to save GLOBAL variable here, e.g.

SET @saved_innodb_ft_result_cache_limit= @@global.innodb_ft_result_cache_limit;

percona-ysorokin · 2017-02-08T15:06:08Z

 	query->total_size -= SIZEOF_RBT_CREATE;
 }

+/**


InnoDB code (everything in storage/innobase*) requires InnoDB-style code formatting:

Tab characters must be used for indentation

Blocks must be indented by 1 tab

1 tab = 8 spaces (set this in your editor)

SQL code (for instance sql/* and include/*) requires MySQL-style code formatting:

Using spaces only for indentation.

Blocks must be indented by 2 spaces.

The bug was caused by three problems: 1) query->intersection was not freed in case of error caused by exceeding innodb_ft_result_cache_limit. 2) errors from init_ftfuncs were not propagated - this was fixed in 5.7 by bug fix 21140111. This was ported into 5.6 3) bug fix 21140111 was causing assertion failure when innodb_ft_result_cache was exceeded in DELETE command. This was also fixed.

Summary: Add ddst_dict_init handler api support for rocksdb dd Currently rocksdb ddst_dict_init() doesn't add any rocksdb specific tables and tablespace during ddse initialize. Pull Request resolved: facebook/mysql-5.6#1297 Test Plan: CI Differential Revision: D46700996 fbshipit-source-id: fbb80035b54cc068c845e63fe6903c8488fce1d3

laurynas-biveinis self-assigned this Jan 20, 2017

laurynas-biveinis suggested changes Jan 20, 2017

View reviewed changes

robgolebiowski force-pushed the 5.6-ps-bug1634932 branch from d677ed4 to d419459 Compare January 23, 2017 10:11

laurynas-biveinis requested a review from percona-ysorokin January 27, 2017 04:04

laurynas-biveinis suggested changes Jan 27, 2017

View reviewed changes

robgolebiowski force-pushed the 5.6-ps-bug1634932 branch from d419459 to a1811e2 Compare January 31, 2017 14:45

laurynas-biveinis approved these changes Feb 1, 2017

View reviewed changes

laurynas-biveinis suggested changes Feb 1, 2017

View reviewed changes

robgolebiowski force-pushed the 5.6-ps-bug1634932 branch from a1811e2 to 4c4b851 Compare February 1, 2017 18:54

laurynas-biveinis approved these changes Feb 2, 2017

View reviewed changes

percona-ysorokin requested changes Feb 7, 2017

View reviewed changes

robgolebiowski force-pushed the 5.6-ps-bug1634932 branch from 4c4b851 to e85f501 Compare February 7, 2017 17:44

percona-ysorokin requested changes Feb 8, 2017

View reviewed changes

robgolebiowski force-pushed the 5.6-ps-bug1634932 branch from e85f501 to ef2c0bc Compare February 10, 2017 10:56

percona-ysorokin approved these changes Feb 10, 2017

View reviewed changes

robgolebiowski merged commit 23243c0 into percona:5.6 Feb 10, 2017

robgolebiowski deleted the 5.6-ps-bug1634932 branch February 10, 2017 14:36

		}


		/*****************************************************************//


		-- source include/have_innodb.inc

		# Must have debug code to use SET SESSION debug

Uh oh!

Conversation

robgolebiowski commented Jan 19, 2017

laurynas-biveinis commented Jan 20, 2017

laurynas-biveinis commented Jan 20, 2017

laurynas-biveinis commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robgolebiowski commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robgolebiowski commented Feb 1, 2017

laurynas-biveinis left a comment

Choose a reason for hiding this comment

robgolebiowski commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants