- developers - lists.mariadb.org

Re: [Maria-developers] c01b3d91690: MDEV-22551 Server crashes in skip_trailing_space upon ANALYZE on table with long unique, or Assertion `marked_for_read()' fails
by Sergei Golubchik 03 Jun '20

03 Jun '20

Hi, Sachin! On Jun 03, Sachin wrote: > revision-id: c01b3d91690 (mariadb-10.4.11-224-gc01b3d91690) > parent(s): 0552ed86782 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2020-06-02 08:24:13 +0530 > message: > > MDEV-22551 Server crashes in skip_trailing_space upon ANALYZE on table with long unique, or Assertion `marked_for_read()' fails > > If Blob(or parent) field read_set is not set then Do not calc hash in > that case. No, I think this is wrong. The problem, again, is that read_set bits are set incorrectly, and the fix is, again, to set them correctly, not to perform more read_set checks at run time. Think about what's happening here. The server tries to calculate a generated column value (because gcol is present in read_set) when not all argument columns were read (because they are not in read_set). This is an impossible situation, a gcol can _only_ be in read_set if all its dependencies are also there, you've seen in TABLE::mark_virtual_columns_for_write() that for every marked gcol all dependencies are also recursively marked. So in this case ANALYZE breaks this rule and should be fixed not to do it. There are two possible fixes - either ANALYZE should mark dependencies for all gcols (marking blobs in this bug) or ANALYZE should skip and not mark gcols. Better ask somebody who's more famiiar with EITS about what fix is preferred. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e2017fc3a5f: MDEV-22441 implement a generic way to change a value of a variable in scope
by Sergei Golubchik 03 Jun '20

03 Jun '20

Hi, Eugene! On Jun 03, Eugene Kosov wrote: > revision-id: e2017fc3a5f (mariadb-10.2.31-219-ge2017fc3a5f) This should go into 10.5, not into old-old GA 10.2. Besides, 10.2 cannot use C++11 features, 10.5 can. > parent(s): 1b3adaab25c > author: Eugene Kosov <claprix(a)yandex.ru> > committer: Eugene Kosov <eugene.kosov(a)mariadb.com> > timestamp: 2020-05-27 12:45:41 +0300 > message: > > MDEV-22441 implement a generic way to change a value of a variable in scope > > Scope_value<T> class changes a variable value to some temporary value while > it exits and restores an old value in it's destructor. > > make_scope_value() is a function targeted for C++11 which allows to omit > a template parameter which is required by Scope_value<T> I think I'd name it backup_and_change_scoped() or something like that. But it's just a suggestion, feel free to keep make_scope_value() name if you like it better. > Sql_mode_save removed and the only usage is replaced with Scope_value<T> > > Sql_mode_save removal should not be merged in newer branches! > > --- > include/scope.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > sql/sql_acl.cc | 7 ++++--- > sql/sql_class.h | 16 ---------------- > 3 files changed, 50 insertions(+), 19 deletions(-) > > diff --git a/include/scope.h b/include/scope.h > new file mode 100644 > index 00000000000..81061ceed37 > --- /dev/null > +++ b/include/scope.h > @@ -0,0 +1,46 @@ > +/* > + Copyright (c) 2020, MariaDB Corporation. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +#pragma once use include guards, please, not a pragma > + > +namespace maria and, as you remember, we didn't quite decide on a global server-wide namespace, so it's not the MDEV that should introduce it. > +{ > + > +template <typename T> class Scope_value > +{ > +public: > + Scope_value(T &variable, const T &scope_value) > + : variable_(variable), saved_value_(variable) > + { > + variable= scope_value; > + } > + > + ~Scope_value() { variable_= saved_value_; } > + > +private: > + T &variable_; > + T saved_value_; > +}; > + > +// Starting from C++11 this allows to omit template argument like this: > +// auto _ = make_scope_value(var, tmp_value); > +template <typename T> > +Scope_value<T> make_scope_value(T &variable, const T &scope_value) > +{ > + return Scope_value<T>(variable, scope_value); > +} as you've seen from Sql_mode_save usage, sometimes the value is assigned later, not when it's saved. Please, add another helper that doesn't require to provide a value. Like template <typename T> Scope_value<T> backup_scoped(T &variable) { return Scope_value<T>(variable, variable); } > + > +} // namespace maria Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 0552ed86782: MDEV-21804 Assertion `marked_for_read()' failed upon INSERT into table with long unique blob under binlog_row_image=NOBLOB
by Sergei Golubchik 03 Jun '20

