-
Notifications
You must be signed in to change notification settings - Fork 244
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
Driver for Sqlserver #43
base: master
Are you sure you want to change the base?
Conversation
… as part of the driver. Also dropped DriverName from Generic driver type as its not needed anymore for switching between driver types
ikv.id <= ? | ||
ORDER BY ikv.id DESC )` | ||
|
||
listSQL = fmt.Sprintf(`SELECT TOP 100 PERCENT (%s)[a], (%s)[b], %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"m confused why there is a TOP 100 PERCENT
in here by default - is that just so you can hack in a limit by with string replacement on the outer SELECT TOP statement without worrying affecting the inner SELECT?
As I read it, you should be safe to do string replacement on a plain SELECT since you're limiting the number of replacements to 1 which should only catch the outer SELECT? I admit the nested SQL printfs are a bit hard to read so I might be missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the server boots up it tries to range over keys, and that translates itself as the following query
TRAC[0004] QUERY [/registry/mutatingwebhookconfigurations/% 3 false] : SELECT ( SELECT TOP 1 rkv.id FROM kine rkv ORDER BY rkv.id DESC)[a], ( SELECT TOP 1 crkv.prev_revision FROM kine crkv WHERE crkv.name = 'compact_rev_key' ORDER BY crkv.id DESC)[b], kv.id as theid, kv.name, kv.created, kv.deleted, kv.create_revision, kv.prev_revision, kv.lease, kv.value, kv.old_value FROM kine kv JOIN ( SELECT MAX(mkv.id) as id FROM kine mkv WHERE mkv.name LIKE @p1 AND mkv.id <= @p2 GROUP BY mkv.name) maxkv ON maxkv.id = kv.id WHERE ( kv.deleted = 0 OR 'true' = @p3 ) ORDER BY kv.id ASC
ERRO[0004] error while range on /registry/mutatingwebhookconfigurations /registry/mutatingwebhookconfigurations: mssql: The ORDER BY clause is invalid in views, inline functions, derived tables, subqueries, and common table expressions, unless TOP, OFFSET or FOR XML is also specified.
sqlserver syntax forces me to add the extra arguments to ensure I can get all the keys back.
May be it is just my limited knowledge of sqlserver.
Because of this the ApplyLimit function needs to handle the extra TOP 100 PERCENT
, i.e., replace it and apply the correct limit for the specific call.
I'd love to merge this (assuming it's reviewed and working) but under a build flag you'd have to opt into. The issue atm is that each new driver we add then adds a new go dependency to k3s. It would be good to change kine such project that depend on k3s can choose which drivers to import. |
What's the status on this? I would like to really see this work. If there is anything I could do to help get this rolling again let me know. @ibrokethecloud happy to pick this up if you have moved on. |
@phillipsj i am happy to help move it along. I wasn't sure if we wanted to progress this further. |
@ibrokethecloud @phillipsj I would also like to see this to be implemented. One important thing to notice however is that after this PR was created #81 added e2e tests to here so it should be included to this together with rebase for code. This Docker image with Express edition can be used on those. Then when it comes to |
Is this PR still relevant? |
I am not sure. I have not seen much interest in this either. I am happy to close this for now. |
Initial changes for a sqlserver driver.
Had to make some changes to generic driver to allow base queries to be injected into the generic driver due to way queries need TOP and not LIMIT need to be applied to queries.
Also needed to change how limit filter is applied in queries, and had to convert to a separate function to manipulate the query string.