Title
#code-review
theopolis

theopolis

11/13/2019, 11:21 AM
s

seph

11/13/2019, 1:15 PM
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
Stefano Bonicatti

Stefano Bonicatti

11/13/2019, 6:01 PM
oh that sqlite test is fun stuff. It's also not plugged in any of the build systems.
6:02 PM
unless I'm blind
6:05 PM
I'm blind and sublime didn't help. For some reason searching the source file name do not give any results.
theopolis

theopolis

11/14/2019, 2:34 PM
Are we good to go with merging this?
Stefano Bonicatti

Stefano Bonicatti

11/14/2019, 2:48 PM
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
2:49 PM
Meaning, it might do extra work now
theopolis

theopolis

11/14/2019, 2:51 PM
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
2:52 PM
but I'd feel more comfortable reverting that change, meaning
SQLITE_CONSTRAINT
->
SQLITE_OK
Stefano Bonicatti

Stefano Bonicatti

11/14/2019, 2:53 PM
So there's no expected performance regression in the IN() fix packetzero did?
theopolis

theopolis

11/14/2019, 2:53 PM
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.
2:54 PM
correct -- there should be no performance regression for
IN()
, but I can confirm this
2:54 PM
or rather, I would appreciate help confirming this
Stefano Bonicatti

Stefano Bonicatti

11/15/2019, 4:32 PM
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.
4:38 PM
The same should happen with a JOIN and rows
s

seph

11/15/2019, 4:39 PM
Is this an issue around merging this PR, or an issue for future improvement?
Stefano Bonicatti

Stefano Bonicatti

11/15/2019, 4:40 PM
What I described is what happens when introducing this PR.
4:41 PM
Because in the "return SQLITE_CONSTRAINT" case it simply yield earlier as there's no plan to work with
4:42 PM
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 🙂
4:44 PM
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.
4:44 PM
So you don't care about xFilter getting called and the table getting called.. because it's correct.
theopolis

theopolis

11/15/2019, 8:11 PM
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.
Stefano Bonicatti

Stefano Bonicatti

11/15/2019, 8:15 PM
yeah I would short circuit that, since it's basically what returning SQLITE_CONSTRAINT and having no plans does.