surrealdb / surrealdb.go

SurrealDB SDK for Golang
https://surrealdb.com
Apache License 2.0
231 stars 62 forks source link

Discussions #58

Closed ElecTwix closed 1 year ago

ElecTwix commented 1 year ago

I was talking with people on the Discord channel for golang drivers.

I thought it could be nice to have discussions for feature requests and talk about ideas.

I know there are issues with that too but think we need to separate them because there will be some ideas without any standpoint when there is presented to the public then we can talk about the pros and cons and if the public ok with that we can implement it because we usually go with our personal experience but we can listen to others and their opinions about the topic or feature.

mrchocha commented 1 year ago

@ElecTwix one feature suggestion. we can add pagination support for select and query.

s.db.Select("users").skip(10).limit(15)
ElecTwix commented 1 year ago

@ElecTwix one feature suggestion. we can add pagination support for select and query.

s.db.Select("users").skip(10).limit(15)

Yeah, Query Builder can be nice. but what do you think about the discussions section for the git repo? it can be better discussion the topic like you said.

agufagit commented 1 year ago

Need a way to create a surrealdb.DB struct with old non-concurrency safe websocket connection

the new version v0.2.1 puts in Concurrency safe websockets, I'd like a way to create non-concurrency safe websockets.

Concurrency Safe websockets (multiplex) only makes sense if you expose surrealdb api to the internet, which probably will scare most people.

For applications that puts gateway in front of surrealdb api, a pool implementation is needed, because performance will suffer if you have big data to write to or read from the same websocket connection, thus you need multiple websocket connections. In this case, the locking mechanism of concurrency safe websockets is not needed because pool implementation already has locking mechanism.

plally commented 1 year ago

what functionality do you need from a non concurrency safe websocket connection? I think you could still implement a connection pool with the current websockets, Is it just the performance impact of having an extra lock that is the problem?

agufagit commented 1 year ago

yeah, just performance impact, locks are expensive

phughk commented 1 year ago

Heya! Thanks for this and the discussion above

agufagit commented 1 year ago

I don't think concurrency safe websocket is the way to go, exposing surrealdb api to internet is probably fine for hobby side projects or prototype, but for most real world applications, ssl+Oauth2 security are not gonna convince people to expose direct database api to the internet, especially /sql endpoint can parse any surql.

For performance, there is a benchmark here sync: RWMutex.RLock is 2x slower than Mutex.Lock

Current websocket implementation has two locks, connLock sync.Mutex and responseChannelsLock sync.RWMutex

For a single request, it has to acquire connLock once for write, acquire responseChannelsLock 3 times

from that benchmark, 1000 requests for sync.Mutex took an average of 2.6 seconds, 1000 requests for sync.RWMutex took an average of 6.1 seconds

so for 1000 requests of concurrency safe websocket, it's 2.6 + 3 * 6.1 = 20.9 seconds more time

let's say there are a million requests per day, that's 20.9 * 1000 = 20900 seconds = 5.8 hours more time

plally commented 1 year ago

concurrency safe websockets are not just beneficial for exposing surrealdb to the internet, I don't understand what you mean by that? Any scenario where you share a connections across goroutines benefits from this. the previous websocket implementation would panic and even worse silently return the wrong responses to callers.

If you only used a connection in a single goroutine at a time, I am not sure that benchmark would be accurate because there would be no lock contention. If we want to consider this, I think it would be worthwhile to write an actual benchmark for the surrealdb websocket library itself if possible, then we could have more applicable benchmark numbers for this.

agufagit commented 1 year ago

Never mind, you are right, I did a test, it takes only nanoseconds to lock/unlock without lock contention

phughk commented 1 year ago

Heya! Agree with the initiative on here 💪 We would like to keep track of the above requests as separate features. Would it be possible please to create new features so that we can isolate discussions to single topics. Thanks, closing for now.