Consider implementing SessionResetter or Validator

[michaellenaghan]

The database/sql driver docs say:

All Conn implementations should implement the following interfaces: Pinger, SessionResetter, and Validator.

Implementing SessionResetter and/or Validator would probably be a good idea.

One thing to look for: !GetAutocommit().

Another thing to look for: GetAutocommit() && TxnState() != TXN_NONE.

(As I showed in another discussion, the second is possible when a context is cancelled during the stepping of an auto-commit statement.)

So what to do if one of those is found?

In the first case, SessionResetter could try to roll back the transaction?

In the second case, Validator could signal that the connection was invalid?

You might have better ideas.

The main thing, really, is to prevent one problem from quietly causing other problems.

[ncruces]

This will have an impact on performance, though: ResetSession is called after every query; IsValid may be called before ResetSession, or not at all.

On your suggestion:

I don't see how IsValid might be called in that state. To return a connection to the pool, you must've closed all rows, transactions, etc.

On goals:

Ideally ResetSession makes the connection like new again, but SQLite has no such concept, of session: you're supposed to close the connection as that is cheap. But it's not, not really, because we also recreate wazero.

So, thoughts:

I think the developer messing up and Exec a BEGIN or a PRAGMA is out of scope for fixing. That's not how database/sql is supposed to be used. Set PRAGMA on open, use structured transactions.

Errors the developer doesn't control, like cancellation, can make sense to flag, though.

Can you demonstrate a reaching a bad state using only structured transactions, closing rows and transactions, but then cancellation or something out of control happens? That'd make sense to try to fix.

But note that:

• closing a transaction causes an unconditional rollback; if the rollback errors out, the connection is discarded
• closing rows resets the statement; not sure what happens on error, but this should not be a bad state

If, OTOH, the idea is to make stuff bulletproof, the only way is to not really pool connections, and basically “reopen” them everytime. But this is costly, even if I invented an API to reuse wazero.

[michaellenaghan]

ResetSession is called after every query

I only see calls to ResetSession here and here, both in conn?

I only see a call to IsValid here, in putConn?

I guess you're talking about top level autocommit-style queries that get and put a connection automatically?

Anyway, I don't think any of that actually matters. You've shown that you're willing to give up some performance for correctness. The real question is: is there a correctness issue here?

Can you demonstrate a reaching a bad state using only structured transactions, closing rows and transactions, but then cancellation or something out of control happens? That'd make sense to try to fix.

And that's the constraint you've added: is there a correctness issue here assuming "correct" usage? I wasn't assuming that constraint; I was just thinking that getting a connection from the pool that was in a strange state would lead to errors that would be very hard to reproduce, very hard to debug.

Let me think about this a bit more tomorrow, and I'll see if I can think of any holes assuming correct usage.

[ncruces]

My point is that I don't think I can really defend against incorrect usage.

Client-server DBs are built with network connection pooling in mind, so within a single connection they have a concept of session that covers things, some like client config ("locale" or "timezone"), and then they do have a way to reset the session and reuse the connection.

SQLite has no such concept, you're supposed to throw away the connection. If you ask, they ask you why you're pooling them.

So unless you're willing to do that, the question becomes, what useful thing can you do? There's no simple way to reset PRAGMA without (1) collect it all (2) set it again; but this can be extremely slow, generate garbage, etc. I did this partially before.

What you proposed is much cheaper: a couple method calls that don't generate any garbage, don't malloc/free. The question is how you get to such a bad state? Using structured transactions seems hard. I added an API so you can use savepoints with structured transactions. So just use them.

And the bigger error is that, with database/sql you shouldn't do most of these things, because of pooling: you shouldn't be setting a PRAGMA on one connection of the pool; or opening :memory:; or doing BEGIN.

The only sane way would be if you use an actual Conn, which in my experience people don't use much. But then if you do, it's very hard to ensure the connection is in a good state without closing it. I almost want to throw away every connection you take out with Conn, but I don't think I can know the difference.

[michaellenaghan]

OK, understood.