Implement CMD_RESET_CONNECTION #1437

testn · 2021-10-28T12:01:42Z

Address #1328

still in the draft...

To add integration tests

sidorares · 2021-10-28T21:25:04Z

I think mysqljs/mysql has CMD_RESET_CONNECTION implemented ( not sure if .reset() is exposed in the public api ) and it's added automatically to a pool connections before releasing them back. IMO it's a useful feature, maybe worth discussing compatibility and perf impact if we make it on by default

testn · 2021-10-29T01:32:11Z

I think mysqljs/mysql has CMD_RESET_CONNECTION implemented ( not sure if .reset() is exposed in the public api ) and it's added automatically to a pool connections before releasing them back. IMO it's a useful feature, maybe worth discussing compatibility and perf impact if we make it on by default

I have thought of that too. This will help make pooled connection works just like a typical connection including reset local temp table.

sidorares · 2021-10-29T03:20:54Z

looks like it's not merged yet - mysqljs/mysql#1469
it might be implemented with "change user" command wich re-authenticates and resets connection without re-establishing tcp connection.

sidorares · 2021-10-29T03:25:08Z

ok changeUser is sent only when connection did change user on itself and its different from what's configured on the pool - https://github.com/mysqljs/mysql/blob/3430c513d6b0ca279fb7c79b210c9301e9315658/lib/Pool.js#L116

to me that should be done on release in the background. Not sure what's mysql2 behaviour in that case ( get connection, change user, release connection )

testn · 2021-10-29T06:21:07Z

I think that we should add an option to turn it on/off defaulting to on?

sidorares · 2021-10-29T06:51:40Z

agree. default on is a sensible security / "no surprises" option while allowing explicit "off" to gain a bit of performance.

sidorares · 2021-10-29T06:53:23Z

I wonder if it's possible to have "auto" mode - if no change to variables / affected rows / user / db don't do reset

testn · 2021-10-29T07:44:34Z

I wonder if it's possible to have "auto" mode - if no change to variables / affected rows / user / db don't do reset

I think in general, reset should take that into account already. It would just add another roundtrip cost.

sidorares · 2021-11-24T22:24:47Z

hey @testn I might have some time this weekend to clear backlog of PRs - do you need any help with integration tests for this?

testn · 2021-11-29T04:10:38Z

hey @testn I might have some time this weekend to clear backlog of PRs - do you need any help with integration tests for this?

Awesome. I still cannot figure out why it failed in some CI setting.

Also, connection.end() is not "async". However, to end() the connection, I would like to make a reset() call which is async network call. What should we do with it?

danielgindi · 2022-05-29T04:31:23Z

I just stumbled upon a bug caused by this. I was wondering "do they reset the connection?", and the answer is apparently "no".
Now discussing a "no surprises" methodology should always result to a "default: true". As no surprises mean: I expect a clean connection from the pool. How the heck should I know what is currently pending on a random connection I just got from the pool?
Anyone expecting the opposite, is working in some extreme case where there is zero session and wants to gain 1-2% more in perf.

sidorares · 2022-05-29T07:38:40Z

@danielgindi 100% agree ( though would be great to have some tests to support 1-2% perf hit assumption ). Any preference on the name for this flag? resetOnAcquire ( default true ) ?

danielgindi · 2022-05-29T14:17:01Z

@danielgindi 100% agree ( though would be great to have some tests to support 1-2% perf hit assumption ). Any preference on the name for this flag? resetOnAcquire ( default true ) ?

As for the naming - this should just be "connectionReset". Other connectors do this too: https://mysqlconnector.net/connection-options/.
I'd like to have a connectionResetOnRelease option, to automatically avoid deadlocks (if using GET_LOCK etc.), but this could introduce an additional overhead for connections that will not be used again, and should be opt-in.

As for the perf hit - I tested about a year ago with the .net connector, with and without the reset. I believe it's safe to assume it would be the same here. Anyway it's a non-issue, as you never over-optimize at safety's expense.

danielgindi · 2022-05-29T14:22:52Z

@sidorares Note that in some cases, the perf hit may be larger. i.e:

When the roundtrip (ping mysql_server) is too big. Because in this case the code has to wait for an additional roundtrip until it can start querying.
When there's a pending transaction that will be rolled back by the reset connection command, and the rollback is a large set of rows and takes some time.

