- developers - lists.mariadb.org

Re: [Maria-developers] 031efde365c: MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
by Sergei Golubchik 13 Nov '18

13 Nov '18

Hi, Oleksandr! On Nov 07, Oleksandr Byelkin wrote: > revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) > parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-11-07 14:29:47 +0100 > message: > > MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date > > - clean up DEFAULT() to work only with default value and correctly print > itself. > - fix of DBUG_ASSERT about fields read/write > - fix of field marking for write based really on the thd->mark_used_columns flag > diff --git a/sql/field.cc b/sql/field.cc > index caa84dc9932..6cd8940a893 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -70,8 +70,21 @@ const char field_separator=','; > #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ > ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1)) > > -#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))) > -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED DBUG_ASSERT(is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))) > +// Column marked for read or the field set to read out or record[0] or [1] > +#define ASSERT_COLUMN_MARKED_FOR_READ \ > + DBUG_ASSERT(!table || \ > + (!table->read_set || \ > + bitmap_is_set(table->read_set, field_index) || \ > + (!(ptr >= table->record[0] && \ > + ptr < table->record[0] + table->s->reclength)))) > + > +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ > + DBUG_ASSERT(is_stat_field || !table || \ > + (!table->write_set || \ > + bitmap_is_set(table->write_set, field_index) || \ > + (!(ptr >= table->record[0] && \ > + ptr < table->record[0] + table->s->reclength))) || \ > + (table->vcol_set && bitmap_is_set(table->vcol_set, field_index))) Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ. > > #define FLAGSTR(S,F) ((S) & (F) ? #F " " : "") > > diff --git a/sql/item.cc b/sql/item.cc > index 2adec33491b..6828a74f9ff 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -8813,15 +8824,19 @@ bool Item_default_value::fix_fields(THD *thd, Item **items) > goto error; > memcpy((void *)def_field, (void *)field_arg->field, > field_arg->field->size_of()); > - IF_DBUG(def_field->is_stat_field=1,); // a hack to fool ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED > + // If non-constant default value expression > if (def_field->default_value && def_field->default_value->flags) > { > uchar *newptr= (uchar*) thd->alloc(1+def_field->pack_length()); > if (!newptr) > goto error; > + /* > + Even if DEFAULT() do not read tables fields, the default value > + expression can do it. > + */ > fix_session_vcol_expr_for_read(thd, def_field, def_field->default_value); > if (thd->mark_used_columns != MARK_COLUMNS_NONE) > - def_field->default_value->expr->walk(&Item::register_field_in_read_map, 1, 0); > + def_field->default_value->expr->update_used_tables(); what's the difference? > def_field->move_field(newptr+1, def_field->maybe_null() ? newptr : 0, 1); > } > else > diff --git a/sql/item.h b/sql/item.h > index 8d02d981d38..5866f328f38 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5229,6 +5229,11 @@ class Item_default_value : public Item_field > return false; > } > table_map used_tables() const; > + virtual void update_used_tables() > + { > + if (field && field->default_value) > + field->default_value->expr->walk(&Item::register_field_in_read_map, 1, 0); why not field->default_value->expr->update_used_tables() ? > + } > Field *get_tmp_table_field() { return 0; } > Item *get_tmp_table_item(THD *thd) { return this; } > Item_field *field_for_view_update() { return 0; } > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index c282db42fdd..0deb5ec1362 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -5737,7 +5737,7 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list, > TABLE *table= field_to_set->table; > if (thd->mark_used_columns == MARK_COLUMNS_READ) > bitmap_set_bit(table->read_set, field_to_set->field_index); > - else > + else if (thd->mark_used_columns == MARK_COLUMNS_WRITE) > bitmap_set_bit(table->write_set, field_to_set->field_index); what does it affect? > } > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 031efde365c: MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
by Sergei Golubchik 12 Nov '18

12 Nov '18

