Clojurians Log v2

Pushed a minimal test that ensures an attempt to query following a close-connection results in an exception. As I say in a comment, it's a bit ugly and I'm not sure if this is the best final form, but I think it does the job.

Does the pod still work after catching the exception on new queries?

It does keep working in the repl after getting an exception... should I also check in the test script?

yeah maybe good to test. since you mentioned EOF I thought the pod had died :)

Hmm... the old test doesn't generate an EOF on my mac. The EOF was seen on my linux machine. Lemme go test a change there real quick...

Weird. I looked at my terminal history on my linux machine and that EOF definitely happened, but only once. I can't recreate with the current or previous version of the test. I never experienced an actual problem with functionality - so it might have just been some race condition on process vs. sub-process shutdown? I don't know enough about the pod architecture to properly theorize

no problem, just add a test to verify that stuff still works after closed connection use and be done with it :)

It's there already!

merged, thanks!

Thanks for the warm welcome as a contributor! I feel ready to pick up some work on the duckdb pod now

And this could also be applied to the duckdb pod I think, so code written can be relatively well carried over there too

cc @richiardiandrea

I feel since this benefits two very similar projects and it has direct benefit to a user, maybe we can do this one first? I can guide you in the implementation as well @davclark (at least I know what should happen, and maybe you know how to do that in golang)

Please go ahead, I’ll help review the go side too.

Bit bogged down at work

thanks

@davclark I think the simplest would be to have a global sync.Map where the connections can be stored with id

Might necessitate a go version bump but should be okay

@rahul080327 I tried to build go-sqlite locally but couldn't anymore after I messed with transit locally. Maybe you can upgrade it to our local transit-go lib? :)

is sync.Map thread-safe?

Yes hence sync map not usual map

nice

It should be using the forked transit. If you blow away the go module cache and try again, should pull the right thing

https://github.com/babashka/pod-babashka-go-sqlite3/blob/25338b882410c5bf6978399e98623246c1618ba2/go.mod#L12

Ah no it’s in the duck one. Can refer its go.mod?

I’m on phone, bit hard now

no problem

maybe @davclark can do this as his first task to get a success experience with little effort ;)

I'm on my work computer, and haven't actually determined the policy for open source contributions on work-owned devices... so I need a bit to get that set up, or otherwise can maybe pick this up my nighttime (like 9pm-ish EST)

I can take a shot in an hour or so

Happy to wait for dav if not urgent

Getting more gophers in bb world is nice

is this a valid go.mod? I removed some indirect deps, not sure why they are needed.

module 

go 1.15

require (
	 v0.0.0-20250914122507-fb54c87ade49
	 v1.0.0
	 v1.14.22
	 v1.2.1 // indirect
)

Yes it’s referring to a commit. Need a go mod tidy after that too

sorry, I pushed it ;)

okay taking a look now

so we are looking at 2 things? • get-connection: returns a map {:connection "conn-id"} • close-connection: takes this map and cleans up? since these are new ones, shouldnt break anything?

but the queries would now need this extra conn param, do we have a different arity for that?

or (sqlite/execute! (string to db | connection-map) ...)

To get you going @davclark I've made a branch here with the idea: https://github.com/babashka/pod-babashka-go-sqlite3/tree/get-connection

get-connection works, but it contains a bug. The connection is supposed to be stored under a uuid in the syncMap but somehow never gets it

I'll leave that up to you to fix it and also implement close-connection

The way I test this is by running go build -tags "json1 fts5" -o pod-babashka-go-sqlite3 main.go && bb test/script.clj

Awesome - thank you for the easy-mode start!

I also wanted to make connection a keyword -> uuid, but it's now a string -> uuid, but that's a minor thing

https://github.com/babashka/pod-babashka-go-sqlite3/blob/get-connection/main.go#L276 should be syncMap.Store(id.String(), conn)

thanks. still doesn't work though

I mean, that was a good improvement nonetheless

yeah im pushing in the big changes now

ok I pushed something too, small change though

thank you

I just made id into a string, no uuid object to mess around with

id := uuid.New().String()
			

i have exactly that

when parsing the query / exec args I think it might be better to return a connection string (id) instead of part of the db value, so we can distuingish

I now mashed them together which is a bit messy

pushed mine

all tests pass

but leaks the ephemeral connections

nice :)

what do you mean, leak ephemeral?

the non cached ones. the parsequery returns a conn, either a new one or from cache depending on what it sees

there is no good way to close the new one

you read the cache in the parseQuery, that seems a bit intertwined, but I must admit I had this idea too ;)

there is no good way to just close the new one? why not?

yeah bit complected

my idea after contempating this was to return an extra string in the tupel

that signals that we should only read from cache

and only when we read this connection, we don't defer close the connection

why not?so the new one is made in a different function and if er defer close there it would close too soon and the place where its called it cant differntiate cached or not

this is exactly why it shouldn't happen in parseQueryArg

here I defer-ed the close only when it is a new connection, not from cache https://github.com/babashka/pod-babashka-go-sqlite3/blob/b6eda30744da48d7e1d91c002bcfb785d22a5241/main.go#L220

