- developers - lists.mariadb.org

Re: 22e2f082400: MDEV-25370 Update for portion changes autoincrement key in bi-temp table
by Sergei Golubchik 12 Jan '24

12 Jan '24

Hi, Nikita, On Jan 06, Nikita Malyavin wrote: > revision-id: 22e2f082400 (mariadb-10.5.23-39-g22e2f082400) > parent(s): e472b682e06 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-12-20 02:46:09 +0100 > message: > > MDEV-25370 Update for portion changes autoincrement key in bi-temp table > > According to the standard, the autoincrement column (i.e. *identity > column*) should be advanced each insert implicitly made by > UPDATE/DELETE ... FOR PORTION. When you refer to the standard, specify the exact reference so that someone could verify it. E.g. as I've commented in MDEV, "SQL:2016, Part 2, 15.13 Effect of replacing rows in base tables, paragraph 10) b) i)" Also, the standard doesn't have autoincrement, it'd be better to say "According to the standard, the identity column (which autoincrement is a functional analog of) should be advanced...." > This is very unconvenient use in several notable cases. Concider a > WITHOUT OVERLAPS key with an autoinc column: > id int auto_increment, unique(id, p without overlaps) > > An update or delete with FOR PORTION creates a sense that id will > remain unchanged in such case. > > This commit makes an exception the autoinc columns that are part of > WITHOUT OVERLAPS keys and infers the following rule for the newly > inserted rows: Better to say here not that we're making an exception and violating the standard (because we are not), but that in the standard IDENTITY uses its own independent sequence generator, which knows nothing about periods. While autoincrement always requires an index (that is, our "identity" column is a pair {autoincrement, index}, not autoincrement alone), and the index can be declared WITHOUT OVERLAPS, so our "identity" column is period-aware. This is an extension of the standard, and we can freely define how it behaves in the cases not covered by the standard, that is in the WITHOUT OVERLAPS case. > ===== > Let V_j a set of values for currently updating/deleting row. > Let VI_j a set of values to be inserted. > * Case: > (a) if V_i is an identity column that is also a part of a unique constraint > that includes <without overlap specification>, then let VI_i be V_i > (b) otherwise, let VI_i be the value deduced as described by the rules of > ISO/IEC 9075-2, clauses 15.7 Effect of deleting rows from base tables > and 15.13 Effect of replacing rows in base tables. > ===== That's confusing, I'm sure that anyone who doesn't remember the corresponding part of the sql standard will think it's a direct quote from it. You've imitated the style rather convincingly :) It's also rather unnecessary, we don't have identity columns, so nobody cares how they should behave in this case. Perhaps it'd be better if you simply write that if the key, that defines autoincrement values, is declared WITHOUT OVERLAPS, then new rows created by UPDATE/DELETE FOR PORTION OF will use old autoincrement values, as they belong to non-overlapping ranges. But INSERT will generate a new autoincrement value, it will not try to find the smallest value in the overlapping ranges. > Note that this also infers that if an identity column is not a part of > any unique constraint that includes <without overlap specification>, then > its value will be DEFAULT and, that is, auto-incremented. > > Apart from WITHOUT OVERLAPS there is also another notable case, referred > by the reporter - a unique key that has an autoincrement column and a field > from the period specification: > id int auto_increment, unique(id, s), period for p(s, e) > > for this case, no exception is made, and the autoincrementing rules will be > proceeded accordung to the standard (i.e. the value will be advanced on > implicit inserts). > > diff --git a/mysql-test/suite/period/r/overlaps.result b/mysql-test/suite/period/r/overlaps.result > --- a/mysql-test/suite/period/r/overlaps.result > +++ b/mysql-test/suite/period/r/overlaps.result > @@ -449,3 +449,86 @@ VALUES > ('2000-01-01 00:00:00.000000', '2001-01-01 00:00:00.000000', 'abc'); > ERROR 23000: Duplicate entry 'abc-2001-01-01 00:00:00.000000-2000-01-01 00:00:00.000000' for key 'index_name' > DROP TABLE t1; > +create or replace table cars(id int auto_increment, > +price int, s date, e date, > +period for p(s,e), > +primary key(id, p without overlaps)); > +insert into cars(price, s, e) values (1000, '2018-01-01', '2020-01-01'); > +select * from cars; > +id price s e > +1 1000 2018-01-01 2020-01-01 > +update cars for portion of p from '2019-01-01' to '2019-12-01' set price= 1100; > +select * from cars; > +id price s e > +1 1000 2018-01-01 2019-01-01 > +1 1000 2019-12-01 2020-01-01 > +1 1100 2019-01-01 2019-12-01 > +delete from cars for portion of p from '2019-12-10' to '2019-12-20'; > +select * from cars; > +id price s e > +1 1000 2018-01-01 2019-01-01 > +1 1000 2019-12-01 2019-12-10 > +1 1000 2019-12-20 2020-01-01 > +1 1100 2019-01-01 2019-12-01 ok > +# AUTO_INCREMENT field is separate from WITHOUT OVERLAPS > +create or replace table cars(id int primary key auto_increment, > +car_id int, > +price int, s date, e date, > +period for p(s,e), > +unique(car_id, p without overlaps)); > +insert cars(car_id, price, s, e) values (1, 1000, '2018-01-01', '2020-01-01'); > +select * from cars; > +id car_id price s e > +1 1 1000 2018-01-01 2020-01-01 > +update cars for portion of p from '2019-01-01' to '2019-12-01' set price= 1100; > +select * from cars; > +id car_id price s e > +1 1 1100 2019-01-01 2019-12-01 > +2 1 1000 2018-01-01 2019-01-01 > +3 1 1000 2019-12-01 2020-01-01 > +delete from cars for portion of p from '2019-12-10' to '2019-12-20'; > +select * from cars; > +id car_id price s e > +1 1 1100 2019-01-01 2019-12-01 > +2 1 1000 2018-01-01 2019-01-01 > +4 1 1000 2019-12-01 2019-12-10 > +5 1 1000 2019-12-20 2020-01-01 right > +# AUTO_INCREMENT field is both standalone and in WITHOUT OVERLAPS > +create or replace table cars(id int primary key auto_increment, > +price int, s date, e date, > +period for p(s,e), > +unique(id, p without overlaps)); > +insert cars(price, s, e) values (1000, '2018-01-01', '2020-01-01'); > +select * from cars; > +id price s e > +1 1000 2018-01-01 2020-01-01 > +update cars for portion of p from '2019-01-01' to '2019-12-01' set price= 1100; > +ERROR 23000: Duplicate entry '1' for key 'PRIMARY' uhm, I don't know. I'd expect that here PRIMARY would define autoincrement values. > +truncate cars; > +insert cars(price, s, e) values (1000, '2018-01-01', '2020-01-01'); > +delete from cars for portion of p from '2019-12-10' to '2019-12-20'; > +ERROR 23000: Duplicate entry '1' for key 'PRIMARY' > +# AUTO_INCREMENT field is non-unique not sure here. I think here unique should define autoincrement values. That is, in both cases, the most restrictive index should define. > +create or replace table cars(id int auto_increment, key(id), > +car_id int, > +price int, s date, e date, > +period for p(s,e), > +unique(car_id, p without overlaps)); > +insert cars(car_id, price, s, e) values (1, 1000, '2018-01-01', '2020-01-01'); > +select * from cars; > +id car_id price s e > +1 1 1000 2018-01-01 2020-01-01 > +update cars for portion of p from '2019-01-01' to '2019-12-01' set price= 1100; > +select * from cars; > +id car_id price s e > +1 1 1100 2019-01-01 2019-12-01 > +2 1 1000 2018-01-01 2019-01-01 > +3 1 1000 2019-12-01 2020-01-01 > +delete from cars for portion of p from '2019-12-10' to '2019-12-20'; > +select * from cars; > +id car_id price s e > +1 1 1100 2019-01-01 2019-12-01 > +2 1 1000 2018-01-01 2019-01-01 > +4 1 1000 2019-12-01 2019-12-10 > +5 1 1000 2019-12-20 2020-01-01 please also add test for INSERT. Just add a second insert like insert cars(price, s, e) values (1000, '2021-01-01', '2022-01-01'); to show that id will *not* be 1 here, even though 1 would not conflict with anything in the specified date range. Simply to have the expected behavior recorded as a test result. > +drop table cars; > diff --git a/sql/table.cc b/sql/table.cc > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -8917,6 +8916,21 @@ int TABLE::update_generated_fields() > return res; > } > > +void TABLE::period_prepare_autoinc() > +{ > + if (!found_next_number_field) > + return; > + > + key_map::Iterator it(found_next_number_field->part_of_key_not_clustered); > + for (uint i; (i= it++) != key_map::Iterator::BITMAP_END;) > + { > + KEY &key= key_info[i]; > + if (key.without_overlaps) > + return; > + } No, I don't think you need to check *all* indexes that the field is part of. I believe you should only check the "main" index, the one that defines autoincrement field behavior. That is only if (key_info[s->next_number_index].without_overlaps) return; > + next_number_field= found_next_number_field; > +} > + > int TABLE::period_make_insert(Item *src, Field *dst) > { > THD *thd= in_use; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 10

