Channels
#code-review
Title
# code-review
t
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
s
Stefano Bonicatti
11/13/2019, 6:01 PMunless 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
theopolis
11/14/2019, 2:34 PM
Are we good to go with merging this?
s
Stefano Bonicatti
11/14/2019, 2:48 PMHey, 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
theopolis
11/14/2019, 2:51 PM
I am not so much worried about the side-effects there, the change from
SQLITE_OKSQLITE_CONSTRAINTbut I'd feel more comfortable reverting that change, meaning
SQLITE_CONSTRAINTSQLITE_OKs
Stefano Bonicatti
11/14/2019, 2:53 PMSo there's no expected performance regression in the IN() fix packetzero did?
t
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.
correct -- there should be no performance regression for
IN()or rather, I would appreciate help confirming this
s
Stefano Bonicatti
11/15/2019, 4:32 PMSo 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
seph
11/15/2019, 4:39 PM
Is this an issue around merging this PR, or an issue for future improvement?
s
Stefano Bonicatti
11/15/2019, 4:40 PMWhat 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
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.
s
Stefano Bonicatti
11/15/2019, 8:15 PMyeah I would short circuit that, since it's basically what returning SQLITE_CONSTRAINT and having no plans does.
👍 1
3 Views