- developers - lists.mariadb.org

Re: [Maria-developers] bc55e3c438f: MDEV-4742 - provide function to sort string in "natural" order
by Sergei Golubchik 20 Aug '21

20 Aug '21

Hi, Vladislav! On Aug 20, Vladislav Vaintroub wrote: > commit bc55e3c438f > Author: Vladislav Vaintroub <wlad(a)mariadb.com> > Date: Thu Jul 29 13:28:11 2021 +0200 > > MDEV-4742 - provide function to sort string in "natural" order > > The numbers should be compared as numbers, while the rest should be compared > as string. > > Introduce natural_sort_key() function that transforms original string > so that the lexicographic order of such keys is suitable for > natural sort. This comment doesn't mention the second optional argument. Neither does Jira. > diff --git a/mysql-test/main/natural_sort_key.test b/mysql-test/main/natural_sort_key.test > new file mode 100644 > index 00000000000..bee0d8e76bd > --- /dev/null > +++ b/mysql-test/main/natural_sort_key.test > @@ -0,0 +1,64 @@ > +--source include/have_innodb.inc > +--source include/have_sequence.inc > + > +SET NAMES utf8mb4; > +SELECT NATURAL_SORT_KEY(NULL); > + > +# Sort without tables > +SELECT '' c WHERE 0 UNION VALUES('a10'),('a9'),('a1000'), ('a0'),('b'),('b0') ORDER BY NATURAL_SORT_KEY(c); > + > +#Test that max packet overflow produces NULL plus warning > +SELECT NATURAL_SORT_KEY(repeat('a1',@@max_allowed_packet/2-1)); > + > +#Test with virtual(index only) key > +CREATE TABLE t1( > + c VARCHAR(30) CHARACTER SET latin1 COLLATE latin1_bin, > + k VARCHAR(45) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci AS (NATURAL_SORT_KEY(CONVERT(c USING utf8mb4))) INVISIBLE, > + KEY(k,c)) ENGINE=InnoDB; > +INSERT INTO t1 values > + ('A1'),('a1'),('A100'),('a100'),('A2'),('ä2'),('a2'),('A99'), > + ('äb'),('B1'),('B100'),('B9'),('C'),('100'); > +EXPLAIN SELECT c FROM t1 ORDER BY k,c; > +-- echo #Natural sort order. > +# We sort by 2 colums, for stable sort,as we do not currenly have a case and accent insensitive Unicode collation. > +SELECT c FROM t1 ORDER BY k,c; > +-- echo #Unnatural but unicode aware) sort order > +SELECT c FROM t1 ORDER BY CONVERT(c USING utf8mb4) COLLATE utf8mb4_unicode_ci,c; > +# CREATE TABLE AS SELECT, to see that length of the column is correct. > +CREATE TABLE t2 AS SELECT c, NATURAL_SORT_KEY(c) FROM t1 WHERE 0; you could've simply used --enable_metadata > +SHOW CREATE TABLE t2; > +DROP TABLE t1,t2; > + > +#Show encoding of numbers, including fractions, and leading whitespace. > +SELECT RPAD(val,21,' ') value , RPAD(NATURAL_SORT_KEY(val),26,' ') sortkey , LENGTH(NATURAL_SORT_KEY(val)) - LENGTH(val) encoding_overhead > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.0'),('0.1'), ('0,15'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1'),('01'),('0001') > +UNION SELECT CONCAT('1',repeat('0',seq)) FROM seq_1_to_20 > +) AS numbers ORDER BY sortkey; > + > +--echo # Disable fractions handling by passing NULL as second parameter to NATURAL_SORT_KEY > +SELECT val value > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.1'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1'), ('0,1'),('1,001'),('1,002'),('1,010'),('1,02'),('1,1'),('1,3'),('1')) > +AS numbers ORDER BY NATURAL_SORT_KEY(val, NULL); > + > +--echo # Use ',' as decimal separator for NATURAL_SORT_KEY > +SELECT val value, NATURAL_SORT_KEY(val,',') sortkey > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0,1'),('1,001'),('1,002'),('1,010'),('1,02'),('1,1'),('1,3'),('1')) > +AS numbers ORDER BY sortkey; > + > +--echo # Use '.' as decimal separator for NATURAL_SORT_KEY > +SELECT val value,NATURAL_SORT_KEY(val,'.') sortkey > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.1'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1')) > +AS numbers ORDER BY sortkey; > +SET NAMES DEFAULT; > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 7e8ff667e75..a2efd414750 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -5293,6 +5293,232 @@ String *Item_temptable_rowid::val_str(String *str) > return &str_value; > } > > +/** > + Here is how we generate a key suitable for lexicographic comparison > + from a numeric string (representing positive number). > + > + We prepend variable length prefix, that only encodes string length > + The longer the string, the lexicographically larger is the prefix. > + > + Rules for generating prefix is following. > + > + 1. Small length (1 to 9) > + Prefix is single char '0' + length -1 > + This gives the range '0' - '8' > + 2. Medium length(10 to 18) > + Prefix is 2 chars - concat('9', '0'+length-10)' > + This gives range '90'-'98' > + 3. Large length(19) > + Prefix is "990" > + 4. Huge length (20 to 2^32-1) > + Prefix stars with '99', then log10 of the length is added as char > + then the number itself is added > + The range is '99120' - '9994294967295' please also explain here how you handle leading zeros > +*/ > + > +#define NATSORT_BUFSIZE 16 > +static size_t natsort_num2str(size_t n, char *s) > +{ > + if (n < 10) > + { > + s[0]= '0' + (char)n -1; > + return 1; > + } > + > + if (n < 19) > + { > + s[0]= '9'; > + s[1]= '0'+ (char)n- 10; > + return 2; > + } > + > + if (n == 19) > + { > + s[0]= '9'; > + s[1]= '9'; > + s[2]= '0'; > + return 3; > + } > + > + /* Here, n > 19*/ > + s[0]= '9'; > + s[1]= '9'; > + size_t log10n= 0; > + for (size_t tmp= n/10; tmp; tmp /= 10) > + log10n++; > + DBUG_ASSERT(log10n && log10n < 10); first, I don't see where the caller ensures that log10n < 10 (in other words, that n < 9999999999) second, please, never write assert(a && b), always do two separate asserts, so that when it fires, it'd be clear what condition was false > + s[2]='0' + (char)log10n; > + return ll2str(n,s+3,10,0) - s; > +} > + > +/** > + Convert a string to natural sort key. > + @param[in] in - input string > + @param[out] s - output string > + > + We assume that memory is preallocated for the output > + string, so that appends do not fail. > + > + The lead zeros count, if positive, is stored after > + the numeric string. > + > + Fractional parts are stored and compared as strings. > + Even if they have numbers in them, they should just > + be compared as strings. > +*/ > +static void to_natsort_key(const String *in, String *s, my_wc_t decimal_sep) > +{ > + CHARSET_INFO *cs= in->charset(); > + uchar *pos= (uchar *) in->ptr(); > + uchar *end= pos + in->length(); > + size_t num_len= 0; > + size_t leading_zeros= 0; > + uchar *num_start= nullptr; > + bool in_fraction= false; > + size_t fraction_len=0; > + > + for (;;) > + { > + my_wc_t c= 0; > + int charlen= cs->mb_wc(&c, pos, end); > + bool is_digit= charlen >= 1 && (c >= '0' && c <= '9'); what do you mean `>= 1` ? how can it be `> 1` and still satisfy the secon part of the condition? > + > + if (!is_digit && (num_start || leading_zeros)) > + { > + /* > + Handle end of digits run. > + > + Write prefix, natsort_num2str(length) is without leading zeros > + Write numeric value, without leading zeros > + If there were leading zeros, write variable length suffix > + */ > + char buf[NATSORT_BUFSIZE]; > + if (!num_len) > + { > + // number was zeros only > + leading_zeros--; > + num_len= 1; > + } > + > + s->append(buf, natsort_num2str(num_len, buf)); > + if (!num_start) > + s->append('0'); > + else > + s->append((const char *) num_start, pos - num_start, cs); > + > + if (leading_zeros) > + s->append(buf, natsort_num2str(leading_zeros, buf)); hmm. How would it sort "1;", "001;", "1a" ? > + > + /* Reset state.*/ > + num_len= 0; > + num_start= nullptr; > + leading_zeros= 0; > + in_fraction= c == decimal_sep; I don't really like this idea. First, see https://natsort.readthedocs.io/en/master/index.html there are many ways one can potentially want to customize natsort. (of course, none of them have to be in this MDEV) One of those customizations, indeed, is treating floating point numbers as floating point numbers. But comma vs dot is locale thing, and a one string argument it's not how you should tell the function what decimal separator the current locale uses. So. Simple approach - remove the second argument, no floating point support. Complex approach - have a second argument that enables floating point support, but that can be used in the future to specify more customizations, not that is hard-wired to specify decimal separator duplicating and bypassing the locale. > + fraction_len= 0; > + } > + > + if (charlen < 1) > + break; > + > + if (is_digit && !in_fraction) > + { > + /* Do not count leading zeros in num_len, and num_pos */ > + if (c != '0' || num_len) > + { > + num_len++; > + if (!num_start) > + num_start= pos; > + } > + else > + leading_zeros++; > + } > + else > + { > + /* Append a non-digit, or fractional part of the number (handling the same as string) */ > + s->append((const char *) pos, charlen); > + if (in_fraction) > + { > + if (!is_digit && fraction_len) > + in_fraction= false; > + fraction_len++; > + } > + } > + pos+= charlen; > + } > +} > + > + > +String *Item_func_natural_sort_key::val_str(String *s) > +{ > + if (args[0]->is_null()) > + { > + null_value= true; > + return nullptr; > + } > + > + String *in= args[0]->val_str(); > + CHARSET_INFO *cs= in->charset(); > + ulong max_allowed_packet= current_thd->variables.max_allowed_packet; > + size_t reserve_length= (size_t) in->length() + in->length() / 2 + 2; > + if (s->alloc((uint32)reserve_length)) > + goto error_exit; > + s->length(0); > + s->set_charset(cs); > + > + to_natsort_key(in, s, decimal_separator()); > + > + DBUG_ASSERT(s->length() < reserve_length); > + > + if (s->length() > max_allowed_packet) better check this inside to_natsort_key(). max_allowed_packet is supposed to prevent the string from growing too large, instead of just letting it grow and erroring out afterwards. > + { > + push_warning_printf(current_thd, Sql_condition::WARN_LEVEL_WARN, > + ER_WARN_ALLOWED_PACKET_OVERFLOWED, > + ER(ER_WARN_ALLOWED_PACKET_OVERFLOWED), func_name(), > + max_allowed_packet); > + goto error_exit; > + } > + return s; > + > +error_exit: > + null_value=true; > + return nullptr; > +} > + > + > +my_wc_t Item_func_natural_sort_key::decimal_separator() > +{ > + if (m_decimal_separator != DECIMAL_SEP_UNDEFINED) > + return m_decimal_separator; > + > + DBUG_ASSERT(arg_count > 1); > + String *s= args[1]->val_str(); > + if (s && s->length()) > + { > + my_wc_t c; > + if (s->charset()->mb_wc(&c, (const uchar *) s->ptr(), > + (const uchar *) s->end())) > + return c; > + } > + return DECIMAL_SEP_NONE; > +} > + > +bool Item_func_natural_sort_key::fix_length_and_dec(void) > +{ > + if (agg_arg_charsets_for_string_result(collation, args, 1)) > + return true; > + DBUG_ASSERT(collation.collation != NULL); > + uint32 max_clen= args[0]->max_char_length(); > + fix_char_length(max_clen + max_clen/2 + ((max_clen <= 3)?1:0)); > + set_maybe_null(); better, only set maybe_null if the first argument maybe_null or the first argument maybe_too_long > + > + if (arg_count> 1 && args[1]->can_eval_in_optimize()) > + { > + /* Initialize decimal separator */ > + m_decimal_separator= decimal_separator(); > + } > + return false; > +} > + > #ifdef WITH_WSREP > > #include "wsrep_mysqld.h" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a19495728de: MDEV-16355 Add option for mysqldump to read data as of specific timestamp from system-versioned tables
by Sergei Golubchik 15 Aug '21

