https://github.com/osquery/osquery logo
Join Slack
Powered by
<https://github.com/osquery/osquery/pull/6038> plz...
# code-review
t
s
This one looks okay to me, but is also a bit out of my depth on the sql core side. I want to give other folks a chance at it
s
oh that sqlite test is fun stuff. It's also not plugged in any of the build systems.
unless I'm blind
I'm blind and sublime didn't help. For some reason searching the source file name do not give any results.
t
Are we good to go with merging this?
s
Hey, I couldn't review that in the end. There's a thing I'm wondering because, it's true that SQLITE_CONSTRAINT would become SQLITE_OK, but it was following a different path than when we just straight return SQLITE_OK
Meaning, it might do extra work now
t
I am not so much worried about the side-effects there, the change from 
SQLITE_OK
-> 
SQLITE_CONSTRAINT
introduced the report here (that we've made progress on fixing) https://github.com/osquery/osquery/issues/5503
but I'd feel more comfortable reverting that change, meaning 
SQLITE_CONSTRAINT
-> 
SQLITE_OK
s
So there's no expected performance regression in the IN() fix packetzero did?
t
I think the change in 5422 was made based on following SQLite documentation and not studying the effects or trying to mitigate a specific experience/bug.
correct -- there should be no performance regression for 
IN()
, but I can confirm this
or rather, I would appreciate help confirming this
s
So I'm playing with it a bit. So indeed there's some more code executed in the sqlite plan part, but that's minimal and it's only one per plan. I'm not really sure how to have it create more than one plan, I hope it's not possible for it to create hundreds depending on some variable input in the query. That been said what I've noticed is that it seems we are also calling the table even if we know that the required constraints aren't there. Shouldn't we avoid that? There's also a rare case when you have a table which has an index column which is not required and another column which is required. So if you give everything correctly and you use an IN operator on the index column, xFilter gets called for each value in the IN list. Same thing apparently with a JOIN and rows; there's indeed a comment in virtual_table.cpp that talks about that. Though if you still use the IN operator but do not put the other required column in the query, it means that you're calling again xFilter multiple times, also calling the table as a consequence. Not only that but it prints the warning message once for each element in the IN list.
The same should happen with a JOIN and rows
s
Is this an issue around merging this PR, or an issue for future improvement?
s
What I described is what happens when introducing this PR.
Because in the "return SQLITE_CONSTRAINT" case it simply yield earlier as there's no plan to work with
that been said, a part from the improvement of not calling the table when the constraints aren't there, the rare case it seems something we currently don't have in our tables. I'm not also that sure if it makes sense to have (though it's possible to have, I've tested it); still it's there 🙂
namely in all the cases I've seen where we have an Index AND a different required column, the Index is required too, which means that the query can't fail if you are using the Index.
So you don't care about xFilter getting called and the table getting called.. because it's correct.
t
Ok, if I understand correctly then, we are OK to merge #6038 to improve the user experience for incorrect queries. And we could improve it slightly by not calling the table if we detect a missing required constraint.
s
yeah I would short circuit that, since it's basically what returning SQLITE_CONSTRAINT and having no plans does.
👍 1
5 Views
Open in Slack
PreviousNext
Powered by