Hi, Oleksandr! On Nov 12, Oleksandr Byelkin wrote: > > On Nov 07, Oleksandr Byelkin wrote: > > > revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) > > > parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 > > > author: Oleksandr Byelkin > > > committer: Oleksandr Byelkin > > > timestamp: 2018-11-07 14:29:47 +0100 > > > message: > > > > > > MDEV-16217: Assertion `!table || (!table->read_set || > > bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date > > > > > > - clean up DEFAULT() to work only with default value and correctly print > > > itself. > > > - fix of DBUG_ASSERT about fields read/write > > > - fix of field marking for write based really on the thd->mark_used_columns flag > > > > > diff --git a/sql/field.cc b/sql/field.cc > > > index caa84dc9932..6cd8940a893 100644 > > > --- a/sql/field.cc > > > +++ b/sql/field.cc > > > @@ -70,8 +70,21 @@ const char field_separator=','; > > > #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ > > > ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1)) > > > > > > -#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || > > (!table->read_set || bitmap_is_set(table->read_set, field_index))) > > > -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED > > DBUG_ASSERT(is_stat_field || !table || (!table->write_set || > > bitmap_is_set(table->write_set, field_index) || (table->vcol_set && > > bitmap_is_set(table->vcol_set, field_index)))) > > > +// Column marked for read or the field set to read out or record[0] or > > [1] > > > +#define ASSERT_COLUMN_MARKED_FOR_READ \ > > > + DBUG_ASSERT(!table || \ > > > + (!table->read_set || \ > > > + bitmap_is_set(table->read_set, field_index) || \ > > > + (!(ptr >= table->record[0] && \ > > > + ptr < table->record[0] + table->s->reclength)))) > > > + > > > +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ > > > + DBUG_ASSERT(is_stat_field || !table || \ > > > + (!table->write_set || \ > > > + bitmap_is_set(table->write_set, field_index) || \ > > > + (!(ptr >= table->record[0] && \ > > > + ptr < table->record[0] + table->s->reclength))) || \ > > > + (table->vcol_set && bitmap_is_set(table->vcol_set, > > field_index))) > > > > Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? > > I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ. > > I'd prefer to have them symmetric Right, but these macros already have non-symmetric names and non-symmetric definition - the second checks for vcol_set. The way you did it only makes the reader to pause and think in what case one can get `ptr` outside of table->record[0] for writes. Because your assert says "yes, it's ok to write into the column if ptr is outside of table->record[0]". If ptr cannot possibly be outside of record[0] for write, then it shouldn't be in the assert. > > > #define FLAGSTR(S,F) ((S) & (F) ? #F " " : "") > > > > > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > > > index c282db42fdd..0deb5ec1362 100644 > > > --- a/sql/sql_base.cc > > > +++ b/sql/sql_base.cc > > > @@ -5737,7 +5737,7 @@ find_field_in_table_ref(THD *thd, TABLE_LIST > > *table_list, > > > TABLE *table= field_to_set->table; > > > if (thd->mark_used_columns == MARK_COLUMNS_READ) > > > bitmap_set_bit(table->read_set, field_to_set->field_index); > > > - else > > > + else if (thd->mark_used_columns == MARK_COLUMNS_WRITE) > > > bitmap_set_bit(table->write_set, field_to_set->field_index); > > > > what does it affect? > > > > enum enum_mark_columns > { MARK_COLUMNS_NONE, MARK_COLUMNS_READ, MARK_COLUMNS_WRITE}; > > MARK_COLUMNS_NONE really used in the code, I remember that something was > marked incorrectly (it is how I found it) but what it is too late to ask > after many month as fix is done. this code is inside of if (thd->mark_used_columns != MARK_COLUMNS_NONE) so I don't see what your change could possibly do. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] bc7d600a27f: MDEV-15951 system versioning by trx id doesn't work with partitioning
by Sergei Golubchik 12 Nov '18

12 Nov '18

Hi, Nikita! On Nov 12, Nikita Malyavin wrote: > revision-id: bc7d600a27f4c6c14deea5f1d674c621f85b23ba (versioning-1.0.5-141-gbc7d600a27f) > parent(s): 90b292ce31170be33796b07723ffcef56bf8a679 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-09-21 22:42:03 +1000 > message: > > MDEV-15951 system versioning by trx id doesn't work with partitioning > > Fix partitioning for trx_id-versioned tables. > `partition by hash`, `range` and others now work. > `partition by system_time` is forbidden. > Currently we cannot use row_start and row_end in `partition by`, because > insertion of versioned field is done by engine's handler, as well as > row_start/row_end's value set up, which is a transaction id -- so it's > also forbidden. > > The drawback is that it's now impossible to use `partition by key()` > without parameters for such tables, because it references row_start and > row_end implicitly. > > * add handler::vers_can_native() > * drop Table_scope_and_contents_source_st::vers_native() > * drop partition_element::find_engine_flag as unused > * forbid versioning partitioning for trx_id as not supported > * adopt vers tests for trx_id partitioning > * forbid any row_end referencing in `partition by` clauses, > including implicit `by key()` > > --- > mysql-test/suite/versioning/r/partition.result | 79 ++++++++++++++++-- > .../suite/versioning/t/partition.combinations | 3 + > mysql-test/suite/versioning/t/partition.test | 97 ++++++++++++++++++++-- > sql/ha_partition.h | 10 +++ > sql/handler.cc | 34 ++------ > sql/handler.h | 8 +- > sql/partition_element.h | 15 ---- > sql/sql_class.h | 4 +- > sql/sql_partition.cc | 9 ++ > sql/sql_table.cc | 6 +- > sql/table.cc | 4 +- > sql/temporary_tables.cc | 10 ++- > 12 files changed, 214 insertions(+), 65 deletions(-) > > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index bfec0ce2d4b..cf4203b7949 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -1,6 +1,6 @@ > set system_versioning_alter_history=keep; > # Check conventional partitioning on temporal tables > -create table t1 (x int) > +create or replace table t1 (x int, ROW_START, ROW_END, period for system_time(row_start, row_end)) ouch. that looks very weird. please, only replace the type, not the whole column definition. Like everywhere else: --replace_result $sys_datatype_expl SYS_DATATYPE > with system versioning > partition by range columns (x) ( > partition p0 values less than (100), > @@ -501,6 +504,72 @@ set timestamp=1523466002.799571; > insert into t1 values (11),(12); > set timestamp=1523466004.169435; > delete from t1 where pk in (11, 12); > +# MDEV-15951 system versioning by trx id doesn't work with partitioning > +# currently trx_id does not support partitioning by system_time > +create or replace table t1( > +i int, > +row_start bigint unsigned generated always as row start, > +row_end bigint unsigned generated always as row end, > +period for system_time(row_start, row_end) > +) engine=InnoDB with system versioning partition by system_time ( if you have tests with innodb and trx_id, please, put them in a separate test file, say, partition_innodb.test. partition.test is for common tests that are repeated three times, with myisam, innodb, timestamps, and trx_id. There is no need to repeat your test three times. > diff --git a/mysql-test/suite/versioning/t/partition.combinations b/mysql-test/suite/versioning/t/partition.combinations > index 4d73ef5a5ea..561c5656929 100644 > --- a/mysql-test/suite/versioning/t/partition.combinations > +++ b/mysql-test/suite/versioning/t/partition.combinations > @@ -1,5 +1,8 @@ > [timestamp] > default-storage-engine=innodb > > +[trx_id] > +default-storage-engine=innodb > + > [myisam] > default-storage-engine=myisam But then you don't need partition.combinations at all, just add --source engines.inc like other tests do. > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index 8a251016703..ef49011b7f8 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -413,6 +413,16 @@ class ha_partition :public handler > > virtual void return_record_by_parent(); > > + virtual bool vers_can_native(THD *thd) > + { > + // PARTITION BY SYSTEM_TIME is not supported for now > + bool can= !thd->lex->part_info > + || thd->lex->part_info->part_type != VERSIONING_PARTITION; > + for (uint i= 0; i < m_tot_parts && can; i++) > + can= can && m_file[i]->vers_can_native(thd); > + return can; > + } > + > /* > ------------------------------------------------------------------------- > MODULE create/delete handler object > diff --git a/sql/handler.cc b/sql/handler.cc > index 306b0868d15..a7962039463 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -4938,6 +4938,7 @@ int ha_create_table(THD *thd, const char *path, > !create_info->tmp_table(); > > share.frm_image= frm; > + share.orig_table_name= table_name; why? if orig_table_name is NULL, error_table_name() will use table_name, so there's never need to make orig_table_name==table_name. and I don't understand all your other manipulations with orig_table_name. Could you please explain that? > > // open an frm image > if (share.init_from_binary_frm_image(thd, write_frm_now, > @@ -6952,29 +6953,6 @@ bool Vers_parse_info::fix_implicit(THD *thd, Alter_info *alter_info) > return false; > } > > -bool Table_scope_and_contents_source_st::vers_native(THD *thd) const > -{ > - if (ha_check_storage_engine_flag(db_type, HTON_NATIVE_SYS_VERSIONING)) > - return true; > - > -#ifdef WITH_PARTITION_STORAGE_ENGINE > - partition_info *info= thd->work_part_info; > - if (info && !(used_fields & HA_CREATE_USED_ENGINE)) > - { > - if (handlerton *hton= info->default_engine_type) > - return ha_check_storage_engine_flag(hton, HTON_NATIVE_SYS_VERSIONING); > - > - List_iterator_fast<partition_element> it(info->partitions); > - while (partition_element *partition_element= it++) > - { > - if (partition_element->find_engine_flag(HTON_NATIVE_SYS_VERSIONING)) > - return true; > - } > - } > -#endif > - return false; > -} what was wrong with that (and all other changed code in this file) ? > bool Table_scope_and_contents_source_st::vers_fix_system_fields( > THD *thd, Alter_info *alter_info, const TABLE_LIST &create_table, > bool create_select) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-17292.
by Sergei Golubchik 12 Nov '18

12 Nov '18

Hi, Alexey! On Nov 09, Alexey Botchkov wrote: > Hi, Sergei. > > I'm not sure what we want to do about this issue > https://jira.mariadb.org/browse/MDEV-17292 > > It's about pam_user_map.so packaging. > So we do like this compiled an put into the 'appropriate' > directory as /lib/security during install. Right? Yes. Not sure whether it should be hard-coded /lib/security or there's some distro-specific way of determining the correct path. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] e60cbbbda31: MDEV-12575: Server crash in AGGR_OP::put_record or in JOIN_CACHE::free or Invalid write in JOIN::make_aggr_tables_info
by Sergey Petrunia 09 Nov '18

09 Nov '18

Hi Varun, On Fri, Nov 09, 2018 at 02:26:04PM +0530, Varun wrote: > revision-id: e60cbbbda31d7a10e87c16313171b2036a4519c4 (mariadb-10.2.18-52-ge60cbbbda31) > parent(s): f8268f3cce4577c28ab62e53293556d05a74fb1a > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-01 14:27:24 +0530 > message: > > MDEV-12575: Server crash in AGGR_OP::put_record or in JOIN_CACHE::free or Invalid write in JOIN::make_aggr_tables_info > > During the optimize state of a query, we come know that the result set > would atmost contain one row, then for such a query we don't need > to compute GROUP BY, ORDER BY and DISTINCT. > ... > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 1fa80da85a6..a721aed6ce3 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -2245,6 +2245,18 @@ JOIN::optimize_inner() > if (!tables_list || !table_count) > { > choose_tableless_subquery_plan(); > + > + /* The output has atmost one row */ > + if (group_list) > + { > + group_list= 0; > + group_optimized_away= 1; > + rollup.state= ROLLUP::STATE_NONE; > + } > + order=0; > + simple_order=1; > + select_distinct=0; > + Please fix the coding style: - "x= y" (space after the =) - use NULL for pointers - use false for booleans s/atmost/at most/ Ok to push after the above is addressed. > if (select_lex->have_window_funcs()) > { > if (!(join_tab= (JOIN_TAB*) thd->alloc(sizeof(JOIN_TAB)))) > _______________________________________________ > 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

Re: [Maria-developers] [Commits] 0187bb03884: MDEV_-17589: Stack-buffer-overflow with indexed varchar (utf8) field
by Sergey Petrunia 08 Nov '18

08 Nov '18

Hi Varun, On Mon, Nov 05, 2018 at 07:36:17PM +0530, Varun wrote: > revision-id: 0187bb03884a34ca8de96e47f4cb1f3d860e6db9 (mariadb-10.2.16-224-g0187bb03884) > parent(s): 8a346f31b913daa011085afec2b2d38450c73e00 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-05 19:32:26 +0530 > message: > > MDEV_-17589: Stack-buffer-overflow with indexed varchar (utf8) field > > Create a new constant MAX_DATA_LENGTH_FOR_KEY. > Replace the value of MAX_KEY_LENGTH to also include the LENGTH and NULL BYTES. > ... > diff --git a/mysql-test/r/partition_datatype.result b/mysql-test/r/partition_datatype.result > index 2e518c194f0..66d003c97cb 100644 > --- a/mysql-test/r/partition_datatype.result > +++ b/mysql-test/r/partition_datatype.result > @@ -314,7 +314,7 @@ bbbb > drop table t1; > set sql_mode=''; > create table t1 (a varchar(3070)) partition by key (a); > -ERROR HY000: The total length of the partitioning fields is too large > +drop table t1; I don't think it's a good idea that after this patch we allow the table definitions that we didn't allow before. This raises the question of datadir downgrades, etc. I think we should only remove the buffer overrun. Here's patch how to avoid the above: https://gist.github.com/spetrunia/7e7e3fcc60a81a6cb854e77c12c367d3 > create table t1 (a varchar(65532) not null) partition by key (a); > ERROR HY000: The total length of the partitioning fields is too large > create table t1 (a varchar(65533)) partition by key (a); ... > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index c55e8d138f8..c76c8613c57 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1168,8 +1168,8 @@ EXPLAIN > "access_type": "range", > "possible_keys": ["f2"], > "key": "f2", > - "key_length": "3070", > - "used_key_parts": ["f2", "pk1"], > + "key_length": "3074", > + "used_key_parts": ["f2", "pk1", "pk2"], Same reaction as above: This change shows that "Extended keys" feature now allows bigger set of key extensions than before. Can we avoid making this change by changing MAX_KEY_LENGTH to MAX_DATA_LENGTH_FOR_KEY somewhere? > "rows": 1, > "filtered": 100, > "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'", > diff --git a/sql/handler.h b/sql/handler.h > index ed2ef822c88..75f72bdfb53 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -378,6 +378,10 @@ enum enum_alter_inplace_result { > #define HA_KEY_NULL_LENGTH 1 > #define HA_KEY_BLOB_LENGTH 2 > > +#define MAX_KEY_LENGTH (MAX_DATA_LENGTH_FOR_KEY \ > + +(MAX_REF_PARTS \ > + *(HA_KEY_NULL_LENGTH + HA_KEY_BLOB_LENGTH))) > + I think this requires a comment. Please add something like /* Maximum length of any index lookup key, in bytes */ > #define HA_LEX_CREATE_TMP_TABLE 1U > #define HA_CREATE_TMP_ALTER 8U > > @@ -3421,14 +3425,14 @@ class handler :public Sql_alloc > uint max_key_parts() const > { return MY_MIN(MAX_REF_PARTS, max_supported_key_parts()); } > uint max_key_length() const > - { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_length()); } > + { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_length()); } > uint max_key_part_length() const > - { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_part_length()); } > + { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_part_length()); } > > virtual uint max_supported_record_length() const { return HA_MAX_REC_LENGTH; } > virtual uint max_supported_keys() const { return 0; } > virtual uint max_supported_key_parts() const { return MAX_REF_PARTS; } > - virtual uint max_supported_key_length() const { return MAX_KEY_LENGTH; } > + virtual uint max_supported_key_length() const { return MAX_DATA_LENGTH_FOR_KEY; } > virtual uint max_supported_key_part_length() const { return 255; } > virtual uint min_record_length(uint options) const { return 1; } > > diff --git a/sql/sql_const.h b/sql/sql_const.h > index 7395ae3c08a..0be63a2a5ad 100644 > --- a/sql/sql_const.h > +++ b/sql/sql_const.h > @@ -33,7 +33,7 @@ > #define MAX_SYS_VAR_LENGTH 32 > #define MAX_KEY MAX_INDEXES /* Max used keys */ > #define MAX_REF_PARTS 32 /* Max parts used as ref */ Please add a comment, something like /* Maximum length of the data part of an index lookup key. The "data part" is defined as the value itself, not including the NULL-indicator bytes or varchar length bytes ("the Extras"). We need this value because there was a bug where length of the Extras were not counted. You probably need MAX_KEY_LENGTH, not this constant. */ > -#define MAX_KEY_LENGTH 3072 /* max possible key */ > +#define MAX_DATA_LENGTH_FOR_KEY 3072 > #if SIZEOF_OFF_T > 4 > #define MAX_REFLENGTH 8 /* Max length for record ref */ > #else BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] b0d21b7: MDEV-14567 Include full name of object in message about incorrect value for column.
by Sergei Golubchik 07 Nov '18

07 Nov '18

Hi, Alexey! On Nov 07, Alexey Botchkov wrote: > > But that's fine to show, right? No NULL, no garbage. > > Looks like this: > Incorrect datetime value: '0000-00-00 00:00:00' for column > 'test.#sql-691c_15d.c1' at row 1 > No NULL, but that #sql-something name doesn't tell anything and is > always different so we have to add lots of --replace-result in tests. Right, but we already have a lot of replace-result in tests. #sql-691c_15d showing in error messages in a different issue (but see below). > My patch shows it as.this: > Incorrect datetime value: '0000-00-00 00:00:00' for column ' {temporary}.c1' at row 1 This isn't much better in my opinion. It only helps tests to avoid replace-result, and I always believed in solving test problems in tests, not in the server. Meaning, exactly, replace-result. For the end user #sql-691c_15d and {temporary} has about the save amount of information. > > By the way, can this error ever happen for internal temporary tables? > Yes. > In our tests it happens pretty often. > Code like this: > CREATE TABLE t3(c1 DATETIME NOT NULL); > INSERT INTO t3 VALUES (0); > > --echo > SET sql_mode = TRADITIONAL; > > --echo > --error ER_TRUNCATED_WRONG_VALUE > ALTER TABLE t3 ADD INDEX(c1); Oh, ALTER TABLE. This is different. In one of the system versioning related fixes, midenok added a TABLE_SHARE::error_table_name() method to be used in error messages in ALTER. See 912769b7dc. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] cd41d376d41: MDEV-16697: Fix difference between 32bit/windows and 64bit systems in allowed select nest level
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Oleksandr! Why did you do that? You wrote "Fix difference between 32bit/windows and 64bit systems", but to fix just that it would've been enough to change nesting_map to ulonglong. Using Bitmap<> you're basically preparing for any arbitrary >64 nesting depth. Is this your goal? What feature do you need it for? On Nov 06, Oleksandr Byelkin wrote: > revision-id: cd41d376d413d066f3d497a67d039515fd385a1c (versioning-1.0.5-36-gcd41d376d41) > parent(s): 1748a31ae8d69e4939336f644f884e9de3039e7f > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2018-07-05 17:49:44 +0200 > message: > > MDEV-16697: Fix difference between 32bit/windows and 64bit systems in allowed select nest level > > diff --git a/include/my_global.h b/include/my_global.h > index 11d17ad146a..26078114464 100644 > --- a/include/my_global.h > +++ b/include/my_global.h > @@ -984,7 +984,6 @@ typedef unsigned long my_off_t; > TODO Convert these to use Bitmap class. > */ > typedef ulonglong table_map; /* Used for table bits in join */ > -typedef ulong nesting_map; /* Used for flags of nesting constructs */ > > /* often used type names - opaque declarations */ > typedef const struct charset_info_st CHARSET_INFO; > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -33,6 +33,10 @@ > #include "sql_tvc.h" > #include "item.h" > > +/* Used for flags of nesting constructs */ > +#define SELECT_NESTING_MAP_SIZE 64 > +typedef Bitmap<SELECT_NESTING_MAP_SIZE> nesting_map; > + > /* YACC and LEX Definitions */ Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] fa8879bc800: MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Aleksey! Looking at the patch and the test, I get the impression you're fixing the wrong issue. The problem in MDEV-16370 happened when replicating from non-system-versioned table on the master to a system-versioned table on the slave. You don't have a test case for that. Your fix doesn't change the behavior for this use case. So, I don't understand what you've actually fixed. On Nov 06, Aleksey Midenkov wrote: > revision-id: fa8879bc800d2babf48d082ce8c50ffe520e12cc (versioning-1.0.5-140-gfa8879bc800) > parent(s): 28ae79650d3057ffcd849d2ef28dcd80282c85d2 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2018-09-20 10:04:57 +0300 > message: > > MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables > > Rows_log_event::write_row() always overwrite historical row. > > diff --git a/mysql-test/suite/versioning/t/rpl.test b/mysql-test/suite/versioning/t/rpl.test > index a9e3af45af8..1985b17bf6d 100644 > --- a/mysql-test/suite/versioning/t/rpl.test > +++ b/mysql-test/suite/versioning/t/rpl.test > @@ -133,4 +133,24 @@ sync_slave_with_master; > connection master; > drop table t1; > > +--echo # MDEV-16370 row-based binlog events (updates by primary key) > +--echo # can not be applied multiple times to system versioned tables > +create or replace table t1 (pk int primary key, x int) with system versioning; > +insert into t1 values (1, 1); > +--sync_slave_with_master > +flush logs; > +--connection master > +update t1 set x= 2; > +--sync_slave_with_master > +select * from t1; > +--let $datadir= `select @@datadir` > +--let $f2= query_get_value(SHOW SLAVE STATUS, Relay_Log_File, 1) > +--echo # Replaying binlog... > +--exec $MYSQL_BINLOG $datadir/$f2| $MYSQL -S $SLAVE_MYSOCK test > +select * from t1; > +--connection master > + > +drop database test; > +create database test; > + > --source include/rpl_end.inc > diff --git a/sql/log_event.cc b/sql/log_event.cc > index c550adea26b..c4c04743a30 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -13139,13 +13139,26 @@ Rows_log_event::write_row(rpl_group_info *rgi, > } > > // Handle INSERT. > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned()) > { > - ulong sec_part; > - bitmap_set_bit(table->read_set, table->vers_start_field()->field_index); > - // Check whether a row came from unversioned table and fix vers fields. > - if (table->vers_start_field()->get_timestamp(&sec_part) == 0 && sec_part == 0) > - table->vers_update_fields(); > + if (table->versioned(VERS_TIMESTAMP)) > + { > + // Set vers fields when replicating from not system-versioned table. > + ulong sec_part; > + bitmap_set_bit(table->read_set, table->vers_start_field()->field_index); > + // Check whether a row came from unversioned table and fix vers fields. > + if (table->vers_start_field()->get_timestamp(&sec_part) == 0 && sec_part == 0) > + table->vers_update_fields(); > + else > + goto overwrite_historical_row; > + } > + else > + { > +overwrite_historical_row: > + bitmap_set_bit(table->read_set, table->vers_end_field()->field_index); > + if (!overwrite && !table->vers_end_field()->is_max()) > + overwrite= true; > + } > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 503e494ec6e: MDEV-16241 Assertion `inited==RND' failed in handler::ha_rnd_end()
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Aleksey! On Nov 06, Aleksey Midenkov wrote: > revision-id: 503e494ec6e103b23dc55f8fc8ae68ecec8b8316 (versioning-1.0.6-1-g503e494ec6e) > parent(s): 6120ae4acafb23badecc623e33797949e3f6fbbb > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2018-10-22 11:50:10 +0300 > message: > > MDEV-16241 Assertion `inited==RND' failed in handler::ha_rnd_end() > > Discrepancy in open indexes due to overwritten `read_partitions` upon > `ha_open()` in `ha_partition::clone()`. > > [Fixes tempesta-tech/mariadb#551] > > diff --git a/mysql-test/main/partition_innodb.result b/mysql-test/main/partition_innodb.result > index d27abc07984..c63cbc4601d 100644 > --- a/mysql-test/main/partition_innodb.result > +++ b/mysql-test/main/partition_innodb.result > @@ -925,3 +925,14 @@ test_jfg test_jfg11 > test_jfg test_jfg12#P#p1000 > test_jfg test_jfg12#P#pmax > DROP DATABASE test_jfg; > +# > +# MDEV-16241 Assertion `inited==RND' failed in handler::ha_rnd_end() > +# > +CREATE TABLE t1 (pk INT PRIMARY KEY, x INT, y INT, z INT, KEY (x), KEY (y, z)) > +WITH SYSTEM VERSIONING > +PARTITION BY SYSTEM_TIME (PARTITION p1 HISTORY, PARTITION pn CURRENT); > +INSERT INTO t1 VALUES (1, 7, 8, 9), (2, NULL, NULL, NULL), (3, NULL, NULL, NULL); > +SELECT COUNT(*) FROM t1 WHERE x IS NULL AND y IS NULL AND z IS NULL; > +COUNT(*) > +2 > +DROP TABLE t1; please, add a non-system-versioning test too. It's in one of the comments in the bug report. > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 40380ffeec5..901f812a0e6 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -3725,6 +3725,9 @@ void ha_partition::rebind_psi() > handler *ha_partition::clone(const char *name, MEM_ROOT *mem_root) > { > ha_partition *new_handler; > + MY_BITMAP read_partitions; > + MY_BITMAP lock_partitions; > + int error; > > DBUG_ENTER("ha_partition::clone"); > new_handler= new (mem_root) ha_partition(ht, table_share, m_part_info, > @@ -3748,9 +3751,21 @@ handler *ha_partition::clone(const char *name, MEM_ROOT *mem_root) > ALIGN_SIZE(m_ref_length)*2))) > goto err; > > - if (new_handler->ha_open(table, name, > + DBUG_ASSERT(m_tot_parts == m_part_info->read_partitions.n_bits); > + DBUG_ASSERT(m_tot_parts == m_part_info->lock_partitions.n_bits); > + my_bitmap_init(&read_partitions, NULL, m_tot_parts, FALSE); > + my_bitmap_init(&lock_partitions, NULL, m_tot_parts, FALSE); > + bitmap_copy(&read_partitions, &m_part_info->read_partitions); > + bitmap_copy(&lock_partitions, &m_part_info->lock_partitions); > + error= new_handler->ha_open(table, name, > table->db_stat, > - HA_OPEN_IGNORE_IF_LOCKED | HA_OPEN_NO_PSI_CALL)) > + HA_OPEN_IGNORE_IF_LOCKED | HA_OPEN_NO_PSI_CALL); > + bitmap_copy(&m_part_info->read_partitions, &read_partitions); > + bitmap_copy(&m_part_info->lock_partitions, &lock_partitions); > + my_bitmap_free(&read_partitions); > + my_bitmap_free(&lock_partitions); > + > + if (error) I don't think this is needed. ha_partition::open() is aware of clones, it has `if (m_is_clone_of)` to do some stuff only for clones and some only for fresh opens. Perhaps you just need to move the bitmap-resetting code to be done only for fresh opens? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0