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

Reevaluate appropriateness and very nature of Command struct #87

Closed
Abscissa opened this issue Oct 19, 2016 · 5 comments
Closed

Reevaluate appropriateness and very nature of Command struct #87

Abscissa opened this issue Oct 19, 2016 · 5 comments
Labels

Comments

@Abscissa
Copy link

In light of #86 and simplifying the API, I'm uncertain whether the Command struct is even appropriate to have at all. I need to look into how well, or if at all, a single connection can handle multiple commands in play at the same time:

  • If a single connection can only handle one command at a time, period, then it shouldn't be possible to instantiate multiple Commands per connection. Which suggests the Command struct shouldn't exist at all, and its functionality should be folded into Connection.
  • If connections CAN handle multiple commands at a time, then is there even a point in being able to re-use a Command for multiple queries/statements? Perhaps each query/statement should use its own Command. If so, that suggests the user shouldn't need to create a Command themselves - calling a free-function of exec or prepare should implicity instantiate a Command IF, at that point, there's really even any need for the Command stuct at all. Perhaps the Command stuct is still needed for handling clean-up?

Is there any other reason for the Command struct to exist?

@Abscissa Abscissa added the api label Oct 20, 2016
@Abscissa
Copy link
Author

I need to look into how well, or if at all, a single connection can handle multiple commands in play at the same time:

Multiple prepared statements can exist at the same time.

However, when retrieving rows (regardless of whether using prepared statements or not), the current command MUST be either completed or purged before any another command can be initiated. Interleaving other commands with the retrieval of rows is NOT permitted by the protocol:

debug(MYSQL_INTEGRATION_TESTS)
unittest
{
    mixin(scopedCn);

    cn.exec("DROP TABLE IF EXISTS `testjunk1`");
    cn.exec("DROP TABLE IF EXISTS `testjunk2`");
    cn.exec("
        CREATE TABLE `testjunk1` (
            `data1` BIGINT UNSIGNED NOT NULL
        ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci
    ");
    cn.exec("
        CREATE TABLE `testjunk2` (
            `data2` BIGINT UNSIGNED NOT NULL
        ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci
    ");
    cn.exec("INSERT INTO `testjunk1` VALUES (1),(2),(3),(4)");
    cn.exec("INSERT INTO `testjunk2` VALUES (22),(44),(66),(88)");

    import std.stdio;

    auto cmd1 = Command(cn);
    auto cmd2 = Command(cn);
    cmd1.sql = "SELECT * from `testjunk1`";
    cmd2.sql = "SELECT * from `testjunk2`";

    auto rs1 = cmd1.execSQLSequence();
    writeln("rs1.front[0]: ", rs1.front[0]); // Output: 1
    auto row1 = cmd1.getNextRow();
    writeln("row1[0]: ", row1[0]); // Output: 2

    cmd1.purgeResult();

    // Without the "purgeResult" above, this command throws:
    // MySQLProtocolException: Server packet out of order
    auto rs2 = cmd2.execSQLSequence();
    writeln("rs2.front[0]: ", rs2.front[0]);
    auto row2 = cmd2.getNextRow();
    writeln("row2[0]: ", row2[0]); // Output: 44

    row1 = cmd1.getNextRow();
    writeln("row1[0]: ", row1[0]); // Output: 66 (!!!)
    row2 = cmd2.getNextRow();
    writeln("row2[0]: ", row2[0]); // Output: 88

}

Clearly there needs to be more safety to prevent executing a new command while another command is producing result rows on the same connection, in order to prevent both exceptions AND retreiving corrupted data. However, multiple prepared statements should still be permitted to exist on the same connection.

@Marenz
Copy link

Marenz commented Nov 9, 2016

There is also a thing to consider about prepared statements.
As far as I remember they are bound to a connection.

Now, in my structure, I have a pool of connections. When I want to query, I get one of those connections from the pool and run my query and when I am done, I return it.

If no connections are left, my fiber is suspended until there is one left.

In this environment it is almost impossible to use prepared statements at all because I would need to prepare them on every connection.

@Abscissa
Copy link
Author

That's not a limitation of this client library, it's a limitation of the
MySQL server itself. MySQL always ties all prepared statements to the
only the connection that created them. AFAIK, there's nothing the
client (or this library) can do to persist a prepared statement across
more than one connection.

But that said, I should refresh my memory on the typical usage of vibe's
connection pools and the vibe connection pool provided by this library.
If there isn't already a good way to run custom code on every new
connection (for example, to pre-set a prepared statement), then maybe
there should be.

Or, maybe this lib should offer a way to "set" a prepared statement that
automatically gets created on its first use on any connection...but I
wonder if that might be overkill for a low-level library like this.

In any case, I guess that is something to think about...

@Marenz
Copy link

Marenz commented Nov 10, 2016

As a side node, maybe it's interesting for you, this is how I wrapped the library in my project (with vibed):

https://gist.github.com/Marenz/369da81853e5e3933d25df2cffd99c85

@Abscissa
Copy link
Author

Abscissa commented Mar 1, 2017

Fixed in v1.0.0. Gonna open a new issue for the "using prepared statements with connection pools" matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants