Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting wsrep_sync_wait via prepared statement is incompatible with ProxySQL #750

Open
glbyers opened this issue Apr 23, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@glbyers
Copy link

glbyers commented Apr 23, 2024

Describe the bug

With the release of Icinga DB 1.2.0, new functionality was added to always attempt to set the wsrep_sync_wait session variable on connection. This is done as a prepared statement, which is incompatible with ProxySQL; Currently ProxySQL doesn't support the parsing and tracking of SET statements using the binary protocol (prepared statements).

With ProxySQL in front of Galera, this doesn't achieve the desired outcome of transaction consistency; wsrep_sync_wait never gets set. We can't simply rewrite this query with ProxySQL query rewrite feature either, as it's a prepared statement.

Aside from this not achieving the desired outcome, ProxySQL logs become flooded with errors. ie;

2024-04-23 22:05:13 MySQL_Session.cpp:7064:handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo(): [ERROR] Unable to parse query. If correct, report it as a bug: SET SESSION wsrep_sync_wait=?

To Reproduce

To reproduce, you can run ProxySQL in front of a single MariaDB/MySQL database & point Icinga DB to ProxySQL.

Expected behavior

I'm expecting that whatever we set database.options.wsrep_sync_wait to (or the default of 7) is reflected as the value of the session variable. If this cannot be achieved by replacing the prepared statement with a standard ExecContext type query, we hope that it may be possible to set database.options.wsrep_sync_wait to a value that tells Icinga DB to ignore the setting and never try to set it (we can take care of it instead via mysql client configuration). ie.

database:
  options:
    ## Ignore setting. We'll deal with this in /etc/my.cnf.d/client.cnf instead
    wsrep_sync_wait: -1

Your Environment

Icinga DB 1.2.0
Icinga 2.1.4
RHEL 8.9

Additional context

Functionality to add setting of wsrep_sync_wait added in this commit.

I understand this is a limitation of ProxySQL and not IcingaDB, but given this is not likely to be addressed in ProxySQL in the short term, i'm hopping for a workaround in IcingaDB.

@glbyers
Copy link
Author

glbyers commented Apr 24, 2024

FYI, worked around with the following for now;

diff --git a/pkg/config/database.go b/pkg/config/database.go
index 0895d26..2c66496 100644
--- a/pkg/config/database.go
+++ b/pkg/config/database.go
@@ -191,26 +191,18 @@ func unknownDbType(t string) error {
 func setGaleraOpts(ctx context.Context, conn driver.Conn, wsrepSyncWait int64) error {                                                                        
        const galeraOpts = "SET SESSION wsrep_sync_wait=?"

-       stmt, err := conn.(driver.ConnPrepareContext).PrepareContext(ctx, galeraOpts)                                                                          
+       _, err := conn.(driver.ExecerContext).ExecContext(ctx, galeraOpts, []driver.NamedValue{{Value: wsrepSyncWait}})                                        
        if err != nil {
                if errors.Is(err, &mysql.MySQLError{Number: 1193}) { // Unknown system variable                                                                
                        return nil
                }

-               return errors.Wrap(err, "cannot prepare "+galeraOpts)
-       }
-       // This is just for an unexpected exit and any returned error can safely be ignored and in case                                                        
-       // of the normal function exit, the stmt is closed manually, and its error is handled gracefully.                                                      
-       defer func() { _ = stmt.Close() }()
+                if errors.Is(err, driver.ErrSkip) {
+                        return nil
+                }

-       _, err = stmt.(driver.StmtExecContext).ExecContext(ctx, []driver.NamedValue{{Value: wsrepSyncWait}})                                                   
-       if err != nil {
                return errors.Wrap(err, "cannot execute "+galeraOpts)
        }

-       if err = stmt.Close(); err != nil {
-               return errors.Wrap(err, "cannot close prepared statement "+galeraOpts)                                                                         
-       }
-
        return nil
 }

@oxzi
Copy link
Member

oxzi commented Apr 24, 2024

Thanks for both creating and debugging this issue with SQL SET commands within prepared statements on ProxySQL. The ProxySQL author's comment on the other issue is quite informative. Since ProxySQL may not have been on our radar, we weren't aware of its limitations. Even if you now have a working system thanks to your own patch, it's something we should look into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants