July 2020 - developers - lists.mariadb.org

Re: [Maria-developers] ef2519fee4e: MDEV-16546 System versioning setting to allow history modification
by Aleksey Midenkov 08 Jun '21

08 Jun '21

Hello, Sergei! On Fri, May 3, 2019 at 8:43 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On May 03, Aleksey Midenkov wrote: > > revision-id: ef2519fee4e (versioning-1.0.5-17-gef2519fee4e) > > parent(s): 56145be2951 > > author: Aleksey Midenkov <midenok(a)gmail.com> > > committer: Aleksey Midenkov <midenok(a)gmail.com> > > timestamp: 2018-06-28 13:42:09 +0300 > > message: > > > > MDEV-16546 … [View More]System versioning setting to allow history modification > > > > 1. Add server variable system_versioning_modify_history which will > > allow to set values for row_start, row_end in DML operations. > > > > 2. If secure_timestamp is YES or REPLICATION, > > system_versioning_modify_history does not have effect. If > > secure_timestamp is SUPER, system_versioning_modify_history requires > > special privilege (same as for setting current timestamp). > > I thought more about this idea. We don't really want to have the history > editable, do we? Well, I'm thinking about rollback table data to specific point in time. That could be a useful feature. > But it's needed for replication, to keep the master and > slave identical. That's what secure_timestamp is for. > > The idea was that this new variable, system_versioning_modify_history, > will be just a convenience feature, it will not allow history editing > any more than one can do without it. > > But now I suspect that even with secure_timestamp=NO one cannot truly > edit history. One can only insert new rows with arbitrary timestamps. > For example, to insert a row with row_start=1000 and row_end=2000, one > needs to do (if secure_timestamp=NO): > > set timestamp=1000; > insert row; > set timestamp=2000; > delete row; > > But I don't see how one can update or delete a history row with > secure_timestamp=NO. > > Now, with a SUPER privilege and secure_timestamp=NO or SUPER, one can > use the BINLOG command and truly edit the history arbitrarily, by faking > row events. I don't really get it why this is so important: since there is some limitation by configuration and privilege, we are just fine. Everything can be changed at filesystem level after all. > > The conclusion, I believe, is that system_versioning_modify_history > should allow INSERTs when secure_timestamp=NO, and it should allow > UPDATE/DELETE only for a SUPER user when secure_timestamp=NO or SUPER. I don't see a reason to argue on that. The only thing that is not clear, why we don't allow INSERTs when secure_timestamp=SUPER? > > The second thing I don't like at all, is when a table is created like > > CREATE TABLE t1 (a int) WITH SYSTEM VERSIONING > > with row_start/row_end implicit. You don't have it in the test, but > anyway one should be able to load history into such a table, while the > table does not have row_start and row_end columns. From the user point > of view these columns don't exist, they're pseudo-columns, like ROWID. > They just cannot be insertable-into, conceptually. But a user will want > to restore the history, right? I don't have a solution for this yet :( > Any ideas? We don't have to follow the conception if it doesn't help us. Since we have physical row_start/row_end, we don't have to pretend they don't exist. Who will win from that? > > See below a couple of minor comments about the patch itself. > > ... These are going to be fixed. > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok [View Less]

2 15

[Maria-developers] Using JSON_TABLE to create JSON columns?
by Sergey Petrunia 31 May '21

31 May '21

Hi Alexey and everyone, I was looking at whether it's possible use JSON_TABLE to extract portions of JSON document. Apparently it is possible in MySQL with JSON datatype: Q1: select * from json_table('[{"color": "blue", "price": { "high": 10, "low": 5}}, {"color": "red", "price": { "high": 20, "low": 8}}]', '$[*]' columns(color varchar(100) path '$.color', price json path '$.price' ) ) as T; +------… [View More]

3 2

Re: [Maria-developers] 9bf4b92cbc5: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 31 Mar '21

31 Mar '21