03 Jun '20

Hi, Sachin! ok to push with one change, see below. On Jun 03, Sachin wrote: > revision-id: 0552ed86782 (mariadb-10.4.11-223-g0552ed86782) > parent(s): 61b2cd38d48 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2020-06-02 08:24:02 +0530 > message: > > MDEV-21804 Assertion `marked_for_read()' failed upon INSERT into table with long unique blob under binlog_row_image=NOBLOB > > Problem:- Calling mark_columns_per_binlog_row_image() earlier may change the > result of mark_virtual_columns_for_write() , Since it can set the bitmap on > for virtual column, and henceforth mark_virtual_column_deps(field) will > never be called in mark_virtual_column_with_deps. > > This bug is not specific for long unique, It also fails for this case > create table t2(id int primary key, a blob, b varchar(20) as (LEFT(a,2))); > > diff --git a/sql/table.cc b/sql/table.cc > index bc8ddbc0f9d..8edbbe8d599 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -7040,7 +7040,6 @@ void TABLE::mark_columns_needed_for_update() > DBUG_ENTER("TABLE::mark_columns_needed_for_update"); > bool need_signal= false; > > - mark_columns_per_binlog_row_image(); > > if (triggers) > triggers->mark_fields_used(TRG_EVENT_UPDATE); > @@ -7048,6 +7047,7 @@ void TABLE::mark_columns_needed_for_update() > mark_default_fields_for_write(FALSE); > if (vfield) > need_signal|= mark_virtual_columns_for_write(FALSE); > + mark_columns_per_binlog_row_image(); I think that's too early. In that if() below you can see > if (file->ha_table_flags() & HA_REQUIRES_KEY_COLUMNS_FOR_DELETE) > { > KEY *end= key_info + s->keys; ... if (bitmap_fast_test_and_set(read_set, idx)) continue; if (field[idx]->vcol_info) mark_virtual_col(field[idx]); and mark_columns_per_binlog_row_image() might break that. Better to put mark_columns_per_binlog_row_image() at the very end, just before signalling. > @@ -7132,7 +7132,6 @@ void TABLE::mark_columns_needed_for_update() > void TABLE::mark_columns_needed_for_insert() > { > DBUG_ENTER("mark_columns_needed_for_insert"); > - mark_columns_per_binlog_row_image(); > > if (triggers) > { > @@ -7152,6 +7151,7 @@ void TABLE::mark_columns_needed_for_insert() > /* Mark virtual columns for insert */ > if (vfield) > mark_virtual_columns_for_write(TRUE); > + mark_columns_per_binlog_row_image(); this looks ok. > if (check_constraints) > mark_check_constraint_columns_for_read(); > DBUG_VOID_RETURN; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Progress report - Week 4
by Mohammed Hammaad Mateen 02 Jun '20

02 Jun '20

Greetings, Hope you are safe and doing great, This post describes the things I've done in my fourth week [25-31 May] of Community Bonding Period under the mentor-ship of Sergei Golubchik and Oleksandr Byelkin for GSoC-20. The tasks taken up for this week was to study and analyze *INSERT SELECT RETURNING* and developing a test suite for *MULTIPLE TABLE DELETE RETURNING.* The following attachment contains the test cases for evaluating Multiple Delete Returning, as they are developed before actual implementation of code, please do notify me of any improvement needed, I'd be glad to resolve. Regards, Mohammed Hammaad Mateen

2 1

Re: [Maria-developers] fab8349af86: MDEV-21804 Assertion `marked_for_read()' failed upon INSERT into table with long unique blob under binlog_row_image=NOBLOB
by Sergei Golubchik 01 Jun '20

01 Jun '20

