Multi Statements support #2

[badoet]

Add 'multiStatements' param to the dsn string.
multiStatements is for a batch fire and forget db operations.
this will help optimize the round trip.
in a transaction, multiple updates through goroutine seems to not run concurrently

User GoUpdateResource %!s(int64=778018) 64.0037ms
PlayerInventory GoSave %!s(int64=3138) 128.0073ms
User GoUpdateResource %!s(int64=777805) 189.0108ms
PlayerInventory GoSave %!s(int64=15036) 251.0144ms
PlayerInventory GoSave %!s(int64=15448) 313.0179ms
PlayerInventory GoSave %!s(int64=15655) 375.0214ms
PlayerInventory GoSave %!s(int64=15983) 438.025ms
PlayerInventory GoSave %!s(int64=16656) 500.0286ms
PlayerInventory GoSave %!s(int64=3160) 564.0322ms
PlayerInventory GoSave %!s(int64=15034) 629.036ms
PlayerInventory GoSave %!s(int64=3162) 690.0394ms
PlayerInventory GoSave %!s(int64=16759) 753.043ms
PlayerInventory GoSave %!s(int64=9061) 817.0467ms
PlayerInventory GoSave %!s(int64=15037) 881.0504ms
PlayerInventory GoSave %!s(int64=15291) 944.054ms
PlayerInventory GoSave %!s(int64=15038) 1.0080576s
PlayerInventory GoSave %!s(int64=15039) 1.0700612s
PlayerInventory GoSave %!s(int64=15391) 1.1340648s
PlayerInventory GoSave %!s(int64=15390) 1.1970684s
PlayerInventory GoSave %!s(int64=16393) 1.259072s
PlayerInventory GoSave %!s(int64=14719) 1.3230756s
PlayerInventory GoSave %!s(int64=16256) 1.3870793s
PlayerInventory GoSave %!s(int64=15033) 1.4500829s
PlayerInventory GoSave %!s(int64=16279) 1.5130865s
PlayerInventory GoSave %!s(int64=16658) 1.5750901s
PlayerInventory GoSave %!s(int64=16335) 1.6390937s
PlayerInventory GoSave %!s(int64=16645) 1.7020973s
PlayerInventory GoSave %!s(int64=16654) 1.7641009s
PlayerInventory GoSave %!s(int64=16385) 1.8271045s
PlayerInventory GoSave %!s(int64=16287) 1.889108s
PlayerInventory GoSave %!s(int64=15670) 1.9511116s
PlayerInventory GoSave %!s(int64=16363) 2.0151152s
PlayerInventory GoSave %!s(int64=15694) 2.0791189s
PlayerInventory GoSave %!s(int64=16297) 2.1411224s
PlayerInventory GoSave %!s(int64=16263) 2.2051261s
PlayerInventory GoSave %!s(int64=15965) 2.2681297s
PlayerInventory GoSave %!s(int64=16200) 2.3311333s
PlayerInventory GoSave %!s(int64=16306) 2.395137s
PlayerInventory GoSave %!s(int64=16310) 2.4591406s
PlayerInventory GoSave %!s(int64=16666) 2.5211442s
PlayerInventory GoSave %!s(int64=16839) 2.5851478s
PlayerInventory GoSave %!s(int64=15044) 2.6471514s
goroutine done 2.6471514s
tx.Commit() 65.0037ms

Limitations:
n, _ := res.RowsAffected()
n will return 1 instead of the x number of rows updated in the batch query

tested with gocraft/dbr works as intended

[badoet]

currently trying to add the test for the multiple batch update query.
keep running into the error this command out of sync

[MySQL] 2015/05/11 19:45:52 packets.go:340: Busy buffer
--- FAIL: TestMultiQuery (0.46s)
        driver_test.go:153: Error on Query SELECT id, value FROM test;: Commands
 out of sync. Did you run multiple statements at once?

i tried to do an multiple update queries and run a select statement to check if the result is correct.
the problem is in the select statements after the update statement. running it without the multiple statements on the update works fine. only the multiple statement have problem.
the thing is i have tested it with the gocraft/dbr library and it works fine

user := User{Id: userId}
user.Get(sess)
fmt.Printf("User: %+v \n", user)

query := "UPDATE user SET mineral=mineral+1 WHERE id=" + id + ";"
query += "UPDATE user SET xp=xp+1 WHERE id=" + id + ";"
res, err := sess.UpdateBySql(query).Exec()

if err != nil {
    ReturnErr(w, err.Error())
    return
}

n, _ := res.RowsAffected()
fmt.Printf("ROWS AFFECTED: %v \n", n)

user.Get(sess)
fmt.Printf("User: %+v \n", user)

here user.Get(sess) basically does:

func (u *User) Get(sess *dbr.Session) error {
    err := sess.Select("*").
        From(u.TableName()).
        Where("id=?", u.Id).
        LoadStruct(u)
    return err
}

so it leave me very confused as to why the vanilla mysql driver having problem performing this.

func TestMultiQuery(t *testing.T) {
    runTestsWithMultiStatement(t, dsn, func(dbt *DBTest) {
        // Create Table
        dbt.mustExec("CREATE TABLE `test` (`id` int(11) NOT NULL, `value` int(11) NOT NULL) ")

        // Create Data
        _ = dbt.mustExec("INSERT INTO test VALUES (1, 1)")

        // Update
        _ = dbt.mustExec("UPDATE test SET value = 3 WHERE id = 1; UPDATE test SET value = 4 WHERE id = 1; ")

        // Read
        _ = dbt.mustQuery("SELECT id, value FROM test;")

    })
}

