666558 - Increase SQLITE_MAX_SCHEMA_RETRY value

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 2

[Shawn Wilsher :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.

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Shawn Wilsher :sdwilsh]

(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.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 7

[Shawn Wilsher :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`

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.2

per IRC discussion, this skips the configure.in check that is moved to a follow-up bug, since it needs an upstream fix.

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.3

qref :(

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 554266 [details] [diff] [review]
patch v1.3

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

r=sdwilsh

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a00e5e89f3d5

Comment 13

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/a00e5e89f3d5