Add support for the ReadOnly option in BeginTx

[jmanero]

This change is similar to the modernc implementation, which ignores the transaction mode defined in the _txlock DSN parameter when the ReadOnly attribute in TxOptions values passed to BeginTx is true

While the resulting transactions are not actually read-only, this change achieves the result desired by #400 within the confines of the database/sql interface.

In WAL mode, this allows us to use DSN options like

_journal_mode=WAL&_busy_timeout=1000&_txlock=immediate

to enable blocking on concurrent calls to BeginTx(ctx, &sql.TxOptions{}) instead of polling for SQLITE_BUSY error codes, while making non-blocking read-only(ish) calls to BeginTx(ctx, &sql.TxOptions{ReadOnly: true}).

[rittneje]

The fact that this doesn't actually make the transaction readonly seems very confusing.

I understand that having the side effect of doing BEGIN instead of BEGIN IMMEDIATE can be valuable. But what I can then see happening is an application has a bug where they accidentally write to the transaction anyway, which goes undetected in testing and will ultimately cause the very failure mode they are trying to avoid.

To that end, I think we need to leverage PRAGMA query_only in this case. Care must be taking to restore the original value of the pragma once the transaction completes.

[jmanero]

Agreed. Also thinking through the unexpected behavior changes that this could cause to anything that's currently setting ReadOnly with no effect, this should probably have to be explicitly enabled by a new DSN parameter; e.g.

_tx_readonly

• 0, off: Silently ignore TxOptions#ReadOnly default, current behavior
• 1, weak: TxOptions#ReadOnly disables IMMEDIATE and EXCLUSIVE transaction locking defined by _txlock, but the connection can still be upgraded to read-write by executing a mutating operation
• 2, strong: Disables IMMEDIATE and EXCLUSIVE transaction locking, plus PRAGMA query_only is set and unset around the transaction. Mutually exclusive with _query_only=1 DSN parameter

Would you be open to this ^ kind of change @rittneje?

[rittneje]

I'm not sure what the use case for the proposed "weak" mode is.

With regards to backwards compatibility, it seems unfortunate that people who explicitly set ReadOnly to leverage the new functionality instead have it silently ignored by default, just because someone might be misusing it today. It would seem better to honor ReadOnly correctly, and if it breaks an existing application, which must have been misusing it, the solution is to fix the application code. (See also #685.) But I would be interested in @mattn's perspective.

Whether ReadOnly should also overrule the _txlock by default is also an interesting question, as is possible the consumer wants BEGIN IMMEDIATE even for their readonly transactions. It is rather unfortunate that sql.TxOptions was not defined with any mechanism for conveying driver-specific options. (Maybe a good proposal to submit against the standard library.) I suppose we could always add a separate _readonly_txlock setting if there is a legitimate need.

[mattn]

I'm not sure this change is necessary. If you need different transaction behaviors for read-only vs read-write operations, you should use separate connections:

// For writes: use immediate to prevent lock contention
writeDB, _ := sql.Open("sqlite3", "file.db?_journal_mode=WAL&_txlock=immediate")
writeDB.SetMaxOpenConns(1)

// For reads: use deferred or read-only mode
readDB, _ := sql.Open("sqlite3", "file.db?_journal_mode=WAL&_txlock=deferred")
// or even safer:
readDB, _ := sql.Open("sqlite3", "file.db?_journal_mode=WAL&mode=ro")

This approach:

• Makes the intent explicit at the connection level
• Guarantees true read-only behavior with mode=ro
• Allows proper connection pool management (1 writer, multiple readers)

The problem you're trying to solve seems like an application design concern rather than something the driver should handle.

[rittneje]

@mattn I'd certainly agree that having two pools that can be independently configured is the best approach in general. However, I do still think this library ought to respect ReadOnly, even if it doesn't implicitly change the transaction mode. (Right now it violates the contract for BeginTx.)

[MKuranowski]

Any reason why this couldn't be merged? The fact that someone prefers a more convoluted approach which involves going from a simple *sql.DB to something-more-complicated-that-has-multiple-slightly-different-handles shouldn't be any less valid than someone preferring to choose between BEGIN IMMEDIATE and BEGIN DEFERRED transactions.

This doesn't break anything (especially the pooling approach), is very simple and easily explainable for programmers who actually want to use the different transaction modes.

[rittneje]

@MKuranowski As previously mentioned, this solution does not actually make the transaction read-only. I really don't think repurposing the flag to mean something else is a good idea.

[MKuranowski]

@rittneje I am fully aware of that. But I also don't really see any definition in the sql package that the TxOptions.ReadOnly flag should make data mutations fail. It could as well be treated as a hint to the driver that nothing is going to be mutated.

SQLite is already quirky in that it ignores things that would be strict errors in other databases (think CREATE TABLE numbers (i INTEGER); INSERT INTO numbers VALUES ('foo');), so this wouldn't be that big of a stretch.

[rittneje]

But I also don't really see any definition in the sql package that the TxOptions.ReadOnly flag should make data mutations fail.

It's literally in the name. The driver package is also quite clear on the matter:

	// This must also check opts.ReadOnly to determine if the read-only
	// value is true to either set the read-only transaction property if supported
	// or return an error if it is not supported.

SQLite is already quirky in that it ignores things that would be strict errors in other databases.

This exact situation (which the SQLite authors tout as a "feature") has been an eternal source of bugs for developers. We should not add another one.

[MKuranowski]

I'm still not sold on that. That quote also doesn't imply that an error should be raised when a read-only transaction is upgraded from read-only to read-write. SQLite supports read-only transaction property - it's just an optimization for concurrent reads, precisely through BEGIN DEFERRED. Side note: under your strict interpretation the driver should raise an error, but it does not.

If you don't like them - don't use them, but don't prevent others from using a genuinely useful feature of SQLite.

[rittneje]

That quote also doesn't imply that an error should be raised when a read-only transaction is upgraded from read-only to read-write.

A deferred transaction is NOT read-only. And SQLite returns an error because it is unable to guarantee the ACID properties of a transaction if the database is modified between the first read and the first write. This has nothing to do with the ReadOnly flag.

SQLite supports read-only transaction property - it's just an optimization for concurrent reads, precisely through BEGIN DEFERRED.

Again, deferred is not the same as read-only. That's what the query_only pragma is for.

Side note: under your strict interpretation the driver should raise an error, but it does not.

That's #685. And it's not a "strict" interpretation - there is no ambiguity in what database/sql/driver says to do.

If you don't like them - don't use them, but don't prevent others from using a genuinely useful feature of SQLite.

You are arguing to misuse a flag from the standard library and redefine its behavior in a way that violates both the explicit API contract and the principle of least astonishment. If you really want that, you are always free to make a fork.

As far as this PR goes, without enforcing what read-only is supposed to mean, I think it would be a bad idea to take this change. But as I've previously stated, it's reasonable to have it switch to a deferred transaction in addition to enforcing read-only.

Regardless, this conversation seems to be going in circles, so I probably won't be replying any further.