Looks like your editor/autoformatter adds an extreanous indent at the start of these CREATE TABLE statements (same for the neoq_jobs table).

I think this may be how we're achieving job mutual exclusion. Though I do wonder if we should consider using SQLlite's internal mutual exclusion instead and enforce THREADSAFE=1...is that possible?

I was not aware of this mode. I can try experimenting with this and see if it works well.

Hi @acaloiaro, I tried couple of things and here's my understanding:

1. We might not need a custom job mutual exclusion (with sync.RWMutex). Apparently, SQLite by default works in serialized mode aka -DSQLITE_THREADSAFE=1, meaning it handles it internally. It could handle around 3k concurrent writes (neoq.Enqueue) before getting a database is locked error. I think this is why I went ahead with the custom handling earlier.
   But alternatively (and perhaps a better solution), go-sqlite3 driver FAQ recommends using a ?cache=shared mode with db.SetMaxOpenConns(1) (a related thread). It seems to handle as high as 10k concurrent writes and probably more.
   I couldn't observe possible implications from this as long as the transactions are very small - which brings me to the question - do we need to wrap entire handleJob code in a transaction (like how it's done in postgres_backend.go), or do one small transaction before handler.Exec (for deadline check) and one after (marking as processed)? Is there a reason why the former is done? Because otherwise, the latter makes more sense to me as both it follows the general thumb rule and also does not slow down writes. The enqueues get stuck otherwise beyond a point as one of the handleJob will obtain exclusive access to db. Let me know your thoughts.

2. Mutual exclusion would most likely be affected by -DSQLITE_THREADSAFE=0. I didn't try this - it requires manually building SQLite source code and linking it to go-sqlite3 driver. Is this something necessary to consider?

3. WAL Mode works well too without the need for custom handling. According the documentation, it helps in achieving higher concurrency. But it introduces additional files -shm and -wal, and the latter came to be around 3 times the size of db.

Note: QueueListenerChanBufferSize and handler.Concurrency needs to be correctly configured by user based on their specific needs for things to work well.

Let's make sure len(dbURI) > 1, and throw an error that the connection string is malformed, before indexing into its slice.

Alternatively, I thought to have:
dbPath := strings.TrimPrefix(s.config.ConnectionString, "sqlite3://")

When testing for Windows I found that the connection string must be of the form sqlite3://file:///<db_path> for it to open the db successfully. If we do a split on "/" we get an extra "/" attached at the start of the resulting db path. What do you think?

Sounds better than splitting.

I remember making this comment about Postgres transactions. Is it true of Sqlite too?

pgx lib clearly documents that tx.Rollback is safe to call multiple times even after commit. But the same is not there for sql lib. Theoretically rollback shouldn't have any effect after commit right? But if it is a concern then maybe we can do something like:

defer func() {
    if err != nil {
        _ = tx.Rollback(ctx)
        s.logger.Error("Transaction rolled back due to error", slog.Any("error", err))
    }
}()

This confirms that it’s a no-op for the sql.DB interface more generally.

That is from the fact that in their example code they are doing a defer tx.Rollback() like we are doing currently?

I was referring to the following from the example:

Defer the transaction’s rollback. If the transaction succeeds, it will be committed before the function exits, making the deferred rollback call a no-op. If the transaction fails it won’t be committed, meaning that the rollback will be called as the function exits.

I think we should consider having this function receive a *sql.Tx rather than it create one. That way, job handling can use a single transaction for all db operations.

In the PG backend, we do this by adding a tx to the ctx, e.g. from handleJob:

ctx = context.WithValue(ctx, txCtxVarKey, tx)

In any case we'll need atleast 2 transactions right? One for in progress/failed and one for processed/failed ? Since in progress needs to be committed

How important is in progress to you? Is there a specific use case you have in mind?

I intentionally avoided it for postgres because it's an additional database round trip, and the best use case I had for it was to be able to show in-progress jobs in a UI (which doesn't exist). As you can see, leaving in progress out also reduced complexity because the "state machine" for determining its correct value also needs to be considered. I'd be inclined to remove the in progress requirement unless it's serving a specific goal of yours.

I had the very same use case for being able to show in-progress jobs in a UI. But maybe you're right, it adds more complexity. We can perhaps remove the requirement for now as it would also be consistent with other backends.

Let's check for this error and return an error when it's non-nil. Otherwise we could have in-progress jobs without an in-progress status, which may cause confusion.

It is captured in line 440 to 447 right? That err variable gets returned from the function.

I see, my mistake.

This possibly places the test under unrealistic conditions. I would expect real world users to use default concurrency. Will this test work with concurrency > 1?

These are test cases borrowed from postgres backend. I hadn't put much thought on this specific case. I can try your suggestion.

Can we add a done channel here instead? 1.1s is a pretty long fixed wait time. Some of my early tests did this, but I've tried to get away from any fixed wait times in favor of a done channel and a timeout instead.