Hi, Aleksey! On Jun 29, Aleksey Midenkov wrote: > revision-id: 9bf4b92cbc5 (mariadb-10.5.2-168-g9bf4b92cbc5) > parent(s): 478301d9b9a > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-04-17 17:04:24 +0300 > message: > > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > > == Syntax change == > > Keyword AUTO_INCREMENT … [View More](or AUTO) enables partition auto-creation. I still think that AUTO_INCREMENT here looks rather out-of-place, I don't know how to explain to users why AUTO_INCREMENT is allowed here. But if we'll have it, we'll have to support it here for a long time for compatibility reasons. "Because somebody might be already using it" I'd suggest to support only AUTO here. Or, if the parser can do it easily, a much more readable syntax would be create table t1 (x int) with system versioning auto partition by system_time interval 1 hour; this would be the best, easy to read, matches the documentation (where it could be called "auto partitioning"). If it won't cause any difficult parser problems, I'd prefer us to use that. > create or replace table t1 (x int) with system versioning > partition by system_time interval 1 hour auto_increment; > > create or replace table t1 (x int) with system versioning > partition by system_time limit 1000 auto; > > Or with explicit partitions: > > create or replace table t1 (x int) with system versioning > partition by system_time interval 1 hour auto > (partition p0 history, partition pn current); > > == Description == > > Before executing history-generating DML command add N history > partitions, so that N would be sufficient for potentially generated > history. N > 1 may be required when history is rotated by INTERVAL and > timestamp was jumped to future further than interval value. > > It is assumed that one DML command will not generate history rows more > than specified LIMIT. Otherwise history would overflow generated > partition, a warning would be printed, and user action would be > required to rebuild partitions. > > Auto-creation is implemented by synchronous > fast_alter_partition_table() call from the thread of the executed DML > command before the command itself (by the fallback and retry mechanism > similar to Discovery feature, see Open_table_context). > > Creating history partitions was made by the principle of minimal > disruption of the main business process. I.e. when procedure of > creation fails it will not (if possible) fail the original DML > command. Warning info will display what happened and the last history > partition may overflow. In such case partition rebuild is required to > correctly display history info. User application may detect warning > code ER_VERS_HIST_PART_FAILED and stop execution if that is preferred. > > The name for newly added partitions are generated like default > partition names with extension of MDEV-22155 (which avoids name > clashes by extending assignment counter to next free-enough gap). > > These DML commands trigger auto-creation: > > * DELETE (including multi-delete, excluding DELETE HISTORY) > * UPDATE (including multi-update) > * REPLACE (including REPLACE .. SELECT) INSERT ... ON DUPLICATE KEY UPDATE ? > diff --git a/mysql-test/suite/versioning/common.inc b/mysql-test/suite/versioning/common.inc > index 355b571e5a0..b35a5138015 100644 > --- a/mysql-test/suite/versioning/common.inc > +++ b/mysql-test/suite/versioning/common.inc > @@ -6,6 +6,7 @@ if (!$TEST_VERSIONING_SO) > source include/have_innodb.inc; > > set @@session.time_zone='+00:00'; > +set @@global.time_zone='+00:00'; Why is that? I'd expect your auto-adding of partitions to happen in the context of a session, using session time zone. Is it for replication? > select ifnull(max(transaction_id), 0) into @start_trx_id from mysql.transaction_registry; > set @test_start=now(6); > > diff --git a/mysql-test/suite/versioning/r/delete_history.result b/mysql-test/suite/versioning/r/delete_history.result > index cb865a835b3..90c9e4777bb 100644 > --- a/mysql-test/suite/versioning/r/delete_history.result > +++ b/mysql-test/suite/versioning/r/delete_history.result > @@ -63,8 +63,6 @@ insert into t values (1); > update t set a= 2; > update t set a= 3; > delete history from t; > -Warnings: > -Warning 4114 Versioned table `test`.`t`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions good riddance, it didn't make much sense anyway > # The above warning is one command late (MDEV-20345) ^^^ > select * from t for system_time all; > a > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index 85cb736b5bd..5fb893b4f0e 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -1623,7 +1623,7 @@ class ha_partition :public handler could you also, please, fix (in a separate commit) ha_partition::part_records() to take a proper partition_element* argument, not void* ? > for (; part_id < part_id_end; ++part_id) > { > handler *file= m_file[part_id]; > - DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions), part_id)); > + bitmap_set_bit(&(m_part_info->read_partitions), part_id); > file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK | HA_STATUS_OPEN); > part_recs+= file->stats.records; > } > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index e9dbf2b49c3..72312ee7ac4 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -814,10 +814,16 @@ bool partition_info::has_unique_name(partition_element *element) > vers_info->interval Limit by fixed time interval > vers_info->hist_part (out) Working history partition > */ > -void partition_info::vers_set_hist_part(THD *thd) > +uint partition_info::vers_set_hist_part(THD *thd, bool auto_inc) let's not all it auto_inc. it'll add noise to every grep for auto_inc issues. What about auto_add ? > { > + DBUG_ASSERT(!thd->lex->last_table()->vers_conditions.delete_history); > + > + uint create_count= 0; > + auto_inc= auto_inc && vers_info->auto_inc; > + > if (vers_info->limit) > { > + DBUG_ASSERT(!vers_info->interval.is_set()); > ha_partition *hp= (ha_partition*)(table->file); > partition_element *next= NULL; > List_iterator<partition_element> it(partitions); > @@ -836,22 +842,26 @@ void partition_info::vers_set_hist_part(THD *thd) > { > if (next == vers_info->now_part) > { > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name, "LIMIT"); > + if (auto_inc) > + create_count= 1; > + else > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name, "LIMIT"); > } > else > vers_info->hist_part= next; > } > - return; > + // reserve at least one history partition > + if (auto_inc && create_count == 0 && > + vers_info->hist_part->id + 1 == vers_info->now_part->id) > + create_count= 1; Questionable. What does it solve? > } > - > - if (vers_info->interval.is_set()) > + else if (vers_info->interval.is_set() && > + vers_info->hist_part->range_value <= thd->query_start()) > { > - if (vers_info->hist_part->range_value > thd->query_start()) > - return; > - > partition_element *next= NULL; > + bool error= true; > List_iterator<partition_element> it(partitions); > while (next != vers_info->hist_part) > next= it++; > @@ -860,12 +870,200 @@ void partition_info::vers_set_hist_part(THD *thd) > { > vers_info->hist_part= next; > if (next->range_value > thd->query_start()) > - return; > + { > + error= false; > + break; but here you don't "reserve at least one history partition" Why INTERVAL is different? > + } > + } > + if (error) > + { > + if (auto_inc) > + { > + DBUG_ASSERT(thd->query_start() >= vers_info->hist_part->range_value); > + my_time_t diff= thd->query_start() - vers_info->hist_part->range_value; > + if (diff > 0) > + { > + size_t delta= vers_info->interval.seconds(); > + create_count= diff / delta + 1; > + if (diff % delta) > + create_count++; > + } > + else > + create_count= 1; > + } > + else > + { > + my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > + table->s->db.str, table->s->table_name.str, > + vers_info->hist_part->partition_name, "INTERVAL"); > + } > } > - my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG), > - table->s->db.str, table->s->table_name.str, > - vers_info->hist_part->partition_name, "INTERVAL"); > } > + > + return create_count; > +} > + > + > +/** > + @brief Run fast_alter_partition_table() to add new history partitions > + for tables requiring them. > +*/ > +void vers_add_auto_parts(THD *thd, TABLE_LIST* tl, uint num_parts) > +{ > + HA_CREATE_INFO create_info; > + Alter_info alter_info; > + String query; > + partition_info *save_part_info= thd->work_part_info; > + Query_tables_list save_query_tables; > + Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer; > + Diagnostics_area new_stmt_da(thd->query_id, false, true); > + Diagnostics_area *save_stmt_da= thd->get_stmt_da(); > + bool save_no_write_to_binlog= thd->lex->no_write_to_binlog; > + const CSET_STRING save_query= thd->query_string; > + thd->m_reprepare_observer= NULL; > + thd->lex->reset_n_backup_query_tables_list(&save_query_tables); > + thd->in_sub_stmt|= SUB_STMT_AUTO_HIST; > + thd->lex->no_write_to_binlog= !thd->is_current_stmt_binlog_format_row(); > + TABLE *table= tl->table; > + > + DBUG_ASSERT(!thd->is_error()); > + /* NB: we have to preserve m_affected_rows, m_row_count_func, m_last_insert_id, etc */ > + thd->set_stmt_da(&new_stmt_da); > + new_stmt_da.set_overwrite_status(true); Is it needed? You've just started using a new Diagnostics_area, the status should be DA_EMPTY here, nothing to overwrite. > + > + { > + DBUG_ASSERT(table->s->get_table_ref_type() == TABLE_REF_BASE_TABLE); > + DBUG_ASSERT(table->versioned()); > + DBUG_ASSERT(table->part_info); > + DBUG_ASSERT(table->part_info->vers_info); > + alter_info.reset(); > + alter_info.partition_flags= ALTER_PARTITION_ADD|ALTER_PARTITION_AUTO_HIST; > + create_info.init(); > + create_info.alter_info= &alter_info; > + Alter_table_ctx alter_ctx(thd, tl, 1, &table->s->db, &table->s->table_name); > + > + MDL_REQUEST_INIT(&tl->mdl_request, MDL_key::TABLE, tl->db.str, > + tl->table_name.str, MDL_SHARED_NO_WRITE, MDL_TRANSACTION); > + if (thd->mdl_context.acquire_lock(&tl->mdl_request, > + thd->variables.lock_wait_timeout)) > + goto exit; > + table->mdl_ticket= tl->mdl_request.ticket; > + > + create_info.db_type= table->s->db_type(); > + create_info.options|= HA_VERSIONED_TABLE; > + DBUG_ASSERT(create_info.db_type); > + > + create_info.vers_info.set_start(table->s->vers_start_field()->field_name); > + create_info.vers_info.set_end(table->s->vers_end_field()->field_name); > + > + partition_info *part_info= new partition_info(); > + if (unlikely(!part_info)) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + goto exit; > + } > + part_info->use_default_num_partitions= false; > + part_info->use_default_num_subpartitions= false; > + part_info->num_parts= num_parts; > + part_info->num_subparts= table->part_info->num_subparts; > + part_info->subpart_type= table->part_info->subpart_type; > + if (unlikely(part_info->vers_init_info(thd))) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(0)); > + goto exit; > + } > + > + // NB: set_ok_status() requires DA_EMPTY > + thd->get_stmt_da()->reset_diagnostics_area(); Is it needed? You've just started using a new Diagnostics_area, the status should be DA_EMPTY here. > + > + thd->work_part_info= part_info; > + if (part_info->set_up_defaults_for_partitioning(thd, table->file, NULL, > + table->part_info->next_part_no(num_parts))) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "setting up defaults failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + bool partition_changed= false; > + bool fast_alter_partition= false; > + if (prep_alter_part_table(thd, table, &alter_info, &create_info, > + &partition_changed, &fast_alter_partition)) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "alter partitition prepare failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + if (!fast_alter_partition) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "fast alter partitition is not possible"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + DBUG_ASSERT(partition_changed); > + if (mysql_prepare_alter_table(thd, table, &create_info, &alter_info, > + &alter_ctx)) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "alter prepare failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + goto exit; > + } > + > + // Forge query string for rpl logging > + if (!thd->lex->no_write_to_binlog) > + { > + query.set(STRING_WITH_LEN("ALTER TABLE `"), &my_charset_latin1); better use StringBuffer<...> query; because you're doing lots of reallocs now. > + > + if (query.append(table->s->db) || > + query.append(STRING_WITH_LEN("`.`")) || > + query.append(table->s->table_name) || this is wrong, the table and db names can have backticks inside. use append_identifier() instead. > + query.append("` ADD PARTITION PARTITIONS ") || > + query.append_ulonglong(part_info->num_parts) || > + query.append(" AUTO")) > + { > + my_error(ER_OUT_OF_RESOURCES, MYF(ME_ERROR_LOG)); > + goto exit; > + } > + CSET_STRING qs(query.c_ptr(), query.length(), &my_charset_latin1); > + thd->set_query(qs); > + } Should it be binlogged at all? May be just leave it to the slave to auto-add the partition as needed? Looks a bit suspicious now, you log an ALTER TABLE possibly in the middle of a transaction. It will be replayed differently, not as auto-adding, in particular, it will commit. So, possibly different results on the slave, perhaps different gtid. Do clear it out with Andrei please. Or don't binlog auto-adding at all. > + > + if (fast_alter_partition_table(thd, table, &alter_info, &create_info, > + tl, &table->s->db, &table->s->table_name)) > + { > + push_warning(thd, Sql_condition::WARN_LEVEL_WARN, ER_VERS_HIST_PART_FAILED, > + "Auto-increment history partition: " > + "alter partition table failed"); > + my_error(ER_VERS_HIST_PART_FAILED, MYF(ME_WARNING), > + tl->db.str, tl->table_name.str); > + } > + } > + > + if (!thd->transaction.stmt.is_empty()) > + trans_commit_stmt(thd); 1. why would this operation register a transaction? 2. you amended a couple of checks like - if (thd->in_sub_stmt) + if (thd->in_sub_stmt & ~SUB_STMT_AUTO_HIST) but what will happen if it's, for example, an insert inside a trigger? So a real sub_stmt and SUB_STMT_AUTO_HIST? > + > +exit: > + thd->work_part_info= save_part_info; > + thd->m_reprepare_observer= save_reprepare_observer; > + thd->lex->restore_backup_query_tables_list(&save_query_tables); > + thd->in_sub_stmt&= ~SUB_STMT_AUTO_HIST; > + if (!new_stmt_da.is_warning_info_empty()) > + save_stmt_da->copy_sql_conditions_from_wi(thd, new_stmt_da.get_warning_info()); > + thd->set_stmt_da(save_stmt_da); > + thd->lex->no_write_to_binlog= save_no_write_to_binlog; > + thd->set_query(save_query); > } > > > diff --git a/sql/partition_info.h b/sql/partition_info.h > index 7ae2d168068..ca68e61cbe2 100644 > --- a/sql/partition_info.h > +++ b/sql/partition_info.h > @@ -72,9 +73,34 @@ struct Vers_part_info : public Sql_alloc > my_time_t start; > INTERVAL step; > enum interval_type type; > - bool is_set() { return type < INTERVAL_LAST; } > + bool is_set() const { return type < INTERVAL_LAST; } > + size_t seconds() const > + { > + if (step.second) > + return step.second; > + if (step.minute) > + return step.minute * 60; > + if (step.hour) > + return step.hour * 3600; > + if (step.day) > + return step.day * 3600 * 24; > + // comparison is used in rough estimates, it doesn't need to be calendar-correct Are you sure the approximate value is enough here? You use it to estimate how many partitions to create. It's not a big deal if you create more than necessary (although it's not nice and we'll definitely get bug reports if that will happen). But you surely don't want to create less. > + if (step.month) > + return step.month * 3600 * 24 * 30; > + DBUG_ASSERT(step.year); > + return step.year * 86400 * 30 * 365; > + } > + bool lt(size_t secs) const > + { > + return seconds() < secs; > + } > + bool ge(size_t seconds) const > + { > + return !(this->lt(seconds)); > + } lt() and ge() don't seem to be used anywhere. > } interval; > ulonglong limit; > + bool auto_inc; > partition_element *now_part; > partition_element *hist_part; > }; > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index b89be77f282..5583d70f8eb 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1862,6 +1862,25 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > DBUG_PRINT("info",("Using locked table")); > #ifdef WITH_PARTITION_STORAGE_ENGINE > part_names_error= set_partitions_as_used(table_list, table); > + if (table->part_info && > + table->part_info->part_type == VERSIONING_PARTITION && > + !table_list->vers_conditions.delete_history && > + table_list->lock_type >= TL_WRITE_ALLOW_WRITE && > + table_list->mdl_request.type == MDL_SHARED_WRITE) > + { > + switch (thd->lex->sql_command) > + { > + case SQLCOM_DELETE: > + case SQLCOM_UPDATE: > + case SQLCOM_REPLACE: > + case SQLCOM_REPLACE_SELECT: > + case SQLCOM_DELETE_MULTI: > + case SQLCOM_UPDATE_MULTI: > + /* Rotation is still needed under LOCK TABLES */ > + table->part_info->vers_set_hist_part(thd, false); ALTER TABLE works under LOCK TABLES, so this should be possible too, but let's have it as a separate task. > + default:; > + } > + } > #endif > goto reset; > } > @@ -2104,6 +2123,37 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > tc_add_table(thd, table); > } > > +#ifdef WITH_PARTITION_STORAGE_ENGINE > + if (table->part_info && > + table->part_info->part_type == VERSIONING_PARTITION && > + !table_list->vers_conditions.delete_history && > + table_list->lock_type >= TL_WRITE_ALLOW_WRITE && > + table_list->mdl_request.type >= MDL_SHARED_WRITE && > + table_list->mdl_request.type < MDL_EXCLUSIVE) > + { > + switch (thd->lex->sql_command) > + { > + case SQLCOM_LOCK_TABLES: > + case SQLCOM_DELETE: > + case SQLCOM_UPDATE: > + case SQLCOM_REPLACE: > + case SQLCOM_REPLACE_SELECT: > + case SQLCOM_DELETE_MULTI: > + case SQLCOM_UPDATE_MULTI: this is quite complex condition, better not to duplicate it. May be something like bool need_set_hist_part(TABLE_LIST *table_list, enum_sql_command sql_command) { if (table_list->table->part_info && table_list->table->part_info->part_type == VERSIONING_PARTITION && !table_list->vers_conditions.delete_history && table_list->lock_type >= TL_WRITE_ALLOW_WRITE && table_list->mdl_request.type >= MDL_SHARED_WRITE && table_list->mdl_request.type < MDL_EXCLUSIVE) { switch(sql_command) { case SQLCOM_LOCK_TABLES: case SQLCOM_DELETE: case SQLCOM_UPDATE: case SQLCOM_REPLACE: case SQLCOM_REPLACE_SELECT: case SQLCOM_DELETE_MULTI: case SQLCOM_UPDATE_MULTI: return true; } default:; } return false; } > + ot_ctx->vers_create_count= table->part_info->vers_set_hist_part(thd, true); > + if (ot_ctx->vers_create_count) > + { > + ot_ctx->request_backoff_action(Open_table_context::OT_ADD_HISTORY_PARTITION, > + table_list); > + MYSQL_UNBIND_TABLE(table->file); > + tc_release_table(table); There's no MYSQL_UNBIND_TABLE/tc_release_table after other request_backoff_action invocations. Why this one is special? > + DBUG_RETURN(TRUE); > + } > + default:; > + } > + } > +#endif > + > if (!(flags & MYSQL_OPEN_HAS_MDL_LOCK) && > table->s->table_category < TABLE_CATEGORY_INFORMATION) > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 14516811262..145ac5c5f64 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3346,6 +3347,10 @@ class THD: public THD_count, /* this must be first */ > > #ifdef WITH_PARTITION_STORAGE_ENGINE > partition_info *work_part_info; > + /** > + List of tables requiring new history partition. > + */ > + List<TABLE_SHARE> vers_auto_part_tables; this doesn't seem to be used anywhere > #endif > > #ifndef EMBEDDED_LIBRARY > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 7cc1faea79b..3f6c1793432 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -7521,14 +7529,17 @@ add_partition_rule: > > add_part_extra: > /* empty */ > - | '(' part_def_list ')' > + | '(' part_def_list ')' opt_vers_auto_inc > { > - LEX *lex= Lex; > - lex->part_info->num_parts= lex->part_info->partitions.elements; > + Lex->part_info->num_parts= Lex->part_info->partitions.elements; > + if ($4) > + Lex->alter_info.partition_flags|= ALTER_PARTITION_AUTO_HIST; > } > - | PARTITIONS_SYM real_ulong_num > + | PARTITIONS_SYM real_ulong_num opt_vers_auto_inc > { > Lex->part_info->num_parts= $2; > + if ($3) > + Lex->alter_info.partition_flags|= ALTER_PARTITION_AUTO_HIST; I'm confused. I thought that ALTER_PARTITION_AUTO_HIST is what you set in vers_add_auto_parts() to mark auto-adding of a new partition. But here you set it in the parser, when a user specifies AUTO in partition specifications. So it seems you're using this flag for two very different purposes. How do you distinguish between them? And why did you decide to do it this way? > } > ; > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index fbed614489d..25017ee5425 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -4825,6 +4829,13 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info, > DBUG_RETURN(TRUE); > } > > + if (alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST && > + (!table->part_info || !table->part_info->vers_info)) > + { > + my_error(ER_SYNTAX_ERROR, MYF(0)); can it even happen? Or should it be DBUG_ASSERT(0) here? > + DBUG_RETURN(TRUE); > + } > + > partition_info *alt_part_info= thd->lex->part_info; > /* > This variable is TRUE in very special case when we add only DEFAULT > @@ -5312,7 +5323,9 @@ that are reorganised. > now_part= el; > } > } > - if (*fast_alter_table && tab_part_info->vers_info->interval.is_set()) > + if (*fast_alter_table && > + !(alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST) && > + tab_part_info->vers_info->interval.is_set()) this !ALTER_PARTITION_AUTO_HIST - do you mean not vers_add_auto_parts() or not sql statement that uses AUTO ? > { > partition_element *hist_part= tab_part_info->vers_info->hist_part; > if (hist_part->range_value <= thd->query_start()) > @@ -5347,7 +5360,8 @@ that are reorganised. > */ > if (!(alter_info->partition_flags & ALTER_PARTITION_TABLE_REORG)) > { > - if (!alt_part_info->use_default_partitions) > + if (!alt_part_info->use_default_partitions && > + !(alter_info->partition_flags & ALTER_PARTITION_AUTO_HIST)) Sorry, I don't understand that if() at all. What does it do? > { > DBUG_PRINT("info", ("part_info: %p", tab_part_info)); > tab_part_info->use_default_partitions= FALSE; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 4

