IMHO the reported issue is not a bug, because of the following reasons.

Reason 1:-
The MySQL manual states that 'default_storage_engine' and 'storage_engine'
variables are never replicated. Please refer below.

https://dev.mysql.com/doc/refman/5.7/en/replication-features-variables.html
The default_storage_engine and storage_engine system variables are not
replicated, regardless of the logging mode; this is intended to facilitate
replication between different storage engines.

I hope MariaDB also has the same behaviour.

Reason 2:-
SET STATEMENT feature was added as part MDEV-5231. This feature is a port of
"Per query variables from Percona Server".

The ported patch added a test named :rpl_set_statement.test

This test has a scenario do demonstrate that following variables are never
replicated. as part of SET STATEMENTS.

--echo 4) variables that are not replicated at all:
--echo default_storage_engine, storage_engine, max_heap_table_size

Hence the documentation needs to be updated saying the above SET STATEMENTS will
not be replicated.

Please let me know your thoughts.

Hello Andrei,

Please review the latest comments on the JIRA page and provide your thoughts.

Thank you.

sujatha.sivakumar, howdy.

Thanks for briefing me with the feature's history, which really saved a little time !
Your point about not replicated SET of default_storage_engine variable is naturally fair. However two facts should be noted in order to decide about the case.
The first is that the current value of default_storage_engine still plays a role, that is to evaluate
the engine type for CREATE TABLE t (a int);, like here:

MariaDB [test]> SET @@SESSION.default_storage_engine=aria;

MariaDB [test]> CREATE TABLE ta (a INT);

MariaDB [test]> SHOW CREATE TABLE t_a;

+-------+------------------------------------------------------------------------------------------------------+

| Table | Create Table                                                                                         |

+-------+------------------------------------------------------------------------------------------------------+

| t_a   | CREATE TABLE `t_a` (

  `a` int(11) DEFAULT NULL

) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 |

+-------+------------------------------------------------------------------------------------------------------+

After the evaluation the chosen value is also present in Query-log-event (the evidence part is omitted though).

And secondly the whole idea of the SET-STATEMENT
feature is to provide one statement time "dynamic" exception to any current default.
Notice that unlike to the "static" default evaluation case the binlog event does not match in the engine attribute,
which I can read only as a bug.

I hope this makes it more clear.

Cheers,
Andrei

It took time to arrive @ the question, Sujatha.

Hello Andrei,

Good morning. Thank you for the review comments.

The system variables which are set using "SET STATEMENT" are never replicated.
But we just write the query along with the "SET STATEMENT" into the
"Query_log_event". On slave the "SET STATEMENT" syntax is ignored. Only the
actual query gets executed.

But if we look at the 'rpl_set_statement.test' the second test case states that
the system variables which ARE replicated, They must be replicated correctly
with "SET STATEMENT" too.

For example:
-------------------
MariaDB [test]> SET STATEMENT auto_increment_increment=40 FOR INSERT INTO t1
VALUES(@@auto_increment_increment); Query OK, 1 row affected (0.01 sec)

In general 'auto_increment_increment' system variable is replicated. Please have
a look at the following binlog output. "SET @@session.auto_increment_increment=40"
gets added to binary log as part of regular replication. On the slave side this
"SET @@session.auto_increment_increment=40" command will be executed as part of
Query_log_event::do_apply_event. It is not because of
"SET STATEMENT auto_increment_increment=40 ...".

BEGIN

/*!*/;

# at 631

#190531 12:30:07 server id 1  end_log_pos 794     Query    thread_id=5    exec_time=0    error_code=0

SET TIMESTAMP=1559286007/*!*/;

SET @@session.auto_increment_increment=40, @@session.auto_increment_offset=2/*!*/;

SET STATEMENT auto_increment_increment=40 FOR INSERT INTO t1 VALUES(@@auto_increment_increment)

/*!*/;

# at 794

#190531 12:30:07 server id 1  end_log_pos 868     Query    thread_id=5    exec_time=0    error_code=0

SET TIMESTAMP=1559286007/*!*/;

"SET STATEMENT" syntax is ignored on slave. The reason why the implementer chose
it that way is, he/she didn't want to introduce any change of behavior
with respect to system variables which should/should not be replicated.

Hence the original code looks like this.