15 Aug '21

Hi, Aleksey! I'm reviewing it separately from other commits in the branch, because it looks to be functionally independent from, say, MDEV-16029 mysqldump: dump and restore historical data and doesn't have to wait for the latter to be pushed first. I think I was able to guess what the patch would look like, if you'd apply it to vanilla 10.7 without MDEV-16029. But just to be sure, please, let me look it over again after the cherry-pick. Otherwise no comments code-wise. A couple of comments related to naming, see below. On Aug 15, Aleksey Midenkov wrote: > revision-id: a19495728de (versioning-1.0.5-45-ga19495728de) > parent(s): be4605ae086 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2018-07-19 14:47:35 +0300 > message: > > MDEV-16355 Add option for mysqldump to read data as of specific timestamp from system-versioned tables > > mysqldump changes: > > * --asof-timestamp option specifies historical point; "asof" isn't a word. I'd suggest --as-of-timestamp or simply --as-of > * with --dump-history it dumps all prior history, without it dumps only single point in time; > * query forging protection for --asof-timestamp parameter. > > diff --git a/client/mysqldump.c b/client/mysqldump.c > index eee12555271..f426e8c58ef 100644 > --- a/client/mysqldump.c > +++ b/client/mysqldump.c > @@ -1099,6 +1102,13 @@ static int get_options(int *argc, char ***argv) > my_progname_short); > return(EX_USAGE); > } > + if (opt_asof_timestamp && strchr(opt_asof_timestamp, '\'')) > + { > + fprintf(stderr, > + "%s: --asof-timestamp wrong argument!\n", > + my_progname_short); May be, use the wording that is closer to the error one would get from any other invalid character, like, if one would do --asof-timestamp='!@#$%' That is, instead of "wrong argument", something like fprintf(stderr, "%s: Incorrect DATETIME value: '%s'\n", opt_asof_timestamp, my_progname_short); Because MariaDB [test]> select timestamp'2010!@#$%'; ERROR 1525 (HY000): Incorrect DATETIME value: '2010!@#$%' > + return(EX_USAGE); > + } > if (strcmp(default_charset, charset_info->csname) && > !(charset_info= get_charset_by_csname(default_charset, > MY_CS_PRIMARY, MYF(MY_WME)))) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] cdb6db8ce38: MDEV-25444 mysql --binary-mode is not able to replay some mysqlbinlog outputs
by Sergei Golubchik 15 Aug '21

15 Aug '21

Hi, Sachin! On Jul 12, Sachin Kumar wrote: > revision-id: cdb6db8ce38 (mariadb-10.3.26-155-gcdb6db8ce38) > parent(s): 72753d2b65f > author: Sachin Kumar <sachin.setiya(a)mariadb.com> > committer: Sachin Kumar <sachin.setiya(a)mariadb.com> > timestamp: 2021-05-19 15:46:57 +0100 > message: > > MDEV-25444 mysql --binary-mode is not able to replay some mysqlbinlog outputs > > Problem:- Some binary data is inserted into the table using Jconnector. When > binlog dump of the data is applied using mysql cleint it gives syntax error. > > Reason:- > After investigating it turns out to be a issue of mysql client not able to properly > handle \\\0 <0 in binary>. In all binary files where mysql client fails to insert > these 2 bytes are commom (0x5c00) > > Solution:- > I have changed mysql.cc to include for the possibility that binary string can > have \\\0 in it. > > --- > client/mysql.cc | 8 ++++++-- > mysql-test/main/binary_zero_insert.result | 6 ++++++ > mysql-test/main/binary_zero_insert.test | 15 +++++++++++++++ > mysql-test/std_data/binary_zero_insert.bin | Bin 0 -> 87 bytes > 4 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/client/mysql.cc b/client/mysql.cc > index 2a7c4eaf3e5..8530c105820 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -2319,8 +2319,12 @@ static bool add_line(String &buffer, char *line, size_t line_length, > { > // Found possbile one character command like \c > > + inchar = (uchar) *++pos; > + // In Binary mode , when in_string is not null \0 should not be treated as > + // end statement. This can happen when we are in middle of binary data which > + // can contain \0 and its quoted with ' '. > + if (!real_binary_mode && !*in_string && !inchar) Two comments here. 1. if you check the history where this "real_binary_mode" came from, you'd arrive at commit 2db4392bf4cb and the test file mysql_binary_mode.test So, I'd say it make sense to add your test there too. And in the same style, wthout a separate binary file. in my experimening I did it as simple as --- a/mysql-test/main/mysql_binary_mode.test +++ b/mysql-test/main/mysql_binary_mode.test @@ -16,14 +16,14 @@ let $table_name_right= `SELECT 0x410D0A42`; let $table_name_wrong= `SELECT 0x410A42`; # 0x410042 => 'A\0B' -let $char0= `SELECT 0x410042`; +let $char0= `SELECT 0x4100425C0043`; (there already was a test for a \0 byte, but not for an escaped \\\0) but if you'd like you can add a separate test with a header and everything. > - if (!(inchar = (uchar) *++pos)) > - break; // readline adds one '\' 2. What does the old code mean? Why returning if inchar==0 ? What does the comment mean? It's the code from the last millenium, does it even apply now? For example, there can be no \0 bytes in the string, because of if (!real_binary_mode && strlen(line) != line_length) ... in other words, shouldn't we just remove the if? I tried, at least the main suite didn't break. > + break; // readline adds one '\' > if (*in_string || inchar == 'N') // \N is short for NULL > { // Don't allow commands in string > *out++='\\'; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] Pushing down aggregation functions
by Berrocal, Eduardo 04 Aug '21

04 Aug '21

Hello!, Continuing in my series of emails about engine condition pushdown... I wonder if it is possible to push down aggregation functions such as SUM(), MIN(), MAX() or AVG() to the storage engine. Currently, and as far as I can see (I may be wrong here), it is not possible. However, this is something that can be beneficial if the storage engine can perform some of these basic computations itself, like suming over a whole column and returning just the result. The storage engine can even be smart enough to use some hardware advantages like accelerators or computational storage. If you guys think this is indeed possible without drastic code changes, it would be amazing to be pointed at where this is. At first, I would only push down functions if they operate over just one column of the same table. Things can be made more complicated in the future, of course. Thanks a lot!, Eduardo.

2 1

Re: [Maria-developers] 203aa38f9b6: MDEV-26102: gcov 9.1 doesn't support intermediate format any more, but json instead - refactoring needed
by Sergei Golubchik 29 Jul '21

29 Jul '21

Hi, Anel! On Jul 28, Anel Husakovic wrote: > revision-id: 203aa38f9b6 (mariadb-10.2.39-96-g203aa38f9b6) > parent(s): afe00bb7cce > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-07-27 16:09:41 +0200 > message: > > MDEV-26102: gcov 9.1 doesn't support intermediate format any more, but json instead - refactoring needed I've renamed the MDEV to have a shorter title, use the new one in your next commit > > Reviewed by: serg(a)mariadb.com that's not right, I haven't reviewed it yet :) > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index 2c00c64d1ff..e2aadec33e0 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -25,6 +25,8 @@ use warnings; > use Getopt::Long; > use File::Find; > use Cwd qw/realpath/; > +use IO::Uncompress::Gunzip qw(gunzip $GunzipError); > +use JSON::PP; move this down, dcgov should not try to load these modules for for gcc < 9 > my $opt_verbose=0; > my $opt_generate; > @@ -32,6 +34,7 @@ my $opt_help; > my $opt_purge; > my $opt_only_gcov; > my $opt_skip_gcov; > +my $fname; and this > > my %cov; > my $file_no=0; > @@ -62,12 +65,15 @@ my $res; > my $cmd; > if ($opt_purge) > { > - $cmd= "find . -name '*.da' -o -name '*.gcda' -o -name '*.gcov' -o ". > + $cmd= "find . -name '*.da' -o -name '*.gcda*' -o -name '*.gcov' -o ". > "-name '*.dgcov' | grep -v 'README\.gcov' | xargs rm -f ''"; > logv "Running: $cmd"; > system($cmd)==0 or die "system($cmd): $? $!"; > exit 0; > } > + > +my $gcc_version= `gcc -dumpversion`; > +$gcc_version=~ s/(\d).*$/$1/; you don't need this second line, I suspect > > find(\&gcov_one_file, $root); > find(\&write_coverage, $root) if $opt_generate; > @@ -162,6 +168,7 @@ sub gcov_one_file { > } > > # now, read the generated file > + if ($gcc_version <9){ > for my $gcov_file (<$_*.gcov>) { > open FH, '<', "$gcov_file" or die "open(<$gcov_file): $!"; > my $fname; > @@ -183,6 +189,18 @@ sub gcov_one_file { > $cov{$fname}->{$1}+=$2; > } > close(FH); > + } > + } > + else { put both `use` lines here > + my $gcov_file_json; > + gunzip "$_.gcov.json.gz" => \$gcov_file_json or die "gunzip($_.gcov.json.gz): $GunzipError"; > + my $obj= decode_json $gcov_file_json; > + for my $file (@{$obj->{files}}) { > + $fname= $file->{file}; and write `my $fname` here > + for my $line (@{$file->{lines}}){ > + $cov{$fname}->{$line->{line_number}}= $line->{count}; shouldn't it be += $line->{count} ? The point is to accumulate all counters, see how it's done for non-json intermediate format. > + } > + } > } > } > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] ce289840131: MDEV-24511 null field is created with CREATE..SELECT
by Nikita Malyavin 28 Jul '21

28 Jul '21

Hello, Sergei! Looks fine for me. So if you both prefer it more, let's use this variant. Regards, Nikita On Wed, Jul 28, 2021 at 12:08 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > Hi, Nikita! > > On Jul 27, Nikita Malyavin wrote: > > revision-id: ce289840131 (mariadb-10.3.30-31-gce289840131) > > parent(s): b50ea900636 > > author: Nikita Malyavin > > committer: Nikita Malyavin > > timestamp: 2021-07-22 22:38:31 +0300 > > message: > > > > MDEV-24511 null field is created with CREATE..SELECT > > > > The regression has been initially appeared by MDEV-12588 patch > (441349aa). > > > > The reason CREATE... SELECT NULL does not create null field is that > > it calls handle_select(), where the field is created directly from Item, > > by calling type_handler->type_handler_for_tmp_table(), instead of plain > > type_handler. > > > > In UNION case, we'll already have Item_field's containing UNION'ed tmp > > table fields to the moment of select_create::prepare() call. > > > > There, item's type_handler is used directly in > > st_select_lex_unit::join_union_item_types(), instead of > > type_handler_for_tmp_table() or type_handler_for_union(). > > > > Using those two involve too many changes in CREATE...SELECT...UNION > > behavior, which may be reasonably undesired to be pushed in 10.3 > > > > However, patching Field_null::type_handler() directly seems to be > accurate > > enough. > > > > diff --git a/mysql-test/main/union.result b/mysql-test/main/union.result > > index a892f6c9e40..b19427948a6 100644 > > --- a/mysql-test/main/union.result > > +++ b/mysql-test/main/union.result > > @@ -1609,7 +1609,7 @@ NULL binary(0) YES NULL > > CREATE TABLE t5 SELECT NULL UNION SELECT NULL; > > DESC t5; > > Field Type Null Key Default Extra > > -NULL null YES NULL > > +NULL binary(0) YES NULL > > CREATE TABLE t6 > > SELECT * FROM (SELECT * FROM (SELECT NULL)a) b UNION SELECT a FROM t1; > > DESC t6; > > diff --git a/sql/field.h b/sql/field.h > > index a3385736c0a..2af5346bea9 100644 > > --- a/sql/field.h > > +++ b/sql/field.h > > @@ -2526,7 +2526,7 @@ class Field_null :public Field_str { > > :Field_str(ptr_arg, len_arg, null, 1, > > unireg_check_arg, field_name_arg, collation) > > {} > > - const Type_handler *type_handler() const { return &type_handler_null; > } > > + const Type_handler *type_handler() const { return > &type_handler_string; } > > Information_schema_character_attributes > > information_schema_character_attributes() const > > { > > This looks suspiciously risky to me. The problem was specifically in > UNION, but you change the very basic idea of what type the Field_null > is. > > I cannot clearly see what else might this possibly affect, field's type > is used everywhere. > > It would be safer and more logical to fix specifically field creation > for UNION. > > I've discussed it with Bar, who implemented this whole Type_handler > hierarchy, he suggested to use Type_handler::union_element_finalize() > for that. I've attached the patch that does that. > > union_element_finalize() is for 10.4, so I also backported it to 10.3, > it's very straightforward though. > > What do you think? > > Regards, > Sergei > VP of MariaDB Server Engineering > and security(a)mariadb.org > -- Nikita Malyavin, Software Engineer MariaDB Corporation

1 0

Re: [Maria-developers] ce289840131: MDEV-24511 null field is created with CREATE..SELECT
by Sergei Golubchik 28 Jul '21

28 Jul '21

Hi, Nikita! On Jul 27, Nikita Malyavin wrote: > revision-id: ce289840131 (mariadb-10.3.30-31-gce289840131) > parent(s): b50ea900636 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-07-22 22:38:31 +0300 > message: > > MDEV-24511 null field is created with CREATE..SELECT > > The regression has been initially appeared by MDEV-12588 patch (441349aa). > > The reason CREATE... SELECT NULL does not create null field is that > it calls handle_select(), where the field is created directly from Item, > by calling type_handler->type_handler_for_tmp_table(), instead of plain > type_handler. > > In UNION case, we'll already have Item_field's containing UNION'ed tmp > table fields to the moment of select_create::prepare() call. > > There, item's type_handler is used directly in > st_select_lex_unit::join_union_item_types(), instead of > type_handler_for_tmp_table() or type_handler_for_union(). > > Using those two involve too many changes in CREATE...SELECT...UNION > behavior, which may be reasonably undesired to be pushed in 10.3 > > However, patching Field_null::type_handler() directly seems to be accurate > enough. > > diff --git a/mysql-test/main/union.result b/mysql-test/main/union.result > index a892f6c9e40..b19427948a6 100644 > --- a/mysql-test/main/union.result > +++ b/mysql-test/main/union.result > @@ -1609,7 +1609,7 @@ NULL binary(0) YES NULL > CREATE TABLE t5 SELECT NULL UNION SELECT NULL; > DESC t5; > Field Type Null Key Default Extra > -NULL null YES NULL > +NULL binary(0) YES NULL > CREATE TABLE t6 > SELECT * FROM (SELECT * FROM (SELECT NULL)a) b UNION SELECT a FROM t1; > DESC t6; > diff --git a/sql/field.h b/sql/field.h > index a3385736c0a..2af5346bea9 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -2526,7 +2526,7 @@ class Field_null :public Field_str { > :Field_str(ptr_arg, len_arg, null, 1, > unireg_check_arg, field_name_arg, collation) > {} > - const Type_handler *type_handler() const { return &type_handler_null; } > + const Type_handler *type_handler() const { return &type_handler_string; } > Information_schema_character_attributes > information_schema_character_attributes() const > { This looks suspiciously risky to me. The problem was specifically in UNION, but you change the very basic idea of what type the Field_null is. I cannot clearly see what else might this possibly affect, field's type is used everywhere. It would be safer and more logical to fix specifically field creation for UNION. I've discussed it with Bar, who implemented this whole Type_handler hierarchy, he suggested to use Type_handler::union_element_finalize() for that. I've attached the patch that does that. union_element_finalize() is for 10.4, so I also backported it to 10.3, it's very straightforward though. What do you think? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] c34533186ab: MDEV-18734 ASAN heap-use-after-free upon sorting by blob column from partitioned table
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Aleksey! As before, despite the email subject, it's git diff dfa2d0bc13..e533b5fb307 Don't be confused :) On Jul 21, Aleksey Midenkov wrote: > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index c53732a2b51..70ed14448ea 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue() > i= bitmap_get_next_set(&m_part_info->read_partitions, i)) > { > DBUG_PRINT("info", ("init rec-buf for part %u", i)); > - int2store(ptr, i); > + if (table->s->blob_fields) > + { > + for (uint j= 0; j < table->s->blob_fields; ++j, ++objs) > + { > + blob_storage[j]= new (objs) Ordered_blob_storage; > + if (!blob_storage[j]) > + DBUG_RETURN(true); is this possible? > + } > + *((Ordered_blob_storage ***) ptr)= blob_storage; > + blob_storage+= table->s->blob_fields; > + } > + int2store(ptr + sizeof(String **), i); > ptr+= m_priority_queue_rec_len; > } > m_start_key.key= (const uchar*)ptr; > @@ -6291,6 +6333,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() > } > > > +void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore) > +{ > + uint *ptr, *end; > + uint blob_n= 0; > + table->move_fields(table->field, rec_buf, table->record[0]); > + for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields; > + ptr != end; ++ptr, ++blob_n) > + { > + DBUG_ASSERT(*ptr < table->s->fields); > + Field_blob *blob= (Field_blob*) table->field[*ptr]; > + DBUG_ASSERT(blob->flags & BLOB_FLAG); > + DBUG_ASSERT(blob->field_index == *ptr); > + if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null()) > + continue; > + > + Ordered_blob_storage &s= *storage[blob_n]; > + > + if (restore) > + { > + if (!s.blob.is_empty()) > + blob->swap(s.blob, s.set_read_value); don't you need here something like else blob->reset(); to make blob empty? > + } > + else > + { > + bool set_read_value; > + String *cached= blob->cached(set_read_value); > + if (cached) > + { > + cached->swap(s.blob); > + s.set_read_value= set_read_value; > + } > + } > + } > + table->move_fields(table->field, table->record[0], rec_buf); On swap_blobs() you always move all fields from rec_buf to table->record[0]? Why? This looks very suspicious. > +} > + > + > /* > Common routine to handle index_next with ordered results > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] c4835af7e8c: MDEV-16026: Forbid global system_versioning_asof in non-default time zone
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Nikita! I suggest to squash commits from c4835af7e8c to 80e3007a5a1 into one. It'll be simpler, no intermediate juggling with MYSQL_TIME anymore. This squashed commit is ok to push (make sure to mention both MDEVs in the commit comment, but not necessarily in the first line) On Jul 26, Nikita Malyavin wrote: > revision-id: c4835af7e8c (mariadb-10.3.30-30-gc4835af7e8c) > parent(s): ec3289b6de4 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-07-21 00:21:31 +0300 > message: > > MDEV-16026: Forbid global system_versioning_asof in non-default time zone > > * store `system_versioning_asof` in unix time; > * both session and global vars are processed in session timezone; > * setting `default` does not copy global varibale abymore. Instead, it sets > system_time to SYSTEM_TIME_UNSPECIFIED, which means that no 'AS OF' time > is applied and `now()` can be assumed > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a4a5c2d5963: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Rucha! On Jul 26, Rucha Deodhar wrote: > revision-id: a4a5c2d5963 (mariadb-10.5.11-52-ga4a5c2d5963) > parent(s): 675b72d0da1 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-23 19:51:57 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: get_defaults_options() rewrites the value for my_defaults_file, > my_defaults_extra_file and my_defaults_group_suffix to 0. So the config file > can't be read. > Fix: Let handle_options() handle --defaults* option and create a new command > line to pass it to load_defaults(). > > diff --git a/libmariadb b/libmariadb > index 802ce584a26..180c543704d 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit 802ce584a26fdc0ba67fcf35e277bf3c7440956a > +Subproject commit 180c543704d627a50a52aaf60e24ca14e0ec4686 ^^^^^ This is wrong, please, remove it from your commit Otherwise ok to push Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0