Re: [Maria-developers] 5150dfd6ab3: MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table
by Sergei Golubchik 25 Oct '20

25 Oct '20

Hi, Nikita! On Apr 27, Nikita Malyavin wrote: > revision-id: 5150dfd6ab3 (mariadb-10.3.12-230-g5150dfd6ab3) > parent(s): cf78b8c699d > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-06-12 22:11:45 +1000 > message: > > MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table > > * set … [View More]modified_non_trans_table in one missed place This commit needs way more text than that. This is just too misterious now. What asserts fails? What has the test case to do with the fix? Why modified_non_trans_table even matters without replication? Why it's specific to versioning and partitioning? Where trans_safe becomes false? > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index 3fcb59bdb40..99a297417d5 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -541,6 +541,29 @@ t1 CREATE TABLE `t1` ( > PARTITION BY SYSTEM_TIME INTERVAL 7 SECOND > (PARTITION `ver_p1` HISTORY ENGINE = DEFAULT_ENGINE, > PARTITION `ver_pn` CURRENT ENGINE = DEFAULT_ENGINE) > +set timestamp= default; > +# MDEV-17891 Assertion failures in select_insert::abort_result_set and > +# mysql_load upon attempt to replace into a full table > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > +pk integer auto_increment, > +primary key (pk), > +f varchar(45000) > +) with system versioning engine=memory > +partition by system_time interval 1 year (partition p1 history, > +partition pn current); > +# fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +insert into t1 (f) select f from t1; > +ERROR HY000: The table 't1' is full > +# leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +# copy all data into history partition > +replace into t1 select * from t1; > +replace into t1 select * from t1; > +ERROR HY000: The table 't1' is full > +drop table t1; > +set @@max_heap_table_size= 1048576; > # Test cleanup > drop database test; > create database test; > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > index d5c83b4d3bb..23768836efc 100644 > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -474,6 +474,7 @@ set timestamp=1523466002.799571; > insert into t1 values (11),(12); > set timestamp=1523466004.169435; > delete from t1 where pk in (11, 12); > +set timestamp= default; > > --echo # > --echo # MDEV-18136 Server crashes in Item_func_dyncol_create::prepare_arguments > @@ -489,6 +490,34 @@ partition by system_time interval column_get(column_create(7,7), 7 as int) secon > --replace_result $default_engine DEFAULT_ENGINE > show create table t1; > > +--echo # MDEV-17891 Assertion failures in select_insert::abort_result_set and > +--echo # mysql_load upon attempt to replace into a full table > + > +--let $max_heap_table_size_orig= `select @@max_heap_table_size;` > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > + pk integer auto_increment, > + primary key (pk), > + f varchar(45000) > +) with system versioning engine=memory > + partition by system_time interval 1 year (partition p1 history, > + partition pn current); > + > +--echo # fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +--error ER_RECORD_FILE_FULL > +insert into t1 (f) select f from t1; > +--echo # leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +--echo # copy all data into history partition > +replace into t1 select * from t1; > +--error ER_RECORD_FILE_FULL > +replace into t1 select * from t1; > + > +# cleanup > +drop table t1; > +eval set @@max_heap_table_size= $max_heap_table_size_orig; > + > --echo # Test cleanup > drop database test; > create database test; > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 1f3a70721fc..44502ea6704 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1953,6 +1953,8 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > if (likely(!error)) > { > info->deleted++; > + if (!table->file->has_transactions()) > + thd->transaction.stmt.modified_non_trans_table= TRUE; > if (table->versioned(VERS_TIMESTAMP)) > { > store_record(table, record[2]); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 1

