Support queries automatically timing out after X seconds

[otoolep]

Offer this as a query parameter. I think the following C call could work:

https://sqlite.org/c3ref/interrupt.html

Start a timer in Go, and if it fires, call this C function on the connection.

[otoolep]

Ha, the underlying SQLite Go library already supports setting a timeout, and uses this method.

https://github.com/mattn/go-sqlite3/blob/4702d9b5d640f42488752f5cf70a195b748ffe96/sqlite3.go#L2071

Could be worth trying this out.

[mauri870]

I tried working on this and failed miserably. Maybe I'm trying to approach this the wrong way.

I implemented it in such a way that I can invoke QueryContext and ExecContext with a context.WithTimeout with the duration specified by the sql_timeout query parameter. Even tho the context reached the deadline those functions fail to return an error for it.

Any clues on what should be the proper way to implement this?

[otoolep]

Can you show me your code?

[mauri870]

Sorry, I forgot to add the link to my reply, here it is

master...mauri870:rqlite:feature/sql-timeout

[otoolep]

Even tho the context reached the deadline those functions fail to return an error for it.

You mean the deadline doesn't expire? If I was you I would focus on writing a db-level unit test first, to prove to yourself it works/doesn't work and debug that. You are on the right track, as I too believe it'll be a new field in the command.Request proto.

But focus on getting it working at the DB level solely via unit tests. And perhaps just focus on db.Query to start. Show me that failing.

[mauri870]

Thanks, I'll try that

[mauri870]

I added a test here master...mauri870:rqlite:feature/sql-timeout. I can confirm with the debugger that the context is indeed set properly with the deadline from context, but ExecContext does not respect that. I would expect a context.DeadlineExceeded to be returned.

[otoolep]

I'm not sure I'm convinced by that test. That creation will run so fast, maybe it will return before runtime checks the timer, I don't know. Not saying the creation will take shorter than 1 nanosecond, just questioning if the runtime is good enough. Maybe you know.

I think a better test is to create 10,000 rows in the database, perhaps with a fair chunk of data in each row, and perform some slow query (say a few hundred milliseconds) and also set your 1 nanosecond time -- perhaps SELECT *. Then I think we've got something if that test still fails.

Have you checked the tests in the SQLite driver itself (mattn), to see if they test this?

[mauri870]

I tried with a couple thousand entries as well as a time.Sleep before the ExecContext (to make sure the context is indeed expired) but got the same results.

I found this test, which is pretty much what I want to write https://github.com/mattn/go-sqlite3/blob/4702d9b5d640f42488752f5cf70a195b748ffe96/sqlite3_go18_test.go#L162-L182.

I'll dig deeper into it later today.

[otoolep]

Re-opening until I figure out why the integration test for this functionality is failing.

#1669

[otoolep]

The system testing helped improve this feature, I think we're good shape now. Thanks again @mauri870