File: sql/sql_parse.cc

 if (!lex->stmt_var_list.is_empty() && !thd->slave_thread)

  {

    Query_arena backup;

    DBUG_PRINT("info", ("SET STATEMENT %d vars", lex->stmt_var_list.elements));

The internal implementation of 'SET STATEMENT' looks like this

SET @save_value=@@var1;

SET SESSION var1=value1;

stmt;

SET SESSION var1=@save_value;

For example:
-------------------
If user sets 'storage_engine' at session level, it is never replicated.

MariaDB [test]> set @@session.storage_engine=aria;

Query OK, 0 rows affected (0.00 sec)

MariaDB [test]> create table t ( f int);

Query OK, 0 rows affected (0.00 sec)

MariaDB [test]> show create table t;

+-------+----------------------------------------------------------------------------------------------------+

| Table | Create Table                                                                                       |

+-------+----------------------------------------------------------------------------------------------------+

| t     | CREATE TABLE `t` (

  `f` int(11) DEFAULT NULL

) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 |

+-------+----------------------------------------------------------------------------------------------------+

1 row in set (0.00 sec)

Binary log output:
---------------------------

SET @@session.character_set_client=33,@@session.collation_connection=33,@@session.collation_server=8/*!*/;

SET @@session.lc_time_names=0/*!*/;

SET @@session.collation_database=DEFAULT/*!*/;

create table t ( f int)

/*!*/;

DELIMITER ;

# End of log file

Conclusion: Since "SET STATEMENT" also acts like above it should not replicate
the "storage_engine" variable. Hence we should not change this behavior.

Please let me know your thoughts.

sujatha.sivakumar, thanks for a thorough analysis and detailing! The matter finally settled in my mind. Like you say in The internal implementation it's fair to think of SET STATEMENT var1,var2,..., var_n FOR Query as a shortcut: save old values; set var:s; exec Query; restore var:s. Now we notice that among the var:s we have ones that are present in Query_log_event::"context" (replicated), and those that are not. Those that are currently not should not get into the context. Those that are in already don't need {{SET STATEMENT var1,var2,..., var_n }} preamble be replicated.
Hence the original design decision makes sense. Secondly I have to agree with not-a-bug status.
Yet we need to acknowledge and tackle a use cases where a not replicatable SET-STATEMENT var is referred in the Query explicitly like SET STATEMENT var=value INSERT INTO t SET a=@@var. The case is clear consistency issue as var=value is not set into the exec context of Query on slave.

I suggest we consider such cases are replication unsafe. Now the question is how to implement that technically, and also when/how urgently.

Hello,

the same behavior is observed with old_alter_table:

SET STATEMENT old_alter_table = 1 ALTER XXX

While in show processlist on the replica, the SET STATEMENT is displayed in the command, under the hood the ALTER is being performed without setting the variable as requested (State: ALTERING TABLE) and taking much longer than planned. This can be confusing for the User/DBA.

| 948403 | system user | | db_name | Slave_SQL | 329 | altering table | SET STATEMENT old_alter_table = 1 FOR alter table my_table add column_name smallint defaul | 0.000 |

Maybe this should be documented more explicitly as not supported, and some downtime can be avoided.

Cheers,
Joffrey

sujatha.sivakumar: Could we try something like Annotate_log_event that is to embed the original SET-query into a new char[] Query_log_event::status_var::set_query. The variable would be replicated along with others see Query_log_event::write to replace Query_log_event::query on slave.

joffrey92 Indeed! Thank you. We're still considering if/and how to fix.

This is not only related to SET STATEMENT but with the whole environment on the master

Sure, but it has always been so, as a known legacy limitation that we only replicate a tiny part of the environment.
At the point of filing this report, SET STATEMENT was a new addition, and in my opinion it wasn't obvious at all (and still isn't really) that it should behave the same way as if the master were executing consequently SET var=new_value; <query>; SET var=old_value. While I understand the logic induced by the internal implementation, intuitively, it could be expected that if the statement is executed as a whole and is written in the binlog as a whole, it should also be executed by the slave as a whole.

Agree that the simple solution is to include the original SET STATEMENT into the binary log.

I agree with the idea that SET STATEMENT X=Y FOR <stmt> should be identical to SET old=X; SET X=Y; <stmt>; SET X=old.

I think this is needed to ensure that the semantics is clear and correct. This way, it is easy to describe and for the user to understand what it does. And making it work differently opens all kinds of potential corner cases.

That still leaves an issue. It is then a bit strange that the <stmt> is not binlogged the same way, but the entire SET STATEMENT ... original client SQL string appears in the Query_log_event (in statement-based binlogging). And it currently means that mysqlbinlog | mysql does not handle it identical to explicit set/restore, as the SET STATEMENT ... takes effect there.

The best might be to remove the initial SET STATEMENT ... FOR part from the query string entirely before binlogging. We do have some query string rewriting before binlogging already (INSERT DELAYED...), though I'd still prefer to keep such rewriting to an absolute minimum.

This is a good example of how important it is to carefully think through all aspects of a new feature (SET STATEMENT ... in this case). The MariaDB code is so stuffed with features and options that there are almost always tricky interactions that need to be carefully thought through in the design before coding and pushing. At least SET STATEMENT ... doesn't allow subqueries or stored function calls to get the temporary values to set.

knielsen, also considering you were last to comment on the case, I set you as the prime reviewer of the updated description. Looking forward to read your input!

I read the updated description. Unfortunately, it is not clear to me what
exactly is proposed?

I see "effectively lift the design's constrain" and "I suggest to indeed
revise that decision". But I do not see exactly what is proposed as the
implementation.

Also, I do not understand what is meant with this: "The replicated with
session-variables SET-STATEMENT-FOR-Query could be engaged further on cases
when correctness can be accomplished (addresses Michael Widenius's note)
only with more reach execution context than currently Query_log_event is
provided with."

It sounds to me that what is suggested involves creating a list of further
session variables that must be replicated (for Query_log_event) along with
those already replicated (like current timestamp and so on). That is
probably appropriate. Note that such list must be a white-list; each new
variable must be added after suitable consideration, as opposed to
replicating everything except a list of exceptions. This is already how it
is done for the existing variables replicated with the Query_log_event.

As I wrote earlier, it is important that these two behave the same:

SET STATEMENT var= value FOR <query>

and

SET @old= @@SESSION.var;

SET SESSION var= value;

<query>

SET SESSION var= @old;

This is achieved by added more variables to the list of what is replicated
for Query_log_event.

This does not solve the problem that `mysqlbinlog | mysql` will work
differently, obeying all SET STATEMENT ... FOR regardless of which
variable. I am not super fond of my earlier suggestion about query string
rewriting for the Query_log_event. I think the more general problem of
getting `mysqlbinlog | mysql` to be consistent with replication should be
solved in a more general manner, by implementing something that makes this
use the same code paths inside the server as the replication slave, so that
the various quirks and workarounds are the same between the two. Such
general solution is outside the scope of this particular MDEV.