[Maria-developers] Obtaining usernames in MariaDB UDFs
by Denis Scherbakov 02 Aug '20

02 Aug '20

Hello guys, I am building a set of MariaDB UDFs that require to know a name of a user who called them in each and every call. Users should not be able to pretend as someone else, hence usernames as an argument cannot be used. I compile UDFs against MariaDB headers and I was able to obtain the current_thd, but the security_ctx pointer inside is NULL. Could somebody give me a hint what I am doing wrong? Thank you for your answers! Best regards, Denis Scherbakov

2 2

Re: [Maria-developers] 7311586ca25: MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT
by Sergei Golubchik 02 Aug '20

02 Aug '20

Hi, Oleksandr! On Jul 29, Oleksandr Byelkin wrote: > revision-id: 7311586ca25 (mariadb-10.2.31-317-g7311586ca25) > parent(s): a1e52e7f32c > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-07-17 13:47:26 +0200 > message: > > MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT > > Part 1: make better asserts. > > diff --git a/sql/… [View More]field.cc b/sql/field.cc > index 65bd9d22857..cf0cc655888 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -11138,6 +11138,16 @@ bool Field::save_in_field_default_value(bool view_error_processing) > { > THD *thd= table->in_use; > > + if ( // table is not opened properly > + table == NULL || table->pos_in_table_list == NULL || > + table->pos_in_table_list->select_lex == NULL || > + // we are in subquery/derived/CTE > + !table->pos_in_table_list->top_table()->select_lex->is_top_level_select()) > + { > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > + } that's kind of questionable. if this condition can never be true, why do you have my_error() there? Just do asserts, like DBUG_ASSERT(table); DBUG_ASSERT(table->pos_in_table_list); DBUG_ASSERT(table->pos_in_table_list->select_lex); DBUG_ASSERT(table->pos_in_table_list->top_table()->select_lex->is_top_level_select()); > if (flags & NO_DEFAULT_VALUE_FLAG && > real_type() != MYSQL_TYPE_ENUM) > { > @@ -11177,12 +11187,29 @@ bool Field::save_in_field_default_value(bool view_error_processing) > bool Field::save_in_field_ignore_value(bool view_error_processing) > { > enum_sql_command com= table->in_use->lex->sql_command; > + > + if ( // table is not opened properly > + table == NULL || table->pos_in_table_list == NULL || > + table->pos_in_table_list->select_lex == NULL || > + // we are in subquery/derived/CTE > + !table->pos_in_table_list->top_table()->select_lex->is_top_level_select()) > + { > + DBUG_ASSERT(0); // shoud not happened > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > + } ditto > // All insert-like commands > if (com == SQLCOM_INSERT || com == SQLCOM_REPLACE || > com == SQLCOM_INSERT_SELECT || com == SQLCOM_REPLACE_SELECT || > com == SQLCOM_LOAD) > return save_in_field_default_value(view_error_processing); > - return 0; // ignore > + if (com == SQLCOM_UPDATE || com == SQLCOM_UPDATE_MULTI) > + return 0; // ignore > + > + // unexpected command > + DBUG_ASSERT(0); // shoud not happened ditto but why it should not happen? What if one does SELECT DEFAULT(f) FROM t? > + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0)); > + return false; > } > > > diff --git a/sql/item.cc b/sql/item.cc > index 9451d4203ca..382eb3ac47a 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -3930,13 +3930,19 @@ int Item_param::save_in_field(Field *field, bool no_conversions) > case NULL_VALUE: > return set_field_to_null_with_conversions(field, no_conversions); > case DEFAULT_VALUE: > - return field->save_in_field_default_value(field->table->pos_in_table_list-> > + return field->save_in_field_default_value((field->table && > + field->table->pos_in_table_list)? in what case can field->table be NULL here? > + field->table->pos_in_table_list-> > top_table() != > - field->table->pos_in_table_list); > + field->table->pos_in_table_list: > + FALSE); > case IGNORE_VALUE: > - return field->save_in_field_ignore_value(field->table->pos_in_table_list-> > + return field->save_in_field_ignore_value((field->table && > + field->table->pos_in_table_list)? > + field->table->pos_in_table_list-> > top_table() != > - field->table->pos_in_table_list); > + field->table->pos_in_table_list: > + FALSE); > case NO_VALUE: > DBUG_ASSERT(0); // Should not be possible > return true; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 2

Re: [Maria-developers] f854ac4d9e0: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
by Sergei Golubchik 02 Aug '20

02 Aug '20

Hi, Oleksandr! On Jul 29, Oleksandr Byelkin wrote: > revision-id: f854ac4d9e0 (mariadb-10.5.4-20-gf854ac4d9e0) > parent(s): 6cee9b1953b > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-07-03 13:39:49 +0200 > message: > > MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values > > To allocate new net buffer to avoid changing bufer we are reading. please add a more … [View More]descriptive comment. I'm still not sure I understood correctly what the bug was. > diff --git a/sql/net_serv.cc b/sql/net_serv.cc > --- a/sql/net_serv.cc > +++ b/sql/net_serv.cc > @@ -178,6 +178,14 @@ my_bool my_net_init(NET *net, Vio *vio, void *thd, uint my_flags) > DBUG_RETURN(0); > } > > + > +inline void net_reset_new_packet(NET *net) static > +{ > + net->buff_end= net->buff + net->max_packet; > + net->write_pos= net->read_pos= net->buff; > +} > + > + > my_bool net_allocate_new_packet(NET *net, void *thd, uint my_flags) > { > DBUG_ENTER("net_allocate_new_packet"); > @@ -186,11 +194,26 @@ my_bool net_allocate_new_packet(NET *net, void *thd, uint my_flags) > NET_HEADER_SIZE + COMP_HEADER_SIZE + 1, > MYF(MY_WME | my_flags)))) > DBUG_RETURN(1); > - net->buff_end=net->buff+net->max_packet; > - net->write_pos=net->read_pos = net->buff; > + net_reset_new_packet(net); > DBUG_RETURN(0); > } > > +unsigned char *net_try_allocate_new_packet(NET *net, void *thd, uint my_flags) > +{ > + unsigned char * old_buff= net->buff; > + if (net_allocate_new_packet(net, thd, my_flags)) > + { > + /* > + We failed to allocate a new buffer => return the old buffer and > + return an error > + */ > + net->buff= old_buff; > + net_reset_new_packet(net); > + old_buff= NULL; wouldn't it be better to make net_allocate_new_packet not to change net->buff on failure? because currently you have exactly the same problem in sql_parse.cc (in 10.4) and your patch doesn't fix it. simply if (!(buff= (uchar*) my_malloc(...))) DBUG_RETURN(1); net->buff= buff; net->buff_end= net->buff + net->max_packet; net->write_pos= net->read_pos= net->buff; DBUG_RETURN(0); > + } > + return old_buff; > +} > + > > void net_end(NET *net) > { > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -685,7 +685,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > !table->prepare_triggers_for_delete_stmt_or_event()) > will_batch= !table->file->start_bulk_delete(); > > - if (returning) > + if (returning && !thd->get_stmt_da()->is_set()) how are result sets for bulk delete returning sent? > { > if (result->send_result_set_metadata(returning->item_list, > Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > diff --git a/sql/sql_error.cc b/sql/sql_error.cc > --- a/sql/sql_error.cc > +++ b/sql/sql_error.cc > @@ -380,16 +380,33 @@ Diagnostics_area::set_eof_status(THD *thd) > if (unlikely(is_error() || is_disabled())) > return; > > - /* > - If inside a stored procedure, do not return the total > - number of warnings, since they are not available to the client > - anyway. > - */ > - m_statement_warn_count= (thd->spcont ? > - 0 : > - current_statement_warn_count()); > + if (m_status == DA_EOF_BULK) > + { > + /* > + If inside a stored procedure, do not return the total > + number of warnings, since they are not available to the client > + anyway. > + */ > + if (!thd->spcont) > + m_statement_warn_count+= current_statement_warn_count(); > + } > + else > + { > + /* > + If inside a stored procedure, do not return the total > + number of warnings, since they are not available to the client > + anyway. > + */ did you have to repeat the same comment twice? > + if (thd->spcont) > + { > + m_statement_warn_count= 0; > + m_affected_rows= 0; affected rows too? old code didn't do that > + } > + else > + m_statement_warn_count= current_statement_warn_count(); > + m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF); > + } > > - m_status= DA_EOF; > DBUG_VOID_RETURN; > } > > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4364,24 +4368,37 @@ Prepared_statement::execute_bulk_loop(String *expanded_query, > if (state == Query_arena::STMT_ERROR) > { > my_message(last_errno, last_error, MYF(0)); > - thd->set_bulk_execution(0); > - return TRUE; > + goto err; > } > /* Check for non zero parameter count*/ > if (param_count == 0) > { > DBUG_PRINT("error", ("Statement with no parameters for bulk execution.")); > my_error(ER_UNSUPPORTED_PS, MYF(0)); > - thd->set_bulk_execution(0); > - return TRUE; > + goto err; > } > > if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_SAFE)) > { > DBUG_PRINT("error", ("Command is not supported in bulk execution.")); > my_error(ER_UNSUPPORTED_PS, MYF(0)); > - thd->set_bulk_execution(0); > - return TRUE; > + goto err; > + } > + /* > + Here second buffer for not optimized commands, > + optimized commands do it inside thier internal loop. > + */ > + if (!(sql_command_flags[lex->sql_command] & CF_SP_BULK_OPTIMIZED) && > + this->lex->has_returning()) > + { > + // Above check can be true for SELECT in future > + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); > + if ((readbuff= > + net_try_allocate_new_packet(&thd->net, thd, > + MYF(MY_THREAD_SPECIFIC))) == NULL) sorry, I'm confused. You allocate a buffer here, in Prepared_statement::execute_bulk_loop, and then again in mysql_insert() ? > + { > + goto err; > + } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 2

Re: [Maria-developers] ca7e6f8d572: MDEV-19632 Replication aborts with ER_SLAVE_CONVERSION_FAILED upon CREATE ... SELECT in ORACLE mode
by Sergei Golubchik 31 Jul '20

31 Jul '20

Hi, Alexander! On Jul 28, Alexander Barkov wrote: > revision-id: ca7e6f8d572 (mariadb-10.3.21-178-gca7e6f8d572) > parent(s): f3f23b5c4bd > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2020-07-08 12:37:17 +0400 > message: > > MDEV-19632 Replication aborts with ER_SLAVE_CONVERSION_FAILED upon CREATE ... SELECT in ORACLE mode > > diff --git a/mysql-test/main/lowercase_fs_off.result b/mysql-… [View More]test/main/lowercase_fs_off.result > index f2a8ec14641..3f0b08a78c4 100644 > --- a/mysql-test/main/lowercase_fs_off.result > +++ b/mysql-test/main/lowercase_fs_off.result > @@ -158,3 +158,13 @@ show triggers like '%T1%'; > Trigger Event Table Statement Timing Created sql_mode Definer character_set_client collation_connection Database Collation > drop table t1; > set GLOBAL sql_mode=default; > +# > +# MDEV-19632 Replication aborts with ER_SLAVE_CONVERSION_FAILED upon CREATE ... SELECT in ORACLE mode > +# > +# Compatibility schema names respect the filesystem case sensitivity I agree, it should. But this is not consistent with information_schema. So I'd suggest a comment in eq_name method, explaining why compatibility schema names respect the filesystem case sensitivity. (because, e.g., we eventually turn them into real schemas, on disk). > +CREATE TABLE t1 (a MARIADB_SCHEMA.date); > +ERROR HY000: Unknown data type: 'MARIADB_SCHEMA.date' > +CREATE TABLE t1 (a Mariadb_schema.date); > +ERROR HY000: Unknown data type: 'Mariadb_schema.date' > +CREATE TABLE t1 (a mariadb_schema.date); > +DROP TABLE t1; > diff --git a/mysql-test/suite/compat/oracle/t/type_date.test b/mysql-test/suite/compat/oracle/t/type_date.test > index 61f7aa53944..34adc5889b4 100644 > --- a/mysql-test/suite/compat/oracle/t/type_date.test > +++ b/mysql-test/suite/compat/oracle/t/type_date.test > @@ -2,3 +2,101 @@ SET sql_mode=ORACLE; ... > +--echo # > +--echo # Qualified syntax is not supported yet in SP > +--echo # an MDEV for that? > diff --git a/sql/field.cc b/sql/field.cc > index 8accfb35b0b..4ed38f2c06f 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -10407,6 +10407,19 @@ void Column_definition::set_attributes(const Lex_field_type_st &type, > set_handler(type.type_handler()); > charset= cs; > > +#if MYSQL_VERSION_ID > 100500 > +#error When merging to 10.5, please move the code below to > +#error Type_handler_timestamp_common::Column_definition_set_attributes() good > +#else > + /* > + Unlike other types TIMESTAMP fields are NOT NULL by default. > + Unless --explicit-defaults-for-timestamp is given. > + */ > + if (!opt_explicit_defaults_for_timestamp && > + type.type_handler()->field_type() == MYSQL_TYPE_TIMESTAMP) > + flags|= NOT_NULL_FLAG; > +#endif > + > if (type.length()) > { > int err; > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 0b423355170..3fb07601f99 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -2080,6 +2081,13 @@ int Lex_input_stream::scan_ident_start(THD *thd, Lex_ident_cli_st *str) > > uint length= yyLength(); > yyUnget(); // ptr points now after last token char > + > + if (thd->lex->parsing_options.lookup_keywords_after_qualifier) > + { > + if (int tokval= find_keyword(str, length, 0)) > + return tokval; > + } why is that? > + > str->set_ident(m_tok_start, length, is_8bit); > m_cpp_text_start= m_cpp_tok_start; > m_cpp_text_end= m_cpp_text_start + length; > @@ -8284,3 +8292,32 @@ bool LEX::tvc_finalize_derived() > current_select->linkage= DERIVED_TABLE_TYPE; > return tvc_finalize(); > } > + > + > +bool LEX::map_data_type(const Lex_ident_sys_st &schema_name, > + Lex_field_type_st *type) const > +{ > + const Schema *schema= schema_name.str ? > + Schema::find_by_name(schema_name) : > + Schema::find_implied(thd); > + if (!schema) > + { > + char buf[128]; > + const Name type_name= type->type_handler()->name(); > + my_snprintf(buf, sizeof(buf), "%.*s.%.*s", > + (int) schema_name.length, schema_name.str, > + (int) type_name.length(), type_name.ptr()); > +#if MYSQL_VERSION_ID > 100500 > +#error Please remove the old code > + my_error(ER_UNKNOWN_DATA_TYPE, MYF(0), buf); > +#else > + my_printf_error(ER_UNKNOWN_ERROR, "Unknown data type: '%-.64s'", > + MYF(0), buf); > +#endif > + return true; > + } > + const Type_handler *mapped= schema->map_data_type(thd, type->type_handler()); > + if (mapped != type->type_handler()) > + type->set_handler(mapped); why do you need an if() here? type->set_handler(mapped) is not that expensive. > + return false; > +} > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 3dbc7724928..fcc2d44fdd6 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -2224,6 +2224,13 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet, > append_identifier(thd, packet, &field->field_name); > packet->append(' '); > > + const Type_handler *th= field->type_handler(); > + const Schema *implied_schema= Schema::find_implied(thd); > + if (th != implied_schema->map_data_type(thd, th)) > + { > + packet->append(th->schema()->name(), system_charset_info); sorry, I don't see it. How can th->schema() return not mariadb_schema? > + packet->append(STRING_WITH_LEN("."), system_charset_info); > + } > type.set(tmp, sizeof(tmp), system_charset_info); > field->sql_type(type); > packet->append(type.ptr(), type.length(), system_charset_info); > diff --git a/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result b/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result > index 23b7804638f..1feea5e47ee 100644 > --- a/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result > +++ b/storage/test_sql_discovery/mysql-test/sql_discovery/simple.result > @@ -82,7 +82,7 @@ select * from t1; > ERROR HY000: Engine TEST_SQL_DISCOVERY failed to discover table `test`.`t1` with 'create table t1 (a uint)' > show warnings; > Level Code Message > -Error 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'uint)' at line 1 > +Error 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ')' at line 1 this is not ideal :( can you try to keep the old error message? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org [View Less]

2 3

[Maria-developers] Review for: MDEV-17399 Add support for JSON_TABLE, part #7
by Sergey Petrunia 30 Jul '20

30 Jul '20

Hi Alexey, On Thu, Jul 16, 2020 at 04:34:46PM +0400, Alexey Botchkov wrote: > > In the new patch i think i addressed most of your comments. > https://github.com/MariaDB/server/commit/d3a311a8e9508ef94dddb6b7f4d366337a… > ... > > The above doesn't do proper quoting, which is wrong. Testcase: > > > > select * > > from > > json_table('[{"co\\\\lor": "blue", "price": 50}]', > > '$[*]' columns( color varchar(100) path '$.co\\\\lor') … [View More]

1 1

Re: [Maria-developers] 2b527a4e858: MDEV-17729: Assertion `! is_set() || m_can_overwrite_status' failed in
by Sergei Golubchik 30 Jul '20

30 Jul '20

Hi, Rucha! On Jul 30, Rucha Deodhar wrote: > > > > > > - if ((level == Sql_condition::WARN_LEVEL_WARN) && > > > - really_abort_on_warning()) > > > + if ((killed == NOT_KILLED) && (level == Sql_condition::WARN_LEVEL_WARN) && > > > + really_abort_on_warning()) > > > > I don't understand this. I presume the assert that fails is in > > da->set_error_status(), called below as > > > > … [View More]

1 0