[MDEV-28820] MyISAM wrong server status flags Created: 2022-06-13  Updated: 2023-10-22  Resolved: 2023-10-17

Status: Closed
Project: MariaDB Server
Component/s: Locking, Protocol, Storage Engine - MyISAM
Affects Version/s: 10.3, 10.4, 10.5, 10.6, 10.7, 10.8
Fix Version/s: 10.4.32, 10.5.23, 10.6.16, 10.10.7, 10.11.6, 11.0.4, 11.1.3

Type: Bug Priority: Critical
Reporter: Diego Dupin Assignee: Sergei Golubchik
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Problem/Incident
causes CONJ-948 Under certain circumstances, Connecto... Closed
Relates
relates to MDEV-30889 Memory leak issues with MariaDB 10.6.... Stalled

 Description   

created after CONJ-948

Synchronous connectors can rely on OK_Packet/EOF_Packet server status's SERVER_STATUS_IN_TRANS flag that explicitly indicate if a transaction is currently active.

Java connector use that information to avoid issuing a rollback/commit command, skipping the command if there is no transaction.

Problem concerns only MyISAM that never set SERVER_STATUS_IN_TRANS flag. Still there is some case when there is some kind of lock : ( example from serg: )

create table t1 (a int)  engine=myisam;
 
 
 
insert t1 values (1);

and in another 

drop table t1;

this DROP TABLE will wait until the transaction ends.

  • either MyISAM should return SERVER_STATUS_IN_TRANS flag, so connector know there is really a commit to be issued.
  • either MyISAM doesn't set any lock. (It may be just an issue : for some metadata locks are taken on the object name, before the table is opened, before we know it's a MyISAM table.)

Either way is ok.

(Another solution would be to always issue a COMMIT/ROLLBACK statement when there is no need, but that would degrade performance dramatically for some application. For example when connections are given back to pools, pools generally issues a connection.rollback() to ensure connection state, and there is lots of application where that command would represent 25% of the commands)



 Comments   
Comment by Oleksandr Byelkin [ 2022-10-04 ]

I added reporting server status and it is what we have now:

create table t1 (a int) engine=innodb;
 
set autocommit=0;
 
--- INNODB start transaction
 
start transaction;
 
affected rows: 0
 
status: 1
 
select * from t1;
 
a
 
affected rows: 0
 
status: 21
 
insert t1 values (1);
 
affected rows: 1
 
status: 1
 
select * from t1;
 
a
 
1
 
affected rows: 1
 
status: 21
 
drop table t1;
 
create table t1 (a int) engine=aria;
 
commit;
 
set autocommit=0;
 
--- ARIA start transaction
 
start transaction;
 
affected rows: 0
 
status: 1
 
select * from t1;
 
a
 
affected rows: 0
 
status: 1
 
insert t1 values (1);
 
affected rows: 1
 
status: 1
 
select * from t1;
 
a
 
1
 
affected rows: 1
 
status: 1
 
drop table t1;
 
create table t1 (a int) engine=innodb;
 
commit;
 
set autocommit=0;
 
--- INNODB autmatic transaction
 
select * from t1;
 
a
 
affected rows: 0
 
status: 21
 
insert t1 values (1);
 
affected rows: 1
 
status: 1
 
select * from t1;
 
a
 
1
 
affected rows: 1
 
status: 21
 
drop table t1;
 
create table t1 (a int) engine=aria;
 
commit;
 
set autocommit=0;
 
--- ARIA automatic transaction
 
select * from t1;
 
a
 
affected rows: 0
 
status: 0
 
insert t1 values (1);
 
affected rows: 1
 
status: 0
 
select * from t1;
 
a
 
1
 
affected rows: 1
 
status: 0
 
drop table t1;

test case:

--source include/have_innodb.inc
 
create table t1 (a int) engine=innodb;
 
set autocommit=0;
 
--echo --- INNODB start transaction
 
--enable_info
 
start transaction;
 
select * from t1;
 
insert t1 values (1);
 
select * from t1;
 
--disable_info
 
drop table t1;
 
create table t1 (a int) engine=aria;
 
commit;
 
set autocommit=0;
 
--echo --- ARIA start transaction
 
--enable_info
 