Hi, Sachin! On May 31, Sachin wrote: > revision-id: fab8349af86 (mariadb-10.4.11-223-gfab8349af86) > parent(s): 61b2cd38d48 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2020-05-29 11:56:45 +0530 > message: > > MDEV-21804 Assertion `marked_for_read()' failed upon INSERT into table with long unique blob under binlog_row_image=NOBLOB > > Problem:- > In the case of BINLOG_ROW_IMAGE_NOBLOB table->readset is PKE + non blob fields > But while updating virtual fields we need to read the blob field, which will > generate assert failure. > Solution:- > If binlog_row_image == NOBLOB, then set read_bit for the blob field. > This bug is not specific for long unique, It also fails for this case > create table t2(id int primary key, a blob, b varchar(20) as (LEFT(a,2))); > > diff --git a/sql/field.cc b/sql/field.cc > index 0a8fdc3d3f5..bcad056e76a 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -8467,6 +8467,10 @@ longlong Field_blob::val_int(void) > String *Field_blob::val_str(String *val_buffer __attribute__((unused)), > String *val_ptr) > { > + ulong binlog_row_image= table->in_use->variables.binlog_row_image; > + if (table->read_set && !bitmap_is_set(table->read_set, field_index) && > + binlog_row_image == BINLOG_ROW_IMAGE_NOBLOB) > + bitmap_set_bit(table->read_set, field_index); > DBUG_ASSERT(marked_for_read()); > char *blob; > memcpy(&blob, ptr+packlength, sizeof(char*)); A couple of red flags here. First, you don't fix BINLOG_ROW_IMAGE_MINIMAL. And generally you should not modify read/write sets from val_str, if should be done earlier before the execution phase. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Review for: MDEV-17399 Add support for JSON_TABLE, part #5
by Sergey Petrunia 26 May '20

26 May '20

Hi Alexey, On Fri, May 15, 2020 at 04:26:02PM +0400, Alexey Botchkov wrote: > See the next iteration here > https://github.com/MariaDB/server/commit/692cb566096d61b240ec26e84fc7d3c7d1… > So the ha_json_table;:position() was implemented. > The size of the reference depends on the nested path depth - each level > adds some 9 bytes to the initial 5. > That can be decreased but i decided to keep it simple initially and i doubt > we're going to have really deep > nesting in realistic scenarios. This is ok. I'm not sure if the server has a limit on the rowid size. Perhaps there is, and in that case we just need to limit the depth we allow. > And i'd like to ask you for some testing > queries here. Or just the model how > to produce queries that will be using the ::position() extensively. I've provided a testcase for the position() call in my previous email. https://gist.github.com/spetrunia/a905d51731c58f5439bd9f70c64cdc43 As for rnd_pos(), one case that I'm aware of is when the query does a filesort, and the row being sorted either includes a blob, or has the total length exceeding certain limit. I've tried constructing an example with blobs, and it fails with an assertion before the execution reaches rnd_pos() calls: select * from json_table('[{"color": "blue", "price": 50}, {"color": "red", "price": 100}, {"color": "rojo", "price": 10.0}, {"color": "blanco", "price": 11.0}]', '$[*]' columns( color varchar(100) path '$.color', price text path '$.price', seq for ordinality ) ) as T order by color desc; fails an assertion: mysqld: /home/psergey/dev-git/10.5-json/sql/field.cc:8309: virtual int Field_blob::store(const char*, size_t, CHARSET_INFO*): Assertion `marked_for_write_or_computed()' failed. Once that is fixed, this should use position() and rnd_pos() calls. This will likely expose more issues with rnd_pos(), see my comments to the Json_table_nested_path::set_position below. == A problem with VIEWs == mysql> select * -> from -> json_table( -> '[ {"color": "red", "price": 1}, {"color": "black", "price": 2}]', -> '$[*]' columns( color varchar(100) path '$.color')) as `T_A` where T_A.color<>'azul' ; +-------+ | color | +-------+ | red | | black | +-------+ 2 rows in set (6.34 sec) Good so far. Now, let's try creating a VIEW from this: create view v1 as select * from json_table( '[ {"color": "red", "price": 1}, {"color": "black", "price": 2}]', '$[*]' columns( color varchar(100) path '$.color')) as `T_A` where T_A.color<>'azul' ; select * from v1; ERROR 4041 (HY000): Unexpected end of JSON path in argument 2 to function 'JSON_TABLE' Examining the .frm file, I see something that looks like garbage data: query=select `T_A`.`color` AS `color` from JSON_TABLE(\'[ {"color": "red", "price": 1}, {"color": "black", "price": 2}]\', \'\'\0<D8>7^A<9C><FF>^?\0\0\0\0\0\0\0\0\0\0\0\0^A<A5><A5><A5><A5><A5>^H<D0>kWUU\0\0[ {"color": "red", "price": 1}, {"color": "black", "price": 2}]\0$[*]\'\' COLUMNS (`color` PATH \'^A<9C><FF>^?\0\0\0\0\0\0^A\0\0\0\0\0\0\0<A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5>\0\0\0\0<A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5><A5> <A5><A5><A5><A5><A5><A5> D^A<9C><FF>^?\0\0H:^A<9C><FF>^?\0\0<A5><A5><A5><A5><A5><A5><A5><A5>100\0<8F><8F><8F><8F>$.color\')) T_A where `T_A`.`color` <> \'azul\' EXPLAIN EXTENDED ...; SHOW WARNINGS; - also print something odd. == EXPLAIN [FORMAT=JSON] == I think EXPLAIN output should provide an indication that a table function is used. MySQL does it like so: <TODO> == Unneeded recursive rules in grammar == I've already complained about this: the grammar has recursive ON EMPTY, ON ERROR rules, which cause the following to be accepted (note the two ON EMPTY clauses) : select * from json_table('[{"color": "blue", "price": 50},{"color": "red"}]', '$[*]' columns( price varchar(255) path '$.price' default 'xyz' on empty default 'abc' on empty ) ) as T; > commit 692cb566096d61b240ec26e84fc7d3c7d13f024c > Author: Alexey Botchkov <holyfoot(a)askmonty.org> > Date: Fri May 15 15:25:28 2020 +0400 > > MDEV-17399 Add support for JSON_TABLE. > > Syntax for JSON_TABLE added. > The ha_json_table handler added. Executing the JSON_TABLE we > create the temporary table of the ha_json_table, add dependencies > of other tables and sometimes conditions to WHERE. > and sometimes conditions to WHERE. I think this is not true anymore? ... > diff --git a/include/my_base.h b/include/my_base.h > index 7efa5eb9673..89ef3e8e7c1 100644 > --- a/include/my_base.h > +++ b/include/my_base.h > @@ -523,6 +523,13 @@ enum ha_base_keytype { > #define HA_ERR_TABLESPACE_MISSING 194 /* Missing Tablespace */ > #define HA_ERR_SEQUENCE_INVALID_DATA 195 > #define HA_ERR_SEQUENCE_RUN_OUT 196 > + > +/* > + Share the error code to not increment the HA_ERR_LAST for now, > + as it disturbs some storage engine's tests. > + Probably should be fixed later. > +*/ > +#define HA_ERR_INVALID_JSON HA_ERR_TABLE_IN_FK_CHECK We definitely can't have this in the final patch. We need to either: A. Use an engine-specific error code. Check out MyRocks and ha_rocksdb::get_error_message() for an example of how this is done B. Indeed introduce a generic "Invalid JSON" error code. I'm hesitant about B, let's discuss this with other developers. ... > diff --git a/sql/table_function.cc b/sql/table_function.cc > new file mode 100644 > index 00000000000..71f2378ce7d > --- /dev/null > +++ b/sql/table_function.cc ... > + > +class ha_json_table: public handler > +{ Please add comments about these > +protected: > + Table_function_json_table *m_jt; > + String m_tmps; > + String *m_js; > + uchar *m_cur_pos; > +public: > + ha_json_table(TABLE_SHARE *share_arg, Table_function_json_table *jt): > + handler(&table_function_hton.m_hton, share_arg), m_jt(jt) > + { > + /* > + set the mark_trx_read_write_done to avoid the > + handler::mark_trx_read_write_internal() call. > + It relies on &ha_thd()->ha_data[ht->slot].ha_info[0] to be set. > + But we don't set the ha_data for the ha_json_table, and > + that call makes no sence for ha_json_table. > + */ > + mark_trx_read_write_done= 1; > + ref_length= (jt->m_depth+1) * (4 + 1) + > + jt->m_depth * (sizeof(Json_table_nested_path *)); > + } > + ~ha_json_table() {} > + handler *clone(const char *name, MEM_ROOT *mem_root) { return NULL; } > + const char *index_type(uint inx) { return "NONE"; } > + /* Rows also use a fixed-size format */ > + enum row_type get_row_type() const { return ROW_TYPE_FIXED; } > + ulonglong table_flags() const > + { > + return (HA_FAST_KEY_READ | /*HA_NO_BLOBS |*/ HA_NULL_IN_KEY | > + HA_CAN_SQL_HANDLER | > + HA_REC_NOT_IN_SEQ | HA_NO_TRANSACTIONS | > + HA_HAS_RECORDS | HA_CAN_HASH_KEYS); > + } > + ulong index_flags(uint inx, uint part, bool all_parts) const > + { > + return HA_ONLY_WHOLE_INDEX | HA_KEY_SCAN_NOT_ROR; > + } > + uint max_supported_keys() const { return 1; } > + uint max_supported_key_part_length() const { return MAX_KEY_LENGTH; } > + double scan_time() { return 1000000.0; } > + double read_time(uint index, uint ranges, ha_rows rows) { return 0.0; } The above two functions are never called. Please * add a comment saying that. * add a DBUG_ASSERT() into them to back the point made by the comment :-) > + int open(const char *name, int mode, uint test_if_locked); > + int close(void) { return 0; } > + int rnd_init(bool scan); > + int rnd_next(uchar *buf); > + int rnd_pos(uchar * buf, uchar *pos); > + void position(const uchar *record); > + int can_continue_handler_scan() { return 1; } > + int info(uint); > + int extra(enum ha_extra_function operation); > + THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to, > + enum thr_lock_type lock_type) > + { return NULL; } > + int create(const char *name, TABLE *form, HA_CREATE_INFO *create_info) > + { return 1; } > +private: > + void update_key_stats(); > +}; > + ... > +void Json_table_nested_path::set_position(const char *j_start, const uchar *pos) > +{ * This needs a comment. * I don't see where the value of m_ordinality_counter is restored? * The same about m_null - its value is not restored either? > + if (m_nested) > + { > + memcpy(&m_cur_nested, pos, sizeof(m_cur_nested)); > + pos+= sizeof(m_cur_nested); > + } > + > + m_engine.s.c_str= (uchar *) j_start + sint4korr(pos); > + m_engine.state= (int) pos[4]; > + if (m_cur_nested) > + m_cur_nested->set_position(j_start, pos); > +} > + ... > +int ha_json_table::info(uint) > +{ > + /* We don't want 0 or 1 in stats.records. */ > + stats.records= 4; > + return 0; Does this value matter? As far as I understand it doesn't, as the optimizer will use the estimates obtained from Table_function_json_table::get_estimates. Please add a comment about this. > +} > + ... Please document this function. What's last_column? Why does print() method get it and return it? > +int Json_table_nested_path::print(THD *thd, TABLE_LIST *sql_table, String *str, > + List_iterator_fast<Json_table_column> &it, > + Json_table_column **last_column) > +{ > + Json_table_nested_path *c_path= this; ... > diff --git a/sql/table_function.h b/sql/table_function.h > new file mode 100644 > index 00000000000..49d650cb7b2 > --- /dev/null > +++ b/sql/table_function.h > @@ -0,0 +1,168 @@ ... > +#include <json_lib.h> > + > +/* > + The Json_table_nested_path represents the 'current nesting' level > + for a set of JSON_TABLE columns. > + Each column (Json_table_column instance) is linked with corresponding > + 'nested path' object and gets it's piece of JSON to parse during the computation > + phase. > + The root 'nested_path' is always present as a part of Table_function_json_table, > + then other 'nested_paths' can be created and linked into a tree structure when new > + 'NESTED PATH' is met. The nested 'nested_paths' are linked with 'm_nested', the same-level > + 'nested_paths' are linked with 'm_next_nested'. > + So for instance > + JSON_TABLE( '...', '$[*]' > + COLUMNS( a INT PATH '$.a' , > + NESTED PATH '$.b[*]' COLUMNS (b INT PATH '$', > + NESTED PATH '$.c[*]' COLUMNS(x INT PATH '$')), > + NESTED PATH '$.n[*]' COLUMNS (z INT PAHT '$')) > + results in 4 'nested_path' created: > + root nested_b nested_c nested_n > + m_path '$[*]' '$.b[*]' '$.c[*]' '$.n[*] > + m_nested &nested_b &nested_c NULL NULL > + n_next_nested NULL &nested_n NULL NULL > + > +and 4 columns created: > + a b x z > + m_nest &root &nested_b &nested_c &nested_n > +*/ > + > + > +class Json_table_column; > + > +class Json_table_nested_path : public Sql_alloc > +{ > +public: > + bool m_null; > + json_path_t m_path; > + json_engine_t m_engine; > + json_path_t m_cur_path; > + > + /* Counts the rows produced. Value is set to the FOR ORDINALITY coluns */ > + longlong m_ordinality_counter; > + > + Json_table_nested_path *m_parent; > + Json_table_nested_path *m_nested, *m_next_nested; > + Json_table_nested_path **m_nested_hook; > + Json_table_nested_path *m_cur_nested; Please add documentation about the above members. I see the diagram above, but I think text descriptions are also needed for each member. What's m_nested_hook? > + Json_table_nested_path(Json_table_nested_path *parent_nest): > + m_parent(parent_nest), m_nested(0), m_next_nested(0), > + m_nested_hook(&m_nested) {} > + int set_path(THD *thd, const LEX_CSTRING &path); > + void scan_start(CHARSET_INFO *i_cs, const uchar *str, const uchar *end); > + int scan_next(); > + int print(THD *thd, TABLE_LIST *sql_table, String *str, > + List_iterator_fast<Json_table_column> &it, > + Json_table_column **last_column); > + void get_current_position(const char *j_start, uchar *pos) const; > + void set_position(const char *j_start, const uchar *pos); > +}; ... > +class Table_function_json_table : public Sql_alloc Please document the class and the data members. > +{ > +public: > + Item *m_json; > + Json_table_nested_path m_nested_path; > + List<Json_table_column> m_columns; > + table_map m_dep_tables; > + uint m_depth, m_cur_depth; > + > + Table_function_json_table(Item *json): m_json(json), m_nested_path(0), > + m_depth(0), m_cur_depth(0) {} > + > + /* > + Used in sql_yacc.yy. > + Represents the current NESTED PATH level being parsed. > + */ > + Json_table_nested_path *m_sql_nest; > + void add_nested(Json_table_nested_path *np); > + void leave_nested(); BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 070df171c1e: MDEV-16937 Strict SQL with system versioned tables causes issues
by Sergei Golubchik 26 May '20

26 May '20

Hi, Aleksey! ok to push On May 26, Aleksey Midenkov wrote: > revision-id: 070df171c1e (mariadb-10.3.21-91-g070df171c1e) > parent(s): ad41da5c934 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-05-25 14:47:26 +0300 > message: > > MDEV-16937 Strict SQL with system versioned tables causes issues > > Respect system fields in NO_ZERO_DATE mode. > > This is the subject for refactoring in MDEV-19597 > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] cd9cab54aac: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field
by Sergei Golubchik 26 May '20

26 May '20

Hi, Aleksey! Ok to push, thanks! On May 26, Aleksey Midenkov wrote: > revision-id: cd9cab54aac (mariadb-10.2.31-123-gcd9cab54aac) > parent(s): d275ecbd208 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-05-25 20:56:31 +0300 > message: > > MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field > > update_virtual_field() is called as part of index rebuild in > ha_myisam::repair() (MDEV-5800) which is done on bulk INSERT finish. > > Assertion in update_virtual_field() was put as part of MDEV-16222 > because update_virtual_field() returns in_use->is_error(). The idea: > wrongly mixed semantics of error status before update_virtual_field() > and the status returned by update_virtual_field(). The former can > falsely influence the latter. > > diff --git a/sql/table.cc b/sql/table.cc > index d6d86d96016..2429bb12abe 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -7707,15 +7707,17 @@ int TABLE::update_virtual_fields(handler *h, enum_vcol_update_mode update_mode) > > int TABLE::update_virtual_field(Field *vf) > { > - DBUG_ASSERT(!in_use->is_error()); > - Query_arena backup_arena; > DBUG_ENTER("TABLE::update_virtual_field"); > + Query_arena backup_arena; > + Counting_error_handler count_errors; > + in_use->push_internal_handler(&count_errors); > in_use->set_n_backup_active_arena(expr_arena, &backup_arena); > bitmap_clear_all(&tmp_set); > vf->vcol_info->expr->walk(&Item::update_vcol_processor, 0, &tmp_set); > vf->vcol_info->expr->save_in_field(vf, 0); > in_use->restore_active_arena(expr_arena, &backup_arena); > - DBUG_RETURN(in_use->is_error()); > + in_use->pop_internal_handler(); > + DBUG_RETURN(count_errors.errors); > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 7d593466a22: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field
by Sergei Golubchik 25 May '20

25 May '20

Hi, Aleksey! I don't see what you've changed. We've discussed that fix and that one isn't supposed to swap Diagnostics_area's like that. And in your new patch you do exactly the same. Possible correct approaches: * don't return in_use->is_error(), return the return value of vf->vcol_info->expr->walk() || vf->vcol_info->expr->save_in_field() This means that Item_field::update_vcol_processor() should also do the same, I suspect * Use thd->push_internal_handler() and Counting_error_handler. Or, better, Turn_errors_to_warnings_handler with counting. This is the simplest one. there's a third option: * always return 0, because, looking how it's used, I don't really see how update_virtual_field() can ever get an error. But it's not a particularly future-proof approach. And I just might be wrong about errors. On Apr 23, Aleksey Midenkov wrote: > revision-id: 7d593466a22 (mariadb-10.2.28-4-g7d593466a22) > parent(s): 7bc26de591c > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-11-07 10:45:21 +0300 > message: > > MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field > > Preserve and restore statement DA. > > update_virtual_field() is called as part of index rebuild in > ha_myisam::repair() (MDEV-5800) which is done on bulk INSERT finish. > > Assertion in update_virtual_field() was put as part of MDEV-16222 > because update_virtual_field() returns in_use->is_error(). The idea: > wrongly mixed semantics of error status before update_virtual_field() > and the status returned by update_virtual_field(). The former can > falsely influence the latter. > > Preserve global error status and run update_virtual_field() with clear > DA since no matter how SQL command is finished it must update the > index after bulk INSERT. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] Progress Report - Week 3
by Mohammed Hammaad Mateen 25 May '20

25 May '20

Greetings, Hope you are safe and doing great, This post describes the things I've done in my third week [ 18-24 May ] of Community Bonding Period under the mentor-ship of Sergei Golubchik and Oleksandr Byelkin for GSoC-20. The tasks taken up for this week was to study and analyze: *INSERT.... RETURNING:* • *TableList* consists of 1 table instance to use, where each instance is a structure consisting of db argument, table name argument, alias argument and lock type argument. *Referenced this from the table.h file.* • In *sql_parse.cc* <http://sql_parse.cc> under *case SQLCOM_INSERT*: we do a DBUG_ASSERT where first_table == all_table && first_table!=0 // we check if only 1 table is used and the table used is already created. • if lex-> has_returning() we increment the system status var by 1 and perform analyze.. insert.. returning. • we compute result by mysql_insert function by passing the following arguments. - Thread Handler thd - all table instances - list of all the fields used - the values we want to insert - the list of update feilds - and update value list - duplicate flag - ignore - result of select • if Inserting fails due to some reason we equate result to send_explain(thd) • we also update the MYSQL_INSERT_DONE with result of insert and the the row count. *INSERT.. SELECT.. RETURNING:* • We fix the lock on first table. • lock other tables until command is written to the binary log. • the procedure is same as discussed earlier with respect to insert.... returning. • To switch to the second table we traverse from first_table to (->) next_local and we compute select result with the help of select insert function. • we can now unlock the tables and we also need to check if something changed after unlocking, of that happens we should invalidate the table from the query cache using query_cache_invalidate3 function. • Manual cleaning of select result obtained from select insert must be done. Regards, Mohammed Hammaad Mateen

1 0