Hi Ludo, On Sun, 28 Jan 2018 22:47:34 +0100 l...@gnu.org (Ludovic Courtès) wrote:
> I’ve already applied two commits in civodul/guile-sqlite3. I think the > statement cache requires some more work though (see below). Nice. > > If that's OK I'll replace the reference in guix-master, or we could do a > > pull request to the civodul repository. > > I think we should stick to a single repo for guile-sqlite3, though it > prolly shouldn’t be called “civodul/”. Perhaps you can create > guile-sqlite3/guile-sqlite3 and add the two of us plus Andy there for a > start? Hmm, should I create a new user account on notabug for that? > The problem is that interned symbols are potentially not GC’d (though I > think with Guile 2.2 and its weak sets they may be subject to GC.) Too bad. If that's the case it's not as good as it could be. > The other issue is that we’d still be caching potentially a lot more > than needed. For instance, we don’t know whether a statement is a > one-off statement (used only once, for instance because it’s derived > from user parameters passed through the HTTP API or something), Well, that shouldn't happen because it would allow SQL injection. I usually write filters like (:parameter IS NULL OR (foo = :parameter)) so that the SQL text doesn't change. On the other hand, if only the parameter values change it will reuse the same statement. > Thinking more about it, I’m inclined to not try to be smart and instead > let users explicitly ask for caching when they want to. Whatever else, I think it would be good to actually use prepared statements for what they are for ;-) That they prevent sql injection is a nice bonus but what they are supposed to do is prevent the database engine from having to parse text and build a query plan every time someone changes one bit somewhere. So I'm still for "caching" those. I've left sqlite-exec inside guix-cuirass rather than guile-sqlite3 so that the user of the guile-sqlite3 can decide caching, among other things. We could also have two macros, sqlite-exec-reuse and sqlite-exec (right now it decides that implicitly by whether the SQL text is a string literal or not - I think in practise that's what cuirass will always do). The stuff that's in dannym guile-sqlite3 has no new effect if you don't use sqlite-prepare* (note star) - so it should be quite conservative already. sqlite-prepare* is written in a way that it always caches - I purposefully left the two other cases with non-literal strings out. So you can't sqlite-prepare* with a non-literal string. And maybe also remove the arg bindings from in there. Then the new API would be: sqlite-prepare for new prepared statements, sqlite-prepare* for caching prepared statements.