Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection settings are only set for some connections #28

Closed
ncruces opened this issue Jun 7, 2024 · 10 comments
Closed

Connection settings are only set for some connections #28

ncruces opened this issue Jun 7, 2024 · 10 comments

Comments

@ncruces
Copy link

ncruces commented Jun 7, 2024

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.

redka/internal/sqlx/db.go

Lines 105 to 125 in 72a3b9f

func (d *DB[T]) applySettings(pragma map[string]string) error {
if len(pragma) == 0 {
return nil
}
var query strings.Builder
for name, val := range pragma {
query.WriteString("pragma ")
query.WriteString(name)
query.WriteString("=")
query.WriteString(val)
query.WriteString(";")
}
if _, err := d.RW.Exec(query.String()); err != nil {
return err
}
if _, err := d.RO.Exec(query.String()); err != nil {
return err
}
return nil
}

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).

var DefaultPragma = map[string]string{
"journal_mode": "wal",
"synchronous": "normal",
"temp_store": "memory",
"mmap_size": "268435456",
"foreign_keys": "on",
}

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
Copy link
Owner

nalgeon commented Jun 8, 2024

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
Copy link
Author

ncruces commented Jun 8, 2024

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
Copy link
Owner

nalgeon commented Jun 8, 2024

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
Copy link
Author

ncruces commented Jun 8, 2024

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
Copy link
Owner

nalgeon commented Jun 8, 2024

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
Copy link
Author

ncruces commented Jun 8, 2024

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 ncruces/go-sqlite3#75) is ncruces/go-sqlite3#32.

@ncruces
Copy link
Author

ncruces commented Jun 8, 2024

FYI: mattn/go-sqlite3#1248

@nalgeon
Copy link
Owner

nalgeon commented Jun 9, 2024

Okay, here is what I changed:

  • Enforce pragmas using the connection hook for the mattn driver (when Redka is running in server mode).
  • Set pragmas in the connection string for the modernc driver (and others that support it).

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
Copy link
Author

ncruces commented Jun 9, 2024

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

@nalgeon
Copy link
Owner

nalgeon commented Jun 9, 2024

Sure, will do it soon.

@nalgeon nalgeon closed this as completed Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants