- developers - lists.mariadb.org

Re: [Maria-developers] f95616dc477: MDEV-17401: LOAD DATA from very big file into MyISAM table results in EOF error and corrupt index
by Sergei Golubchik 31 Oct '18

31 Oct '18

Hi, Oleksandr! ok to push! On Oct 30, Oleksandr Byelkin wrote: > revision-id: f95616dc4774328381044b53263d6a23ae19bfb4 (mariadb-5.5.61-39-gf95616dc477) > parent(s): 893ebb739e24637985892da1555f2e6f1a8ac5b6 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-10-30 19:36:17 +0100 > message: > > MDEV-17401: LOAD DATA from very big file into MyISAM table results in EOF error and corrupt index > > my_read fixed as in higher versions. > my_pread made as my_read aware of partial read of huge chunks of files > MY_FULL_IO enabled for file operations > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] c29706f: MDEV-17286 Decimal field multiplication bug.
by Sergei Golubchik 30 Oct '18

30 Oct '18

Hi, Alexey! On Oct 30, Alexey Botchkov wrote: > > What if both arguments will be zeros, but have intg=DIG_PER_DEC1-1. > > Won't the result have more that DIG_PER_DEC1 zeros? > > Leading zero 'digits' are truncated, so we never get more than DIG_PER_DEC > there. Actually we don't need to do that check at all (intg <= > DIG_PER_DEC1), > i removed it already. Put it in an assert please. I find asserts quite useful as a documentation tool :) Then ok to push! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] c29706f: MDEV-17286 Decimal field multiplication bug.
by Sergei Golubchik 30 Oct '18

30 Oct '18

Hi, Alexey! On Oct 24, Alexey Botchkov wrote: > revision-id: c29706f7560148488791acca3c92dde1b188334e (mariadb-5.5.61-32-gc29706f) > parent(s): e31e697f17f79ffa6913499e7e2d29866f24b475 > committer: Alexey Botchkov > timestamp: 2018-10-24 01:26:49 +0400 > message: > > MDEV-17286 Decimal field multiplication bug. > > We should clear trailing zeroes in frac part. Otherwise > that tail is growing quickly and forces unnecessary truncating of > arguments. This looks good, thanks. Please add a test for create select, as discussed. And fix the bug number in the commit comment. See one question below. > @@ -2109,6 +2104,14 @@ int decimal_mul(const decimal_t *from1, const decimal_t *from2, decimal_t *to) > for (; d_to_move--; cur_d++, buf1++) > *cur_d= *buf1; > } > + > + /* Now we have to check for -0.000 case */ > + if (to->sign && to->intg <= DIG_PER_DEC1 && to->frac == 0 && > + to->buf[0] == 0) What if both arguments will be zeros, but have intg=DIG_PER_DEC1-1. Won't the result have more that DIG_PER_DEC1 zeros? > + { > + /* We got decimal zero */ > + decimal_make_zero(to); > + } > return error; > } > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] c29706f: MDEV-17286 Decimal field multiplication bug.
by Sergei Golubchik 29 Oct '18

29 Oct '18

Hi, Alexey! On Oct 24, Alexey Botchkov wrote: > revision-id: c29706f7560148488791acca3c92dde1b188334e (mariadb-5.5.61-32-gc29706f) > parent(s): e31e697f17f79ffa6913499e7e2d29866f24b475 > committer: Alexey Botchkov > timestamp: 2018-10-24 01:26:49 +0400 > message: > > MDEV-17286 Decimal field multiplication bug. it's MDEV-17256 :) > > We should clear trailing zeroes in frac part. Otherwise > that tail is growing quickly and forces unnecessary truncating of > arguments. I don't understand this, sorry. Could you elaborate, please? I understand the "tail is growing quickly" part. How does it "force unnecessary truncating of arguments"? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-12321 authentication plugin: SET PASSWORD support
by Sergei Golubchik 27 Oct '18

27 Oct '18

Hi, Please review commits for MDEV-12321 "authentication plugin: SET PASSWORD support" They're now in the bb-10.4-serg branch, the last seven commits: ========================= commit 62e8340f513 Author: Sergei Golubchik <serg(a)mariadb.org> Date: Wed Oct 17 12:48:13 2018 +0200 MDEV-12321 authentication plugin: SET PASSWORD support Support SET PASSWORD for authentication plugins. Authentication plugin API is extended with two optional methods: * hash_password() is used to compute a password hash (or digest) from the plain-text password. This digest will be stored in mysql.user table * preprocess_hash() is used to convert this digest into some memory representation that can be later used to authenticate a user. Build-in plugins convert the hash from hexadecimal or base64 to binary, to avoid doing it on every authentication attempt. Note a change in behavior: when loading privileges (on startup or on FLUSH PRIVILEGES) an account with an unknown plugin was loaded with a warning (e.g. "Plugin 'foo' is not loaded"). But such an account could not be used for authentication until the plugin is installed. Now an account like that will not be loaded at all (with a warning, still). Indeed, without plugin's preprocess_hash() method the server cannot know how to load an account. Thus, if a new authentication plugin is installed run-time, one might need FLUSH PRIVILEGES to activate all existing accounts that were using this new plugin. commit 8266ca26f27 Author: Sergei Golubchik <serg(a)mariadb.org> Date: Mon Oct 15 01:39:03 2018 +0200 misc cleanups * remove dead code (from .yy) * remove redundant commands from the test * extract common code into a reusable function (get_auth_plugin, push_new_user) * rename update_user_table->update_user_table_password * simplify acl_update_role * don't strdup a string that's already in a memroot (in ACL_ROLE::ACL_ROLE(ACL_USER*)) * create parent_grantee and role_grants dynamic arrays with size 0. to avoid any memory allocations when roles aren't used. commit fd0bcb5e791 Author: Sergei Golubchik <serg(a)mariadb.org> Date: Sun Oct 14 13:52:52 2018 +0200 Use mysql.user.authentication_string for password Don't distinguish between a "password hash" and "authentication string" anymore. Now both are stored in mysql.user.authentication_string, both are handled identically internally. A "password hash" is just how some particular plugins interpret authentication string. Set mysql.user.plugin even if there is no password. The server will use mysql_native_password plugin in these cases, let's make it expicit. Remove LEX_USER::pwhash. commit 15f30100e0d Author: Sergei Golubchik <serg(a)mariadb.org> Date: Sat Oct 13 18:32:05 2018 +0200 cleanup: sql_acl.cc remove fix_plugin_ptr() it was doing two my_strcasecmp() unconditionally, to optimize away one conditional my_strcasecmp() later. commit df9d95d2a85 Author: Sergei Golubchik <serg(a)mariadb.org> Date: Sat Oct 13 11:30:39 2018 +0200 cleanup: sql_acl.cc remove username=NULL Some parts of sql_acl.cc historically assumed that empty username is represented by username=NULL, other parts used username="" for that. And most of the code wasn't sure and checked both (like in `if (!user || !user[0])`). Change it to use an empty string everywhere. commit 8b3cbc1e469 Author: Sergei Golubchik <serg(a)mariadb.org> Date: Fri Oct 12 18:48:15 2018 +0200 cleanup: sql_acl.cc password->LEX_CSTRING commit 2fc7a20da6a Author: Sergei Golubchik <serg(a)mariadb.org> Date: Fri Oct 12 19:24:28 2018 +0200 cleanup: safe_lexcstrdup_root() ========================= I've split somewhat independent changes into separate commits, but, of course, if you'd like to review one big patch you can `git diff` them all together. Now, the syntax problem. Old MySQL syntax (for the last ~20 years) was (1) GRANT ... TO user IDENTIFIED BY 'plain-text password' (2) GRANT ... TO user IDENTIFIED BY PASSWORD 'password hash' (3) SET PASSWORD = 'password hash' (4) SET PASSWORD = PASSWORD('plain-text password') (5) SET PASSWORD = OLD_PASSWORD('plain-text password') here, syntax (1) and (4) were forcing mysql_native_password authentication, (5) was forcing mysql_old_password, and (2) and (3) were auto-detecting, based on the hash length. Later MariaDB and MySQL added support for pluggable authentication with the syntax (6) GRANT ... TO user IDENTIFIED VIA plugin AS 'password hash' MySQL 5.7 added support for specifying plain-text password for plugins using the syntax (7) GRANT ... TO user IDENTIFIED WITH plugin BY 'plain-text password' I don't quite like it because there's no logical reason why "BY" means a plain-text password, while "AS" means a hash. Also we support "USING" instead of "AS", which also means a hash. One can easily get lost in all these USING/AS/BY and what special semantics each of them has. The syntax I've implemented is based on SET PASSWORD: (8) GRANT ... TO user IDENTIFIED VIA plugin AS PASSWORD('plain-text password') This is quite intuitive and pretends that there's a sql function PASSWORD() which returns a hash and it's used as an expression where a hash is anyway expected. Same works in SET PASSWORD too, obviously. A PASSWORD() function actually exists and returns the password hash. The problem here is that PASSWORD() function becomes quite magical. It returns a different password, depending on what plugin the user is using. One can still do SELECT PASSWORD("foo"), OLD_PASSWORD("foo"), but they'll return values for mysql_native_password and mysql_old_password as before. In the context of SET PASSWORD or GRANT (or CREATE/ALTER USER) it becomes context dependent, it's a bit difficult to swallow. Another approach would be not to pretend it's a function. Say (9) GRANT ... TO user IDENTIFIED VIA plugin AS PASSWORD 'plain-text password' SET PASSWORD = PASSWORD 'plain-text password' but, unfortunately, it is exactly backwards from the historical behaviour of (1) and (2). All in all I'm leaning towards (8), but I'm not quite happy with it :( One way to solve it could be to extend PASSWORD() function to allow a second argument, plugin name, like in SELECT PASSWORD("foo", "ed25519") Yet another way could be to remove SQL-level functions PASSWORD() and OLD_PASSWORD(). That would be my favorite, they always were nothing but trouble. But I wouldn't risk doing it now :) Regards, Sergei

2 3

Re: [Maria-developers] cc5c19ae523: MDEV-17401: LOAD DATA from very big file into MyISAM table results in EOF error and corrupt index
by Sergei Golubchik 24 Oct '18

24 Oct '18

Hi, Oleksandr! On Oct 12, Oleksandr Byelkin wrote: > revision-id: cc5c19ae5233ba90de086de76043774ae6c78cd7 (mariadb-5.5.61-30-gcc5c19ae523) > parent(s): acf8fc1ff8a7b2d49e25279670b04b8eb096ce0c > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-10-12 09:07:05 +0200 > message: > > MDEV-17401: LOAD DATA from very big file into MyISAM table results in EOF error and corrupt index > > my_read fixed as in higher versions. > my_pread made as my_read aware of partial read of huge chunks of files > MY_FULL_IO enabled for file operations > > --- > mysys/mf_iocache.c | 4 ++++ > mysys/my_pread.c | 25 ++++++++++++++++++------- > mysys/my_read.c | 52 ++++++++++++++++++++++++++++------------------------ > 3 files changed, 50 insertions(+), 31 deletions(-) > > diff --git a/mysys/mf_iocache.c b/mysys/mf_iocache.c > index bd71e2ae527..9a051cf6a9e 100644 > --- a/mysys/mf_iocache.c > +++ b/mysys/mf_iocache.c > @@ -282,6 +282,10 @@ int init_io_cache(IO_CACHE *info, File file, size_t cachesize, > } > info->inited=info->aio_result.pending=0; > #endif > + if (type == READ_CACHE || type == WRITE_CACHE || type == SEQ_READ_APPEND) > + info->myflags|= MY_FULL_IO; > + else > + info->myflags&= ~MY_FULL_IO; 1. that's a bit redundant, it'd be enough to do it in init_io_cache. but ok. 2. why you didn't remove MY_FULL_IO setting from reinit_io_cache? > DBUG_RETURN(0); > } /* init_io_cache */ > > diff --git a/mysys/my_pread.c b/mysys/my_pread.c > index 51b4c5f5599..5cbd6b4316b 100644 > --- a/mysys/my_pread.c > +++ b/mysys/my_pread.c > @@ -47,8 +47,7 @@ > size_t my_pread(File Filedes, uchar *Buffer, size_t Count, my_off_t offset, > myf MyFlags) > { > - size_t readbytes; > - int error= 0; > + size_t readbytes, save_count= 0; > > DBUG_ENTER("my_pread"); > > @@ -66,11 +65,10 @@ size_t my_pread(File Filedes, uchar *Buffer, size_t Count, my_off_t offset, > #else > readbytes= pread(Filedes, Buffer, Count, offset); > #endif > - error = (readbytes != Count); > > - if (error) > + if (readbytes != Count) > { > - my_errno= errno ? errno : -1; > + my_errno= errno; > if (errno == 0 || (readbytes != (size_t) -1 && > (MyFlags & (MY_NABP | MY_FNABP)))) > my_errno= HA_ERR_FILE_TOO_SHORT; > @@ -82,6 +80,17 @@ size_t my_pread(File Filedes, uchar *Buffer, size_t Count, my_off_t offset, > (int) readbytes)); > continue; /* Interrupted */ > } > + > + /* Do a read retry if we didn't get enough data on first read */ > + if (readbytes != (size_t) -1 && readbytes != 0 && > + (MyFlags & MY_FULL_IO)) > + { > + Buffer+= readbytes; > + Count-= readbytes; > + save_count+= readbytes; > + continue; eh, did you test your patch? I don't see how it could work without offset+= readbytes; > + } > + > if (MyFlags & (MY_WME | MY_FAE | MY_FNABP)) > { > if (readbytes == (size_t) -1) > @@ -97,8 +106,10 @@ size_t my_pread(File Filedes, uchar *Buffer, size_t Count, my_off_t offset, > DBUG_RETURN(MY_FILE_ERROR); /* Return with error */ > } > if (MyFlags & (MY_NABP | MY_FNABP)) > - DBUG_RETURN(0); /* Read went ok; Return 0 */ > - DBUG_RETURN(readbytes); /* purecov: inspected */ > + readbytes= 0; /* Read went ok; Return 0 */ > + else > + readbytes+= save_count; > + DBUG_RETURN(readbytes); > } > } /* my_pread */ > > diff --git a/mysys/my_read.c b/mysys/my_read.c > index 883a1c5fdc7..47f2d725f65 100644 > --- a/mysys/my_read.c > +++ b/mysys/my_read.c > @@ -35,17 +35,16 @@ this is copied from 10.1 verbatim, I presume Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] c850799967c: MDEV-10963 Fragmented BINLOG query
by Sergei Golubchik 23 Oct '18

23 Oct '18

Hi, Andrei! First, when I run it I got: Warning: 112 bytes lost at 0x7f212f26a670, allocated by T@0 at sql/sql_binlog.cc:68, sql/sql_binlog.cc:181, sql/sql_parse.cc:5625, sql/sql_parse.cc:7465, sql/sql_parse.cc:1497, sql/sql_parse.cc:1124, sql/sql_connect.cc:1330, sql/sql_connect.cc:1243 so you have a memory leak with your thd->lex->comment.str Second, BINGLOG CONCAT() is totally possible, inventing a new keyword defeats the whole point of using a familiar and intuitive syntax. I've attached a simple patch that implements it. On the other hand, I just thought that CONCAT() syntax might be not so intuitive after all, as - strictly speaking - a result of CONCAT cannot be larger than max_allowed_packet. So it might be confusing to use CONCAT here. May be just use your idea of BINLOG @a, @b; with no CONCAT or anything else at all? Also, I'm thinking whether BINLOG (in this last form) could automatically clear its variables @a and @b. Without an explicit SET @a=NULL. It's a bit weird to have such a side effect. But it'll help to save memory - the event size it over 3GB, right? So you'll have it split in two variables. Then you create a concatenated copy (it's over 3GB more), then you decode it (already more than 9GB in memory), and then it's executed where copies of huge blob values are created again, may be more than once. Freeing @a and @b and the concatenated copy as soon as possible will save over 6GB in RAM usage. Note, we cannot auto-reset variables in the syntax with CONCAT(), DEFRAGMENT() or whatever, because functions cannot do it to their arguments. On Oct 18, Andrei Elkin wrote: > revision-id: c850799967c561d08242987269d94af6ae4c7c5e (mariadb-10.1.35-59-gc850799967c) > parent(s): d3a8b5aa9cee24a6397662e30df2e915f45460e0 > author: Andrei Elkin <andrei.elkin(a)mariadb.com> > committer: Andrei Elkin <andrei.elkin(a)mariadb.com> > timestamp: 2018-09-21 18:48:06 +0300 > message: > > MDEV-10963 Fragmented BINLOG query > > The problem was originally stated in > http://bugs.mysql.com/bug.php?id=82212 > The size of an base64-encoded Rows_log_event exceeds its > vanilla byte representation in 4/3 times. > When a binlogged event size is about 1GB mysqlbinlog generates > a BINLOG query that can't be send out due to its size. > > It is fixed with fragmenting the BINLOG argument C-string into > (approximate) halves when the base64 encoded event is over 1GB size. > The mysqlbinlog in such case puts out > > SET @binlog_fragment_0='base64-encoded-fragment_0'; > SET @binlog_fragment_1='base64-encoded-fragment_1'; > BINLOG DEFRAGMENT(@binlog_fragment_0, @binlog_fragment_1); > > to represent a big BINLOG 'base64-encoded-"total"'. > Two more statements are composed to promptly release memory > SET @binlog_fragment_0=NULL; > SET @binlog_fragment_1=NULL; > > The 2 fragments are enough, though the client and server still may > need to tweak their @@max_allowed_packet to satisfy to the fragment > size (which they would have to do anyway with greater number of > fragments, should that be desired). > > On the lower level the following changes are made: > > Log_event::print_base64() > remains to call encoder and store the encoded data into a cache but > now *without* doing any formatting. The latter is left for time > when the cache is copied to an output file (e.g mysqlbinlog output). > No formatting behavior is also reflected by the change in the meaning > of the last argument which specifies whether to cache the encoded data. > > my_b_copy_to_file() > is turned into my_b_copy_to_file_frag() > which accepts format specifier arguments to build a proper syntax BINLOG > query in both the fragmented (n_frag > 1) and non-fragmented (n_frag == 1) > cases. > > Rows_log_event::print_helper() > Takes decision whether to fragment, prepares respective format specifiers > and invokes the cache-to-file copying function, which is now > > copy_cache_frag_to_file_and_reinit() > replaces original copy_event_cache_to_file_and_reinit() to pass > extra arguments to my_b_copy_to_file() successor of > > my_b_copy_to_file_frag() > replaces the former pure copier not to also conduct wrapping the encoded > data per format specifiers. > With its 'n_frag' argument as 1 > and the rest or args NULL works as the original function. > > diff --git a/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row_frag.test b/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row_frag.test > new file mode 100644 > index 00000000000..5318f8acec6 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_mysqlbinlog_row_frag.test > @@ -0,0 +1,43 @@ > +--source include/have_debug.inc > +--source include/have_log_bin.inc > +--source include/have_binlog_format_row.inc > + > +--let $MYSQLD_DATADIR= `select @@datadir` > +--let $max_size=1024 > + > +CREATE TABLE t (a TEXT); > +# events of interest are guaranteed to stay in 000001 log > +RESET MASTER; > +--eval INSERT INTO t SET a=repeat('a', $max_size) > +SELECT a from t into @a; > +FLUSH LOGS; > +DELETE FROM t; > + > +--exec $MYSQL_BINLOG --debug-binlog-row-event-max-encoded-size=256 $MYSQLD_DATADIR/master-bin.000001 > $MYSQLTEST_VARDIR/tmp/mysqlbinlog.sql > + > +--exec $MYSQL test < $MYSQLTEST_VARDIR/tmp/mysqlbinlog.sql how do you know the event was split, if you aren't looking into mysqlbinlog.sql file? may be your code doesn't work at all, or it was removed by some incorrect merge? the test will pass anyway... > + > +SELECT a LIKE @a as 'true' FROM t; > +SELECT @binlog_fragment_0, @binlog_fragment_1 as 'NULL'; > + > +# improper syntax error > +--echo BINLOG number-of-fragments must be exactly two > +--error ER_PARSE_ERROR > +BINLOG DEFRAGMENT(@binlog_fragment); > +--error ER_PARSE_ERROR > +BINLOG DEFRAGMENT(@binlog_fragment, @binlog_fragment, @binlog_fragment); > + > +# corrupted fragments error check (to the expected error code notice, > +# the same error code occurs in a similar unfragmented case) > +SET @binlog_fragment_0='012345'; > +SET @binlog_fragment_1='012345'; > +--error ER_SYNTAX_ERROR > +BINLOG DEFRAGMENT(@binlog_fragment_0, @binlog_fragment_1); > + > +# Not existing fragment is not allowed > +SET @binlog_fragment_0='012345'; > +--error ER_BASE64_DECODE_ERROR > +BINLOG DEFRAGMENT(@binlog_fragment_0, @binlog_fragment_not_exist); > + > +--echo # Cleanup > +DROP TABLE t; > diff --git a/mysys/mf_iocache2.c b/mysys/mf_iocache2.c > index 2499094037d..6b7ff8a7568 100644 > --- a/mysys/mf_iocache2.c > +++ b/mysys/mf_iocache2.c > @@ -22,13 +22,23 @@ > #include <stdarg.h> > #include <m_ctype.h> > > -/* > +/** > Copy contents of an IO_CACHE to a file. > > SYNOPSIS > - my_b_copy_to_file() > - cache IO_CACHE to copy from > - file File to copy to > + my_b_copy_to_file_frag > + > + cache IO_CACHE to copy from > + file File to copy to > + n_frag # of fragments > + > + Other arguments represent format strings to enable wrapping > + of the fragments and their total, including > + > + before_frag before a fragment > + after_frag after a fragment > + after_last_frag after all the fragments > + final_per_frag in the end per each fragment if you edit a comment, change it to Doxygen syntax. Especially, as you start it with Doxygen's /**, it is simply invalid to have SYNOPSIS and DESCRIPTION inside. > > DESCRIPTION > Copy the contents of the cache to the file. The cache will be > @@ -38,33 +48,120 @@ > If a failure to write fully occurs, the cache is only copied > partially. > > - TODO > - Make this function solid by handling partial reads from the cache > - in a correct manner: it should be atomic. > + The copying is made in so many steps as the number of fragments as > + specified by the parameter 'n_frag'. Each step is wrapped with > + writing to the file 'before_frag' and 'after_frag' formated > + strings, unless the parameters are NULL. In the end, optionally, > + first 'after_last_frag' string is appended to 'file' followed by > + 'final_per_frag' per each fragment. > + final item. > > RETURN VALUE > 0 All OK > 1 An error occurred > + > + TODO > + Make this function solid by handling partial reads from the cache > + in a correct manner: it should be atomic. > */ > int > -my_b_copy_to_file(IO_CACHE *cache, FILE *file) > +my_b_copy_to_file_frag(IO_CACHE *cache, FILE *file, > + uint n_frag, > + const char* before_frag, > + const char* after_frag, > + const char* after_last, > + const char* final_per_frag, > + char* buf) > { > - size_t bytes_in_cache; > - DBUG_ENTER("my_b_copy_to_file"); > + size_t bytes_in_cache; // block, may have short size in the last one > + size_t written_off_last_block; // consumed part of the block by last fragment > + size_t total_size= my_b_tell(cache); > + size_t frag_size= total_size / n_frag + 1; > + size_t total_written= 0; > + size_t frag_written; // bytes collected in the current fragment > + uint i; > + > + DBUG_ENTER("my_b_copy_to_file_frag"); > + > + DBUG_ASSERT(cache->type == WRITE_CACHE); > > /* Reinit the cache to read from the beginning of the cache */ > if (reinit_io_cache(cache, READ_CACHE, 0L, FALSE, FALSE)) > DBUG_RETURN(1); > - bytes_in_cache= my_b_bytes_in_cache(cache); > - do > - { > - if (my_fwrite(file, cache->read_pos, bytes_in_cache, > - MYF(MY_WME | MY_NABP)) == (size_t) -1) > - DBUG_RETURN(1); > - } while ((bytes_in_cache= my_b_fill(cache))); > - if(cache->error == -1) > - DBUG_RETURN(1); > - DBUG_RETURN(0); > + > + for (i= 0, written_off_last_block= 0, bytes_in_cache= my_b_bytes_in_cache(cache); > + i < n_frag; > + i++, total_written += frag_written) > + { > + frag_written= 0; > + if (before_frag) > + { > + sprintf(buf, before_frag, i); > + my_fwrite(file, (uchar*) buf, strlen(buf), MYF(MY_WME | MY_NABP)); > + } > + do > + { > + /* > + Either the current block is the last (L) in making the > + current fragment (and possibly has some extra not to fit (LG) into > + the fragment), or (I) the current (whole then) block is > + intermediate. > + */ > + size_t block_to_write= (frag_written + bytes_in_cache >= frag_size) ? > + frag_size - frag_written : bytes_in_cache; > + > + DBUG_ASSERT(n_frag != 1 || > + (block_to_write == bytes_in_cache && > + written_off_last_block == 0)); > + > + if (my_fwrite(file, cache->read_pos + written_off_last_block, > + block_to_write, > + MYF(MY_WME | MY_NABP)) == (size_t) -1) > + /* no cache->error is set here */ > + DBUG_RETURN(1); > + > + frag_written += block_to_write; > + if (frag_written == frag_size) // (L) > + { > + DBUG_ASSERT(block_to_write <= bytes_in_cache); > + written_off_last_block= block_to_write; > + bytes_in_cache -= written_off_last_block; // (LG) when bytes>0 > + /* > + Nothing should be left in cache at the end of the > + last fragment construction. > + */ > + DBUG_ASSERT(i != n_frag - 1 || bytes_in_cache == 0); > + > + break; > + } > + else > + { > + written_off_last_block= 0; // (I) > + } > + } while ((bytes_in_cache= my_b_fill(cache))); > + > + if (after_frag) > + { > + sprintf(buf, after_frag, NULL); > + my_fwrite(file, (uchar*) buf, strlen(buf), MYF(MY_WME | MY_NABP)); > + } > + } > + > + DBUG_ASSERT(total_written == total_size); // output == input > + > + if (after_last) > + { > + sprintf(buf, after_last, n_frag); > + my_fwrite(file, (uchar*) buf, strlen(buf), MYF(MY_WME | MY_NABP)); > + } > + > + for (i= 0; final_per_frag && i < n_frag ; i++) > + { > + sprintf(buf, final_per_frag, i); > + my_fwrite(file, (uchar*) buf, strlen(buf), MYF(MY_WME | MY_NABP)); > + } > + > + DBUG_RETURN(cache->error == -1); this is a big and very specialized function with non-standard indentation. Please, move it from mysys to mysqlbinlog.cc and fix the indentation. > } > > > diff --git a/sql/log_event.cc b/sql/log_event.cc > index e1912ad4620..0c39200148f 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -10474,12 +10482,108 @@ void Rows_log_event::pack_info(Protocol *protocol) > #endif > > #ifdef MYSQL_CLIENT > +void copy_cache_to_file_wrapped(FILE *file, > + PRINT_EVENT_INFO *print_event_info, > + IO_CACHE *body, > + bool do_print_encoded) > +{ > + uint n_frag= 1; > + const char* before_frag= NULL; > + char* after_frag= NULL; > + char* after_last= NULL; > + char* final_per_frag= NULL; > + /* > + 2 fragments can always represent near 1GB row-based > + base64-encoded event as two strings each of size less than > + max(max_allowed_packet). Greater number of fragments does not > + save from potential need to tweak (increase) @@max_allowed_packet > + before to process the fragments. So 2 is safe and enough. > + */ > + const char fmt_last_frag2[]= > + "\nBINLOG DEFRAGMENT(@binlog_fragment_0, @binlog_fragment_1)%s\n"; > + const char fmt_last_per_frag[]= "\nSET @binlog_fragment_%%d = NULL%s\n"; this is completely unnecessary "generalization", print simply "SET @binlog_fragment_0=NULL,binlog_fragment_1=NULL%s\n" in particular, as your "BINLOG DEFRAGMENT" statement is not generalized. (or to the cleanup inside BINLOG, as discussed above). > + const char fmt_before_frag[]= "\nSET @binlog_fragment_%d ='\n"; > + /* > + Buffer to pass to copy_cache_frag_to_file_and_reinit to > + compute formatted strings according to specifiers. > + The sizes may depend on an actual fragment number size in terms of decimal > + signs so its maximum is estimated (not precisely yet safely) below. > + */ > + char buf[(sizeof(fmt_last_frag2) + sizeof(fmt_last_per_frag)) > + + ((sizeof(n_frag) * 8)/3 + 1) // decimal index > + + sizeof(print_event_info->delimiter) + 3]; // delim, \n and 0. > + > + if (do_print_encoded) > + { > + after_frag= (char*) my_malloc(sizeof(buf), MYF(MY_WME)); > + sprintf(after_frag, "'%s\n", print_event_info->delimiter); > + if (my_b_tell(body) > opt_binlog_rows_event_max_encoded_size) > + n_frag= 2; > + if (n_frag > 1) > + { > + before_frag= fmt_before_frag; > + after_last= (char*) my_malloc(sizeof(buf), MYF(MY_WME)); > + sprintf(after_last, fmt_last_frag2, (char*) print_event_info->delimiter); > + final_per_frag= (char*) my_malloc(sizeof(buf), MYF(MY_WME)); > + sprintf(final_per_frag, fmt_last_per_frag, > + (char*) print_event_info->delimiter); > + } > + else > + { > + before_frag= "\nBINLOG '\n"; > + } > + } > + if (copy_cache_frag_to_file_and_reinit(body, file, n_frag, > + before_frag, after_frag, > + after_last, final_per_frag, buf)) 1. get rid of this copy_cache_frag_to_file_and_reinit function, it's not helping. 2. instead of pushing all this complex logic of printing fragments with before and after and in-between and start and final, it'd be *much* simpler to do it here and just extend my_b_copy_to_file() to print at most N bytes. > + { > + body->error= -1; > + goto err; > + } > + > +err: > + my_free(after_frag); > + my_free(after_last); > + my_free(final_per_frag); > +} > + > +/* > + The function invokes base64 encoder to run on the current > + event string and store the result into two caches. > + When the event ends the current statement the caches are is copied into > + the argument file. > + Copying is also concerned how to wrap the event, specifically to produce > + a valid SQL syntax. > + When the encoded data size is within max(MAX_ALLOWED_PACKET) > + a regular BINLOG query is composed. Otherwise it is build as fragmented > + > + SET @binlog_fragment_0='...'; > + SET @binlog_fragment_1='...'; > + BINLOG DEFRAGMENT(@binlog_fragment_0, @binlog_fragment_1); > + > + where fragments are represented by a sequence of "indexed" user > + variables. > + Two more statements are composed as well > + > + SET @binlog_fragment_0=NULL; > + SET @binlog_fragment_1=NULL; > + > + to promptly release memory. > + > + NOTE. > + If any changes made don't forget to duplicate them to > + Old_rows_log_event as long as it's supported. > +*/ > void Rows_log_event::print_helper(FILE *file, > PRINT_EVENT_INFO *print_event_info, > char const *const name) > { > IO_CACHE *const head= &print_event_info->head_cache; > IO_CACHE *const body= &print_event_info->body_cache; > + bool do_print_encoded= > + print_event_info->base64_output_mode != BASE64_OUTPUT_DECODE_ROWS && > + !print_event_info->short_form; > + > if (!print_event_info->short_form) > { > bool const last_stmt_event= get_flags(STMT_END_F); > diff --git a/sql/sql_binlog.cc b/sql/sql_binlog.cc > index 91cf038907e..b15d72e036a 100644 > --- a/sql/sql_binlog.cc > +++ b/sql/sql_binlog.cc > @@ -28,6 +28,73 @@ > // START_EVENT_V3, > // Log_event_type, > // Log_event > + > +/* > + Copy fragments into the standard placeholder thd->lex->comment.str. > + > + Compute the size of the (still) encoded total, > + allocate and then copy fragments one after another. > + The size can exceed max(max_allowed_packet) which is not a > + problem as no String instance is created off this char array. > + > + Return 0 at success, -1 otherwise. > +*/ > +int binlog_defragment(THD *thd) > +{ > + LEX_STRING *curr_frag_name; > + > + thd->lex->comment.length= 0; > + thd->lex->comment.str= NULL; > + /* compute the total size */ > + for (uint i= 0; i < thd->lex->fragmented_binlog_event.n_frag; i++) > + { > + user_var_entry *entry; > + > + curr_frag_name= &thd->lex->fragmented_binlog_event.frag_name[i]; > + entry= > + (user_var_entry*) my_hash_search(&thd->user_vars, > + (uchar*) curr_frag_name->str, > + curr_frag_name->length); > + if (!entry || entry->type != STRING_RESULT) > + { > + my_printf_error(ER_BASE64_DECODE_ERROR, > + "%s: BINLOG fragment user " > + "variable '%s' has unexpectedly no value", MYF(0), > + ER_THD(thd, ER_BASE64_DECODE_ERROR), curr_frag_name->str); > + return -1; > + } > + thd->lex->comment.length += entry->length; > + } > + thd->lex->comment.str= // to be freed by the caller > + (char *) my_malloc(thd->lex->comment.length, MYF(MY_WME)); > + if (!thd->lex->comment.str) > + { > + my_error(ER_OUTOFMEMORY, MYF(ME_FATALERROR), 1); > + return -1; > + } > + > + /* fragments are merged into allocated buf */ > + size_t gathered_length= 0; > + for (uint i= 0; i < thd->lex->fragmented_binlog_event.n_frag; i++) > + { > + user_var_entry *entry; > + > + curr_frag_name= &thd->lex->fragmented_binlog_event.frag_name[i]; > + entry= > + (user_var_entry*) my_hash_search(&thd->user_vars, > + (uchar*) curr_frag_name->str, > + curr_frag_name->length); > + memcpy(thd->lex->comment.str + gathered_length, > + entry->value, entry->length); > + > + gathered_length += entry->length; > + } > + DBUG_ASSERT(gathered_length == thd->lex->comment.length); ok. 1. BINLOG uses LEX::comment to avoid increasing sizeof(LEX) for a rarely used statement. So don't increase it for a much-more-rarely used event overlow case. That is, remove fragmented_binlog_event. You can use a pair of LEX_STRING's in LEX (e.g. comment/ident) or (less hackish) List<LEX_STRING>, which are plenty in LEX. 2. Don't look up variables twice, remember them in the first loop. You can unroll it too, there're only two variables anyway. E.g. the second loop can be replaced by just memcpy(thd->lex->comment.str, var[0]->value, var[0]->length); memcpy(thd->lex->comment.str + var[0]->length, var[1]->value, var[1]->length); or even shorter, if you start your function with LEX_STRING *out= &thd->lex->comment; 3. Not "unexpectedly no value". ER_WRONG_TYPE_FOR_VAR looks more appropriate there. > + > + return 0; > +} > + > + > /** > Execute a BINLOG statement. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 4

[Maria-developers] MDEV-13912
by Michal Schorm 22 Oct '18

22 Oct '18

Hello, can I get some status update on MDEV-13912 please? It is still an issue for the customer on my side and I'd love to see fixed it in 5.5.62 release. Michal -- Michal Schorm Associate Software Engineer Core Services - Databases Team Red Hat

2 2

Re: [Maria-developers] cf9bafc: MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings
by Sergei Golubchik 19 Oct '18

19 Oct '18

Hi, Sergey! On Oct 19, Sergey Vojtovich wrote: > revision-id: cf9bafc8cb0943c914d118525888140ea15e6b55 (mariadb-10.0.36-50-gcf9bafc) > parent(s): 8e716138cef2d9be603cdf71701d82bcb72dfd69 > author: Sergey Vojtovich > committer: Sergey Vojtovich > timestamp: 2018-10-18 23:50:50 +0400 > message: > > MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings > in thr_lock / has_old_lock upon FLUSH TABLES > > Explicit partition access of partitioned MEMORY table under LOCK TABLES > may cause subsequent statements to crash the server, deadlock, trigger > valgrind warnings or ASAN errors. Freed memory was being used due to > incorrect cleanup. > > At least MyISAM and InnoDB don't seem to be affected, since their > THR_LOCK structures don't survive FLUSH TABLES. MEMORY keeps table shared > data (including THR_LOCK) even if there're no open instances. > > There's partition_info::lock_partitions bitmap, which holds bits of > partitions allowed to be accessed after pruning. This bitmap is > updated for each individual statement. > > This bitmap was abused in ha_partition::store_lock() such that when we > need to unlock a table, locked by LOCK TABLES, only locks for partitions > that were accessed by previous statement were released. > > Eventually FLUSH TABLES frees THR_LOCK_DATA objects, which are still > linked into THR_LOCK lists. When such THR_LOCK gets reused we end up with > freed memory access. > > Fixed by using ha_partition::m_locked_partitions bitmap similarly to > ha_partition::external_lock(). Thanks, good comment. > Some unused code removed. Generally, it's better to do that in a separate commit. It's very easy to do with git citool, and doesn't requite any advance planning, you just edit whatever you like and then run 'git citool' twice, selecting what lines you want to commit each time. > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 0488ebf..02de92a 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -3907,9 +3907,14 @@ THR_LOCK_DATA **ha_partition::store_lock(THD *thd, > } > else > { > - for (i= bitmap_get_first_set(&(m_part_info->lock_partitions)); > + MY_BITMAP *used_partitions= lock_type == TL_UNLOCK || > + lock_type == TL_IGNORE ? why TL_IGNORE too? external_lock only checks for TL_UNLOCK. > + &m_locked_partitions : > + &m_part_info->lock_partitions; > + > + for (i= bitmap_get_first_set(used_partitions); > i < m_tot_parts; > - i= bitmap_get_next_set(&m_part_info->lock_partitions, i)) > + i= bitmap_get_next_set(used_partitions, i)) > { > DBUG_PRINT("info", ("store lock %d iteration", i)); > to= m_file[i]->store_lock(thd, to, lock_type); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] 7ce7184cf0e: MDEV-17133 dump thread reads from a past position
by Sergei Golubchik 19 Oct '18

