Bug 103685 - SQL... dialog Status: "Commands out of sync" when connecting to MySQL using direct (native) connector
Summary: SQL... dialog Status: "Commands out of sync" when connecting to MySQL using d...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
5.2.2.2 release
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.0.0 target:5.4.3
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-03 19:57 UTC by Howard Johnson
Modified: 2017-09-25 12:50 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Howard Johnson 2016-11-03 19:57:21 UTC
When connecting to a MySQL database (served by a Mariadb 10.1 server) via the LibreOffice "direct" connector method (i.e. a "Native" connector), the Execute SQL Statement dialog can get confused and get stuck issuing this incorrect Status:

  "Commands out of sync; you can't run this command now".


This occurs only when both of the following are true:

* the 'Show output of "select" statements' box is not checked, and 
* when a query returns results.


Connecting via JDBC does not exhibit this bug.  In other words, JDBC works where MySQL native is broken.


I suspect that what needs to be fixed is to get any results and throw them away when 'Show output of "select" statements' is not checked.  This would keep MySQL happy.




=== How to setup the bug =======================================


--- STEP 1 - CONNECTING -------------

File | New | Database.
-----------
Database Wizard
[x] Connect to an existing database
[MySQL]
Next>>

------------
[x] Connect directly
Next>>

------------
Database name: a_database
 [*] Server/port
      Server: localhost
      Port: 3306
Next>>

------------
User name  root
[v] Password required
Test connection
    Ok
Next>>

-----------
..register the database in LibreOffice?  No
[x] Open the database for editing
Finish

-----------
Save as: bigbase mysql direct.odb



--- STEP 2 - TESTING SQL... ----------
Select: Tools | SQL...

-----------
In the "Execute SQL Statement" dialog box:

SQL Command

  Command to execute:


     select * from `table1`;
     [x] Show output of "select" statements
     Execute
     Status: Command successfully executed.  (This is ok)

     ---
     select * from `table1`;
     [x] Show output of "select" statements
     Execute
     Status: Command successfully executed.  (This is ok)

     -------
     select * from `table1`;
     [ ] Show output of "select" statements
     Execute
     Status: Command successfully executed.  (This is ok)

     ---
     select * from `table1`;
     [x] Show output of "select" statements
     Execute
     Status: Commands out of sync; you can't run this command now  (This FAILS)



----
This is the full accumulated output in the status box:

1: Command successfully executed.

2: Command successfully executed.

3: Command successfully executed.

4: Commands out of sync; you can't run this command now
Comment 1 Howard Johnson 2016-11-03 20:04:23 UTC
While you're at it, this dialog could use a little better status to indicate that it is still working, but slowly.  I have a 800 record database with a bunch of fields and a "select * from table;" took so long that I thought LibreOffice had crashed, as it appeared to just hang forever.

Later I discovered that it just took a very long time.  Then I added this, ""select * from table LIMIT 100;"

And/OR you might suggest to add a LIMIT to clue people into the fact that it might be slow on big tables.

Thanks!
Comment 2 robert 2016-11-03 20:53:13 UTC
Could confirm this buggy behavior.
Every time you change from unchecked 'Show output ...' in Tools > SQL to checked 'Show output ...' "Commands out of sync; you can't run this command now" appears.

Have tested with direct driver. LO 5.2.3.3 on OpenSUSE 42.1 64bit rpm Linux.
Comment 3 Julien Nabet 2017-09-19 20:27:01 UTC
Lionel: When in SQL direct, Select is used + show results is checked, "executeQuery" is used and it seems there's no pb.
In the other cases, "execute" is used. I tried to debug and think the pb is in mariadb-connector-c since "Commands out of sync" comes from this part but not easy to follow. The execute method should return with status = MYSQL_STATUS_READY but it's not the case.
In mysql-connector-cpp/driver/mysql_statement.cpp
we go line 87:
     80 MySQL_Statement::do_query(const ::sql::SQLString &q)
     81 {
     82         CPP_ENTER("MySQL_Statement::do_query");
     83         CPP_INFO_FMT("this=%p", this);
     84         checkClosed();
     85 
     86         if (proxy->query(q) && proxy->errNo()) {
     87                 CPP_ERR_FMT("Error during proxy->query : %d:(%s) %s", proxy->errNo(), proxy->sqlstate().c_str(), proxy->error().c_str());
     88                 sql::mysql::util::throwSQLException(*proxy.get());
     89         }
     90 
     91         warningsCount= proxy->warning_count();
     92 
     93         warningsHaveBeenLoaded= false;
     94 }


