From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4ADBB23AC9 for ; Mon, 17 Dec 2018 12:35:10 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id lzZThtTaDoB6 for ; Mon, 17 Dec 2018 12:35:10 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 707CE2333F for ; Mon, 17 Dec 2018 12:35:09 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: do not analyze incorrect statistics From: "n.pettik" In-Reply-To: <5bcf80f0f9675871a39fb14cff5f216af7857759.1544880511.git.imeevma@gmail.com> Date: Mon, 17 Dec 2018 20:35:04 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <3D334916-245D-4A80-B4F4-424BE1B18C2D@tarantool.org> References: <5bcf80f0f9675871a39fb14cff5f216af7857759.1544880511.git.imeevma@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Imeev Mergen > After this patch, the analysis of statistics in "_sql_stat1" and > "_sql_stat4" with the wrong table name or table index will not be > executed. Could you please provide more detailed descriptions? Like what was exact reason of crash, what did you do etc. =46rom backtrace I see that segfault takes place somewhere in lua_pushstring(). >=20 > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 3f49280..9865407 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -1222,27 +1222,26 @@ analysis_loader(void *data, int argc, char = **argv, char **unused) > UNUSED_PARAMETER2(unused, argc); > if (argv =3D=3D 0 || argv[0] =3D=3D 0 || argv[2] =3D=3D 0) > return 0; > - struct analysis_index_info *info =3D (struct analysis_index_info = *) data; > - assert(info->stats !=3D NULL); > - struct index_stat *stat =3D &info->stats[info->index_count++]; > struct space *space =3D space_by_name(argv[0]); > if (space =3D=3D NULL) > - return -1; > - struct index *index; > + return 0; > uint32_t iid =3D box_index_id_by_name(space->def->id, argv[1], > strlen(argv[1])); > /* > * Convention is if index's name matches with space's > * one, then it is primary index. > */ > - if (iid !=3D BOX_ID_NIL) { > - index =3D space_index(space, iid); > - } else { > + if (iid =3D=3D BOX_ID_NIL) { > if (sqlite3_stricmp(argv[0], argv[1]) !=3D 0) > - return -1; > - index =3D space_index(space, 0); > + return 0; > + iid =3D 0; > } > - assert(index !=3D NULL); > + struct index *index =3D space_index(space, iid); > + if (index =3D=3D NULL) > + return 0; > + struct analysis_index_info *info =3D (struct analysis_index_info = *) data; > + assert(info->stats !=3D NULL); > + struct index_stat *stat =3D &info->stats[info->index_count++]; Why do you need all these refactoring things? AFAIU the only thing you need to do is return 0 instead of -1; otherwise error will be = handled without error message which results in segfault. Ofc, index_count should be incremented after index verification. > /* > * Additional field is used to describe total > * count of tuples in index. Although now all > @@ -1395,16 +1394,18 @@ load_stat_from_space(struct sqlite3 *db, const = char *sql_select_prepare, > continue; > uint32_t sample_count =3D sqlite3_column_int(stmt, 2); > struct space *space =3D space_by_name(space_name); > - assert(space !=3D NULL); > - struct index *index; > + if (space =3D=3D NULL) > + continue; > uint32_t iid =3D box_index_id_by_name(space->def->id, = index_name, > strlen(index_name)); > - if (sqlite3_stricmp(space_name, index_name) =3D=3D 0 && > - iid =3D=3D BOX_ID_NIL) > - index =3D space_index(space, 0); > - else > - index =3D space_index(space, iid); > - assert(index !=3D NULL); > + if (iid =3D=3D BOX_ID_NIL) { > + if (sqlite3_stricmp(space_name, index_name) !=3D = 0) > + continue; > + iid =3D 0; > + } The same question here: do you really need this refactoring? Let=E2=80=99s make patch as small as you can. I don=E2=80=99t think that = such refactoring makes code cleaner, it only complicates review process. Tell me if I am wrong. > + struct index *index =3D space_index(space, iid); > + if (index =3D=3D NULL) > + continue; > uint32_t column_count =3D = index->def->key_def->part_count; > struct index_stat *stat =3D &stats[current_idx_count]; > stat->sample_field_count =3D column_count; > @@ -1463,18 +1464,18 @@ load_stat_from_space(struct sqlite3 *db, const = char *sql_select_prepare, > if (index_name =3D=3D NULL) > continue; > struct space *space =3D space_by_name(space_name); > - assert(space !=3D NULL); > - struct index *index; > + if (space =3D=3D NULL) > + continue; > uint32_t iid =3D box_index_id_by_name(space->def->id, = index_name, > strlen(index_name)); > - if (iid !=3D BOX_ID_NIL) { > - index =3D space_index(space, iid); > - } else { > + if (iid =3D=3D BOX_ID_NIL) { > if (sqlite3_stricmp(space_name, index_name) !=3D = 0) > - return -1; > - index =3D space_index(space, 0); > + continue; > + iid =3D 0; > } > - assert(index !=3D NULL); > + struct index *index =3D space_index(space, iid); > + if (index =3D=3D NULL) > + continue; > uint32_t column_count =3D = index->def->key_def->part_count; > if (index !=3D prev_index) { > if (prev_index !=3D NULL) { > @@ -1544,18 +1545,18 @@ load_stat_to_index(struct sqlite3 *db, const = char *sql_select_load, > if (index_name =3D=3D NULL) > continue; > struct space *space =3D space_by_name(space_name); > - assert(space !=3D NULL); > - struct index *index; > + if (space =3D=3D NULL) > + continue; > uint32_t iid =3D box_index_id_by_name(space->def->id, = index_name, > strlen(index_name)); > - if (iid !=3D BOX_ID_NIL) { > - index =3D space_index(space, iid); > - } else { > + if (iid =3D=3D BOX_ID_NIL) { > if (sqlite3_stricmp(space_name, index_name) !=3D = 0) > - return -1; > - index =3D space_index(space, 0); > + continue; > + iid =3D 0; > } > - assert(index !=3D NULL); > + struct index *index =3D space_index(space, iid); > + if (index =3D=3D NULL) > + continue; > free(index->def->opts.stat); > index->def->opts.stat =3D stats[current_idx_count++]; > } > diff --git a/test/sql-tap/analyze1.test.lua = b/test/sql-tap/analyze1.test.lua > index ea414e9..df5aaf8 100755 > --- a/test/sql-tap/analyze1.test.lua > +++ b/test/sql-tap/analyze1.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(38) > +test:plan(40) >=20 > --!./tcltestrunner.lua > -- 2005 July 22 > @@ -346,6 +346,31 @@ test:do_execsql_test( > -- > }) >=20 > +-- > +-- gh-3866 Wrong data in _sql_stat* tables leads to SEGMENTATION > +-- FAULT > +=E2=80=94 Nit: I would better say =E2=80=9Cwrong space name leads to segfault=E2=80=9D= , since tests on wrong statistics inserted to stat tables are above your tests. > +test:do_execsql_test( > + "analyze-4.3", > + [[ > + DELETE FROM "_sql_stat1"; > + DELETE FROM "_sql_stat4"; > + INSERT INTO "_sql_stat1" VALUES('abc', 'bca', 'cab'); > + ANALYZE t4; > + ]], { > + -- > + -- > + }) > + > +test:do_execsql_test( > + "analyze-4.4", > + [[ > + INSERT INTO "_sql_stat4" VALUES('abc', 'bca', 'cab', 'acb', = 'bac', 'cba'); > + ANALYZE t4; > + ]], { > + -- > + -- > + }) Add also test, where name of table is correct, but name of index is = wrong.