In these cases, it may even be better to always reset the connection when returning to the pool.
And then we can just mark it as "not ready yet for acquiring" until the reset has completed.

You can see some discussion here: mysql-net/MySqlConnector#178

sidorares · 2022-05-30T01:40:46Z

I'd like to have a connectionResetOnRelease option, to automatically avoid deadlocks (if using GET_LOCK etc.), but this could introduce an additional overhead for connections that will not be used again, and should be opt-in.

hm, doing reset on connection release might actually be better for performance ( or rather consumer latency ) as in this case there is a chance that reset is happening while no one is waiting

sidorares · 2022-05-30T01:44:58Z

lib/connection.js

@@ -707,6 +707,10 @@ class Connection extends EventEmitter {
    return this.addCommand(new Commands.Ping(cb));
  }

+  reset(cb) {


I believe this need to perform some additional client cleanup, for example prepared statements need to be discarded.

ok let me take a look

Preparedstatement is discarded automatically
https://mariadb.com/kb/en/com_reset_connection/

yes, server side, but we need to delete statements cache otherwise it might be used incorrectly with execute() after reset

I think this is what needs to be done

add private connection helper method connection._initialiseStatementCache({ disableDispose: boolean })

something along the lines:

_initialiseStatementCache({ disableDispose }) { if (this._statements) { if (disableDispose) { this._statements.dispose = null; } this._statements.clear(); } this._statements = new LRU({ max: this.config.maxPreparedStatements, dispose: function(key, statement) { statement.close(); } }) }

replace line 66 in connection.js with this._initialiseStatementCache({ disableDispose: false });

in ResetConnectionCommand.resetConnectionResponse(packet, connection) add call connection._initialiseStatementCache({ disableDispose: true }) at the beginning before calling onResult()

Ah... got it

I thought statement can be reused even though the connection is reset

I need to double check but I'm pretty sure statement id is sequential 1, 2, 3, ... for each prepare() command ( can't remember if 0 or 1 based ) and after reset it goes again as 1, 2, 3, ...

small correction: set dispose to noop instead of null, e.i this._statements.dispose = () => {};

danielgindi · 2022-06-13T15:45:20Z

@testn I believe that releaseConnection should look like this:

  releaseConnection(connection, reset = true) {
    let cb;
    if (!connection._pool) {
      // The connection has been removed from the pool and is no longer good.
      if (this._connectionQueue.length) {
        cb = this._connectionQueue.shift();
        process.nextTick(this.getConnection.bind(this, cb));
      }
    } else {
      if (reset) {
        connection.reset(() => {
          this.releaseConnection(connection, false);
        });
      } else {
        if (this._connectionQueue.length) {
          cb = this._connectionQueue.shift();
          process.nextTick(cb.bind(null, null, connection));
        } else {
          this._freeConnections.push(connection);
          this.emit('release', connection);
        }
      }
    }
  }

danielgindi · 2022-06-14T13:41:30Z

We've tried this in a production environment, after modifying the releaseConnection function -> it looks like problem solved!

danielgindi · 2022-06-19T05:28:13Z

@testn I believe that releaseConnection should look like this:

  releaseConnection(connection, reset = true) {

    let cb;

    if (!connection._pool) {

      // The connection has been removed from the pool and is no longer good.

      if (this._connectionQueue.length) {

        cb = this._connectionQueue.shift();

        process.nextTick(this.getConnection.bind(this, cb));

      }

    } else {

      if (reset) {

        connection.reset(() => {

          this.releaseConnection(connection, false);

        });

      } else {

        if (this._connectionQueue.length) {

          cb = this._connectionQueue.shift();

          process.nextTick(cb.bind(null, null, connection));

        } else {

          this._freeConnections.push(connection);

          this.emit('release', connection);

        }

      }

    }

  }

@testn have you taken a look at this? This is to actually trigger the reset.

testn · 2022-06-19T06:27:30Z

