Various sqlite backend optimizations and cleanups #972
Conversation
No functional changes, just makes the thing a little clearer.
Rpm can reuse an iterator cursor for a write when rewriting an existing header (such as when updating file states of already installed package), which in sqlite terms turns a SELECT into INSERT, destroying the iteration in progress. Detect the condition and use a local cursor as necessary, this also means we don't need to track the query fmt string in cursors.
Caching statements is not stupid but when it makes us go slower... Also without the cache we have a better handle on locks and all, revisit the caching issue later.
Spinning new cursors for each key value is just mind-bogglingly stupid (if simple), use a sub-cursor with key binding instead. Which isn't any more complicated, really.
The upper layers sort the dbi set as necessary, this is nothing but waste of perfectly good computing cycles.
We don't need to pull the key values from the db here, we already have it.
| dbiCursorFree(dbi, kc); | ||
| } | ||
| if (set) | ||
| rc = sqlite_idxdbByKey(dbi, dbc->subc, dbc->key, dbc->keylen, set); |
There was a problem hiding this comment.
I think there's an error here. It looks like there should be braces here...
There was a problem hiding this comment.
What makes you think that? There's no need for braces for a single line.
There was a problem hiding this comment.
Well, aside from a couple lines up there is braces used even with a single line branch on if, the rc = RPMRC_OK made me think there's some kind of error here.
There was a problem hiding this comment.
The brace use for single line items varies a lot throughout rpm, the only hard rule there is that anything spanning multiple lines in the source must use braces, even if the compiler thinks it's just one line.
I generally dislike unnecessary braces but sometimes they are introduced in anticipation for surrounding code, at other times they get left behind from other code changes, at other times they add to readability and at others, they're just ugly.
| dbiCursorFree(dbi, kc); | ||
| } | ||
| if (set) | ||
| rc = sqlite_idxdbByKey(dbi, dbc->subc, dbc->key, dbc->keylen, set); | ||
| rc = RPMRC_OK; |
There was a problem hiding this comment.
Is this statement supposed to be here? It essentially clobbers the result from the line above...
There was a problem hiding this comment.
It's a bit fishy alright, but it's not being introduced in this patch.
IIRC it's there for a reason though, and with this patch all the more so.
|
With sqlite changes, it's worth remembering it's all very much still a work-in-progress thing and should be reviewed with different mindset than changes to released features and such. The whole db schema is likely to be revamped etc, this is all just trying out things. |
No description provided.