- developers - lists.mariadb.org

[Maria-developers] MDEV-25875: RE 02469b: MDEV-17399 JSON_TABLE Accept JSON values for the JSON fields
by Sergey Petrunia 16 Jun '21

16 Jun '21

Hi Alexey, This is review input re https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a0702…. First, our workflow dictates that this patch should have its own MDEV entry. I've filed it https://jira.mariadb.org/browse/MDEV-25875. Please use it in commit descriptions. == Updates mixed with another patch == The patch has .result changes like: > @@ -551,10 +551,12 @@ JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT NULL ON ERROR)) jt; > ERROR 42000: 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 'NULL ON ERROR)) jt' at line 2 > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON EMPTY)) jt; > -ERROR 42000: 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 '0 ON EMPTY)) jt' at line 2 > +x > +0 > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON ERROR)) jt; > -ERROR 42000: 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 '0 ON ERROR)) jt' at line 2 > +x > +NULL > SELECT * FROM > JSON_TABLE('{}', '$' COLUMNS (x DATE > PATH '$.x' These are obviously from another patch (the one about "accepting non-string literals as default"). Please move them to that patch. == Comments == > @@ -377,6 +378,25 @@ static void store_json_in_field(Field *f, const json_engine_t *je) > } > > > +static int store_json_in_json(Field *f, json_engine_t *je) > +{ Please add a comment for the function. Something like "Store the current JSON element pointed by je into field f, as a separate JSON document" The name "store_json_in_json" doesn't look good to me. Do you think store_element_as_json_doc() would be better? > + const uchar *from= je->value_begin; > + const uchar *to; > + > + if (json_value_scalar(je)) > + to= je->value_end; > + else > + { > + int error; > + if ((error= json_skip_level(je))) > + return error; > + to= je->s.c_str; > + } > + f->store((const char *) from, (uint32) (to - from), je->s.cs); > + return 0; > +} > + > + > bool Json_table_nested_path::check_error(const char *str) > { > if (m_engine.s.error) > @@ -541,7 +561,12 @@ int ha_json_table::fill_column_values(THD *thd, uchar * buf, uchar *pos) > } > else > { > - if (!(error= !json_value_scalar(&je))) > + if (jc->m_format_json) > + { > + if (!(error= store_json_in_json(*f, &je))) > + error= er_handler.errors; Can there be a case when this assignment assigns any value other than 0? (My logic - er_handler.errors already had some error status, why did we call store_json_in_json? If there was an error inside store_json_in_json, we do not get to this assignment, as store_json_in_json() returned non-zero error. Is this correct?) > + } > + else if (!(error= !json_value_scalar(&je))) > { > store_json_in_field(*f, &je); > error= er_handler.errors; > @@ -868,6 +893,10 @@ int Json_table_column::set(THD *thd, enum_type ctype, const LEX_CSTRING &path) > anctual content. Not sure though if we should. > */ > m_path.s.c_str= (const uchar *) path.str; > + > + if (ctype == PATH) > + m_format_json= m_field->type_handler() == &type_handler_json_longtext; This will probably cause warnings on some compiler. Please wrap into "MY_TEST(...)" or just brackets, "z= (x == y)". == A discrepancy between this and MySQL == I've found this case: select * from json_table('{"foo": null}', '$' columns( jscol json path '$.foo') ) as T; with the above patch, it produces a JSON null value: +-------+ | jscol | +-------+ | null | +-------+ while in MySQL-8 it produces an SQL NULL: +-------+ | jscol | +-------+ | NULL | +-------+ I'm not yet sure what we should do about this. Any thoughts? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 1

[Maria-developers] New Member (GSoC MDEV-25015)
by Alan Cueva 15 Jun '21

15 Jun '21

Hello everyone, I am Alan Cueva, during the summer I will be working on the Custom formatting of strings in MariaDB queries (MDEV-25015 <https://jira.mariadb.org/browse/MDEV-25015>) project, but I would like to continue contributing in this community. I hope to be useful and learn from everyone. Thanks!

1 0

Re: [Maria-developers] 27060eb6ba5: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
by Sergei Golubchik 10 Jun '21

10 Jun '21

Hi, Oleksandr! On Oct 12, Oleksandr Byelkin wrote: > revision-id: 27060eb6ba5 (mariadb-10.5.4-220-g27060eb6ba5) > parent(s): 861cd4ce286 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-10-07 15:22:41 +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. You still didn't clarify the commit comment > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > index 7280236e43f..5aaff3cf623 100644 > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -685,8 +685,14 @@ 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) > + /* > + thd->get_stmt_da()->is_set() means first iteration of prepared statement > + with array binding operation execution (non optimized so it is not > + INSERT) > + */ > + if (returning && !thd->get_stmt_da()->is_set()) > { > + DBUG_ASSERT(thd->lex->sql_command != SQLCOM_INSERT); a strange assert to see in sql_delete.cc :) can one even reach mysql_delete() with SQLCOM_INSERT? not just with returning and !thd->get_stmt_da()->is_set(), anywhere? > if (result->send_result_set_metadata(returning->item_list, > Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) > goto cleanup; > diff --git a/sql/sql_error.cc b/sql/sql_error.cc > index b3ef0d89a98..a753af2b34d 100644 > --- 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 (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. > + */ I don't think it helps to duplicate the comment. You could just put it once before the if. > if (thd->spcont) > { > m_statement_warn_count= 0; > + m_affected_rows= 0; why do you reset m_affected_rows too? > } > else > m_statement_warn_count= current_statement_warn_count(); > + m_status= (is_bulk_op() ? DA_EOF_BULK : DA_EOF); > + } do we have tests for bulk operations and CALL? > > - m_status= DA_EOF; > DBUG_VOID_RETURN; > } > > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index ecb56e70f88..c144d3a8d7e 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4357,24 +4361,37 @@ Prepared_statement::execute_bulk_loop(String *expanded_query, > > + /* > + 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) && why "SP" and not "PS" ? > + this->lex->has_returning()) what about CALL? It won't have lex->has_returning(). What about SELECT? Can you bind an array to a parameter in SELECT or CALL? > + { > + // Above check can be true for SELECT in future > + DBUG_ASSERT(lex->sql_command != SQLCOM_SELECT); how can SQLCOM_SELECT have lex->has_returning() ? > + readbuff= thd->net.buff; // old buffer > + if (net_allocate_new_packet(&thd->net, thd, MYF(MY_THREAD_SPECIFIC))) > + { > + readbuff= NULL; // failure, net_allocate_new_packet keeps old buffer > + goto err; > + } > } > > #ifndef EMBEDDED_LIBRARY Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] 557ab2992dd: MDEV-21117 Incremental patch for a few review notes addressed.
by Sergei Golubchik 09 Jun '21

09 Jun '21

Hi, Andrei! Again, this is a review of the git diff f09d33f521b 557ab2992dd, not only 557ab2992dd. On Jun 02, Andrei Elkin wrote: > revision-id: 557ab2992dd (mariadb-10.6.0-67-g557ab2992dd) > parent(s): 4cc5a3074da > author: Andrei Elkin <andrei.elkin(a)mariadb.com> > committer: Andrei Elkin <andrei.elkin(a)mariadb.com> > timestamp: 2021-05-27 17:37:22 +0300 > message: > > MDEV-21117 Incremental patch for a few review notes addressed. > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc > new file mode 100644 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc > @@ -0,0 +1,65 @@ > +connect(master1,localhost,root,,); > +connect(master2,localhost,root,,); > +connect(master3,localhost,root,,); > + > +--connection default > + > +# First to commit few transactions > +INSERT INTO t VALUES (10); > +INSERT INTO tm VALUES (10); > + > +--connection master1 > +# Hold insert after write to binlog and before "run_commit_ordered" in engine > +SET DEBUG_SYNC= "commit_before_get_LOCK_commit_ordered SIGNAL master1_ready WAIT_FOR signal_never_arrives"; > +--send_eval $query1 > + > +--connection master2 > +SET DEBUG_SYNC= "now WAIT_FOR master1_ready"; > +SET DEBUG_SYNC= "commit_before_get_LOCK_after_binlog_sync SIGNAL master2_ready"; > +if ($pre_q2) > +{ > + --send_eval $pre_q2; > +} > +# To binlog non-xid transactional group which will be truncated all right > +if (!$pre_q2) > +{ > + --send_eval $query2 > +} eh, this makes no sense. In the previous version of the patch you've run pre_q2 before sending query2. now you send either pre_q2 or query2. so you don't need pre_q2 anymore now, just set query2 to the what used to be pre_q2. > + > +--connection master3 > +SET DEBUG_SYNC= "now WAIT_FOR master2_ready"; > +SELECT @@global.gtid_binlog_pos as 'Before the crash'; > + > +--connection default > +--source include/kill_mysqld.inc > +--disconnect master1 > +--disconnect master2 > +--disconnect master3 > + > +# > +# Server restart > +# > +--let $restart_parameters= --rpl-semi-sync-slave-enabled=1 > +--source include/start_mysqld.inc > + > +# Check error log for a successful truncate message. > +--let $log_error_ = $MYSQLTEST_VARDIR/log/mysqld.1.err > + > +--let SEARCH_FILE=$log_error_ > +--let SEARCH_PATTERN=Successfully truncated.*to remove transactions starting from GTID $truncate_gtid_pos > +--replace_regex /FOUND [0-9]+/FOUND #/ can it be found an unpredictable number of times? Why would binlog be truncated more than once? > +--source include/search_pattern_in_file.inc > + > +--echo Pre-crash binlog file content: > +--let $binlog_file= query_get_value(show binary logs, Log_name, $binlog_file_index) > +--source include/show_binlog_events.inc > + > +SELECT @@global.gtid_binlog_pos as 'After the crash'; > +--echo "One row should be present in table 't'" > +SELECT * FROM t; > + > +# prepare binlog file index for the next test > +--inc $binlog_file_index > + > +# Local cleanup > +DELETE FROM t; > diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.test b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test > new file mode 100644 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test > @@ -0,0 +1,104 @@ > +# ==== Purpose ==== > +# > +# Test verifies the truncation of single binary log file. > +# > +# ==== References ==== > +# > +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server > + > +--source include/have_innodb.inc > +--source include/have_aria.inc > +# File: binlog_truncate_active_log.inc included in test makes use of > +# 'debug_sync' facility. > +--source include/have_debug_sync.inc > +--source include/have_binlog_format_statement.inc > + > +call mtr.add_suppression("Can.t init tc log"); > +call mtr.add_suppression("Aborting"); > + > +# The following cases are tested: > +# A. 2pc transaction is followed by a blank "zero-engines" one > +# B. 2pc transaction follows the blank one > +# C. Similarly to A, with the XA blank transaction > + > +RESET MASTER; > +CREATE TABLE t (f INT) ENGINE=INNODB; > +CREATE TABLE t2 (f INT) ENGINE=INNODB; > +CREATE TABLE tm (f INT) ENGINE=Aria; > + > +# Old (pre-crash) binlog file index initial value. > +# It keeps incremented at the end of each case. > +--let $binlog_file_index=1 > + > +--echo # Case A. > +# Using 'debug_sync' hold 'query1' execution after 'query1' is flushed and > +# synced to binary log but not yet committed. In an another connection hold > +# 'query2' execution after 'query2' is flushed and synced to binlog. > +# Crash and restart server with --rpl-semi-sync-slave-enabled=1 > +# > +# During recovery of binary log 'query1' status is checked with InnoDB engine, > +# it will be in prepared but not yet commited. All transactions starting from > +# 'query1' onwards will be removed from the binary log. > +# Show-binlog-events is to prove that. > + > +--let $truncate_gtid_pos = 0-1-6 > +--let $query1 = INSERT INTO t VALUES (20) > +--let $query2 = DELETE FROM t2 WHERE f = 0 /* no such record */ > +--source binlog_truncate_active_log.inc > + > +--echo # Case B. > +# The inverted sequence ends up to truncate starting from $query2 > +--let $truncate_gtid_pos = 0-1-10 > +--let $query1 = DELETE FROM t2 WHERE f = 0 > +--let $query2 = INSERT INTO t VALUES (20) > +--source binlog_truncate_active_log.inc > + > + > +--echo # Case C. > +delimiter |; > +CREATE PROCEDURE sp_blank_xa() > +BEGIN > + XA START 'blank'; > + DELETE FROM t2 WHERE f = 0 /* no such record */; > + XA END 'blank'; > + XA PREPARE 'blank'; > +END| > +delimiter ;| > + > +# The same as in A with $query2 being the zero-engine XA transaction. > +# Both $query1 and $query2 are going to be truncated. > +--let $truncate_gtid_pos = 0-1-14 > +--let $query1 = INSERT INTO t VALUES (20) > +--let $pre_q2 = CALL sp_blank_xa > +--let $query2 = XA COMMIT 'blank' hm.. now your binlog_truncate_active_log.inc only runs query2 is pre_q2 is empty. So this XA COMMIT isn't done at all. Very confusing. > +--source binlog_truncate_active_log.inc > + > +DROP PROCEDURE sp_blank_xa; > + > + > +--echo # Case D. > +delimiter |; > +CREATE PROCEDURE sp_xa() > +BEGIN > + XA START 'xid'; > + DELETE FROM t WHERE f = 10; > + XA END 'xid'; > + XA PREPARE 'xid'; > +END| > +delimiter ;| > + > +# The same as in B with $query1 being the prepared XA transaction. > +# Truncation must occurs at $query2. > +--let $truncate_gtid_pos = 0-1-20 > +--let $query1 = CALL sp_xa > +--let $pre_q2 = > +--let $query2 = INSERT INTO t2 VALUES (20) > +--source binlog_truncate_active_log.inc > + > +DROP PROCEDURE sp_xa; > + > + > +--echo # Cleanup > +DROP TABLE t,t2,tm; > + > +--echo # End of the tests > diff --git a/sql/handler.h b/sql/handler.h > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -931,6 +932,32 @@ struct xid_t { > }; > typedef struct xid_t XID; > > +/* > + Enumerates a sequence in the order of > + their creation that is in the top-down order of the index file. > + Ranges from zero through MAX_binlog_id. > + Not confuse the value with the binlog file numerical suffix, > + neither with the binlog file line in the binlog index file. Ah! Good, thanks. I thought it was a binlog file numerical suffix, indeed. > +*/ > +typedef uint Binlog_file_id; > +const Binlog_file_id MAX_binlog_id= UINT_MAX; > +/* > + Compound binlog-id and byte offset of transaction's first event > + in a sequence (e.g the recovery sequence) of binlog files. > + Binlog_offset(0,0) is the minimum value to mean > + the first byte of the first binlog file. > +*/ > +typedef std::pair<Binlog_file_id, my_off_t> Binlog_offset; > + > +/* binlog-based recovery transaction descriptor */ > +struct xid_recovery_member > +{ > + my_xid xid; > + uint in_engine_prepare; // number of engines that have xid prepared > + bool decided_to_commit; > + Binlog_offset binlog_coord; // semisync recovery binlog offset > +}; > + > /* for recover() handlerton call */ > #define MIN_XID_LIST_SIZE 128 > #define MAX_XID_LIST_SIZE (1024*128) > diff --git a/sql/log.cc b/sql/log.cc > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -1692,17 +1698,13 @@ int binlog_init(void *p) > binlog_hton->savepoint_rollback= binlog_savepoint_rollback; > binlog_hton->savepoint_rollback_can_release_mdl= > binlog_savepoint_rollback_can_release_mdl; > - binlog_hton->commit= binlog_commit; > + binlog_hton->commit= binlog_commit_dummy; you could use a lambda for that too, [](handlerton*,THD*,bool) { return 0; } > binlog_hton->rollback= binlog_rollback; > binlog_hton->drop_table= [](handlerton *, const char*) { return -1; }; Oh did you actually need it? Or it's for completeness only? I mean, was there some code that tried to drop a "binlog" table? > if (WSREP_ON || opt_bin_log) > { > binlog_hton->prepare= binlog_prepare; > binlog_hton->start_consistent_snapshot= binlog_start_consistent_snapshot; > - binlog_hton->commit_by_xid= binlog_commit_by_xid; > - binlog_hton->rollback_by_xid= binlog_rollback_by_xid; > - // recover needs to be set to make xa{commit,rollback}_handlerton effective > - binlog_hton->recover= binlog_xa_recover_dummy; > } > binlog_hton->flags= HTON_NOT_USER_SELECTABLE | HTON_HIDDEN | HTON_NO_ROLLBACK; > return 0; > @@ -9975,6 +9977,154 @@ int TC_LOG::using_heuristic_recover() > /****** transaction coordinator log for 2pc - binlog() based solution ******/ > #define TC_LOG_BINLOG MYSQL_BIN_LOG > > +/** > + Truncates the current binlog to specified position. Removes the rest of binlogs > + which are present after this binlog file. > + > + @param truncate_file Holds the binlog name to be truncated > + @param truncate_pos Position within binlog from where it needs to > + truncated. > + > + @retval true ok > + @retval false error > + > +*/ > +bool MYSQL_BIN_LOG::truncate_and_remove_binlogs(const char *file_name, > + my_off_t pos, > + rpl_gtid *ptr_gtid) > +{ > + int error= 0; > +#ifdef HAVE_REPLICATION > + LOG_INFO log_info; > + THD *thd= current_thd; > + my_off_t index_file_offset= 0; > + File file= -1; > + MY_STAT s; > + my_off_t old_size; > + > + if ((error= find_log_pos(&log_info, file_name, 1))) > + { > + sql_print_error("Failed to locate binary log file:%s." > + "Error:%d", file_name, error); > + goto end; > + } > + > + while (!(error= find_next_log(&log_info, 1))) > + { > + if (!index_file_offset) > + { > + index_file_offset= log_info.index_file_start_offset; > + if ((error= open_purge_index_file(TRUE))) > + { > + sql_print_error("Failed to open purge index " > + "file:%s. Error:%d", purge_index_file_name, error); > + goto end; > + } > + } > + if ((error= register_purge_index_entry(log_info.log_file_name))) > + { > + sql_print_error("Failed to copy %s to purge index" > + " file. Error:%d", log_info.log_file_name, error); > + goto end; > + } > + } > + > + if (error != LOG_INFO_EOF) > + { > + sql_print_error("Failed to find the next binlog to " > + "add to purge index register. Error:%d", error); > + goto end; > + } > + > + if (is_inited_purge_index_file()) > + { > + if (!index_file_offset) > + index_file_offset= log_info.index_file_start_offset; > + > + if ((error= sync_purge_index_file())) > + { > + sql_print_error("Failed to flush purge index " > + "file. Error:%d", error); > + goto end; > + } > + > + // Trim index file > + if ((error= > + mysql_file_chsize(index_file.file, index_file_offset, '\n', > + MYF(MY_WME))) || > + (error= > + mysql_file_sync(index_file.file, MYF(MY_WME|MY_SYNC_FILESIZE)))) > + { > + sql_print_error("Failed to trim binlog index " > + "file:%s to offset:%llu. Error:%d", index_file_name, > + index_file_offset, error); > + goto end; > + } > + > + /* Reset data in old index cache */ > + if ((error= reinit_io_cache(&index_file, READ_CACHE, (my_off_t) 0, 0, 1))) > + { > + sql_print_error("Failed to reinit binlog index " > + "file. Error:%d", error); > + goto end; > + } > + > + /* Read each entry from purge_index_file and delete the file. */ > + if ((error= purge_index_entry(thd, NULL, TRUE))) > + { > + sql_print_error("Failed to process registered " > + "files that would be purged."); > + goto end; > + } > + } > + > + DBUG_ASSERT(pos); > + > + if ((file= mysql_file_open(key_file_binlog, file_name, > + O_RDWR | O_BINARY, MYF(MY_WME))) < 0) > + { > + error= 1; > + sql_print_error("Failed to open binlog file:%s for " > + "truncation.", file_name); > + goto end; > + } > + my_stat(file_name, &s, MYF(0)); > + old_size= s.st_size; > + clear_inuse_flag_when_closing(file); > + /* Change binlog file size to truncate_pos */ > + if ((error= > + mysql_file_chsize(file, pos, 0, MYF(MY_WME))) || > + (error= mysql_file_sync(file, MYF(MY_WME|MY_SYNC_FILESIZE)))) I'd rather not to overload if() conditions with function calls like this, they're rather annoying to single-step through and impossible to put a breakpoint on a specific function call. > + { > + sql_print_error("Failed to trim the " > + "binlog file:%s to size:%llu. Error:%d", > + file_name, pos, error); > + goto end; > + } > + else > + { > + char buf[21]; > + my_stat(file_name, &s, MYF(0)); > + my_off_t new_size= s.st_size; do you need it? new_size == pos, isn't it? You yourself use the word "size" to describe pos in sql_print_error just few lines above :) > + > + longlong10_to_str(ptr_gtid->seq_no, buf, 10); > + sql_print_information("Successfully truncated binlog file:%s " > + "to pos:%llu to remove transactions starting from " > + "GTID %u-%u-%s; " > + "former file size shrunk from %llu to %llu.", This isn't quite English. Better just "previous (or old) file size: %llu", that is not "former" and no need to specify the new size twice. Also, you say that you "truncated binlog file" here, but on failure you fail to "trim the binlog file". I suggest to use consistent wording, either always "trim" or always "truncate" (and always with or without "the") - both on success and on failure. Personally, I favor "truncate the binlog file" > + file_name, pos, > + ptr_gtid->domain_id, ptr_gtid->server_id, buf, > + old_size, new_size); > + } > + > +end: > + if (file >= 0) > + mysql_file_close(file, MYF(MY_WME)); > + > + error= error || close_purge_index_file(); > +#endif > + return error > 0; > +} > int TC_LOG_BINLOG::open(const char *opt_name) > { > int error= 1; > @@ -10449,7 +10599,528 @@ start_binlog_background_thread() ... > + > +Recovery_context::Recovery_context() : > + prev_event_pos(0), > + last_gtid_standalone(false), last_gtid_valid(false), last_gtid_no2pc(false), > + last_gtid_engines(0), > + do_truncate(rpl_semi_sync_slave_enabled), > + truncate_validated(false), truncate_reset_done(false), > + truncate_set_in_1st(false), id_binlog(MAX_binlog_id), > + checksum_alg(BINLOG_CHECKSUM_ALG_UNDEF), gtid_maybe_to_truncate(NULL) > +{ > + last_gtid_coord= Binlog_offset(0,0); > + binlog_truncate_coord= binlog_truncate_coord_1st_round= Binlog_offset(0,0); > + binlog_unsafe_coord= Binlog_offset(0,0); Okay, I admit, just typedef'ing std::pair is a huge improvement already > + binlog_truncate_file_name[0]= 0; > + binlog_unsafe_file_name [0]= 0; > + binlog_unsafe_gtid= truncate_gtid= truncate_gtid_1st_round= rpl_gtid(); > + if (do_truncate) > + gtid_maybe_to_truncate= new Dynamic_array<rpl_gtid>(16, 16); > +} > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 1

[Maria-developers] Tips on Writing/Reading JSON
by Michael Okoko 09 Jun '21

09 Jun '21

Hi Alexey, I'm Michael Okoko, a GSoC '21 student working on Using JSON as the on-disk format for MariaDB histograms(https://jira.mariadb.org/browse/MDEV-21130). The project requires writing and reading/parsing JSON, and I was wondering if you could provide some tips and pointers regarding a proper JSON writer/JSON parser to use. Regards, Michael.

2 1

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 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

2 15

[Maria-developers] Request for Discussion: Structure for JSON histogram objects
by Michael Okoko 04 Jun '21

04 Jun '21

Hi, MariaDB community! I'm implementing the patch for Using JSON as the on-disk format for MariaDB histograms(https://jira.mariadb.org/browse/MDEV-21130). as part of GSoC '21 and hoping to get insights and ideas from the community on how the JSON format for histograms should be structured. We are planning to change the columns of `mysql.column_stats` into something like: CREATE TABLE `column_stats` ( `db_name` varchar(64) COLLATE utf8mb3_bin NOT NULL, `table_name` varchar(64) COLLATE utf8mb3_bin NOT NULL, `column_name` varchar(64) COLLATE utf8mb3_bin NOT NULL, `histogram` blob ); The histogram column above will then be responsible for storing the JSON document for that row. Currently, the JSON structure we are looking at is based on the existing columns in `column_stats` table as well MySQL's JSON histogram implementation( https://dev.mysql.com/doc/refman/8.0/en/optimizer-statistics.html) Here's an example: { "min_value": 0, "max_value": 0, "hist_type": "singleton", "bucket": [ [1,0.3333333333333333], [2, 0.6666666666666666], [3,1] ], "nulls_ratios": 0, "last_updated": "2021-06-02 13:32:40.000000", "sampling_rate": 1, } - The min_value and max_values retain the meanings described at ( https://mariadb.com/kb/en/mysqlcolumn_stats-table/) - hist_type can be one of `SINGLETON` (where the number of buckets is >= the number of distinct values in the specified column) or `EQUI-HEIGHT` (where the number of buckets is < the number of distinct values found in the column). - nulls_ratio is a value between 0.0 and 1.0 indicating the fraction of column values that are SQL NULL values. If 0, the column contains no NULL values. - `last_updated` indicates when the histogram was generated, as a UTC value in YYYY-MM-DD hh:mm:ss.uuuuuu format. - `sampling_rate` is also a value between 0.0 and 1.0 indicating the fraction of column values that were sampled to create the histogram. A value of 1 means that all of the data was read (no sampling).

3 2

Re: [Maria-developers] 557ab2992dd: MDEV-21117 Incremental patch for a few review notes addressed.
by Sergei Golubchik 03 Jun '21

03 Jun '21

Hi, Andrei! On Jun 03, Andrei Elkin wrote: > >> - binlog_hton->commit= binlog_commit; > >> + binlog_hton->commit= binlog_commit_dummy; > >> binlog_hton->rollback= binlog_rollback; > >> binlog_hton->drop_table= [](handlerton *, const char*) { return -1; }; > > > > Oh did you actually need it? Or it's for completeness only? > > I am confused a bit, > 6c52931680a4 (Sergei Golubchik 2020-06-22 1703) binlog_hton->drop_table= [](handlerton *, const char*) { return -1; }; > > And I never reviewed the patch. Grrr. Sorry! When reviewing I diffed this patch with a previous version that I reviewed (a useful speedup technique at the late phase, when commits only change slightly) and this drop_table line was in the new patch and not in the old. Of course, it was because the new patch was in 10.6 and the old in 10.2, there were quite a few changes in the diff because of that. I tried to ignore them, but somehow missed this one. Note, there's no + before the drop_table line, it's not yours, it's indeed something that already was in 10.6, I should've noticed, my mistake :( Why I added it in 6c52931680a4? It was for DROP TABLE FORCE, that simply does plugin_foreach() and tries to hton->drop_table() in every hton available. In binlog_hton too. The goal was to solve cases like a table in InnoDB data dictionary but with no frm, or Aria table where MAD file exists, but no MAI - cases when DROP TABLE doesn't see the table, but CREATE TABLE still fails as some remnants of a table still exist. > >> @@ -9975,6 +9977,154 @@ int TC_LOG::using_heuristic_recover() > >> + else > >> + { > >> + char buf[21]; > >> + my_stat(file_name, &s, MYF(0)); > >> + my_off_t new_size= s.st_size; > > > > do you need it? new_size == pos, isn't it? You yourself use the word > > "size" to describe pos in sql_print_error just few lines above :) > > Sure. Let turn it into an > > assert( pos == new_size ) > > [x] and this is, what I think, you'd call "too much checks" :) I'd personally would've dropped my_stat and new_size, especially as I also suggest to remove them from the sql_print_information below. > >> + > >> + longlong10_to_str(ptr_gtid->seq_no, buf, 10); > >> + sql_print_information("Successfully truncated binlog file:%s " > >> + "to pos:%llu to remove transactions starting from " > >> + "GTID %u-%u-%s; " > >> + "former file size shrunk from %llu to %llu.", > > > > This isn't quite English. Better just "previous (or old) file > > size: %llu", that is not "former" and no need to specify the new size twice. > > > > Also, you say that you "truncated binlog file" here, but on failure > > you fail to "trim the binlog file". I suggest to use consistent > > wording, either always "trim" or always "truncate" (and always with > > or without "the") - both on success and on failure. Personally, I favor > > "truncate the binlog file" > > some two styles of two writers of these blocks mixed up :-). (Partly > relates to the size in above). > > [x] > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 8e79d168614: MDEV-25672 table alias from previous statement interferes later commands.
by Sergei Golubchik 02 Jun '21

02 Jun '21

Hi, Aleksey! On Jun 02, Aleksey Midenkov wrote: > revision-id: 8e79d168614 (mariadb-10.2.31-989-g8e79d168614) > parent(s): 433dd490d33 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-05-31 16:20:38 +0300 > message: > > MDEV-25672 table alias from previous statement interferes later commands. I'm sorry. I don't understand what all this new code for. To make sure that vcol's field->table_name is refreshed for every statement? What is vcol's field->table_name used for besides your CREATE TABLE check? If it's not used for anything then a much simpler fix would be narrow the check - it only needs to be run for new vcols in CREATE TABLE, disabling it for old vcols in ALTER TABLE will do the trick. Like - if (p.table_name.length && table_name) + if (!field && p.table_name.length && table_name) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] e92037989f7: MDEV-21117: refine the server binlog-based recovery for semisync
by Sergei Golubchik 01 Jun '21

01 Jun '21

Hi, Andrei! On Jun 01, Andrei Elkin wrote: > >>>> > >>>> It's actually turns out not to be easy. A sequence of execution events > >>>> > >>>> E1.prepare(trx), E2.prepare(trx), E1.commit(trx), *crash* > >>>> > >>>> does not make E1 such as Innodb committed persistently in file. > >>>> At recovery in E1 trx may be found in prepared state, unless > >>>> before the crash I'd do something like Binlog CheckPoint (BCP) > >>>> notification request and wait of it. > >>> > The committed state is done asynchronously (by virtue of MDEV-232 that > implements a part of BCP). Oh. Indeed. Sorry, I've missed that. Still a prepare of another transaction can force this commit to disk, as far as I understand. So what about this: you stop the thread on a debug sync after the first commit then another connection starts committing in InnoDB, it stops on a debug sync after the prepare - before anything is written into binlog then you crash. looks like it'll create the case you need > >> No doubt it's technically possible to wait for the flushing and > >> crash, but still does not look easy. > >> Consider if we defer this sort of mtr test to MDEV-18959? > > Sujatha just contributed `74a13b4de2` with an mtr test that crashes > after the asynchronous fsync is done to the partially (2 engine case) > committed state. ... > Aslo now that `74a13b4de2` provides with a method to crash after the > "commmit" fsync we might also employ it for RQG. No, this is very different. This is a crash happening where Sujatha expects it to happen, there's no value in that now, it's needed for regression testing, to make sure the recovery won't be broken in the future. But it makes no sense to keep testing now how recovery works in case of a crash happening at exactly the place where Sujatha wanted it to happen and after she verified that crash at that exactly line does not break the recovery. The whole point of RQG tests is to try crashes where you did not expect them to happen. Adding DBUG_EXECUTE_IF() doesn't help this goal at all. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0