Closed Bug 666558 Opened 8 years ago Closed 8 years ago

Increase SQLITE_MAX_SCHEMA_RETRY value

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

This is the maximum time SQLite should try to recompile statements when the schema changes, as discussed in bug 610789 the default value is low for our needs, so to move on with 3.7.7 update (and next ones) we should increase it.
From my local testing we may hit about 10 retries, with a pessimistic approach, I'd set the limit to 25.
Attached patch patch v1.0 (obsolete) — Splinter Review
Shawn, do you think for this we also need the configure.in check? So far we hit this only in a stress test, real-life cases should be rare (but not impossible), forcing the config in configure.in may exclude system SQLite in a lot of cases where it would be sufficient. Going over the limit just means a statement fails to be executed and throws.
Attachment #541352 - Flags: review?(sdwilsh)
(In reply to comment #1)
> Shawn, do you think for this we also need the configure.in check? So far we
> hit this only in a stress test, real-life cases should be rare (but not
> impossible), forcing the config in configure.in may exclude system SQLite in
> a lot of cases where it would be sufficient. Going over the limit just means
> a statement fails to be executed and throws.
I'd like to see a configure.in test, yeah.
well, my question was not about the test, but about the fact we want a configure.in entry at all for this setting, since the default value is fine for real-life, not for our stress tests.
(In reply to comment #3)
> well, my question was not about the test, but about the fact we want a
> configure.in entry at all for this setting, since the default value is fine
> for real-life, not for our stress tests.
But that stress test could easily happen in the real world.  I'd much rather we enforce that system SQLite is the same as ours.
ok, I think it's unlikely, but I don't have a strong opinion on both sides, so I will update the patch asap with the configure check.
Attached patch patch v1.1 (obsolete) — Splinter Review
This adds the configure.in check, the only problem is that SQLite API doesn't allow me to check the exact value, just if the configure option is used or not'. So I've only documented that whoever adds the option should check the value in db/sqlite3/src/Makefile.in, should be enough since this option is so new I doubt anyone may have set it up, and also would make no sense for anyone to set it to a lower number than default.
Attachment #541352 - Attachment is obsolete: true
Attachment #547099 - Flags: review?(sdwilsh)
Attachment #541352 - Flags: review?(sdwilsh)
Comment on attachment 547099 [details] [diff] [review]
patch v1.1

Review of attachment 547099 [details] [diff] [review]:
-----------------------------------------------------------------

Can you file the bug to remove this and mark it dependent on bug 679267 please?

r=sdwilsh with comment addressed (and sorry it took me so long to look at this).

::: configure.in
@@ +6719,5 @@
> +        AC_TRY_RUN([
> +            #include "sqlite3.h"
> +
> +            int main(int argc, char **argv){
> +              return !sqlite3_compileoption_used("SQLITE_MAX_SCHEMA_RETRY");

you should be able to do `SQLITE_MAX_SCHEMA_RETRY=25` like we do with `SQLITE_THREADSAFE`
Attachment #547099 - Flags: review?(sdwilsh) → review+
So, after some testing, looks like SQlite team forgot to add SQLITE_MAX_SCHEMA_RETRY to azCompileOpt vector, this means sqlite3_compileoption_used is unable to check it, so we can't have any configure.in check for this.
Blocks: 680297
Attached patch patch v1.2 (obsolete) — Splinter Review
per IRC discussion, this skips the configure.in check that is moved to a follow-up bug, since it needs an upstream fix.
Attachment #547099 - Attachment is obsolete: true
Attached patch patch v1.3Splinter Review
qref :(
Attachment #554265 - Attachment is obsolete: true
Comment on attachment 554266 [details] [diff] [review]
patch v1.3

Review of attachment 554266 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh
Attachment #554266 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/a00e5e89f3d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.