Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

ibrokethecloud
Copy link
Contributor

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.

… 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
Copy link
Member

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.

Copy link
Contributor Author

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.

@ibuildthecloud
Copy link
Contributor

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.

@phillipsj
Copy link

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.

@ibrokethecloud
Copy link
Contributor Author

@phillipsj i am happy to help move it along. I wasn't sure if we wanted to progress this further.

@warmchang warmchang mentioned this pull request Aug 21, 2021
@olljanat
Copy link

olljanat commented Oct 8, 2022

@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 TOP 100 PERCENT in query. I have been working with MS SQL DBs a lot and and newer seen TOP 100 PERCENT used and actually whole query look quite complex so would be nice to get copy of example database which explanation that what that query actually should return? I sure that there is better way to do it.

@dereknola
Copy link
Member

Is this PR still relevant?

@ibrokethecloud
Copy link
Contributor Author

I am not sure. I have not seen much interest in this either. I am happy to close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants