ncruces / go-sqlite3

Go bindings to SQLite using wazero
https://pkg.go.dev/github.com/ncruces/go-sqlite3
MIT License
402 stars 12 forks source link

export the database/sql driver implementation type #79

Closed NyaaaWhatsUpDoc closed 4 months ago

NyaaaWhatsUpDoc commented 4 months ago

This allows for further user customization of the database/sql sqlite driver, for example in GoToSocial where we would require translating any returned errors. This does so while not breaking the current API.

ncruces commented 4 months ago

I'm not against this, but it'll take some convincing. Can you show me how you'd use this, what do you need it for?

The driver was initially a singleton, now it's a global variable. You're assuming it is set on an init? But what if two client packages need different customizations?

ncruces commented 4 months ago

I think I understand why you might want to export the current singleton: so it can be wrapped.

I can also understand the frustration with me taking over the "sqlite3" driver name. I was always conflicted about it, but decided it was the least bad option (and still added an escape hatch).

I don't understand why the driver would need to stop being a singleton and become a global variable.

NyaaaWhatsUpDoc commented 4 months ago

Okay so thoughts for designing it were as follows:

i'm not hugely tied to that second point so if you'd prefer i can remove the exported global variable!

ncruces commented 4 months ago

Yes, I would really rather not export the global variable. But if you have a better idea for driver registration (and me not taking over "sqlite3") I'm all ears.

I can explain why I did it like this (and why I disagree with modernc using "sqlite" instead). Having multiple SQLite drivers in the same process is not just wasteful but outright dangerous, for reasons I explain here.

So, in a sense, I want to panic if people try to use mattn and my driver in the same process (ideally I'd panic for modernc too).

The counter argument is that all 3 drivers (mattn, modernc, ncruces) have different DSNs, so they should have different registration names.

Ideally, none of the drivers would "auto-register," but that's the Go convention, so that's out.

The only solution I see, is what I did with package embed: a package driver to register, and a package driver/impl (?) that exports the implementation for customization. Would that be better? I can prototype it for you.

Sorry, this is the kind of stuff that I'd really like to get right at once, but that only happens when someone shows up with a weird requirement.

NyaaaWhatsUpDoc commented 4 months ago

Yes, I would really rather not export the global variable. But if you have a better idea for driver registration (and me not taking over "sqlite3") I'm all ears.

I can explain why I did it like this (and why I disagree with modernc using "sqlite" instead). Having multiple SQLite drivers in the same process is not just wasteful but outright dangerous, for reasons I explain here.

So, in a sense, I want to panic if people try to use mattn and my driver in the same process (ideally I'd panic for modernc too).

The counter argument is that all 3 drivers (mattn, modernc, ncruces) have different DSNs, so they should have different registration names.

Ideally, none of the drivers would "auto-register," but that's the Go convention, so that's out.

The only solution I see, is what I did with package embed: a package driver to register, and a package driver/impl (?) that exports the implementation for customization. Would that be better? I can prototype it for you.

Sorry, this is the kind of stuff that I'd really like to get right at once, but that only happens when someone shows up with a weird requirement.

no worries that makes sense! will make the change :+1:

NyaaaWhatsUpDoc commented 4 months ago

@ncruces have removed the global variable, and renamed the driver implementation to instead be SQLite{}

ncruces commented 4 months ago

Just a heads up: because I have encryption in the pipeline, it will take me some time to tag a release of this.

I'm planing to make some small backwards incompatible changes to the VFS API in #82, and I really want to get them right. I also want to make sure encryption is works the way it should, as that's a sensitive matter.

If you're not creating a VFS (and I guess no one is, yet), you can use main for a while, then update. That won't cause any breakages.