Re: 6835d2ce268: MDEV-28649 json_array(false) in the definition of view return 0 instead of "true".
by Sergei Golubchik 12 Jan '24

12 Jan '24

Hi, Alexey, On Jan 12, Alexey Botchkov wrote: > revision-id: 6835d2ce268 (mariadb-10.4.32-116-g6835d2ce268) > parent(s): fbe604d8839 > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2023-12-15 00:56:20 +0400 > message: > > MDEV-28649 json_array(false) in the definition of view return 0 instead of "true". > > The 'temporary-only Field_bool introduced'; Why did you need a new Field and to change Type_handler_bool from long to tiny? On the first glance it seems that a dedicated Item_bool::print() should've been enough. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 424ee6eac78: MDEV-32837 long unique does not work like unique key when using replace
by Sergei Golubchik 11 Jan '24

11 Jan '24

Hi, Alexander, On Jan 11, Alexander Barkov wrote: > revision-id: 424ee6eac78 (mariadb-10.5.23-57-g424ee6eac78) > parent(s): c9902a20b3a > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-01-10 15:35:25 +0400 > message: > > MDEV-32837 long unique does not work like unique key when using replace > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index ec9e379768f..edc2be207f3 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2065,7 +2065,8 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > we just should not expose this fact to users by invoking > ON UPDATE triggers. > */ > - if (last_uniq_key(table,key_nr) && > + if ((!table->s->long_unique_table || table->versioned()) && > + last_uniq_key(table,key_nr) && > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { 1. why table->versioned() ? 2. use show status in your test for handler_update/delete to see what REPLACE actually did 3. technically, you can keep the optimization if the long unique is the last *and there are no in-engine uniques*, this can be checked with && !(table->key_info->flags & HA_NOSAME) 4. but here's the problem that last_uniq_key() doesn't see long uniques, so it'll treat every one of them as the last. So it should be something like bool is_long_unique= table->s->key_info[key_nr].flags & HA_UNIQUE_HASH; if ((is_long_unique ? last_uniq_key(table, table->s->key_info, key_nr) && !(table->key_info->flags & HA_NOSAME) : last_uniq_key(table, table->key_info, key_nr)) && !table->file->referenced_by_foreign_key() && (!table->triggers || !table->triggers->has_delete_triggers())) I suspect this will correctly use the UPDATE optimization in all cases (only long unique / mix of uniques)x(last / not last key conflicting) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 014167880d2: MDEV-32559 ensure spider alter table init queries are executed after ddl recovery
by Sergei Golubchik 10 Jan '24

10 Jan '24

Hi, Yuchen, On Jan 05, Yuchen Pei wrote: > revision-id: 014167880d2 (mariadb-10.10.4-52-g014167880d2) > parent(s): 21822625aa6 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-11-08 13:10:38 +1100 > message: > > MDEV-32559 ensure spider alter table init queries are executed after ddl recovery > > If spider was initialised in init_server_components(), i.e. with > --plugin-load-add, then all alter table init queries are to be > executed in the signal_ddl_recovery_done() handlerton callback. Since > this is part of the spider plugin init, we update the callback > signature to return an int, and deinit the plugin when it fails the > callback. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index a3971a00a59..a922562d043 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -5159,6 +5159,14 @@ static int init_server_components() > if (!opt_abort && ddl_log_initialize()) > unireg_abort(1); > > + /* Plugin initialisation done here should defer any ALTER TABLE > + queries to after the ddl recovery is done, in the > + signal_ddl_recovery_done() callback called by > + ha_signal_ddl_recovery_done(). As such, between the plugin_init() > + call and the ha_signal_ddl_recovery_done() call below only things > + related to prepare for recovery should be done and nothing else, and > + definitely not anything assuming that all plugins have been > + initialised. */ Better put this comment in plugin.h near the (*init)(void*) declaration. This is part of the (*init)() callback documentation it should be there, not in some random .cc file that plugin authors shouldn't be required to look at. > if (plugin_init(&remaining_argc, remaining_argv, > (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) | > (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0))) > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 3576730723e..85a8f14a6da 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -2553,9 +2554,15 @@ bool plugin_foreach_with_mask(THD *thd, plugin_foreach_func *func, > > for (idx= 0; idx < total; idx++) > { > - /* It will stop iterating on first engine error when "func" returns TRUE */ > + /* It will stop iterating on first engine error or mark DELETED if > + reap_on_fail is true when "func" returns TRUE. */ > if ((res= func(thd, plugins[idx], arg))) > + { > + if (reap_on_fail) > + plugin_ref_to_int(plugins[idx])->state= PLUGIN_IS_DELETED; may be you don't need a new argument for plugin_foreach_with_mask, and can put this logic into the `func` callback? That is into static my_bool signal_ddl_recovery_done(THD *, plugin_ref plugin, void *) { handlerton *hton= plugin_hton(plugin); if (hton->signal_ddl_recovery_done) if ((hton->signal_ddl_recovery_done)(hton)) plugin_ref_to_int(plugin)->state= PLUGIN_IS_DELETED; return 0; } or something like that. > + else > break; > + } > } > > plugin_unlock_list(0, plugins, total); > diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc > index a9782a8a822..e50942d449f 100644 > --- a/storage/spider/spd_table.cc > +++ b/storage/spider/spd_table.cc > @@ -6376,15 +6376,13 @@ bool spider_init_system_tables() > DBUG_RETURN(TRUE); > } > > - const int size= sizeof(spider_init_queries) / sizeof(spider_init_queries[0]); > - for (int i= 0; i < size; i++) > + for (uint i= 0; i < size; i++) > { > - const LEX_STRING *query= &spider_init_queries[i]; > + const LEX_STRING *query= &queries[i]; I don't see much of a point, why not to run *all* queries from spider_ddl_recovery_done() ? > if (mysql_real_query(mysql, query->str, query->length)) > { > - fprintf(stderr, > - "[ERROR] SPIDER plugin initialization failed at '%s' by '%s'\n", > - query->str, mysql_error(mysql)); > + sql_print_error("SPIDER: plugin initialization failed at '%s' by '%s'\n", > + query->str, mysql_error(mysql)); > > mysql_close(mysql); > DBUG_RETURN(TRUE); > @@ -6667,10 +6665,20 @@ int spider_db_init( > spider_udf_table_mon_list_hash[roop_count].array.size_of_element); > } > > - if (spider_init_system_tables()) > - { > + if (spider_init_system_tables( > + spider_init_queries, > + sizeof(spider_init_queries) / sizeof(spider_init_queries[0]))) > + goto error_system_table_creation; > + > + /* If plugin_init() has already been called, it implies this > + function is called after the ddl recovery is done, thus we can > + safely execute ALTER TABLE init queries. Otherwise it is deferred to > + the spider_ddl_recovery_done() callback. */ > + if (plugins_are_initialized && > + spider_init_system_tables(spider_init_alter_table_queries, > + sizeof(spider_init_alter_table_queries) / > + sizeof(spider_init_alter_table_queries[0]))) Don't do that, instead, please, take the "ddl_recovery_done" change from the commit 0930eb86cb00edb2a - it makes the storage engine API simpler, the engine doesn't need to implement two code paths and try to detect whether it's loaded at run-time or at startup. > goto error_system_table_creation; > - } > > if (!(spider_table_sts_threads = (SPIDER_THREAD *) > spider_bulk_malloc(NULL, 256, MYF(MY_WME | MY_ZEROFILL), > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: fixes for your commits to make buildbot green
by Kristian Nielsen 09 Jan '24

09 Jan '24

Sergei Golubchik <serg(a)mariadb.org> writes: > as you know, I've cherry-picked all your 11.4 features into one branch > to assemble a preview release 11.4.0 Thanks, Serg! > This has required some fixes to make buildbot greener. Thanks. I think buildbot was not active on 11.4 branches when I tried earlier, but now they seem to run. Was something changed in buildbot around 11.4 branches recently? > There's nothing specific you need to do right now. But eventually you'll > rebase on top of the latest 11.4. Somewhere around that time, please > take a look at bb-11.4-release or bb-11.4-serg branch, see commits > related to your MDEV's and consider them. > a40dcb3da6a MDEV-4991 don't use ulong for sysvars > 723ebcb5df7 MDEV-4991 ASAN bug > f3d2d4609c3 MDEV-4991 compilation failure on fulltest2 > bd295d0919b MDEV-4991 update forgotten test results These are now applied (with a few modifications) and rebased on latest 11.4, in bb-11.4-knielsen-mdev4991 > I suggest you squash them with your commits, no need to keep indivial Sure, will do before final merge to 11.4. When is the deadline for pushing into the 11.4 release? - Kristian.

2 1

Re: [PATCH] MDEV-33031 Assertion failure upon reading from performance
by Yuchen Pei 09 Jan '24

09 Jan '24

Hi Sergei, Regarding 3be269542f4d18eaee0ad8fbeffa55708557879f, I have one comment (see below), otherwise ok to push. > [... 62 lines elided] > diff --git a/storage/spider/spd_param.cc b/storage/spider/spd_param.cc > index 2da262cd2bc..308857d153a 100644 > --- a/storage/spider/spd_param.cc > +++ b/storage/spider/spd_param.cc > @@ -116,16 +116,20 @@ extern volatile ulonglong spider_mon_table_cache_version_req; > MYSQL_SYSVAR_NAME(param_name).def_val; \ > } > +extern handlerton *spider_hton_ptr; > static int spider_trx_status_var(THD *thd, SHOW_VAR *var, char *buff, > ulonglong SPIDER_TRX::*counter) > { > - int error_num = 0; > - SPIDER_TRX *trx; > DBUG_ENTER("spider_direct_update"); > var->type = SHOW_LONGLONG; > - if ((trx = spider_get_trx(thd, TRUE, &error_num))) > - var->value = (char *) &(trx->*counter); > - DBUG_RETURN(error_num); > + var->value= buff; > + if (thd != current_thd) > + mysql_mutex_lock(&thd->LOCK_thd_data); > + SPIDER_TRX *trx = (SPIDER_TRX*)thd_get_ha_data(thd, spider_hton_ptr); > + *(ulonglong*)buff= trx ? trx->*counter : 0; > + if (thd != current_thd) > + mysql_mutex_unlock(&thd->LOCK_thd_data); > + DBUG_RETURN(0); If it always returns 0, how about we change the signature of this function to return void? > } Best, Yuchen

2 1

Re: [PATCH] cleanup: spider status variables
by Yuchen Pei 09 Jan '24

09 Jan '24

Hi Sergei, > reduce code duplication > --- > storage/spider/spd_param.cc | 49 ++++++++++++------------------------- > 1 file changed, 16 insertions(+), 33 deletions(-) b7bcbda853947a5be8307eda7026ca1ff567a885 looks ok to push. > [... 93 lines elided] Best, Yuchen

1 0

Re: dc50afee924: MDEV-32501 KEY_PERIOD_USAGE reveals information to unprivileged user
by Sergei Golubchik 08 Jan '24

08 Jan '24

Hi, Nikita, ok to push under condition that you do the change as below, if it's applicable. On Jan 05, Nikita Malyavin wrote: > revision-id: dc50afee924 (mariadb-11.3.1-13-gdc50afee924) > parent(s): 080d8c3bd8f > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-01-05 00:07:06 +0100 > message: > > MDEV-32501 KEY_PERIOD_USAGE reveals information to unprivileged user > > Restrict access to KEY_PERIOD_USAGE: show the constraint record iff any > non-select privilege on any table column is granted. > > Also drop the unprivileged user in the end of test and add merge anchor. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 95a9d3647b0..e5516466e20 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -7798,6 +7798,12 @@ int get_schema_key_period_usage_record(THD *thd, TABLE_LIST *tables, > if (!period_name) > return 0; > > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + check_grant(thd, TABLE_ACLS, tables, 1, 1, 1); > + if ((tables->grant.all_privilege() & ~SELECT_ACL & TABLE_ACLS) == NO_ACL) > + return 0; > +#endif Compare with get_schema_key_column_usage_record() above your get_schema_key_period_usage_record(). Perhaps you can also check `thd->col_access` and avoid check_grant()? > bool err= false; > for (uint k= 0; !err && k < keys_total; k++) > { > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: c50eed1e904: MDEV-32645 CAST(AS UNSIGNED) fails with --view-protocol
by Sergei Golubchik 06 Jan '24

06 Jan '24

Hi, Alexander, Just one comment below On Jan 06, Alexander Barkov wrote: > revision-id: c50eed1e904 (mariadb-10.4.30-318-gc50eed1e904) > parent(s): 28cdbab1637 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-11-13 11:18:16 +0400 > message: > > MDEV-32645 CAST(AS UNSIGNED) fails with --view-protocol > > Item_float::neg() did not preserve the "presentation" from "this". > So > CAST(-1e0 AS UNSIGNED) -- cast from double to unsigned > changes its meaning to: > CAST(-1 AS UNSIGNED) -- cast signed to undigned > > Fixing Item_float::neg() to construct the new value for > Item_float::presentation as minus sign followed by its old value: > '1e0' -> '-1e0' > > diff --git a/sql/item.cc b/sql/item.cc > index 259c4bf8f4d..b88f78fbb59 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -6896,7 +6896,17 @@ Item *Item_float::neg(THD *thd) > else if (value < 0 && max_length) > max_length--; > value= -value; > - presentation= 0; > + if (presentation) > + { may be, still if (*presentation == '-') presentation++ else > + size_t presentation_length= strlen(presentation); > + if (char *tmp= (char*) thd->alloc(presentation_length + 2)) > + { > + tmp[0]= '-'; > + // Copy with the trailing '\0' > + memcpy(tmp + 1, presentation, presentation_length + 1); > + presentation= tmp; > + } > + } > name= null_clex_str; > return this; > } > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 9e7c80f8006: MDEV-32803 Assertion `total == 0' failed in Event_log::write_cache_raw
by Sergei Golubchik 05 Jan '24

05 Jan '24

Hi, Nikita, First, I'd suggest this change: --- a/sql/online_alter.cc +++ b/sql/online_alter.cc @@ -257,8 +257,7 @@ int online_alter_end_trans(Online_alter_cache_list { my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG), binlog->get_name(), errno); - cleanup_cache_list(cache_list, is_ending_transaction); - DBUG_RETURN(error); + break; } } and also I'd suggest to move if (ending_trans) from cleanup_cache_list() to the caller (there's only one anyway). It'll make the code clearer, in particular your statement below that "the cache ... wasn't destroyed". Better do these changes in a separate commit, they aren't part of the bug fix. The fix itself is ok to push. On Jan 05, Nikita Malyavin wrote: > revision-id: 9e7c80f8006 (mariadb-11.2.1-27-g9e7c80f8006) > parent(s): 0c5176b7744 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-11-17 23:40:10 +0100 > message: > > MDEV-32803 Assertion `total == 0' failed in Event_log::write_cache_raw > > A second DML in a transaction for a table of non-rollbackable engine > leads to a cache corruption, because the cache wasn't reset after a > statement end, but also wasn't destroyed. > > This patch resets the cache for a reuse by subsequent statements in > current transaction. > > diff --git a/sql/online_alter.cc b/sql/online_alter.cc > index e0afe602ca8..5f418d8ec29 100644 > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -152,6 +152,7 @@ int online_alter_log_row(TABLE* table, const uchar *before_record, > if (!table->online_alter_cache) > { > table->online_alter_cache= get_cache_data(thd, table); > + DBUG_ASSERT(table->online_alter_cache->cache_log.type == WRITE_CACHE); > trans_register_ha(thd, false, online_alter_hton, 0); > if (thd->in_multi_stmt_transaction_mode()) > trans_register_ha(thd, true, online_alter_hton, 0); > @@ -237,6 +238,8 @@ int online_alter_end_trans(Online_alter_cache_list &cache_list, THD *thd, > mysql_mutex_lock(binlog->get_log_lock()); > error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > + if (!is_ending_transaction) > + cache.reset(); > } > } > else if (!commit) // rollback > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0