maria-developers team mailing list archive
Mailing list archive
Re: d66d5457589d: MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade to 10.5, mysql_upgrade should take of that
On Nov 12, sujatha wrote:
> >> diff --git a/mysql-test/main/grant.test b/mysql-test/main/grant.test
> >> index 38baf6738255..63be18ecb292 100644
> >> --- a/mysql-test/main/grant.test
> >> +++ b/mysql-test/main/grant.test
> >> @@ -1833,6 +1833,7 @@ GRANT REPLICATION SLAVE ON *.* TO mysqltest_u1@localhost;
> >> GRANT SHOW DATABASES ON *.* TO mysqltest_u1@localhost;
> >> GRANT SHUTDOWN ON *.* TO mysqltest_u1@localhost;
> >> GRANT USAGE ON *.* TO mysqltest_u1@localhost;
> >> +GRANT REPLICA MONITOR ON *.* TO mysqltest_u1@localhost;
> > But the parser should, of course, accept both, as everywhere.
> Yes, both REPLICA MONITOR/ SLAVE MONITOR will work.
Right, I know. I meant that it's fine (and even good) to use
GRANT REPLICA MONITOR in test files, no matter how it's shown
in SHOW GRANTS.
> >> diff --git a/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test
> >> new file mode 100644
> >> index 000000000000..f7b2af2e0c51
> >> --- /dev/null
> >> +++ b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test
> >> @@ -0,0 +1,90 @@
> >> +# ==== Purpose ====
> >> +#
> >> +# REPLICA MONITOR privilege is required to execute following commands.
> >> +# SHOW SLAVE STATUS
> >> +# SHOW RELAYLOG EVENTS
> >> +#
> >> +# ==== Implementation ====
> >> +#
> >> +# Step1: GRANT ALL privileges for a new user 'user1' and then REVOKE REPLICA
> >> +# MONITOR and SUPER privileges.
> >> +# Step2: Execute SHOW SLAVE STAUTS/SHOW RELAYLOG commands and expect
> >> +# ER_SPECIFIC_ACCESS_DENIED_ERROR. This also verifies that REPLICATION
> >> +# SLAVE ADMIN privilege is not required for these two commands.
> >> +# Step3: GRANT REPLICA MONITOR privilege and observe that both commands are
> >> +# allowd to execute.
> >> +# Step4: GRANT SUPER privilege and observe that only SHOW SLAVE STATUS command
> >> +# is allowed.
> > why do you need a full master-slave replication setup for that?
> It can be added in main suite as well. Since these are replication
> specific privileges, added them as part of 'rpl' suite.
I think it's better to have it in the main suite.
rpl suite is for testing replication features that need a working
replication topology set up.
For example binlog specific tests that do not need working replication
go into the binlog suite, not in rpl.
> >> +--error ER_SPECIFIC_ACCESS_DENIED_ERROR
> >> +SHOW SLAVE STATUS;
> >> +--echo "Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW RELAYLOG EVENTS"
> >> +--echo "Expected error: Access denied; you need (at least one of) the REPLICA MONITOR privilege(s) for this operation"
> >> +--disable_ps_protocol
> > why?
> I observed following failure during buildbot testing
> rpl.rpl_replica_monitor_priv 'mix' w1 [ fail ] Test ended at 2020-11-10
> 11:22:28 CURRENT_TEST: rpl.rpl_replica_monitor_priv mysqltest: At line
> 46: query 'SHOW RELAYLOG EVENTS' failed with wrong errno 1295: 'This
> command is not supported in the prepared statement protocol yet',
> instead of 1227... To address this issue I disabled the command.
This is a bug.
It is assumed that mysqltest will only use ps protocol for statements
that support it. It has a big regex to detect that. Alas, it only
which matches all SHOW commands, and it'd be rather annoying and fragile
to list all SHOW variants excluding just RELAYLOG (and BINLOG) EVENTS.
On the other hand, it seems really easy to make them to support ps
protocol. Perhaps it only needs something like (untested):
diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
@@ -2369,6 +2369,14 @@ static bool check_prepared_statement(Prepared_statement >
+ case SQLCOM_SHOW_BINLOG_EVENTS:
+ case SQLCOM_SHOW_RELAYLOG_EVENTS:
+ List<Item> field_list;
+ Log_event::init_show_field_list(thd, &field_list);
+ if ((res= send_stmt_metadata(thd, stmt, &field_list)) == 2)
#endif /* EMBEDDED_LIBRARY */
if ((res= mysql_test_show_create_routine(stmt, &sp_handler_procedure)) == >
Could you try that, please? If it'll work, better to commit it
> >> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> >> index ef96e8f24843..8a3551c14f53 100644
> >> --- a/sql/sql_acl.cc
> >> +++ b/sql/sql_acl.cc
> >> @@ -1057,6 +1057,9 @@ class User_table_tabular: public User_table
> >> if (access & REPL_SLAVE_ACL)
> >> access|= REPL_MASTER_ADMIN_ACL;
> >> + if (access & FILE_ACL)
> >> + access|= REPLICA_MONITOR_ACL;
> > Why is that?
> Please find the reason mentioned as part of the following comment.
Yes, I've seen it already, but I still don't understand
what FILE_ACL has to do with REPLICA_MONITOR_ACL.
> > So, indeed, old code was correct, that if() should be inside SUPER_ACL,
> > one should only get REPL_MASTER_ADMIN_ACL if one had both SUPER_ACL and
> > REPL_SLAVE_ACL.
> I will try to rewrite this part such that
> SUPER_ACL / REPL_CLIENT_ACL - SLAVE_MONITOR_ACL
> REPL_SLAVE_ACL - SLAVE_MONITOR_ACL
VP of MariaDB Server Engineering