[MDEV-28820] MyISAM wrong server status flags - Jira

Details

Type: Bug
Status: Closed (View Workflow)
Priority: Critical
Resolution: Fixed
Affects Version/s: 10.3(EOL), 10.4(EOL), 10.5, 10.6, 10.7(EOL), 10.8(EOL)
Fix Version/s: 10.4.32, 10.5.23, 10.6.16, 10.10.7, 10.11.6, 11.0.4, 11.1.3
Component/s: Locking, Protocol, Storage Engine - MyISAM
Labels:
None

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)

Attachments

Issue Links

causes

CONJ-948 Under certain circumstances, Connector J does not send "commit"

Closed

relates to

MDEV-30889 Memory leak issues with MariaDB 10.6.12 and OOM Crashes

Confirmed

Activity

Ascending order - Click to sort in descending order

Oleksandr Byelkin added a comment - 2022-10-04 13:11

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;

affected rows: 0

status: 21

insert t1 values (1);

affected rows: 1

status: 1

select * from t1;

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;

affected rows: 0

status: 1

insert t1 values (1);

affected rows: 1

status: 1

select * from t1;

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;

affected rows: 0

status: 21

insert t1 values (1);

affected rows: 1

status: 1

select * from t1;

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;

affected rows: 0

status: 0

insert t1 values (1);

affected rows: 1

status: 0

select * from t1;

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);

Oleksandr Byelkin added a comment - 2022-10-04 13:11 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);

Oleksandr Byelkin added a comment - 2022-10-11 09:26

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

Oleksandr Byelkin added a comment - 2022-10-11 09:26 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

Oleksandr Byelkin added a comment - 2022-11-16 10:03

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

Oleksandr Byelkin added a comment - 2022-11-16 10:03 I updated time estimate (I have feeling we need to rewrite a lot to make it working as connector wants)

Oleksandr Byelkin added a comment - 2023-03-28 07:38

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.

Oleksandr Byelkin added a comment - 2023-03-28 07:38 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.

Oleksandr Byelkin added a comment - 2023-10-17 09:49

OK to push

Oleksandr Byelkin added a comment - 2023-10-17 09:49 OK to push

MariaDB Server

MyISAM wrong server status flags

Details

Description

Attachments

Issue Links

Activity

People

Dates

Git Integration