nalgeon / redka

Redis re-implemented with SQLite
BSD 3-Clause "New" or "Revised" License
3.44k stars 94 forks source link

Connection settings are only set for some connections #28

Closed ncruces closed 3 months ago

ncruces commented 3 months ago

In the code below, both d.RW and d.RO are connection pools. The pragmas are only executed on one connection from each pool and not others.

https://github.com/nalgeon/redka/blob/72a3b9f5a53f240ef24bb0b8ea09cf04aba12852/internal/sqlx/db.go#L105-L125

Some of the default pragmas (like journal_mode=wal) persist across connections; others might be the default values (synchronous=default); others are harmless if not applied (mmap_size); but others are a behaviour change (foreign_keys).

https://github.com/nalgeon/redka/blob/72a3b9f5a53f240ef24bb0b8ea09cf04aba12852/internal/sqlx/db.go#L20-L26

As the dev of an alternative SQLite driver, I appreciate redka being driver agnostic, but I don't know of a way to consistently specify pragmas in a portable way.

Mine is compatible with modernc (pragmas are URI parameters like pragma=journal_mode(wal)), but mattn takes a different route.

nalgeon commented 3 months ago

Hey Nuno, thank you for pointing this out!

d.RW is forced to have a single connection, so it should not be a problem. And d.RO is read-only, so the effect of the problem may not be that critical (still a problem, of course).

No idea how to fix this in a cross-driver-compatible way. Maybe I should just abandon the "any driver" approach and support only specific ones, using each one's features.

ncruces commented 3 months ago

Well, e.g. mmap_size is most useful to read-only connections, so that's a problem.

One way is to receive the *sql.DB (or an interface), but that goes against having one RW and one RO (which is a good idea!). This is what GORM does.

Another is to allow configuring (RO and RW?) DSNs, as all drivers generally have a way to configure these things in the DSN, they're just incompatible.

Both options push those decisions to the library users though. I don't know of a good option here.

nalgeon commented 3 months ago

Oh, you can use the *sql.DB with the redka.OpenDB function. But it still leaves the redka.Open buggy.

In order not to push the decisions to the library users, Redka needs to be aware of each driver specifics. I'll consider that.

ncruces commented 3 months ago

Then I guess users can already fix it themselves, if they know what settings they want to apply.

The big problem is that the only reliable way to do this for mattn is: their custom URI parameters (like _fk=true), and or setting up a connection hook (which requires importing the package, and installing a custom driver).

There's really no good options here. 😔

nalgeon commented 3 months ago

I'll think about it and hopefully come back with something.

On a completely unrelated note, would you be interested in my feedback on working with your driver in Redka?

ncruces commented 3 months ago

Definitely, 100%. I think my driver is, in a sense, “ready to go.”

WAL was, IMO, the big blocker to adoption, but it works fine on 64-bit Linux/macOS, and OK on 64-bit BSD/illumos. Windows is missing but might be possible with some effort.

I'm also more confident that it works because they guys at gotosocial have tried it and I've had good feedback over it (they may default to it).

The big caveat now (besides https://github.com/ncruces/go-sqlite3/issues/75) is https://github.com/ncruces/go-sqlite3/discussions/32.

ncruces commented 3 months ago

FYI: https://github.com/mattn/go-sqlite3/issues/1248

nalgeon commented 3 months ago

Okay, here is what I changed:

I think it should be enough for now.

As for the ncruces driver, it does not work with Redka at this time. I see a couple of possible reasons, will report them later.

ncruces commented 3 months ago

Feel free to open an issue in my repo for that. Anything I can, I'm interested in fixing.

nalgeon commented 3 months ago

Sure, will do it soon.