19 Oct '18

Hi, Andrei! ok to push. But please add a meaningful commit comment. Thanks! On Oct 19, Andrei Elkin wrote: > revision-id: 7ce7184cf0e354a1c9114bb31df1fb63b1fbbcc7 (mariadb-10.1.35-59-g7ce7184cf0e) > parent(s): d3a8b5aa9cee24a6397662e30df2e915f45460e0 > author: Andrei Elkin <andrei.elkin(a)mariadb.com> > committer: Andrei Elkin <andrei.elkin(a)mariadb.com> > timestamp: 2018-09-18 23:21:18 +0300 > message: > > MDEV-17133 dump thread reads from a past position > > --- > mysys/mf_iocache.c | 8 +++-- > unittest/sql/mf_iocache-t.cc | 74 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 79 insertions(+), 3 deletions(-) > > diff --git a/mysys/mf_iocache.c b/mysys/mf_iocache.c > index 56b1ae3fc6e..8f829a8c9c2 100644 > --- a/mysys/mf_iocache.c > +++ b/mysys/mf_iocache.c > @@ -563,7 +563,7 @@ int _my_b_write(IO_CACHE *info, const uchar *Buffer, size_t Count) > > int _my_b_cache_read(IO_CACHE *info, uchar *Buffer, size_t Count) > { > - size_t length, diff_length, left_length= 0, max_length; > + size_t length= 0, diff_length, left_length= 0, max_length; > my_off_t pos_in_file; > DBUG_ENTER("_my_b_cache_read"); > > @@ -668,7 +668,11 @@ int _my_b_cache_read(IO_CACHE *info, uchar *Buffer, size_t Count) > else > { > info->error= 0; > - DBUG_RETURN(0); /* EOF */ > + if (length == 0) > + { > + DBUG_RETURN(0); // EOF > + } > + length= 0; > } > } > else if ((length= mysql_file_read(info->file,info->buffer, max_length, Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0