start transaction;
 
select * from t1;
 
insert t1 values (1);
 
select * from t1;
 
--disable_info
 
drop table t1;
 
create table t1 (a int) engine=innodb;
 
commit;
 
set autocommit=0;
 
--echo --- INNODB autmatic transaction
 
--enable_info
 
select * from t1;
 
insert t1 values (1);
 
select * from t1;
 
--disable_info
 
drop table t1;
 
create table t1 (a int) engine=aria;
 
commit;
 
set autocommit=0;
 
--echo --- ARIA automatic transaction
 
--enable_info
 
select * from t1;
 
insert t1 values (1);
 
select * from t1;
 
--disable_info
 
drop table t1;

diff:

diff --git a/client/mysqltest.cc b/client/mysqltest.cc
 
index 2a6d4265e59..20b7762b0cf 100644
 
--- a/client/mysqltest.cc
 
+++ b/client/mysqltest.cc
 
@@ -7729,11 +7729,13 @@ void append_metadata(DYNAMIC_STRING *ds,
 
 */
 
 
 void append_info(DYNAMIC_STRING *ds, ulonglong affected_rows,
 
-                 const char *info)
 
+                 const char *info, unsigned int status)
 
 {
 
   char buf[40], buff2[21];
 
   sprintf(buf,"affected rows: %s\n", llstr(affected_rows, buff2));
 
   dynstr_append(ds, buf);
 
+  sprintf(buf,"status: %x\n", status);
 
+  dynstr_append(ds, buf);
 
   if (info)
 
   {
 
     dynstr_append(ds, "info: ");
 
@@ -7985,7 +7987,8 @@ void run_query_normal(struct st_connection *cn, struct st_command *command,
 
         query to find the warnings.
 
       */
 
       if (!disable_info)
 
-	append_info(ds, mysql_affected_rows(mysql), mysql_info(mysql));
 
+	append_info(ds, mysql_affected_rows(mysql), mysql_info(mysql),
 
+                    mysql->server_status);
 
 
       if (display_session_track_info)
 
         append_session_track_info(ds, mysql);
 
@@ -8409,7 +8412,8 @@ void run_query_stmt(struct st_connection *cn, struct st_command *command,
 
       otherwise.
 
     */
 
     if (!disable_info)
 
-      append_info(ds, mysql_stmt_affected_rows(stmt), mysql_info(mysql));
 
+      append_info(ds, mysql_stmt_affected_rows(stmt), mysql_info(mysql),
 
+                  mysql->server_status);
 
 
     if (display_session_track_info)
 
       append_session_track_info(ds, mysql);

Comment by Oleksandr Byelkin [ 2022-10-11 ]

Try to fix which does not go well:

diff --git a/sql/mdl.cc b/sql/mdl.cc
 
index a53a1f08c2c..7f55dbf6fd4 100644
 
--- a/sql/mdl.cc
 
+++ b/sql/mdl.cc
 
@@ -1931,6 +1931,13 @@ MDL_context::try_acquire_lock_impl(MDL_request *mdl_request,
 
     m_tickets[mdl_request->duration].push_front(ticket);
 
 
     mdl_request->ticket= ticket;
 
+
 
+    if (mdl_request->duration == MDL_TRANSACTION &&
 
+        mdl_request->type == MDL_EXCLUSIVE)
 
+    {
 
+      m_owner->get_thd()->server_status|= SERVER_STATUS_IN_TRANS;
 
+    }
 
+
 
   }
 
   else
 
     *out_ticket= ticket;

there are asserts which now fires and problem like that : 

Cannot modify @@session.sql_log_bin inside a transaction

Comment by Oleksandr Byelkin [ 2022-11-16 ]

I updated time estimate (I have feeling we need to rewrite a lot to make it working as connector wants)

Comment by Oleksandr Byelkin [ 2023-03-28 ]

The fix require rewriting the status usage in the server, because the status flag (SERVER_STATUS_IN_TRANS) also taken into account by many server parts (like releasing transactional locks and so on), I am not sure such rewrite can be done in old versions.

Comment by Oleksandr Byelkin [ 2023-10-17 ]

OK to push

Generated at Thu Feb 08 10:03:43 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.