I think we want to restore that behavior

yes im thinking more, something with types

instead of this:

(*sql.DB, string, []interface{}, error)

and then it's solved

yep

at least, we if we can read from this stupid map, which didn't work in my case

we're more or less thinking the same

spend 1 hour with go, love clojure even more

well, spend x hrs with y, y not being clojure, same effect

i have another idea (probably weird) go 1.24 has runtime.AddCleanup which lets you add a fn which is called when the thing is about to be GC'ed. can attach that to the non cached ones https://medium.com/@ajitem/improved-finalization-in-go-1-24-introducing-runtime-addcleanup-e2f7a43831c2

pushed: https://github.com/babashka/pod-babashka-go-sqlite3/commit/37b42760d177f9e50b0093f33fbb37a2e33d1550

returning the connid idea

maybe @davclark can take over the close-connection

now that we have the connId separate, I think it's still better to get the cached connection or make a new one + defer

oh I see, you didn't go with the weird option

returning a connection from a parse function doesn't feel right though

yeah the gc one is non deterministic and no idea what impact it may have

so how about this • parseQuery returns a DBConn interface, which is implemented by CachedConn or Conn • has a Connect method, which we would call returning the appropriate connection • has a Close method which is a no op for the cached ones, defer call can be made for both • for close-connection, we directly close it from the sync.Map

complex, but trying to think how to decomplect the parse

that way parse doesnt have side effects

pushed the change

decomplected

@rahul080327

yeah was trying to avoid that repeated code

the code repetition can be avoided by collapsing query and exec into one thing that has a flag whether it's query or exec

but it's no big deal to me. it's golang, it's meant to be straightforward stupid code

heh

or maybe I can factor this out in a function, which returns a sql.DB + a bool that says if it should be deferred

going to try that and then sign off

yep something like that is good enough

that bool idea came to me too, but gopher in my head says do polymorphic interfaces

simplified it a little bit: https://github.com/babashka/pod-babashka-go-sqlite3/commit/fb68ae35edaca179a157160525510ae052231d36

pretty good i would say

perhaps it's better to make connId nil though

but minor thing

instead of the empty string I mean

seems a bit sloppy

yeah then making it into a pointer not too worth it i think

can't strings be nil?

nope

ok then we'll leave it

*string is needed

ok, then this is fine

good, that leaves us with close-connection...

maybe dav has fun with it, will take a look tomorrow too

yep, feel free to make close-connection dav, shouldn't be that much work. I think we'll just merge this to main for now, right?

its in a good state

I should actually be working on my presentation (I give the first version next week at a meetup) and I take any chance to distract me from that, even golang ;)

deadline driven dev is real

oh dang, I squashed the commit and forgot to make you co-author, sorry

eh no worries, have bashed on it quite a bit already 😛

behold this beautiful code that now works:

#!/usr/bin/env bb

(ns go-sqlite3
  (:require
   [babashka.fs :as fs]
   [babashka.pods :as pods]))

(pods/load-pod 'org.babashka/go-sqlite3 "0.3.12")
(require '[pod.babashka.go-sqlite3 :as sqlite])

(when-not (fs/exists? "/tmp/foo.db")
  (sqlite/execute! "/tmp/foo.db" ["create table foo (bar)"]))
(let [conn (sqlite/get-connection "/tmp/foo.db")]
  (sqlite/execute! conn ["insert into foo (bar) values (?)" "baz"])
  (prn (sqlite/query conn ["select * from foo"])))

I'm going to use this meme for my talk, thanks

This meme is under GPLv3

ok I wont' then

Heh, as long as you give your talk source code

still working on that :-s

While the topic of this thread has meandered... I pushed a not-yet-working version of a close-connection function here: https://github.com/babashka/pod-babashka-go-sqlite3/compare/main...davclark:pod-babashka-go-sqlite3:close-connection?expand=1 It compiles, but then the test hangs. No action needed for now and hopefully I'll get some time to finish this tonight BUT I'm going camping for the weekend, so wanted to share ahead of time in case you wanted to have a look. I also still need to remove the id from the syncMap, and maybe the error handling / messaging could be a bit more thoughtful. But I figure it's better to get something mediocre working before trying to polish it up.

I also wonder what you'd think about DRY'ing up the argument parsing?

That's awesome @davclark. Maybe you can make a separate PR for close-connecting and a separate one for DRY-ing up some stuff

Wisdom!

I think the test hangs because you're not sending a reply to babashka on success

I knew you'd figure it out right quick 😛

golang forces you to think about error conditions, but ignore success ;)

Pushed what appears to be a working version now

Includes deleting the entry in the map

I think it's at a point where it makes sense to pull the trigger on a PR. It's not ready, but that gives some space for async feedback

Sound good?

sure!

left some feedback

You're so fast! I was thinking the same about test cases. I'll see if I can get that done tonight, but might be Monday

no hurry, thanks for the contrib!

This is perfect! I could also imagine something similar for the Java backing the JVM Clojure too?

indeed