- developers - lists.mariadb.org

Re: [Maria-developers] 6a8268b7893: MDEV-28915: mysql_upgrade fails due to old_mode="", with "Cannot load from mysql.proc. The table is probably corrupted"
by Sergei Golubchik 28 Dec '22

28 Dec '22

Hi, Rucha, On Dec 28, Rucha Deodhar wrote: > revision-id: 6a8268b7893 (mariadb-10.6.11-62-g6a8268b7893) > parent(s): 4ca5a0ec98d > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-12-15 17:45:25 +0530 > message: > > MDEV-28915: mysql_upgrade fails due to old_mode="", with "Cannot load from > mysql.proc. The table is probably corrupted" > > Analysis: When mysql_upgrade runs statements for upgrade, characterset is > converted to utf8mb4 because server starts with old_mode that interprets > utf8 to utf8mb4, but mysql.proc table has "utf8mb3" as hardcoded, so > it crashes with corrupted table. > > Fix: Changed Table_check_intact::check() definition to allow both > utf8mb3 and utf8mb4 by checking prefix and changing the upgrade scripts > to explicitly use utf8mb3 > > diff --git a/mysql-test/main/mysql_upgrade-28915.opt b/mysql-test/main/mysql_upgrade-28915.opt > new file mode 100644 > index 00000000000..0be5567a64d > --- /dev/null > +++ b/mysql-test/main/mysql_upgrade-28915.opt > @@ -0,0 +1 @@ > +--old-mode=NO_DUP_KEY_WARNINGS_WITH_IGNORE it's a bit confusing, try to use --old-mode= that is, empty old_mode, otherwise one starts thinking what this bug has to do with NO_DUP_KEY_WARNINGS_WITH_IGNORE. > diff --git a/mysql-test/main/mysql_upgrade-28915.test b/mysql-test/main/mysql_upgrade-28915.test > new file mode 100644 > index 00000000000..61b1fc263cb > --- /dev/null > +++ b/mysql-test/main/mysql_upgrade-28915.test > @@ -0,0 +1,78 @@ > +--source include/mysql_upgrade_preparation.inc > + > +let $MYSQLD_DATADIR= `select @@datadir`; > + > +--echo # > +--echo # Beginning of 10.6 test > +--echo # > +--echo # MDEV-28915: mysql_upgrade fails due to old_mode="", with "Cannot load > +--echo # from mysql.proc. The table is probably corrupted" > + > +--echo # Show that tables are created with utf8mb3 even without UTF8_IS_UTF8MB3 (see the .opt file) I don't think you've shown it. mtr does not recreate system tables for every test. You need to force it to re-bootstrap with one of the options that do it, for example, --innodb-page-size=16k (search mtr for "rebootstrap" for details). But even that won't help, because it'll only add "rebootstrap" options to the server's command line when bootstrapping. May be a combination of .cnf and .opt can do what you want, I didn't try. > + > +SHOW CREATE TABLE mysql.proc; > +SHOW CREATE TABLE mysql.event; > + > +--exec $MYSQL_UPGRADE --force 2>&1 > + > +--remove_file $MYSQLD_DATADIR/mysql_upgrade_info > + > +SHOW CREATE TABLE mysql.proc; > +SHOW CREATE TABLE mysql.event; > + > +--echo # Emulate that tables were created with utf8mb4 by an older version > + > +ALTER TABLE mysql.proc MODIFY db CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci NOT NULL, > + MODIFY name CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci NOT NULL, > + MODIFY specific_name CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY definer VARCHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY comment TEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY character_set_client CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY collation_connection CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL, > + MODIFY db_collation CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci DEFAULT NULL; > + > +ALTER TABLE mysql.event MODIFY db CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '', > + MODIFY name CHAR(64) CHARACTER SET utf8mb4 NOT NULL DEFAULT '', > + MODIFY definer VARCHAR(384) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '', > + MODIFY comment CHAR(64) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL DEFAULT '', > + MODIFY character_set_client CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL, > + MODIFY collation_connection CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL, > + MODIFY db_collation CHAR(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL; > + > +--echo # mysql_upgrade changes columns from utf8mb4 to utf8mb3 > + > +--exec $MYSQL_UPGRADE --force 2>&1 > +--remove_file $MYSQLD_DATADIR/mysql_upgrade_info > + > +--vertical_results > + > +SHOW CREATE TABLE mysql.proc; > +SHOW CREATE TABLE mysql.event; > + > +CREATE TABLE t1 (id1 INT, val1 VARCHAR(5)); > + > +DELIMITER |; > + > +CREATE PROCEDURE sp1 () > + BEGIN > + SELECT val1 FROM t1; > + END| You've made changes in sp.cc and event_db_repository.cc to ensure that these tables are usable even with utf8mb4 charset. But you don't test it if it works. You create a procedure and an event here, *after* tables were converted back to utf8mb3. you likely want to do it earlier, before mysql_upgrade, just after ALTER TABLE. > + > +DELIMITER ;| > + > +SELECT name, body_utf8, body FROM mysql.proc WHERE name like 'sp1'; > +CALL sp1(); > +SELECT name, body_utf8, body FROM mysql.proc WHERE name like 'sp1'; > + > +SET GLOBAL event_scheduler=ON; > + > +SELECT name, body_utf8, body FROM mysql.event; > +CREATE EVENT ev1 ON SCHEDULE EVERY 1 SECOND DO INSERT INTO t1 VALUES (1, 'abc'); > +SELECT name, body_utf8, body FROM mysql.event; > + > +SET GLOBAL event_scheduler=OFF; > +DROP EVENT ev1; > +DROP PROCEDURE sp1; > +DROP TABLE t1; > + > +--echo # end of 10.6 test Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4ddf606debf: MDEV-30151 parse error 1=2 not between/in
by Sergei Golubchik 28 Dec '22

28 Dec '22