@testn I believe that releaseConnection should look like this:

  releaseConnection(connection, reset = true) {

    let cb;

    if (!connection._pool) {

      // The connection has been removed from the pool and is no longer good.

      if (this._connectionQueue.length) {

        cb = this._connectionQueue.shift();

        process.nextTick(this.getConnection.bind(this, cb));

      }

    } else {

      if (reset) {

        connection.reset(() => {

          this.releaseConnection(connection, false);

        });

      } else {

        if (this._connectionQueue.length) {

          cb = this._connectionQueue.shift();

          process.nextTick(cb.bind(null, null, connection));

        } else {

          this._freeConnections.push(connection);

          this.emit('release', connection);

        }

      }

    }

  }

@testn have you taken a look at this? This is to actually trigger the reset.

Can you explain a little bit?

danielgindi · 2022-06-19T06:44:24Z

@testn we want the connector to reset a connection in the background before making it available again.
So what I did there is added a flag, that makes it reset the connection, and then call the logic again to return to the pool.
It could of course be split into two functions instead, like releaseConnection and _releaseConnectionImpl.

Now if there was an error, even in the reset command, then it will have its _pool prop removed, which will make the function release the connection without returning to the pool.

We can make this flag based, but this should be the default behavior anyway.

mscdex · 2024-03-09T02:36:50Z

What's left to get this over the finish line?

testn force-pushed the add-reset-connection branch from 8e3972b to 3995a72 Compare October 28, 2021 15:38

Implement CMD_RESET_CONNECTION

299ed3d

testn force-pushed the add-reset-connection branch from 3995a72 to 299ed3d Compare October 28, 2021 15:40

sidorares mentioned this pull request Dec 7, 2021

poolConnection.changeUser changing for the whole Pool.getConnection() #1469

Open

sidorares reviewed May 30, 2022

View reviewed changes

Merge branch 'sidorares:master' into add-reset-connection

175bd0c

sidorares added this to the 3.0.0 milestone Nov 11, 2022

sidorares modified the milestones: 3.0.0, 3.1.0 Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CMD_RESET_CONNECTION #1437

Implement CMD_RESET_CONNECTION #1437

testn commented Oct 28, 2021

sidorares commented Oct 28, 2021

testn commented Oct 29, 2021

sidorares commented Oct 29, 2021

sidorares commented Oct 29, 2021

testn commented Oct 29, 2021

sidorares commented Oct 29, 2021

sidorares commented Oct 29, 2021 •

edited

testn commented Oct 29, 2021

sidorares commented Nov 24, 2021

testn commented Nov 29, 2021

danielgindi commented May 29, 2022

sidorares commented May 29, 2022

danielgindi commented May 29, 2022

danielgindi commented May 29, 2022

sidorares commented May 30, 2022

sidorares May 30, 2022

testn May 30, 2022

testn May 30, 2022

sidorares Jun 14, 2022

testn Jun 14, 2022

testn Jun 14, 2022

sidorares Jun 14, 2022

sidorares Jun 14, 2022

testn Jun 15, 2022

danielgindi commented Jun 13, 2022 •

edited

danielgindi commented Jun 14, 2022

danielgindi commented Jun 19, 2022

testn commented Jun 19, 2022

danielgindi commented Jun 19, 2022

mscdex commented Mar 9, 2024

Implement CMD_RESET_CONNECTION #1437

Are you sure you want to change the base?

Implement CMD_RESET_CONNECTION #1437

Conversation

testn commented Oct 28, 2021

sidorares commented Oct 28, 2021

testn commented Oct 29, 2021

sidorares commented Oct 29, 2021

sidorares commented Oct 29, 2021

testn commented Oct 29, 2021

sidorares commented Oct 29, 2021

sidorares commented Oct 29, 2021 • edited

testn commented Oct 29, 2021

sidorares commented Nov 24, 2021

testn commented Nov 29, 2021

danielgindi commented May 29, 2022

sidorares commented May 29, 2022

danielgindi commented May 29, 2022

danielgindi commented May 29, 2022

sidorares commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielgindi commented Jun 13, 2022 • edited

danielgindi commented Jun 14, 2022

danielgindi commented Jun 19, 2022

testn commented Jun 19, 2022

danielgindi commented Jun 19, 2022

mscdex commented Mar 9, 2024

sidorares commented Oct 29, 2021 •

edited

danielgindi commented Jun 13, 2022 •

edited