func runTestsWithMultiStatement(t *testing.T, dsn string, tests ...func(dbt *DBTest)) {
    if !available {
        t.Skipf("MySQL-Server not running on %s", netAddr)
    }

    dsn3 := dsn + "&multiStatements=true"
    var db3 *sql.DB
    if _, err := parseDSN(dsn3); err != errInvalidDSNUnsafeCollation {
        db3, err = sql.Open("mysql", dsn3)
        if err != nil {
            t.Fatalf("Error connecting: %s", err.Error())
        }
        defer db3.Close()
    }

    dbt3 := &DBTest{t, db3}
    for _, test := range tests {
        test(dbt3)
        dbt3.db.Exec("DROP TABLE IF EXISTS test")
    }
}

test code shortened for less verbose

[PuppyKhan]

The problem is not the select statements, it is the original multiple update which leaves unread "OK" responses in the buffer. You see the "Commands out of sync" error when the driver attempts to reuse a buffer with an additional response that hasn't been read remaining in it. This is a flaw in the Go SQL interface, not the driver, as it has no ability to handle the multiple responses. There are some hacks available but they all boil down to reading and discarding additional responses which is fine if the user keeps the usage to only one needed result set but can't work it doing multiple query results.

[arnehormann]

There's another effort for this in #338...
I'm cowardly deferring the decision if this has a chance to get merged to @julienschmidt.

Pro: we get a lot of tickets for stored procedures and gain an often used feature.
Con: I see potential maintenance trouble and tickets for swallowed results in the future - and we deviate from our "as close to database/sql/driver as possible" mantra - and in a surprising, result discarding way.

The hardest problem I see here is that we can only sensibly return the result for the very first or very last executed query (by documented policy) - and that will probably include fun queries like setting delimiters to something else than ;, especially when the stored procedures are not written with the driver in mind.
It will be okay for Exec only stuff, it will be extremely confusing for Query.

[PuppyKhan]

Actually, this might not work for Exec calls because the driver loses control over the result set so can never get to the next one to discard it. I think these fixes/hacks, including #338, will only work for Query and QueryRow calls.

For Stored Procedures (my problem which first got me involved in this issue) the very last result set is always an OK, so you would have to use the first one.

[badoet]

u have actually deviate a little from the database/sql/driver when you start implementing the interpolateParams feature. that wasnt the intended design for the driver.
as such, the interpolateParams is added in as an optional feature through the dsn params.

The solution in #338 is mandating the multiStatements flag to be added to the packets.go.

Whereas my solution is adding it to an optional dsn setting that the dev can switch on after understanding what he can or cannot do with it.
I am currently only pushing for the multiStatements flag. i understand there is some issues with the multiResults flag which is a limitation in the golang's sql driver.
but i believe as for the multiStatements, there is no reason as not to support it. It is a very commonly used optimization strategy with no known side effects.

I am currently only addressing the Exec command. Not Query calls, nor stored procedure.

[badoet]

Thanks for the heads up, i have incorporated some changes from #338 for the discarding the extra results return after the multi statement calls.
the test passed now.

[cnphpbb]

This way is very good.
But this way for development is very painful.
You need to change a lot of code.

[badoet]

the only change u need is to add '&multiStatements=true' to ur dsn string

[arnehormann]

@badoet maybe we deviated with interpolateParams - but if so, we did it in a way that's never confusing and fully enables it as a feature. Whatever we do with stored procedures, they won't work in a database/sql/driver compatible way without some unavailable features and strange corner cases requiring deeper knowledge of MySQL internals.
I also dislike adding new dsn parameters. If this gets in, it should be configurable so it is less confusing when it fails.

[cnphpbb]

The built-in function & stored procedures in SQL, which also has a switch to choose?

[badoet]

hmm. again, my use case is very specific in a way.
its not stored procedure
its batch sql update. e.g.
UPDATE test SET value = 3 WHERE id = 1;
UPDATE test SET value = 4 WHERE id = 1;
don't u agree that all the database driver in other languages like java, nodejs, php, ruby, c ,etc..
can perform this batch query?
only golang cant do this.
MySQL is main market is in the web application sector.
this kind of batch update query is very common use case
again im not talking about stored procedure.
its just a batch sql update
not stored procedure.

please stop deviating my point to 'stored procedure'. again, it is not!

[arnehormann]

Ok, I misunderstood. I'm automatically thinking of stored procedures when multi statement suport comes up because we get that request often enough...

[julienschmidt]

I'll try to add support for statements with multiple return values (e.g. stored procedures) based on lucalooz@9542bdd first.
I'll then use your PR to add optional support for multi statements

[amacneil]

What's the status of this PR? I would also like to execute multiple statements (discarding the results is fine).

[badoet]

this has been put on hold for so long that there r conflicts with master to resolve now :(

[amacneil]

Only took a couple minutes to merge: amacneil@73eacf5

[derekperkins]

👍

[asticode]

Damn, this PR would be a life-savior !

[julienschmidt]

It will be merged within the next days :)

[julienschmidt]

New PR in #411