Hi, Alexander, I'm not sure I understand everything you did there, so questions below On Dec 28, Alexander Barkov wrote: > revision-id: 4ddf606debf (mariadb-10.3.37-59-g4ddf606debf) > parent(s): 8f30973234d > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-12-13 10:35:16 +0400 > message: > > MDEV-30151 parse error 1=2 not between/in > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 1f6485dac6a..ca614ff63e9 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1687,7 +1687,8 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > > %left PREC_BELOW_NOT > > -%nonassoc NOT_SYM > +/* The precendence of boolean NOT is in fact here. See the comment below. */ > + > %left '=' EQUAL_SYM GE '>' LE '<' NE > %nonassoc IS > %right BETWEEN_SYM > @@ -1699,6 +1700,24 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > %left '*' '/' '%' DIV_SYM MOD_SYM > %left '^' > %left MYSQL_CONCAT_SYM > +/* > + Boolean negation has a special branch in "expr" starting with NOT_SYM. > + The precedence of logical negation is determined by the grammar itself > + (without using Bison terminal symbol precedence) in this order > + - Boolean factor (i.e. logical AND) > + - Boolean NOT > + - Boolean test (such as '=', IS NULL, IS TRUE) > + > + But we also need a precedence for NOT_SYM in other contexts, > + to shift (without reduce) in these cases: > + predicate <here> NOT IN ... > + predicate <here> NOT BETWEEN ... > + predicate <here> NOT LIKE ... > + predicate <here> NOT REGEXP ... > + If the precedence of NOT_SYM was low, it would reduce immediately > + after scanning "predicate" and then produce a syntax error on "NOT". > +*/ > +%nonassoc NOT_SYM > %nonassoc NEG '~' NOT2_SYM BINARY > %nonassoc COLLATE_SYM > > @@ -1938,6 +1957,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > literal insert_ident order_ident temporal_literal > simple_ident expr sum_expr in_sum_expr > variable variable_aux > + boolean_test boolean_factor > predicate bit_expr parenthesized_expr > table_wild simple_expr column_default_non_parenthesized_expr udf_expr > primary_expr string_factor_expr mysql_concatenation_expr > @@ -9840,79 +9860,87 @@ expr: > MYSQL_YYABORT; > } > } > - | NOT_SYM expr %prec NOT_SYM > + | boolean_factor what does that achieve? > + ; > + > +boolean_factor: > + NOT_SYM boolean_factor > { > $$= negate_expression(thd, $2); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS TRUE_SYM %prec IS > + | boolean_test %prec PREC_BELOW_NOT > + ; > + > +boolean_test: > + boolean_test IS TRUE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_istrue(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not TRUE_SYM %prec IS > + | boolean_test IS not TRUE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnottrue(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS FALSE_SYM %prec IS > + | boolean_test IS FALSE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isfalse(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not FALSE_SYM %prec IS > + | boolean_test IS not FALSE_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnotfalse(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS UNKNOWN_SYM %prec IS > + | boolean_test IS UNKNOWN_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not UNKNOWN_SYM %prec IS > + | boolean_test IS not UNKNOWN_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnotnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS NULL_SYM %prec PREC_BELOW_NOT > + | boolean_test IS NULL_SYM %prec PREC_BELOW_NOT can you try %prec IS here? I suspect that if this affects anything, it'll cause bugs > { > $$= new (thd->mem_root) Item_func_isnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr IS not NULL_SYM %prec IS > + | boolean_test IS not NULL_SYM %prec IS > { > $$= new (thd->mem_root) Item_func_isnotnull(thd, $1); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr EQUAL_SYM predicate %prec EQUAL_SYM > + | boolean_test EQUAL_SYM predicate %prec EQUAL_SYM > { > $$= new (thd->mem_root) Item_func_equal(thd, $1, $3); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr comp_op predicate %prec '=' > + | boolean_test comp_op predicate %prec '=' > { > $$= (*$2)(0)->create(thd, $1, $3); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | expr comp_op all_or_any '(' subselect ')' %prec '=' > + | boolean_test comp_op all_or_any '(' subselect ')' %prec '=' > { > $$= all_any_subquery_creator(thd, $1, $2, $3, $5); > if (unlikely($$ == NULL)) > MYSQL_YYABORT; > } > - | predicate > + | predicate %prec BETWEEN_SYM > ; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4d9f8a3c31e: MDEV-28669 addendum: additional tests and mtr changes
by Sergei Golubchik 27 Dec '22

27 Dec '22

Hi, Julius, On Dec 27, Julius Goryavsky wrote: > commit 4d9f8a3c31e > Author: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > Date: Mon Dec 19 10:29:55 2022 +0100 > > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index 12afab4d28c..8df8198fc99 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -313,7 +313,7 @@ my %mysqld_logs; > my $opt_debug_sync_timeout= 300; # Default timeout for WAIT_FOR actions. > my $warn_seconds = 60; > > -my $rebootstrap_re= '--innodb[-_](?:page[-_]size|checksum[-_]algorithm|undo[-_]tablespaces|log[-_]group[-_]home[-_]dir|data[-_]home[-_]dir)|data[-_]file[-_]path|force_rebootstrap'; > +my $rebootstrap_re= '--(?:loose[-_])?(?:innodb[-_](?:page[-_]size|file(?:[-_]format|per[-_]table)|compression[-_](?:default|algorithm)|checksum(?:s|[-_]algorithm)|undo[-_](?:directory|tablespaces)|(?:data|log[-_]group)[-_]home[-_]dir|buffer[-_]pool[-_]filename|data[-_]file[-_]path|default[-_](?:encryption[-_]key[-_]id|page[-_]encryption)|sys[-_]|(?:index|table)[-_]stats)|aria[-_]log[-_](?:dir[-_]path|file[-_]size))|force_rebootstrap'; 1. drop force_rebootstrap please 2. this code was already not very readable, but got fairly ridiculous now. Let's change the approach. E.g. put them all in a hash, like my %rebootstrap_check = map { $_ => 1 } qw( innodb-page-size innodb-file-format ... ); you'll need to normalize an option before looking it up in the hash > > sub testcase_timeout ($) { return $opt_testcase_timeout * 60; } > sub check_timeout ($) { return testcase_timeout($_[0]); } > @@ -2762,7 +2762,29 @@ sub mysql_server_start($) { > return; > } > > - my $datadir= $mysqld->value('datadir'); > + my $extra_opts= get_extra_opts($mysqld, $tinfo); > + > + # The test options can contain the --datadir parameter, but > + # the bootstrap function can only read datadir location from > + # a .cnf file. So we need to parse the options to get the > + # actual location of the data directory, considering both > + # the options and the .cnf file, and then store the resulting > + # value in a "$mysqld" hash - to use later, when we need to > + # know the actual location of the data directory: > + my $datadir=""; > + foreach my $opt ( @$extra_opts ) > + { > + if ($opt =~ /^--datadir=/) { > + $datadir = substr($opt, 10); > + last; strictly speaking, `last` is incorrect, there can be more than one --datadir > + } > + } > + # If datadir is not in the options, then read it from .cnf: > + if (! $datadir) { > + $datadir = $mysqld->value('datadir'); > + } > + $mysqld->{'datadir'} = $datadir; > + > if (not $opt_start_dirty) > { > > @@ -2785,17 +2807,59 @@ sub mysql_server_start($) { > } > } > > + # Run <tname>-master.sh > + if ($mysqld->option('#!run-master-sh') and > + defined $tinfo->{master_sh} and > + run_system('/bin/sh ' . $tinfo->{master_sh}) ) > + { > + $tinfo->{'comment'}= "Failed to execute '$tinfo->{master_sh}'"; > + return 1; > + } > + > + # Run <tname>-slave.sh > + if ($mysqld->option('#!run-slave-sh') and > + defined $tinfo->{slave_sh} and > + run_system('/bin/sh ' . $tinfo->{slave_sh})) > + { > + $tinfo->{'comment'}= "Failed to execute '$tinfo->{slave_sh}'"; > + return 1; > + } > + > my $mysqld_basedir= $mysqld->value('basedir'); > - my $extra_opts= get_extra_opts($mysqld, $tinfo); > > if ( $basedir eq $mysqld_basedir ) > { > if (! $opt_start_dirty) # If dirty, keep possibly grown system db > { > + # Find the name of the current section in the configuration > + # file and its suffix: > + my $section = $mysqld->{name}; > + my $server_name; > + my $suffix = ""; > + if (index($section, '.') != -1) { > + ($server_name, $suffix) = $section =~ /^\s*([^\s.]+)\s*\.\s*([^\s]+)/; > + } > + else { > + $server_name = $section =~ /^\s*([^\s]+)/; > + } Replace all 11 lines above with: my ($suffix) = ($mysqld->{name} =~ /\.(.*)/); > # Some InnoDB options are incompatible with the default bootstrap. > # If they are used, re-bootstrap > my @rebootstrap_opts; > @rebootstrap_opts = grep {/$rebootstrap_re/o} @$extra_opts if $extra_opts; > + # Let's store the additional bootstrap options in > + # the environment variable - they may be used later > + # in the mtr tests - for re-bootstrap or run the > + # recovery, etc: > + my $extra_text = "--datadir=$datadir"; that's rather random. Why do you add only this one option, that doesn't make sense. Better handle any additional requirements in the test. > + if (@rebootstrap_opts) { why do you need an if() ? > + $extra_text .= ' '.join(' ', @rebootstrap_opts); > + } > + if ($suffix) { > + $ENV{'MTR_BOOTSTRAP_OPTS_'.$suffix} = $extra_text; > + } > + else { > + $ENV{'MTR_BOOTSTRAP_OPTS'} = $extra_text; Is that even possible? > + } > if (@rebootstrap_opts) > { > mtr_verbose("Re-bootstrap with @rebootstrap_opts"); > diff --git a/mysql-test/suite/galera/disabled.def b/mysql-test/suite/galera/disabled.def > index 84babda2fa0..18e7c074059 100644 > --- a/mysql-test/suite/galera/disabled.def > +++ b/mysql-test/suite/galera/disabled.def > @@ -28,3 +28,4 @@ query_cache: MDEV-15805 Test failure on galera.query_cache > versioning_trx_id: MDEV-18590: galera.versioning_trx_id: Test failure: mysqltest: Result content mismatch > galera_wsrep_provider_unset_set: wsrep_provider is read-only for security reasons > pxc-421: wsrep_provider is read-only for security reasons > +galera_sst_rsync_innodb_nest : MDEV-29591 nesting innodb subdirectories in datadir causes SST to fail Why do you disable it if the whole commit is about fixing the test? How can I see that the test works? > diff --git a/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff b/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff > new file mode 100644 > index 00000000000..027f4860a39 > --- /dev/null > +++ b/mysql-test/suite/galera/r/galera_sst_rsync_innodb__innodb_dir,debug.rdiff > @@ -0,0 +1,114 @@ looks like a wrong file name? > +--- galera_sst_rsync_innodb_nest.result > ++++ galera_sst_rsync_innodb_nest.reject > +@@ -286,3 +286,111 @@ > + DROP TABLE t1; > + COMMIT; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f984f49fba7: MDEV-30203 - deb fix piuparts
by Sergei Golubchik 21 Dec '22

21 Dec '22

Hi, Daniel, This is a review of `git diff 12786f0e779f 5f76981eb649`, that is, for commits: 5f76981eb64 rpm: packages that dont match component name 25eaa0aebfa MDEV-30205: [Cmtr: suite look in mariadb-test path 0981201f027 mtr - wsrep use mariadb executables eba0b35035a mtr: maradb names for *embedded and mariadb-client-test 974ca88f0fe deb: fix autotest - exclude for MDEV-30205 b19d75408bb Deb: Breaks/Replaces/COnflicts c60f0341950 MDEV-30203 deb breaks/replaces on compat packages f984f49fba7 MDEV-30203 - deb fix piuparts 310d025ff0d MDEV-30205: fix deb autopkgtest 89f6cf47ee2 rpm: fix packaging of compat/non-compat scripts links 494ed76d15a deb: mysqlhotcopy to mariadb-client-compat d03075f3a1f deb: server-core mysql* -> server-compat 9453812193d wsrep_sst_mariabackup to use mariadb-backup (and message accordingly) a4f843a7a5a Fix uncollected symlink eb9a469bf9a Add package recommended dependencies c135be62274 Remove duplicate entry 84c7cd29c61 Fix Debian packaging for mariabackup 5dbc7ae8bb4 MDEV-30203: compat packages - RPM remove mariabackup link eb4fa5d750f MDEV-30203: debian - make compat packages d4be0b9be73 MDEV-30203: Create mysql symlink packages (RPM) - scripts 441879fd493 MDEV-30203: Create mysql symlink packages (RPM) On Dec 19, Daniel Black wrote: > diff --git a/cmake/cpack_rpm.cmake b/cmake/cpack_rpm.cmake > index 339363b2169..7b48fd64f4c 100644 > --- a/cmake/cpack_rpm.cmake > +++ b/cmake/cpack_rpm.cmake > @@ -85,6 +85,35 @@ SET(CPACK_RPM_server_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}") > SET(CPACK_RPM_test_PACKAGE_SUMMARY "MariaDB database regression test suite") > SET(CPACK_RPM_test_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}") > > +# "set/append array" - append a set of strings, separated by a space > +MACRO(SETA var) > + FOREACH(v ${ARGN}) > + SET(${var} "${${var}} ${v}") > + ENDFOREACH() > +ENDMACRO(SETA) > + > +FOREACH(SYM_COMPONENT Server Client Test) > + SET(SYM ${SYM_COMPONENT}_symlinks) > + string(TOLOWER ${SYM} SYM) * why all cmake commands are uppercase, but `string` is lowercase? * SET is redundant, you can do STRING(TOLOWER ${SYM_COMPONENT}_symlinks SYM) * may be, better to use ${SYM_COMPONENT}-symlinks ? Like in MariaDB-server-symlinks ? > + SET(SYMCOMP ${SYM_COMPONENT}Symlinks) > + string(TOUPPER ${SYMCOMP} SYMCOMP_UPPER) > + SET(CPACK_COMPONENT_${SYMCOMP_UPPER}_GROUP "${SYM}") > + SET(CPACK_COMPONENTS_ALL "${CPACK_COMPONENTS_ALL}" "${SYMCOMP}") > + SET(CPACK_RPM_${SYM}_PACKAGE_SUMMARY "MySQL compatible symlinks for MariaDB database ${SYM_COMPONENT} binaries/scripts") > + SET(CPACK_RPM_${SYM}_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}") > + SET(CPACK_RPM_${SYM}_PACKAGE_ARCHITECTURE "noarch") > + SETA(CPACK_RPM_${SYM_COMPONENT}_PACKAGE_RECOMMENDS "MariaDB-${SYM}") You forgot to say that symlink package DEPENDS on the non-symlink package > +ENDFOREACH() > + > +# TODO change to 11.0 on rebase > +SETA(CPACK_RPM_client_symlinks_PACKAGE_CONFLICTS > + "MariaDB-client < 10.11.2" > + "MariaDB-server < 10.11.2") > +SETA(CPACK_RPM_server_symlinks_PACKAGE_CONFLICTS > + "MariaDB-server < 10.11.2") > +SETA(CPACK_RPM_test_symlinks_PACKAGE_CONFLICTS > + "MariaDB-test < 10.11.2") > + > # libmariadb3 > SET(CPACK_RPM_shared_PACKAGE_SUMMARY "LGPL MariaDB database client library") > SET(CPACK_RPM_shared_PACKAGE_DESCRIPTION "This is LGPL MariaDB client library that can be used to connect to MySQL > diff --git a/cmake/symlinks.cmake b/cmake/symlinks.cmake > index 3f3b4e4a9b5..a4eb6c2b42c 100644 > --- a/cmake/symlinks.cmake > +++ b/cmake/symlinks.cmake > @@ -12,7 +12,6 @@ endmacro() > REGISTER_SYMLINK("mariadb" "mysql") > REGISTER_SYMLINK("mariadb-access" "mysqlaccess") > REGISTER_SYMLINK("mariadb-admin" "mysqladmin") > -REGISTER_SYMLINK("mariadb-backup" "mariabackup") better not. there're scripts that might be using it. This is why I suggested `strncmp(my_progname, "mariadb", 7)` and not `strncmp(my_progname, "maria", 5)` in your PR > REGISTER_SYMLINK("mariadb-binlog" "mysqlbinlog") > REGISTER_SYMLINK("mariadb-check" "mysqlcheck") > REGISTER_SYMLINK("mariadb-client-test-embedded" "mysql_client_test_embedded") > diff --git a/debian/control b/debian/control > index 81418c13656..6b225ca4b3d 100644 > --- a/debian/control > +++ b/debian/control > @@ -548,7 +548,8 @@ Provides: default-mysql-client, > virtual-mysql-client > Recommends: libdbd-mariadb-perl | libdbd-mysql-perl, > libdbi-perl, > - libterm-readkey-perl > + libterm-readkey-perl, > + mariadb-client-compat Please, no. deb and rpm packages are different enough already. don't use different suffixes for them. Choose either -symlink or -compat (or something else) and use it consistently both in deb and in rpm. > Description: MariaDB database client binaries > MariaDB is a fast, stable and true multi-user, multi-threaded SQL database > server. SQL (Structured Query Language) is the most popular database query > @@ -558,6 +559,110 @@ Description: MariaDB database client binaries > This package includes the client binaries and the additional tools > innotop and mariadb-report (mysqlreport). > > +Package: mariadb-client-compat > +Architecture: all > +Depends: mariadb-client > +Multi-Arch: foreign > +Description: MySQL compatibility links to mariadb-client binaries/scripts. > +Conflicts: mariadb-client (<< ${source:Version}), > + mariadb-client-10.0, I wonder, would it work, if you won't set any Conflicts/Breaks, but only set Depends? It'd be a lot simpler that way (if it'll work). > + mariadb-client-10.1, > + mariadb-client-10.2, > + mariadb-client-10.3, > + mariadb-client-10.4, > + mariadb-client-10.5, > + mariadb-client-10.6, > + mariadb-client-10.7, > + mariadb-client-10.8, > + mariadb-client-5.1, > + mariadb-client-5.2, > + mariadb-client-5.3, > + mariadb-client-5.5, > + mariadb-client-core (<< ${source:Version}), > + mariadb-client-core-10.0, > + mariadb-client-core-10.1, > + mariadb-client-core-10.2, > + mariadb-client-core-10.3, > + mariadb-client-core-10.4, > + mariadb-client-core-10.5, > + mariadb-client-core-10.6, > + mariadb-client-core-10.7, > + mariadb-client-core-10.8, > + mariadb-server (<< ${source:Version}), > + mariadb-server-10.0, > + mariadb-server-10.1, > + mariadb-server-10.2, > + mariadb-server-10.3, > + mariadb-server-10.4, > + mariadb-server-10.5, > + mariadb-server-10.6, > + mariadb-server-10.7, > + mariadb-server-10.8, > + mysql-client (<< 5.0.51), > + mysql-client-5.0, > + mysql-client-5.1, > + mysql-client-5.5, > + mysql-client-5.6, > + mysql-client-5.7, > + mysql-client-8.0, > + mysql-client-core-5.0, > + mysql-client-core-5.1, > + mysql-client-core-5.5, > + mysql-client-core-5.6, > + mysql-client-core-5.7, > + mysql-client-core-8.0, > + mysql-server-core-5.7, > + mysql-server-core-8.0, > + percona-server-server-5.6, > + percona-server-server, > + percona-xtradb-cluster-server-5.6, > + percona-xtradb-cluster-server-5.7, > + percona-xtradb-cluster-server > +Breaks: mariadb-client (<< ${source:Version}), > + mariadb-client-10.0, > + mariadb-client-10.1, > + mariadb-client-10.2, > + mariadb-client-10.3, > + mariadb-client-10.4, > + mariadb-client-10.5, > + mariadb-client-10.6, > + mariadb-client-10.7, > + mariadb-client-10.8, > + mariadb-client-5.1, > + mariadb-client-core (<< ${source:Version}), > + mariadb-client-core-10.0, > + mariadb-client-core-10.1, > + mariadb-client-core-10.2, > + mariadb-client-core-10.3, > + mariadb-client-core-10.4, > + mariadb-client-core-10.5, > + mariadb-client-core-10.6, > + mariadb-client-core-10.7, > + mariadb-client-core-10.8, > + mariadb-server (<< ${source:Version}), > + mariadb-server-10.0, > + mariadb-server-10.1, > + mariadb-server-10.2, > + mariadb-server-10.3, > + mariadb-server-10.4, > + mariadb-server-10.5, > + mariadb-server-10.6, > + mariadb-server-10.7, > + mariadb-server-10.8, > + mysql-server-5.5, > + mysql-server-5.6, > + mysql-server-5.7, > + mysql-server-8.0, > + mysql-server-core-5.5, > + mysql-server-core-5.6, > + mysql-server-core-5.7, > + mysql-server-core-8.0, > + percona-server-server-5.6, > + percona-server-server, > + percona-xtradb-cluster-server-5.6, > + percona-xtradb-cluster-server-5.7, > + percona-xtradb-cluster-server > + > Package: mariadb-server-core > Architecture: any > Depends: mariadb-common (>= ${source:Version}), > diff --git a/debian/mariadb-backup.install b/debian/mariadb-backup.install > index e450f8f46a0..a288377ddd0 100644 > --- a/debian/mariadb-backup.install > +++ b/debian/mariadb-backup.install > @@ -1,6 +1,4 @@ > -usr/bin/mariabackup same here. needs mariadb-backup-compat > usr/bin/mariadb-backup > usr/bin/mbstream > -usr/share/man/man1/mariabackup.1 > usr/share/man/man1/mariadb-backup.1 > usr/share/man/man1/mbstream.1 > diff --git a/debian/mariadb-client.links b/debian/mariadb-client.links > index 62e3651daf5..8ccce12c609 100644 > --- a/debian/mariadb-client.links > +++ b/debian/mariadb-client.links > @@ -2,11 +2,6 @@ usr/bin/mariadb-check usr/bin/mariadb-analyze > usr/bin/mariadb-check usr/bin/mariadb-optimize > usr/bin/mariadb-check usr/bin/mariadb-repair > usr/bin/mariadb-check usr/bin/mariadbcheck > -usr/bin/mariadb-check usr/bin/mysqlanalyze > -usr/bin/mariadb-check usr/bin/mysqlcheck > -usr/bin/mariadb-check usr/bin/mysqloptimize > -usr/bin/mariadb-check usr/bin/mysqlrepair > -usr/bin/mariadb-report usr/bin/mysqlreport > usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-analyze.1.gz > usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-optimize.1.gz > usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-repair.1.gz you forgot to move symlinks to man pages > diff --git a/debian/mariadb-server-compat.install b/debian/mariadb-server-compat.install > new file mode 100644 > index 00000000000..36701d8e0c6 > --- /dev/null > +++ b/debian/mariadb-server-compat.install > @@ -0,0 +1,12 @@ > +usr/bin/mysqld_multi > +usr/bin/mysqld_safe > +usr/bin/mysqld_safe_helper > +usr/bin/mysql_install_db > +usr/bin/mysql_upgrade > +usr/sbin/mysqld > +usr/share/man/man1/mysqld_multi.1 > +usr/share/man/man1/mysqld_safe.1 > +usr/share/man/man1/mysqld_safe_helper.1 great. do you do the same for rpms? I didn't see where manpages are moved to -symlinks packages in rpms. > +usr/share/man/man1/mysql_install_db.1 > +usr/share/man/man1/mysql_upgrade.1 > +usr/share/man/man8/mysqld.8 > diff --git a/debian/mariadb-test-compat.install b/debian/mariadb-test-compat.install > new file mode 100644 > index 00000000000..7b498d7d1c8 > --- /dev/null > +++ b/debian/mariadb-test-compat.install > @@ -0,0 +1 @@ > +usr/share/mysql/mysql-test/mysql-test-run.pl You forgot mysql-test-run, mysqltest, mysql_client_test, and their *embedded variants. But, you know, it's probably not worth a new package at all. It's the *test* package (not interesting for users, never used in production, so no immutability requirements). I suggest to keep those symlinks in the mariadb-test package. Your other PR will add a warning. And eventually we drop them. Optional/Recommended/Suggested - sounds like too much toubles for a *test* package. > diff --git a/mysql-test/lib/mtr_cases.pm b/mysql-test/lib/mtr_cases.pm > index fe202279ac7..ad1ac6fdf88 100644 > --- a/mysql-test/lib/mtr_cases.pm > +++ b/mysql-test/lib/mtr_cases.pm > @@ -387,14 +387,14 @@ sub collect_suite_name($$) > else > { > my @dirs = my_find_dir(dirname($::glob_mysql_test_dir), > - ["mysql-test/suite", @plugin_suitedirs ], > + ["mariadb-test/suite", "mysql-test/suite", @plugin_suitedirs ], Eh, no. That is, yes, but not in this set of commits, not in this MDEV > $suitename); > # > # if $suitename contained wildcards, we'll have many suites and > # their overlays here. Let's group them appropriately. > # > for (@dirs) { > - m@^.*/(?:mysql-test/suite|$plugin_suitedir_regex)/(.*)$@o or confess $_; > + m@^.*/(?:mariadb-test/suite|mysql-test/suite|$plugin_suitedir_regex)/(.*)$@o or confess $_; > push @{$suites{$1}}, $_; > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] MDEV-14443 test cases only
by Sergei Golubchik 19 Dec '22

19 Dec '22

Hi, Vicențiu, This is a review of _test cases only_. Mainly, to discuss the behavior. Random thoughts here and comments inline in the diff. There are two models for DENY that I can think of: "a hole in grants" and "a mask". The difference lies in where DENY is applied. In the "a hole in grants" model one has a certain set of grants (may be with "holes"), a role has another set of grants, and when one does SET ROLE, one gets a union of those grants. Like, one has SELECT on db1.* and DENY to db1.t1, and some role has SELECT on db1.t1 - by setting this role a user will have full SELECT on db1.* While in the "mask" model, denies are separate entities, when setting a role a union of denies is subtracted from the union of grants. In the example above a user would still be denied access to db1.t1. I personally see the "hole" model as more logical and easier to understand. It also allows to "patch" holes with roles, what the "mask" model cannot do. The "mask" model needs getting used to, the fact that after SET ROLE one can end up having less access than before. It's not clear who can DENY what and to whom. And DENY is very difficult to work around, esp. as one can DENY IGNORE DENIES. But it allows to DENY something to PUBLIC, and the "hole" model cannot really do that. And I think it'd be a popular use case. Ideally, I'd prefer to have some kind of a compromise model that's as simple to understand as a "hole", but allows to deny to public. Unfortunately, I don't see how to do that. You've implemented the "mask" behavior. Okay. It solves an important use case. More comments below: > diff --git a/mysql-test/suite/deny/columns.result b/mysql-test/suite/deny/columns.result > --- /dev/null > +++ b/mysql-test/suite/deny/columns.result > @@ -0,0 +1,126 @@ > +create user foo; > +create database deny_db; > +create table deny_db.t1 (a int, b int, secret int); > +create table deny_db.t2 (a2 int, b2 int, secret2 int); > +insert into deny_db.t2 values (100, 200, 300); > +grant all on *.* to foo; > +revoke ignore denies on *.* from foo; > +grant all on deny_db.* to foo; > +grant all on deny_db.t1 to foo; > +grant all on deny_db.t2 to foo; > +grant select (a, b, secret) on deny_db.t1 to foo; > +grant insert (a, b, secret) on deny_db.t1 to foo; > +grant update (a, b, secret) on deny_db.t1 to foo; > +grant references (a, b, secret) on deny_db.t1 to foo; > +grant select (a2, b2, secret2) on deny_db.t2 to foo; > +grant insert (a2, b2, secret2) on deny_db.t2 to foo; > +grant update (a2, b2, secret2) on deny_db.t2 to foo; > +grant references (a2, b2, secret2) on deny_db.t2 to foo; this is quite confusing, tons of redundant grants on different levels, it makes it very unclear of what causes what. > +# > +# Test select and insert ... select commands. > +# > +deny select (secret) on deny_db.t1 to foo; > +connect con1,localhost,foo,,; > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' case in point. Normally, this should've been SELECT command denied to user 'foo'@'localhost' for table 't1' like in create database db; create table db.t1 (a int, b int, secret int); insert db.t1 values (1,2,100); create user foo@localhost; grant select (a,b) on db.t1 to foo@localhost; connect foo,localhost,foo; select a,b from db.t1; --error ER_TABLEACCESS_DENIED_ERROR select * from db.t1; --error ER_COLUMNACCESS_DENIED_ERROR select a,b,secret from db.t1; that is `select *` is ER_TABLEACCESS_DENIED_ERROR, and if I mention the column by name, then it's ER_COLUMNACCESS_DENIED_ERROR But you have so many redundant overlapping grants, that I really cannot see what error is correct in your test here. > +select secret from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +# > +# These are different code paths. Check both. > +# > +insert into deny_db.t1 (a, b, secret) values (1, 2, 3); > +insert into deny_db.t1 values (1, 2, 3); > +# > +# Check INSERT SELECT as we need to account for 2 different kinds of > +# privileges. > +# > +insert into deny_db.t1 select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +insert into deny_db.t1 select * from deny_db.t2; > +show full columns from deny_db.t1; > +Field Type Collation Null Key Default Extra Privileges Comment > +a int(11) NULL YES NULL select,insert,update,references > +b int(11) NULL YES NULL select,insert,update,references > +secret int(11) NULL YES NULL insert,update,references do you have a test where a denied column isn't shown by SHOW COLUMNS? that is, where you don't have other privileges on it? > +show full columns from deny_db.t2; > +Field Type Collation Null Key Default Extra Privileges Comment > +a2 int(11) NULL YES NULL select,insert,update,references > +b2 int(11) NULL YES NULL select,insert,update,references > +secret2 int(11) NULL YES NULL select,insert,update,references > +disconnect con1; > +connection default; > +revoke deny select (secret) on deny_db.t1 from foo; > +deny select(secret2) on deny_db.t2 to foo; > +connect con1,localhost,foo,,; > +insert into deny_db.t1 (a, b, secret) values (1, 2, 3); > +insert into deny_db.t1 values (1, 2, 3); > +insert into deny_db.t1 select * from deny_db.t1; > +insert into deny_db.t1 select * from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret2' in table 't2' > +show full columns from deny_db.t1; > +Field Type Collation Null Key Default Extra Privileges Comment > +a int(11) NULL YES NULL select,insert,update,references > +b int(11) NULL YES NULL select,insert,update,references > +secret int(11) NULL YES NULL select,insert,update,references > +show full columns from deny_db.t2; > +Field Type Collation Null Key Default Extra Privileges Comment > +a2 int(11) NULL YES NULL select,insert,update,references > +b2 int(11) NULL YES NULL select,insert,update,references > +secret2 int(11) NULL YES NULL insert,update,references > +disconnect con1; > +# > +# Test insert commands > +# > +connection default; > +deny insert (secret) on deny_db.t1 to foo; > +connect con1,localhost,foo,,; > +insert into deny_db.t1 (a, b, secret) values (1, 2, 3); > +ERROR 42000: INSERT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +insert into deny_db.t1 values (1, 2, 3); > +ERROR 42000: INSERT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' didn't test this one, but likely should behave as select? that is, ER_TABLEACCESS_DENIED_ERROR please, test with GRANT (without DENY) before making any changes > +insert into deny_db.t1 select * from deny_db.t1; > +ERROR 42000: INSERT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +insert into deny_db.t1 select * from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret2' in table 't2' > +show full columns from deny_db.t1; > +Field Type Collation Null Key Default Extra Privileges Comment > +a int(11) NULL YES NULL select,insert,update,references > +b int(11) NULL YES NULL select,insert,update,references > +secret int(11) NULL YES NULL select,update,references > +disconnect con1; > +# > +# Test update commands. > +# > +connection default; > +deny select (secret) on deny_db.t1 to foo; > +deny select (secret2) on deny_db.t2 to foo; > +connect con1, localhost,foo,,; > +update deny_db.t1 set a=111, b=222 where secret=10; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +update deny_db.t1 set a=123 where a = (select secret2 from deny_db.t2); > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'secret2' in table 't2' > +show full columns from deny_db.t1; > +Field Type Collation Null Key Default Extra Privileges Comment > +a int(11) NULL YES NULL select,insert,update,references > +b int(11) NULL YES NULL select,insert,update,references > +secret int(11) NULL YES NULL update,references > +show full columns from deny_db.t2; > +Field Type Collation Null Key Default Extra Privileges Comment > +a2 int(11) NULL YES NULL select,insert,update,references > +b2 int(11) NULL YES NULL select,insert,update,references > +secret2 int(11) NULL YES NULL insert,update,references > +disconnect con1; > +connection default; > +deny update (secret) on deny_db.t1 to foo; > +connect con1, localhost,foo,,; > +update deny_db.t1 set a=111, b=222; > +update deny_db.t1 set secret=333, a=111; > +ERROR 42000: UPDATE command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +update deny_db.t1 set secret=333; > +ERROR 42000: UPDATE command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +update deny_db.t1 set a=123, secret=333; > +ERROR 42000: UPDATE command denied to user 'foo'@'localhost' for column 'secret' in table 't1' > +disconnect con1; > +connection default; > +drop user foo; > +drop database deny_db; > diff --git a/mysql-test/suite/deny/columns_role.result b/mysql-test/suite/deny/columns_role.result > --- /dev/null > +++ b/mysql-test/suite/deny/columns_role.result > @@ -0,0 +1,57 @@ > +create database some_db; > +create view some_db.v1 as (select 1 as col); > +create user foo; > +create role bar; > +grant select on *.* to foo; > +grant show view on *.* to foo; > +grant bar to foo; > +grant select(col) on some_db.v1 to foo; > +revoke ignore denies on *.* from foo; why foo has ignore denies here? I don't see where it was granted. > +deny select (col) on some_db.v1 to bar; > +connect con1,localhost,foo,,; > +describe some_db.v1; > +Field Type Null Key Default Extra > +col int(1) NO 0 > +set role bar; > +describe some_db.v1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'v1' this is very questionable. Should SET ROLE ever restrict what one can do? > +show create view some_db.v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `some_db`.`v1` AS (select 1 AS `col`) latin1 latin1_swedish_ci > +disconnect con1; > +connection default; > +drop user foo; > +drop role bar; > +drop database some_db; > +# > +# Test if only role level denies are active. > +# > +create database some_db; > +create view some_db.v1 as (select 1 as col); > +create user foo; > +create role bar; > +grant bar to foo; > +grant select(col) on some_db.v1 to foo; > +deny select (col) on some_db.v1 to bar; > +connect con1,localhost,foo,,; > +# > +# Due to select(col) privilege on v1 we can see some_db here. > +# > +show databases; > +Database > +information_schema > +some_db > +test > +set role bar; > +# > +# Now the role's denies take effect and we can no longer see some_db. > +# > +show databases; > +Database > +information_schema > +test > +disconnect con1; > +connection default; > +drop user foo; > +drop role bar; > +drop database some_db; > diff --git a/mysql-test/suite/deny/deny_datastructures.result b/mysql-test/suite/deny/deny_datastructures.result > --- /dev/null > +++ b/mysql-test/suite/deny/deny_datastructures.result > @@ -0,0 +1,109 @@ > +create user foo; > +create role bar; > +# > +# Test deny format for global privileges. > +# > +deny select on *.* to foo; > +deny insert on *.* to bar; > +select user, host, JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +user host JSON_EXTRACT(priv, '$.deny') > +foo % {"global": 1, "version_id": VERSION_ID} > +select user, host, JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'bar'; > +user host JSON_EXTRACT(priv, '$.deny') > +bar {"global": 2, "version_id": VERSION_ID} > +flush privileges; > +deny select on some_db.* to foo; > +deny show view on some_db.* to foo; > +deny insert on some_other_db.* to foo; > +deny insert on some_other_db.* to bar; > +deny select on some_other_db.some_table to foo; > +deny insert on some_other_db.some_table_second to foo; > +deny update on some_other_db.some_table_third to foo; > +deny select(a, b) on some_other_db.some_table_third to foo; > +deny select, insert(a, b, c) on some_other_db.some_table_fourth to foo; > +select user, host, JSON_DETAILED(JSON_EXTRACT(priv, '$.deny')) from mysql.global_priv where user = 'foo'; > +user host JSON_DETAILED(JSON_EXTRACT(priv, '$.deny')) > +foo % { > + "global": 1, > + "db": > + [ > + > + { > + "name": "`some_db`", > + "access": 4194305 > + }, > + > + { > + "name": "`some_other_db`", > + "access": 2 > + } > + ], > + "table": > + [ > + > + { > + "name": "`some_other_db`.`some_table_second`", you sure you want that? you'd need to parse the name back when reading privileges, why would you need this extra complexity? I'd suggest something like "name" : [ "some_other_db", "some_table_second" ] (one component for db grants, three components for column grants) This way you don't need to parse the identifier to split on dots and remove backticks. You can even use [] for global grants and not split them into global/db/table/column lists, but have simply [ { "name ": [], "access": 1 }, { "name": ["some_db"], "access": 4194305 }, { "name": ["some_other_db"], "access": 2 } { "name": ["some_other_db", "some_table_second"], "access": 2 }, { "name": ["some_other_db", "some_table"], "access": 1 }, { "name": ["some_other_db", "some_table_third"], "access": 4 }, { "name": ["some_other_db", "some_table_fourth"], "access": 1 } { "name": ["some_other_db", "some_table_third", "a"], "access": 1 }, { "name": ["some_other_db", "some_table_fourth", "b"], "access": 2 }, { "name": ["some_other_db", "some_table_fourth", "c"], "access": 2 }, { "name": ["some_other_db", "some_table_third", "b"], "access": 1 }, ] May be it's easier if they're split into global/db/table/column, but there's defenitely no reason to put all identifier chain in one json property, quoted according to SQL rules. > + "access": 2 > + }, > + > + { > + "name": "`some_other_db`.`some_table`", > + "access": 1 > + }, > + > + { > + "name": "`some_other_db`.`some_table_third`", > + "access": 4 > + }, > + > + { > + "name": "`some_other_db`.`some_table_fourth`", > + "access": 1 > + } > + ], > + "column": > + [ > + > + { > + "name": "`some_other_db`.`some_table_third`.`a`", > + "access": 1 > + }, > + > + { > + "name": "`some_other_db`.`some_table_fourth`.`b`", > + "access": 2 > + }, > + > + { > + "name": "`some_other_db`.`some_table_fourth`.`c`", > + "access": 2 > + }, > + > + { > + "name": "`some_other_db`.`some_table_third`.`b`", > + "access": 1 > + }, > + > + { > + "name": "`some_other_db`.`some_table_fourth`.`a`", > + "access": 2 > + } > + ], > + "version_id": VERSION_ID > +} > +select user, host, JSON_DETAILED(JSON_EXTRACT(priv, '$.deny')) from mysql.global_priv where user = 'bar'; > +user host JSON_DETAILED(JSON_EXTRACT(priv, '$.deny')) > +bar { > + "global": 2, > + "db": > + [ > + > + { > + "name": "`some_other_db`", > + "access": 2 > + } > + ], > + "version_id": VERSION_ID > +} > +drop user foo; > +drop role bar; > diff --git a/mysql-test/suite/deny/dml.test b/mysql-test/suite/deny/dml.test > --- /dev/null > +++ b/mysql-test/suite/deny/dml.test > @@ -0,0 +1,241 @@ would be good to have a comment at the top of every test file you add to mention the 1) MDEV and 2) the purpose for that file to exist. E.g. in this case I wouldn't need to waste time guessing how dml.test is different from columns.test, if both have column-level grants. And even the content looks suspiciously the same. > +--source include/not_embedded.inc > + > +create user foo; > + > +create database deny_db; > +create table deny_db.t1 (a int, b int, secret int); > +insert into deny_db.t1 values (1, 2, 3); > + > +create table deny_db.t2 (a int, b int, secret int); > +insert into deny_db.t2 values (10, 20, 30); > + > +grant select (a) on deny_db.t1 to foo; > +grant insert (a) on deny_db.t1 to foo; > +grant update (a) on deny_db.t1 to foo; > +grant delete on deny_db.t1 to foo; > + > +--connect (con1,localhost,foo,,) > +insert into deny_db.t1 (a) values (1), (2), (3); > +disconnect con1; > + > +connection default; > +deny insert on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a) values (1), (2), (3); > +update deny_db.t1 set a=a*10; > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +connection default; > +select * from deny_db.t1 order by a; > +truncate deny_db.t1; > + > +revoke deny insert on deny_db.* from foo; > + > + > +--connect (con1,localhost,foo,,) > +insert into deny_db.t1 (a) values (1), (2), (3); > +--echo # > +--echo # Test that revoking insert deny allows inserting only into allowed > +--echo # to insert columns. > +--echo # > +insert into deny_db.t1 (a) values (1), (2), (3); > + > +--error ER_COLUMNACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > +disconnect con1; > + > +connection default; > +--echo # > +--echo # Test delete/update command denies. > +--echo # > +deny select on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--echo # > +--echo # Test not allowed to delete / update because of lack of select > +--echo # rights. > +--echo # > + > +show grants; > +--error ER_COLUMNACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10 where a = 1; Did you verify that in all cases the error is the same as if the privilege simply wasn't granted in the first place? In this case the error is correct, I've checked. > + > +--error ER_COLUMNACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > + > +connection default; > +--echo # > +--echo # Now deny update and delete directly, but keep select. > +--echo # > +revoke deny select on deny_db.* from foo; > +deny delete, update on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--echo # > +--echo # Test not allowed to delete / update because of lack of delete/update > +--echo # rights. > +--echo # > + > +--error ER_TABLEACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > + > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +--echo # > +--echo # Test masking table level grants. > +--echo # > + > +connection default; > +grant select on deny_db.t1 to foo; > +grant insert on deny_db.t1 to foo; > +grant update on deny_db.t1 to foo; > +grant delete on deny_db.t1 to foo; > + > +--connect (con1,localhost,foo,,) > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > +disconnect con1; > + > +connection default; > +deny insert on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > + > +--error ER_TABLEACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > + > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > + > +disconnect con1; > + > +connection default; > +revoke deny update, delete on deny_db.* from foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > + > +update deny_db.t1 set a=a*10; > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +--echo # > +--echo # Test masking database & global level grants. > +--echo # > +connection default; > +grant select, insert, update, delete on *.* to foo; > +grant select, insert, update, delete on deny_db.* to foo; > +grant select, insert, update, delete on deny_db.t1 to foo; > +deny select, insert on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > + > +--error ER_COLUMNACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > + > +--error ER_COLUMNACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +connection default; > +revoke deny select on deny_db.* from foo; > +deny update, delete on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > + > +--error ER_TABLEACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > + > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +connection default; > +deny select on deny_db.* to foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > + > +--error ER_TABLEACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > + > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +--echo # > +--echo # Test revoking all denies. > +--echo # > +connection default; > +revoke deny select, insert, update, delete on deny_db.* from foo; by the way, what does REVOKE ALL PRIVILEGES do, does it remove denies? please, add a test. > +truncate deny_db.t1; > + > +--connect (con1,localhost,foo,,) > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > +update deny_db.t1 set a=a*10; > +select * from deny_db.t1; > +delete from deny_db.t1 where a = 20; > +disconnect con1; > + > +connection default; > + > +--echo # > +--echo # Test masking via table level denies > +--echo # > + > +deny select on deny_db.t1 to foo; > + > +--connect (con1,localhost,foo,,) > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > +--error ER_COLUMNACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > +--error ER_COLUMNACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10 where a = 10; > +--echo # > +--echo # Delete works without select. > +--echo # > +delete from deny_db.t1; > +--error ER_COLUMNACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > + > +--error ER_TABLEACCESS_DENIED_ERROR > +select * from deny_db.t1; > +disconnect con1; > + > +connection default; > +revoke deny select on deny_db.t1 from foo; > +deny insert on deny_db.t1 to foo; > +deny update on deny_db.t1 to foo; > +deny delete on deny_db.t1 to foo; > + > +--connect (con1,localhost,foo,,) > +--error ER_TABLEACCESS_DENIED_ERROR > +insert into deny_db.t1 (a,b) values (1,10), (2, 20), (3, 30); > +--error ER_TABLEACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10; > +--error ER_TABLEACCESS_DENIED_ERROR > +update deny_db.t1 set a=a*10 where a = 10; > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from deny_db.t1; > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from deny_db.t1 where a = 20; > + > +select * from deny_db.t1; > + > +connection default; > +drop database deny_db; > +drop user foo; > diff --git a/mysql-test/suite/deny/global.result b/mysql-test/suite/deny/global.result > --- /dev/null > +++ b/mysql-test/suite/deny/global.result > @@ -0,0 +1,327 @@ > +create user foo; > +create database deny_db; > +create table deny_db.t1 (a int, b int, secret int); > +insert into deny_db.t1 values (1, 2, 3); > +create table deny_db.t2 (a int, b int, secret int); > +insert into deny_db.t2 values (10, 20, 30); > +# > +# Test global denies. > +# > +show databases; > +Database > +deny_db > +information_schema > +mtr > +mysql > +performance_schema > +sys > +test > +grant select (secret) on deny_db.t1 to foo; > +connect con1,localhost,foo,,; > +select * from deny_db2.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +show databases; > +Database > +deny_db > +information_schema > +test > +use deny_db; > +# > +# It is still possible to leak column names without denies. > +# > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'a' in table 't1' this is a bug. See my test above, it's ER_TABLEACCESS_DENIED_ERROR. Apparently, it returns ER_TABLEACCESS_DENIED_ERROR if there's at least one granted column before the non-granted column. That is, if you'd have create table deny_db.t1 (secret int, a int, b int); you'd get ER_TABLEACCESS_DENIED_ERROR. please fix it (where applicable, 10.3, perhaps?) > +select a from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'a' in table 't1' > +select b from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for column 'b' in table 't1' > +select table_name, column_name from information_schema.columns > +where table_schema = 'deny_db'; > +table_name column_name > +t1 secret > +# > +# Invalid field error is returned if table exists. > +# > +select c from deny_db.t1; > +ERROR 42S22: Unknown column 'c' in 'field list' I feel like the whole column-leaking behavior is a bug too. This, likely, should be ER_COLUMNACCESS_DENIED_ERROR too > +# > +# Table access denied if table does exist but is not granted. > +# > +select c from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +# > +# Same error if table does not exist. > +# > +select c from deny_db.t_not_exists; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't_not_exists' > +select secret from deny_db.t1; > +secret > +3 > +# > +# Now apply deny. > +# > +connection default; > +deny select on *.* to foo; I see you reconnect to apply denies. What are the rules here? With grants, table/column grants work right away, while global and current-db grants are cached in the thd. but table/column denies aren't stored in separate tables, that's why the question. What are the rules here? > +disconnect con1; > +connect con1,localhost,foo,,; > +show databases; > +Database > +information_schema > +test > +use information_schema; > +use deny_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'deny_db' > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select c from deny_db.t_not_exists; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't_not_exists' > +select secret from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +# > +# Grant table access then test deny. > +# > +connection default; > +grant select on deny_db.t1 to foo; > +connection con1; > +show databases; > +Database > +information_schema > +test > +use information_schema; > +use deny_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'deny_db' > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select c from deny_db.t_not_exists; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't_not_exists' > +select secret from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +# > +# Grant database access then test deny. > +# > +connection default; > +grant select on deny_db.* to foo; > +connection con1; > +show databases; > +Database > +information_schema > +test > +use information_schema; > +use deny_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'deny_db' > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select c from deny_db.t_not_exists; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't_not_exists' > +select secret from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +# > +# Grant global access then test deny. > +# > +connection default; > +grant select on *.* to foo; > +connection con1; > +show databases; > +Database > +information_schema > +test > +use information_schema; > +use deny_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'deny_db' > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select c from deny_db.t_not_exists; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't_not_exists' > +select secret from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +connection default; > +# > +# Test underlying data loading. > +# > +select user, host, JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +user host JSON_EXTRACT(priv, '$.deny') > +foo % {"global": 1, "version_id": 101100} > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +DENY SELECT ON *.* TO `foo`@`%` > +GRANT SELECT ON `deny_db`.* TO `foo`@`%` > +GRANT SELECT, SELECT (secret) ON `deny_db`.`t1` TO `foo`@`%` meaning, the user will see what is denied to him? I expect it'll be a questionable item, some users will want denies to be not shown. Although I don't see how they cannot be. > +flush privileges; > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +DENY SELECT ON *.* TO `foo`@`%` > +GRANT SELECT ON `deny_db`.* TO `foo`@`%` > +GRANT SELECT, SELECT (secret) ON `deny_db`.`t1` TO `foo`@`%` > +connection con1; > +show databases; > +Database > +information_schema > +test > +use information_schema; > +use deny_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'deny_db' > +select * from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select c from deny_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select c from deny_db.t_not_exists; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't_not_exists' > +select secret from deny_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +disconnect con1; > +connection default; > +# > +# Now test insert not being denied. > +# > +grant insert(secret) on deny_db.t1 to foo; > +connect con1,localhost,foo,,; > +show databases; > +Database > +deny_db > +information_schema > +test > +insert into deny_db.t1(secret) values (10000); > +disconnect con1; > +connection default; > +drop database deny_db; > +drop user foo; > +# > +# Test denying all rights globally. please, add tests with DENY TO PUBLIC > +# User should still be able to connect and see information_schema > +# *at least*. > +# > +create user foo; > +create database some_db; > +grant select on *.* to foo; > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +deny select on *.* to foo; > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +DENY SELECT ON *.* TO `foo`@`%` > +deny all on *.* to foo; > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +DENY ALL PRIVILEGES ON *.* TO `foo`@`%` > +connect con1,localhost,foo,,information_schema; > +show databases; > +Database > +information_schema > +disconnect con1; > +connection default; > +drop user foo; > +drop database some_db; > +############################################## > +# Test SELECT command interacting with deny. # > +############################################## > +create user foo; > +create user bar; > +create database some_db; > +create table some_db.t1 (a int, secret int); > +insert into some_db.t1 values (1, 100); > +use some_db; > +create view v1 as (select a from t1); > +create view v2 as (select secret from t1); > +connect con1,localhost,foo,,; > +select table_name, table_type from information_schema.tables where table_schema like 'some_db'; > +table_name table_type > +disconnect con1; > +connection default; > +grant select on *.* to foo; > +grant select on some_db.* to foo; > +grant select on some_db.t1 to foo; > +grant select(a) on some_db.t1 to foo; > +# > +# See what foo sees before denies. > +# > +connect con1,localhost,foo,,; > +select table_name, table_type from information_schema.tables where table_schema like 'some_db'; > +table_name table_type > +t1 BASE TABLE > +v1 VIEW > +v2 VIEW > +disconnect con1; > +connection default; > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +GRANT SELECT ON `some_db`.* TO `foo`@`%` > +GRANT SELECT, SELECT (a) ON `some_db`.`t1` TO `foo`@`%` > +deny select on *.* to foo; > +show grants for foo; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +DENY SELECT ON *.* TO `foo`@`%` > +GRANT SELECT ON `some_db`.* TO `foo`@`%` > +GRANT SELECT, SELECT (a) ON `some_db`.`t1` TO `foo`@`%` > +connect con1,localhost,foo,,; > +show grants; > +Grants for foo@% > +GRANT SELECT ON *.* TO `foo`@`%` > +DENY SELECT ON *.* TO `foo`@`%` > +GRANT SELECT ON `some_db`.* TO `foo`@`%` > +GRANT SELECT, SELECT (a) ON `some_db`.`t1` TO `foo`@`%` > +use some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +select * from some_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +select * from some_db.v1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'v1' > +select * from some_db.v2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'v2' > +select table_name, table_type from information_schema.tables where table_schema like "some_db"; > +table_name table_type > +select table_name, column_name from information_schema.columns where table_schema like "some_db"; > +table_name column_name > +disconnect con1; > +connection default; > +grant insert(a) on some_db.t1 to bar; > +grant insert(a) on some_db.t1 to foo; > +connect con1,localhost,foo,,; > +select table_name, column_name from information_schema.columns where table_schema like "some_db"; > +table_name column_name > +t1 a > +disconnect con1; > +connect con2,localhost,bar,,; > +select table_name, column_name from information_schema.columns where table_schema like "some_db"; > +table_name column_name > +t1 a > +disconnect con2; > +connection default; > +deny insert on *.* to foo; > +deny insert on *.* to bar; > +connect con1,localhost,foo,,; > +select table_name, column_name from information_schema.columns where table_schema like "some_db"; > +table_name column_name > +disconnect con1; > +connect con2,localhost,bar,,; > +select table_name, column_name from information_schema.columns where table_schema like "some_db"; > +table_name column_name > +disconnect con2; > +connection default; > +drop user foo; > +drop user bar; > +drop database some_db; > diff --git a/mysql-test/suite/deny/ignore_denies.result b/mysql-test/suite/deny/ignore_denies.result > --- /dev/null > +++ b/mysql-test/suite/deny/ignore_denies.result > @@ -0,0 +1,76 @@ > +create user foo; > +create role bar; > +grant select, ignore denies on *.* to foo; > +deny select on *.* to foo; > +deny select on mysql.* to foo; > +deny select on mysql.global_priv to foo; > +deny select(user) on mysql.global_priv to foo; > +grant bar to foo; > +grant ignore denies on *.* to bar; > +connect con1,localhost,foo,,; > +# > +# The user should still be able to access mysql.global_priv due > +# to the IGNORE DENIES privilege being active. > +# > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +JSON_EXTRACT(priv, '$.deny') > +{"global": 1, "db": [{"name": "`mysql`", "access": 1}], "table": [{"name": "`mysql`.`global_priv`", "access": 1}], "column": [{"name": "`mysql`.`global_priv`.`user`", "access": 1}], "version_id": VERSION_ID} > +disconnect con1; > +connection default; > +# > +# Now test inheriting IGNORE DENIES from a role. > +# > +revoke ignore denies on *.* from foo; > +connect con1,localhost,foo,,; > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'global_priv' > +# > +# Bar provides foo with IGNORE DENIES privilege. > +# > +set role bar; > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +JSON_EXTRACT(priv, '$.deny') > +{"global": 1, "db": [{"name": "`mysql`", "access": 1}], "table": [{"name": "`mysql`.`global_priv`", "access": 1}], "column": [{"name": "`mysql`.`global_priv`.`user`", "access": 1}], "version_id": VERSION_ID} > +disconnect con1; > +connection default; > +# > +# Denying IGNORE DENIES, overrides a user's ability to ignore denies. This is very questionable. One can DENY IGNORE DENIES TO PUBLIC and basically lock the server down for everyone, there will be no way to override that. (this is an observation, not ready to suggest a solution atm) > +# > +deny ignore denies on *.* to foo; > +grant ignore denies on *.* to foo; > +connect con1,localhost,foo,,; > +# > +# Foo can't ignore the SELECT priv deny. > +# > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'global_priv' > +# > +# And neither if bar role provides the ignore deny priv. > +# > +set role bar; > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'global_priv' > +disconnect con1; > +connection default; > +# > +# Now test what happens when IGNORE DENIES is denied from a role. > +# > +revoke deny ignore denies on *.* from foo; > +deny ignore denies on *.* to bar; > +connect con1,localhost,foo,,; > +# > +# Foo can ignore the SELECT priv deny when Bar isn't activated > +# > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +JSON_EXTRACT(priv, '$.deny') > +{"global": 1, "db": [{"name": "`mysql`", "access": 1}], "table": [{"name": "`mysql`.`global_priv`", "access": 1}], "column": [{"name": "`mysql`.`global_priv`.`user`", "access": 1}], "version_id": VERSION_ID} > +# > +# But when Bar is active, denies can not be ignored any more. > +# > +set role bar; > +select JSON_EXTRACT(priv, '$.deny') from mysql.global_priv where user = 'foo'; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 'global_priv' > +disconnect con1; > +connection default; > +drop user foo; > +drop role bar; > diff --git a/mysql-test/suite/deny/show_generic.result b/mysql-test/suite/deny/show_generic.result > --- /dev/null > +++ b/mysql-test/suite/deny/show_generic.result > @@ -0,0 +1,197 @@ > +# > +# This test checks that denies take part in SHOW commands properly. > +# > +create user foo; > +create database some_db; > +create table some_db.t1 (id int); > +connect con1,localhost,foo,,; > +show tables from some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +disconnect con1; > +connection default; > +grant insert on some_db.t1 to foo; > +connect con1,localhost,foo,,; > +show tables from some_db; > +Tables_in_some_db > +t1 > +disconnect con1; > +connection default; > +deny insert on some_db.* to foo; > +connect con1,localhost,foo,,; > +show databases; > +Database > +information_schema > +test > +show tables from some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +disconnect con1; > +connection default; > +# > +# Test that dropping a user does not give access to connections that > +# previously had elements denied. > +# > +# We will use two users, foo which will receive denies, bar which will > +# not. This will showcase the difference in behaviour. > +# > +create user bar; > +# > +# This global grant is cached in sctx->master_access on connection. > +# > +grant select on *.* to foo; > +grant select on *.* to bar; > +connect con1,localhost,foo,,; > +show tables from some_db; > +Tables_in_some_db > +t1 > +disconnect con1; > +connection default; > +# > +# Here we add a mask for some_db, it should no longer be visible to foo. > +# > +deny select on some_db.* to foo; > +connect con1,localhost,foo,,; > +use some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +show tables from some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +# > +# bar user has access to the database (no denies present) > +# > +connect con2,localhost,bar,,; > +use some_db; > +show tables from some_db; > +Tables_in_some_db > +t1 > +# > +# Do not disconnect foo & bar, but drop the users. > +# > +connection default; > +drop user foo; > +drop user bar; > +# > +# Our current running connection, because it featured denies previously > +# now doesn't have access. > +# > +connection con1; > +use some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +show tables from some_db; > +ERROR 42000: Access denied for user 'foo'@'%' to database 'some_db' > +disconnect con1; > +# > +# However, bar does not have any denies active. In this case we can keep > +# the previous MariaDB behaviour of using the "cache" within > +# sctx->master_access to grant access. This comment is confusing. As far as I can see both foo and bar preserve the behavior after `drop user`. Which is fine and what I was expected anyway. But comments imply there's some difference in behavior between them? > +# > +connection con2; > +use some_db; > +show tables from some_db; > +Tables_in_some_db > +t1 > +disconnect con2; > +connection default; > +drop database some_db; > +# > +# Test table level denies interacting with show tables > +# > +create database some_db; > +create user foo; > +create table some_db.t1 (a int, b int); > +create table some_db.t2 (a int, b int); > +grant select on *.* to foo; > +deny select on some_db.t1 to foo; > +deny insert on some_db.t2 to foo; > +select user, host, JSON_EXTRACT(priv, '$.deny') from mysql.global_priv > +where user = 'foo'; > +user host JSON_EXTRACT(priv, '$.deny') > +foo % {"table": [{"name": "`some_db`.`t2`", "access": 2}, {"name": "`some_db`.`t1`", "access": 1}], "version_id": VERSION_ID} > +connect con1,localhost,foo,,; > +show tables from some_db; > +Tables_in_some_db > +t2 > +show columns from some_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +show columns from some_db.t2; > +Field Type Null Key Default Extra > +a int(11) YES NULL > +b int(11) YES NULL > +select table_name, column_name, privileges from information_schema.columns > +where table_schema like 'some_db' order by table_name, column_name; > +table_name column_name privileges > +t2 a select > +t2 b select > +disconnect con1; > +connection default; > +grant insert on some_db.* to foo; > +connect con1,localhost,foo,,; > +show tables from some_db; > +Tables_in_some_db > +t1 > +t2 > +# > +# See MDEV-28783, this should not error out when global/db grants exist > +# (except for SELECT priv). > +# > +show columns from some_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +show columns from some_db.t2; > +Field Type Null Key Default Extra > +a int(11) YES NULL > +b int(11) YES NULL > +select table_name, column_name, privileges from information_schema.columns > +where table_schema like 'some_db' order by table_name, column_name; > +table_name column_name privileges > +t1 a insert > +t1 b insert > +t2 a select > +t2 b select > +disconnect con1; > +connection default; > +deny insert on some_db.t1 to foo; > +deny select on some_db.t2 to foo; > +connect con1,localhost,foo,,; > +# > +# some_db should still be visible, but it should show up as empty. > +# > +show databases; > +Database > +information_schema > +mtr > +mysql > +performance_schema > +some_db > +sys > +test > +show tables from some_db; > +Tables_in_some_db > +show columns from some_db.t1; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't1' > +show columns from some_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select table_name, column_name, privileges from information_schema.columns > +where table_schema like 'some_db' order by table_name, column_name; > +table_name column_name privileges > +disconnect con1; > +connection default; > +grant select(a) on some_db.t1 to foo; > +grant update(a) on some_db.t1 to foo; > +connect con1,localhost,foo,,; > +# > +# Update privilege on the column is not masked, only see a column. > +# > +show tables from some_db; > +Tables_in_some_db > +t1 > +show columns from some_db.t1; > +Field Type Null Key Default Extra > +a int(11) YES NULL > +show columns from some_db.t2; > +ERROR 42000: SELECT command denied to user 'foo'@'localhost' for table 't2' > +select table_name, column_name, privileges from information_schema.columns > +where table_schema like 'some_db' order by table_name, column_name; > +table_name column_name privileges > +t1 a update > +disconnect con1; > +connection default; > +drop user foo; > +drop database some_db; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Review input for MDEV-21092, 21095, 29997
by Sergei Petrunia 08 Dec '22

08 Dec '22

Hi Rex, Please find below review input for the collection of patches for these MDEVs. First, please try to have each piece of functionality in its own commit. You can use "git rebase -i" to make your last commits be one commit, as well as re-order them. Then, you can do a carefully considered "git push --force" to overwrite the contents of your branch with the new set of commits. in prune_partitions(): + String parts; + String_list parts_list; + + make_used_partitions_str(thd->mem_root, prune_param.part_info, &parts, parts_list ); + trace_partition_pruning.add("partition_prune", parts.ptr()); This creates a list of used partitions regardless whether we need it or not (and typically trace=off, so we don't'). Please put this code into if (unlikely(thd->trace_started())) { ... } Consider this testcase: create table t2 (a int, b int) partition by hash(a) partitions 10; create table t3 (a int, b int) partition by hash(a) partitions 10; set optimizer_trace=1; explain format=json select * from t2,t3 where t2.a in (2,3,4) and t3.a in (4,5); select * from information_schema.optimizer_trace\G This gives: ... { "partition_prune": "p2,p3,p4" }, { "partition_prune": "p4,p5" }, ... which makes it impossible to see which table is pruning is done for. Let's make the trace be: { "prune_partitions" : { "table": "t2", "used_partitions": "p2,p3,p4" } } { "prune_partitions" : { "table": "t3", "used_partitions": "p4,p5" } } . in item_subselect.cc: + { + OPT_TRACE_TRANSFORM(thd, trace_wrapper, trace_transform, + in_subs->get_select_lex()->select_number, + "EXISTS (SELECT)", "IN (SELECT)"); + trace_transform.add( "upper_not", ( upper_not?true:false ) ); + } coding style: everything inside { } should be indented one level (=2 spaces) right. in sql_select.cc: "attached_conditions_summary": [ { "table": "t1", "attached": "t1.c < 30", "index": "t1.a < 10 and t1.b < 20" } Please change "index" to "index_condition". and "attached" to "attached_condition". Let's not print index_condition if it is null. (Yes, this is somewhat inconsistent with printing "attached":null) in item.cc: + #include "my_json_writer.h" + const char *dbug_print_optrace( ) dbug_print_optrace() is fine. I think it needs a comment like this: /* Debug printout: print the trace trace we're currently writing if any, and return it. */ (Feel free to fix poor English if necessary) Please fix the coding style in the function body as discussed on Slack. Please move #include to the other #includes at the top of the file. Can add a comment, "// For dbug_print_optrace" Would dbug_print_opt_trace() be a better name? + const char *dbug_print_items(Item *item) + { + *big_buf= 0; + + dbug_add_print_items( item ); + return big_buf; + } This function doesn't seem to be needed? For the rest of the functions: as discussed on Slack, we can add them in a separate commit if we decide that they're useful. Could you provide a very short text describing: The entry point (which function is to be called from gdb?). The use case. Is it "to print List<Item>" ? I don't recall seeing this data structure in the server... Is there a lot of need to print Item_cond and then print its children? This looks self-repetitive... I'd like to see a log of an example gdb session...

2 2

[Maria-developers] How to get started with contribution.
by Pinak faldu 29 Nov '22

29 Nov '22

Respected sir/madam, I am Pinak Faldu, a computer science undergraduate student. Currently, I am in my first year of a master's degree at Sinhgad College. I am new to open source contributions, but I am very familiar with ReactJS, MongoDB, MySql, Javascript, and C++. I would like to contribute to your organization, but I don't know where to begin. Could you please tell me how to get started? Hoping to hear from you soon. Regards Pinak

2 1

Re: [Maria-developers] 52f489ebccb: MDEV-29069 follow-up: support partially suitable keys
by Sergei Golubchik 02 Nov '22

02 Nov '22

Hi, Nikita, This review applies to the combined diff e2f8dff^..52f489e On Oct 04, Nikita Malyavin wrote: > diff --git a/sql/log_event.h b/sql/log_event.h > index 91a9eeee10f..8452e665c98 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -5275,8 +5278,11 @@ class Rows_log_event : public Log_event > uchar *m_key; /* Buffer to keep key value during searches */ > KEY *m_key_info; /* Pointer to KEY info for m_key_nr */ > uint m_key_nr; /* Key number */ > + uint m_key_parts_suit; /* A number of key_parts suited to lookup */ that's very confusing name. Took me a while to understand what you meant. Better call it m_usable_key_parts > bool master_had_triggers; /* set after tables opening */ > > + uint key_parts_suit_event(const KEY *key) const; and this could be "get_usable_key_parts()" > + > int find_key(); // Find a best key to use in find_row() > int find_row(rpl_group_info *); > int write_row(rpl_group_info *, const bool); > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 422d496d5c3..25705d13641 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -6062,7 +6065,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > */ > sql_mode_t saved_sql_mode= thd->variables.sql_mode; > if (!is_auto_inc_in_extra_columns()) > - thd->variables.sql_mode= (rpl_data.is_online_alter() ? saved_sql_mode :0) > + thd->variables.sql_mode= (m_online_alter ? saved_sql_mode :0) wouldn't it be more correct to use saved_sql_mode also in normal replication? > | MODE_NO_AUTO_VALUE_ON_ZERO; > > // row processing loop > @@ -7949,14 +7952,35 @@ uint8 Write_rows_log_event::get_trg_event_map() > **************************************************************************/ > > #if defined(HAVE_REPLICATION) > -/* > - Compares table->record[0] and table->record[1] > +/** > + @brief Compares table->record[0] and table->record[1] > + > + @param masrter_columns a number of columns on the source replica, > + 0 if ONLINE ALTER TABLE > > - Returns TRUE if different. > + @returns true if different. > */ > -static bool record_compare(TABLE *table) > +static bool record_compare(TABLE *table, uint master_columns) > { > - bool result= FALSE; > + bool result= false; > + > + /* > + Determine if the optimized check is possible (and we can > + goto record_compare_exit). > + We should ensure that all extra columns (i.e. fieldnr > master_columns) > + have explicit values. If not, we will exclude them from comparison, > + as they can contain non-deterministic values. > + > + master_columns == 0 case is optimization for ONLINE ALTER to check > + all columns fast. > + */ > + bool all_values_set= master_columns == 0 > + && bitmap_is_set_all(&table->has_value_set); > + for (uint i = master_columns; all_values_set && i < table->s->fields; i++) > + { > + all_values_set= bitmap_is_set(&table->has_value_set, i); > + } wait, this doesn't make any sense. You only do the loop if all_values_set is true. But it is true only if bitmap_is_set_all(&table->has_value_set). There is no point in doing bitmap_is_set() for individual bits, when you already know that all bits are set :) shouldn't it simply be bool all_values_set= bitmap_is_set_all(&table->has_value_set); ? but see below, perhaps you don't need all_values_set at all > + > /** > Compare full record only if: > - there are no blob fields (otherwise we would also need > @@ -7969,40 +7993,43 @@ static bool record_compare(TABLE *table) > */ > if ((table->s->blob_fields + > table->s->varchar_fields + > - table->s->null_fields) == 0) > + table->s->null_fields) == 0 > + && all_values_set) > { > result= cmp_record(table,record[1]); > goto record_compare_exit; > } > > /* Compare null bits */ > - if (memcmp(table->null_flags, > - table->null_flags+table->s->rec_buff_length, > - table->s->null_bytes)) > + if (all_values_set && memcmp(table->null_flags, > + table->null_flags+table->s->rec_buff_length, > + table->s->null_bytes)) > { > - result= TRUE; // Diff in NULL value > + result= true; // Diff in NULL value > goto record_compare_exit; > } > > /* Compare fields */ > for (Field **ptr=table->field ; *ptr ; ptr++) > { > + Field *f= *ptr; > if (table->versioned() && (*ptr)->vers_sys_field()) > { > continue; > } > - /** > - We only compare field contents that are not null. > - NULL fields (i.e., their null bits) were compared > - earlier. > - */ > - if (!(*(ptr))->is_null()) > + /* > + We only compare fields that exist on the source (or in ONLINE > + ALTER case, that were in the original table). For reference, > + replica tables can also contain extra fields. > + */ > + if (f->field_index > master_columns && !f->has_explicit_value()) > + continue; same question as in a previous review. Do you mean that when f->field_index < master_columns, it's ok to compare values below even when !f->has_explicit_value() ? > + > + if (f->is_null() != f->is_null(table->s->rec_buff_length) > + || f->cmp_binary_offset(table->s->rec_buff_length)) may be it's better to create a fast copy of this function, like if (bitmap_is_set_all(&table->has_value_set)) { ... old record_compare() code starting from if ((table->s->blob_fields + table->s->varchar_fields + table->s->null_fields) == 0) } else { ... your code, only the loop over fields. } in this case you won't even need all_values_set or master_columns > { > - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length)) > - { > - result= TRUE; > - goto record_compare_exit; > - } > + result= true; > + goto record_compare_exit; > } > } > > @@ -8010,6 +8037,32 @@ static bool record_compare(TABLE *table) > return result; > } > > +/** > + Newly added fields with non-deterministic defaults (i.e. DEFAULT(RANDOM()), > + CURRENT_TIMESTAMP, AUTO_INCREMENT) should be excluded from key search. > + Basically we exclude all the default-filled fields based on > + has_explicit_value bitmap. > + */ > +uint Rows_log_event::key_parts_suit_event(const KEY *key) const > +{ > + uint master_columns= m_online_alter ? 0 : m_cols.n_bits; get_master_cols() > + uint p= 0; > + for (;p < key->ext_key_parts; p++) > + { > + Field *f= key->key_part[p].field; > + uint non_deterministic_default= f->default_value && > + f->default_value->flags | VCOL_NOT_STRICTLY_DETERMINISTIC; > + > + int next_number_field= f->table->next_number_field ? > + f->table->next_number_field->field_index : -1; eh? 1. why it's *in the loop* ? 2. why not simply || f == f->table->next_number_field or, to be extra safe || f->table->field[f->field_index] == f->table->next_number_field > + > + if (f->field_index >= master_columns I don't think this condition ^^^ is needed. Same as elsewhere, if f->field_index < master_columns but !bitmap_is_set, does it mean the keyseg is ok? I don't think so. > + && !bitmap_is_set(&m_table->has_value_set, f->field_index) > + && (non_deterministic_default || next_number_field == f->field_index)) > + break; > + } > + return p; > +} > > /** > Find the best key to use when locating the row in @c find_row(). > @@ -8024,13 +8077,26 @@ static bool record_compare(TABLE *table) > */ > int Rows_log_event::find_key() > { > - uint i, best_key_nr, last_part; > + uint i, best_key_nr; > KEY *key, *UNINIT_VAR(best_key); > ulong UNINIT_VAR(best_rec_per_key), tmp; > DBUG_ENTER("Rows_log_event::find_key"); > DBUG_ASSERT(m_table); > > + #ifdef DBUG_TRACE #ifndef DBUG_OFF > + auto _ = make_scope_exit([this]() { > + DBUG_EXECUTE_IF("rpl_report_chosen_key", > + push_warning_printf(m_table->in_use, > + Sql_condition::WARN_LEVEL_NOTE, > + ER_UNKNOWN_ERROR, "Key chosen: %d", > + m_key_nr == MAX_KEY ? > + -1 : m_key_nr);); > + }); > + #endif > + > + m_key_nr= MAX_KEY; > best_key_nr= MAX_KEY; > + uint parts_suit= 0; > > /* > Keys are sorted so that any primary key is first, followed by unique keys, > @@ -8041,33 +8107,38 @@ int Rows_log_event::find_key() > { > if (!m_table->s->keys_in_use.is_set(i)) > continue; > + parts_suit= key_parts_suit_event(key); > + if (!parts_suit) > + continue; > /* > We cannot use a unique key with NULL-able columns to uniquely identify > a row (but we can still select it for range scan below if nothing better > is available). > */ > - if ((key->flags & (HA_NOSAME | HA_NULL_PART_KEY)) == HA_NOSAME) > + if ((key->flags & (HA_NOSAME | HA_NULL_PART_KEY)) == HA_NOSAME && > + parts_suit == key->user_defined_key_parts) this doesn't make use of ext key parts. Should be something like if (parts_suit == key->ext_key_parts || (parts_suit >= key->user_defined_key_parts && (key->flags & ....))) > { > best_key_nr= i; > best_key= key; > + m_key_parts_suit= parts_suit; > break; > } > /* > We can only use a non-unique key if it allows range scans (ie. skip > FULLTEXT indexes and such). > */ > - last_part= key->user_defined_key_parts - 1; > DBUG_PRINT("info", ("Index %s rec_per_key[%u]= %lu", > - key->name.str, last_part, key->rec_per_key[last_part])); > - if (!(m_table->file->index_flags(i, last_part, 1) & HA_READ_NEXT)) > + key->name.str, parts_suit, key->rec_per_key[parts_suit])); > + if (!(m_table->file->index_flags(i, parts_suit, 1) & HA_READ_NEXT)) > continue; > > - tmp= key->rec_per_key[last_part]; > + tmp= key->rec_per_key[parts_suit - 1]; > if (best_key_nr == MAX_KEY || (tmp > 0 && tmp < best_rec_per_key)) > { > best_key_nr= i; > best_key= key; > best_rec_per_key= tmp; > + m_key_parts_suit= parts_suit; > } > } > > @@ -8494,12 +8568,20 @@ Delete_rows_log_event::do_before_row_operations(const Slave_reporting_capability > if (get_flags(STMT_END_F)) > status_var_increment(thd->status_var.com_stat[SQLCOM_DELETE]); > > + const uchar *curr_row_end= m_curr_row_end; > + unpack_row(rgi, m_table, m_width, m_curr_row, &m_cols, > + &curr_row_end, &m_master_reclength, m_rows_end); > + > + KEY *pk_info= m_table->key_info + m_table->s->primary_key; > if ((m_table->file->ha_table_flags() & HA_PRIMARY_KEY_REQUIRED_FOR_POSITION) && > - m_table->s->primary_key < MAX_KEY) > + m_table->s->primary_key < MAX_KEY && > + key_parts_suit_event(pk_info) == pk_info->ext_key_parts) > { > /* > We don't need to allocate any memory for m_key since it is not used. > */ > + m_key_nr= m_table->s->primary_key; > + m_key_parts_suit= pk_info->ext_key_parts; > return 0; > } > if (do_invoke_trigger()) already commented in the previous review that if "We don't need to allocate any memory" than you `return 0` early wthout doing do_invoke_trigger(). Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 11

Re: [Maria-developers] b2800c06b22: MDEV-28545 MyISAM reorganize partition corrupt older table format
by Sergei Golubchik 25 Oct '22

25 Oct '22

Hi, Alexander, Ok to push. One minor comment below: On Oct 25, Alexander Barkov wrote: > revision-id: b2800c06b22 (mariadb-10.4.26-45-gb2800c06b22) > parent(s): 34163154077 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-10-25 11:53:39 +0400 > message: > > MDEV-28545 MyISAM reorganize partition corrupt older table format > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 6aae927800e..a621825c395 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10068,6 +10068,18 @@ do_continue:; > > set_table_default_charset(thd, create_info, alter_ctx.db); > > + > +#ifdef WITH_PARTITION_STORAGE_ENGINE > + if (!fast_alter_partition) > +#endif To avoid ifdef's you can use if (IF_PARTITIONING(!fast_alter_partition, 1)) > + { > + /* > + We cannot alter partitions and change column data types at the same > + time. > + */ > + Create_field::upgrade_data_types(alter_info->create_list); > + } > + > if (create_info->check_fields(thd, alter_info, > table_list->table_name, table_list->db) || > create_info->fix_period_fields(thd, alter_info)) > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 152295d1cb1: rpl: extra DEFAULTs should not be UPDATEd
by Sergei Golubchik 24 Oct '22

24 Oct '22

Hi, Nikita, As we've discussed on slack, this is wrong. The (serialized) behavior should be as if all DML happened before ALTER. Because if some DML would've happened after ALTER, it would be able to see the new table definition and refer to new columns. On Oct 24, Nikita Malyavin wrote: > revision-id: 152295d1cb1 (mariadb-10.6.1-525-g152295d1cb1) > parent(s): 90274a7428e > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-10-14 23:17:59 +0300 > message: > > rpl: extra DEFAULTs should not be UPDATEd > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 25705d13641..a72005d70ab 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -8832,11 +8832,17 @@ Update_rows_log_event::do_exec_row(rpl_group_info *rgi) > message= thd->wsrep_info; > #endif /* WSREP_PROC_INFO */ > > - /* this also updates m_curr_row_end */ > thd_proc_info(thd, message); > - if (unlikely((error= unpack_current_row(rgi, &m_cols_ai)))) > + Field **default_field= m_table->default_field; > + m_table->default_field= NULL; > + /* this also updates m_curr_row_end */ > + error= unpack_current_row(rgi, &m_cols_ai); > + m_table->default_field= default_field; > + > + if (unlikely(error)) > goto err; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0