The pb in tdf#112423 must be similar
Comment 4 Julien Nabet 2017-09-19 20:45:18 UTC
Lionel: Taking a look to MariaDB website, 3.0.2 version of the C connector is out (see https://downloads.mariadb.org/connector-c/3.0.2/), compared to 2.0.0. But there are also 2.1.0 + 2.2 and 2.3 branches if we don't want to jump to 3.X version

About mysql connector c++, there's now 1.1.10 (see https://dev.mysql.com/doc/relnotes/connector-cpp/en/news-1-1.html), compared to 1.1.4

Of course, upgrading all this + updating the associated patches (THIS is really a PITA, I don't know a good and simple procedure for this) from LO is quite a big work and not for beginners I suppose.

I don't know on what you're working right now but i'm afraid nobody would give it a try if you don't have time or are on other things.
Comment 5 Julien Nabet 2017-09-19 22:29:04 UTC
Lionel:
I forced to update status by calling cppStatement->getResultSet()
patch:
diff --git a/mysqlc/source/mysqlc_statement.cxx b/mysqlc/source/mysqlc_statement.cxx
index 3a082004831a..6e425c2a28cd 100644
--- a/mysqlc/source/mysqlc_statement.cxx
+++ b/mysqlc/source/mysqlc_statement.cxx
@@ -141,6 +141,7 @@ sal_Bool SAL_CALL OCommonStatement::execute(const rtl::OUString& sql)
     bool success = false;
     try {
         success = cppStatement->execute(rtl::OUStringToOString(sSqlStatement, m_pConnection->getConnectionSettings().encoding).getStr());
+        cppStatement->getResultSet();
     } catch (const sql::SQLException &e) {
         mysqlc_sdbc_driver::translateAndThrow(e, *this, m_pConnection->getConnectionEncoding());
     }

I suppose it's a bit quick and dirty but at least, I don't reproduce the problem with it.

I also tried tdf#112423 but no better.

Do you think I should push the patch?
Comment 6 Lionel Elie Mamane 2017-09-20 05:03:33 UTC
(In reply to Julien Nabet from comment #5)
> Lionel:
> I forced to update status by calling cppStatement->getResultSet()
> patch:

> --- a/mysqlc/source/mysqlc_statement.cxx
> +++ b/mysqlc/source/mysqlc_statement.cxx
> @@ -141,6 +141,7 @@ sal_Bool SAL_CALL OCommonStatement::execute(const
>      try {
>          success =
> cppStatement->execute(rtl::OUStringToOString(sSqlStatement,
> m_pConnection->getConnectionSettings().encoding).getStr());
> +        cppStatement->getResultSet();
>      } catch (const sql::SQLException &e) {

This looks like it fetches the first result set and then throws it away. Always. Even when the caller wants the result. I'm rather sure this will cause other problems. E.g. when the query actually returns multiple result sets, the caller will not get the first one, only the second, third, etc.

The solution is probably in code that _calls_ that. E.g. the code that implements the "Execute SQL Statement" dialog.
Comment 7 Julien Nabet 2017-09-20 05:17:41 UTC
(In reply to Lionel Elie Mamane from comment #6)
> (In reply to Julien Nabet from comment #5)
> > Lionel:
>...
> This looks like it fetches the first result set and then throws it away.
> Always. Even when the caller wants the result. I'm rather sure this will
> cause other problems. E.g. when the query actually returns multiple result
> sets, the caller will not get the first one, only the second, third, etc.
> 
> The solution is probably in code that _calls_ that. E.g. the code that
> implements the "Execute SQL Statement" dialog.

Also, in case of a Select like here, I wonder if we shouldn't simply call executeQuery (as when checkbox is checked). The only diff would be not displaying the result.
Anyway, I'll give a try to your suggestion.
Comment 8 Julien Nabet 2017-09-20 08:05:54 UTC
The call to getResultSet() is really just to update the status, not for using a rs.

Some code pointer:
DirectSQLDialog::implExecuteStatement from
https://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/dlg/directsql.cxx#179
197                  if (_rStatement.toAsciiUpperCase().startsWith("SELECT") && m_pShowOutput->IsChecked())
198                  {
199                      // execute it as a query
200                      xResultSet = xStatement->executeQuery(_rStatement);
...
226                  } else {
227                      // execute it
228                      xStatement->execute(_rStatement);
229                  }

Location where mysql c++ connector is called for execute
https://opengrok.libreoffice.org/xref/core/mysqlc/source/mysqlc_statement.cxx#135
135  sal_Bool SAL_CALL OCommonStatement::execute(const rtl::OUString& sql)
136  {
137      MutexGuard aGuard(m_aMutex);
138      checkDisposed(rBHelper.bDisposed);
139      const rtl::OUString sSqlStatement = m_pConnection->transFormPreparedStatement( sql );
140  
141      bool success = false;
142      try {
143          success = cppStatement->execute(rtl::OUStringToOString(sSqlStatement, m_pConnection->getConnectionSettings().encoding).getStr());
144      } catch (const sql::SQLException &e) {
145          mysqlc_sdbc_driver::translateAndThrow(e, *this, m_pConnection->getConnectionEncoding());
146      }
147      return success;
148  }

and in case of executeQuery
150  Reference< XResultSet > SAL_CALL OCommonStatement::executeQuery(const rtl::OUString& sql)
151  {
152      MutexGuard aGuard(m_aMutex);
153      checkDisposed(rBHelper.bDisposed);
154      const rtl::OUString sSqlStatement = m_pConnection->transFormPreparedStatement(sql);
155  
156      Reference< XResultSet > xResultSet;
157      try {
158          std::unique_ptr< sql::ResultSet > rset(cppStatement->executeQuery(rtl::OUStringToOString(sSqlStatement, m_pConnection->getConnectionEncoding()).getStr()));
159          xResultSet = new OResultSet(this, rset.get(), m_pConnection->getConnectionEncoding());
160          rset.release();
161      } catch (const sql::SQLException &e) {
162          mysqlc_sdbc_driver::translateAndThrow(e, *this, m_pConnection->getConnectionEncoding());
163      }
164      return xResultSet;
165  }

In executeQuery, we retrieve a rs that we can release, it's not the case for execute.

I'll check if we could call something else than getResultSet() (a method to close/reset the statement so the status is set to MYSQL_STATUS_READY) because it's the only thing lacking here ; there's no error in the whole mysql c++ connector and mariadb c connector (unless I missed it)
Comment 9 Julien Nabet 2017-09-21 22:39:27 UTC
(In reply to Lionel Elie Mamane from comment #6)
...
> This looks like it fetches the first result set and then throws it away.
> Always. Even when the caller wants the result. I'm rather sure this will
> cause other problems. E.g. when the query actually returns multiple result
> sets, the caller will not get the first one, only the second, third, etc.
> 
> The solution is probably in code that _calls_ that. E.g. the code that
> implements the "Execute SQL Statement" dialog.

I'm not sure to understand, execute and executeUpdate never use resultsets or did I miss something?
If yes, I don't achieve to use getMoreResults/getResultSet/disposeResultSet  to throw away rs correctly.
Here's a small bt to see the link between execute methods:
#0  0x00007f0022c89549 in connectivity::mysqlc::OCommonStatement::execute(rtl::OUString const&) (this=0x555557f4ccf0, sql="CALL `AllNames`;")
    at /home/julien/lo/libreoffice/mysqlc/source/mysqlc_statement.cxx:138
#1  0x00007f0029afefcb in OStatement::execute(rtl::OUString const&) (this=0x555557f4e640, _rSQL="CALL `AllNames`;")
    at /home/julien/lo/libreoffice/dbaccess/source/core/api/statement.cxx:489
#2  0x00007f0026e6b2b6 in dbaui::DirectSQLDialog::implExecuteStatement(rtl::OUString const&) (this=0x555557ec9890, _rStatement="CALL `AllNames`;")
    at /home/julien/lo/libreoffice/dbaccess/source/ui/dlg/directsql.cxx:228
Comment 10 robert 2017-09-22 14:41:39 UTC
(In reply to Julien Nabet from comment #9)
> 
> I'm not sure to understand, execute and executeUpdate never use resultsets
> or did I miss something?
> If yes, I don't achieve to use getMoreResults/getResultSet/disposeResultSet 
> to throw away rs correctly.

The procedure you are using for the test won't give a resultset in Tools > SQL or in Query-GUI. But it has an resultset you can fetch through macrocode or if you call the procedure in MySQL-console (or for example PHPMyAdmin).

ExecuteUpdate shouldn't give a resultset. ExecuteQuery (with the same code for calling a procedure) should give a resultset.
Comment 11 Julien Nabet 2017-09-22 14:49:59 UTC
(In reply to robert from comment #10)
> (In reply to Julien Nabet from comment #9)
> > 
> > I'm not sure to understand, execute and executeUpdate never use resultsets
> > or did I miss something?
> > If yes, I don't achieve to use getMoreResults/getResultSet/disposeResultSet 
> > to throw away rs correctly.
> 
> The procedure you are using for the test won't give a resultset in Tools >
> SQL or in Query-GUI. But it has an resultset you can fetch through macrocode
> or if you call the procedure in MySQL-console (or for example PHPMyAdmin).
Ok
> 
> ExecuteUpdate shouldn't give a resultset. ExecuteQuery (with the same code
> for calling a procedure) should give a resultset.
Depending where we're looking at we can have until 3 functions :
- executeQuery
- execute
- executeUpdate

for Tools/SQL, it's just executeQuery or execute. But the levels below have the 3 ones.
Anyway, I'll try to find a way to obtain the right result.
Comment 12 Julien Nabet 2017-09-22 21:46:07 UTC
Lionel: Since it seems the cleaning must be in dbaccess/source/ui/dlg/directsql.cxx so resultsets can be available in dbaccess/source/core/api/statement.cxx for macros or other things, I'm stuck.
Indeed, I began with:
diff --git a/mysqlc/source/mysqlc_statement.cxx b/mysqlc/source/mysqlc_statement.cxx
index 3a082004831a..9ff3ce163fb6 100644
--- a/mysqlc/source/mysqlc_statement.cxx
+++ b/mysqlc/source/mysqlc_statement.cxx
@@ -240,7 +240,7 @@ sal_Bool SAL_CALL OCommonStatement::getMoreResults()
 
     // if your driver supports more than only one resultset
     // and has one more at this moment return true
-    return false;
+    return cppStatement->getMoreResults();
 }

but then, I don't know how to call getMoreResults() from directsql.cxx.
I tried to declare OStatement::getMoreResults (because  OStatementBase::getMoreResults already existts) in statement.hxx + statement.cxx but I've got an error when building.
I think it's perhaps linked to XStatement.idl but don't want to touch this since it's a public interface and I don't know the consequences.
I also tried to use XMultipleResults but since execute method doesn't return anything, I can't use XMultipleResults
Comment 13 Lionel Elie Mamane 2017-09-23 07:08:09 UTC
(In reply to Julien Nabet from comment #8)
> The call to getResultSet() is really just to update the status, not for
> using a rs.

I thought from memory that calling getResultSet twice in a row would implicitly retrieve the second result  of the query. That is indeed wrong. To advance to the next result, one must call getMoreResults(). However, MySQL C++ connector implements (part of) the JDBC API in C++. So looking at the JDBC documentation https://docs.oracle.com/javase/7/docs/api/java/sql/Statement.html#getResultSet() I see:

 This method should be called only once per result.

So unless MySQL C++ connector declares it made that safe in its documentation, we should avoid calling it and throwing away the result; we will not necessarily get it back correctly when calling getResultSet() the next time.

> In executeQuery, we retrieve a rs that we can release, it's not the case for
> execute.

Yes, because in executeQuery, there is necessarily a ResultSet to be retrieved. It is an error to call executeQuery on a query that does not return results (such as a basic UPDATE / DELETE query). The query can be a query that returns a result containing zero rows, but it must return a result.

It is allowed to call execute() on any kind of query: returning a single result (of zero, one or more rows) or not returning any result, or returning several results. It is then the caller's job to call getResultSet() and getMoreResults() to retrieve the results.

There is example code on how to use execute() correctly there:

https://blogs.msdn.microsoft.com/jdbcteam/2008/08/01/use-execute-and-getmoreresults-methods-for-those-pesky-complex-sql-queries/
Comment 14 Lionel Elie Mamane 2017-09-23 07:14:50 UTC
(In reply to Julien Nabet from comment #9)

> I'm not sure to understand, execute and executeUpdate never use resultsets
> or did I miss something?

In complement to my previous comment:
executeUpdate() can only be used for a query that does not return a ResultSet, but returns exactly one (no more, no less) update counts. E.g. a basic UPDATE or DELETE.

executeQuery() can only be used for a query that returns exactly *one* ResultSet and (I think) *no* update count.

execute() can be be used for a query that returns zero or more ResultSets and zero or more update counts (but a query will always return at least one or the other).

Since we don't know that the user types in the direct sql window, I think we should use execute() and use the algorithm linked to in my previous comment to process the result correctly. If the "show result" checkbox is checked, then use the resultset, else just close it immediately and go to the next result.
Comment 15 Lionel Elie Mamane 2017-09-23 07:25:03 UTC
(In reply to Julien Nabet from comment #12)

> Indeed, I began with:
> diff --git a/mysqlc/source/mysqlc_statement.cxx
> b/mysqlc/source/mysqlc_statement.cxx
> index 3a082004831a..9ff3ce163fb6 100644
> --- a/mysqlc/source/mysqlc_statement.cxx
> +++ b/mysqlc/source/mysqlc_statement.cxx
> @@ -240,7 +240,7 @@ sal_Bool SAL_CALL OCommonStatement::getMoreResults()
>  
>      // if your driver supports more than only one resultset
>      // and has one more at this moment return true
> -    return false;
> +    return cppStatement->getMoreResults();
>  }

This looks good.

> but then, I don't know how to call getMoreResults() from directsql.cxx.

See the algorithm linked to in comment 13.

> I tried to declare OStatement::getMoreResults

No need, it inherits it from OCommonStatement.

> (because OStatementBase::getMoreResults already exists) in statement.hxx +
> statement.cxx but I've got an error when building.

While it is not necessary, it is definitely allowed. What error did you get?

> I think it's perhaps linked to XStatement.idl but don't want to touch this
> since it's a public interface and I don't know the consequences.

Yes, don't change that.

> I also tried to use XMultipleResults but since execute method doesn't return
> anything, I can't use XMultipleResults

The XMultipleResults interface is exactly what you have to use. But I see that mysqlc doesn't implement it, which is... "dommage" because it is pretty easy since MySQL Connector C++ implements it!

Something like:

 sal_Int32 SAL_CALL OCommonStatement::getUpdateCount()
 {
-    return 0;
+    return cppStatement->getUpdateCount();
 }

in addition to your above patch should be all that is needed.
Comment 16 Commit Notification 2017-09-24 06:27:01 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=444730a67dbd2ad6cebe666b2cd23c67d5c668f2

tdf#103685: "Commands out of sync" when connecting to MySQL using direct

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Julien Nabet 2017-09-24 08:56:01 UTC
For 5.4 branch, we just need a review for https://gerrit.libreoffice.org/#/c/42701/1
Let's put this one to FIXED then.
Comment 18 Commit Notification 2017-09-25 12:50:57 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6d01cf0cabb1168159c3b013bc64b0a33a96f0c7&h=libreoffice-5-4

tdf#103685: "Commands out of sync" when connecting to MySQL using direct

It will be available in 5.4.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.