658135 - Use sqlite3_stmt_readonly to check if multiple async statements need a transaction

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

I made this patch to try how it was working. It's maybe worth doing this to avoid large read transaction (like the autocomplete queries) since they may hurt wal checkpoints.
No idea how to test it, though.

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> I made this patch to try how it was working. It's maybe worth doing this to
> avoid large read transaction (like the autocomplete queries) since they may
> hurt wal checkpoints.
Yeah, we totally want to do this (I may have filed a bug on this already, but I can't recall).

> No idea how to test it, though.
I recall asuth made some code in test_true_async.cpp to make the background thread block until told to go.  You could use that, and then see if the connection has a transaction that is active or not.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #1)
> > No idea how to test it, though.
> I recall asuth made some code in test_true_async.cpp to make the background
> thread block until told to go.  You could use that, and then see if the
> connection has a transaction that is active or not.

Found that code, trying to make the test.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 533490 [details] [diff] [review]
patch v1.0

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

::: storage/src/mozStorageAsyncStatementExecution.cpp
@@ +582,5 @@
> +  PRUint32 len = mStatements.Length();
> +  for (PRUint32 i = 0; i < len && len > 1 && !needsTransaction; ++i) {
> +    sqlite3_stmt *stmt;
> +    if (SQLITE_OK == mStatements[i].getSqliteStatement(&stmt)) {
> +      needsTransaction = !::sqlite3_stmt_readonly(stmt);

we should be updating StatementData::needsTransaction here and calling it instead of calling ::sqlite3_stmt_readonly here.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yeah, I thought to that, but I didn't know if it would have been a perf problem for some other storage path using needsTransaction (actually I could have checked all needsTransaction callers in mxr).
I'm still figuring out why we were creating a transaction for a single statement, since SQLite creates implicit transactions for any write).

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> yeah, I thought to that, but I didn't know if it would have been a perf problem
> for some other storage path using needsTransaction (actually I could have
> checked all needsTransaction callers in mxr).
I don't think anything else but this bit of code uses it.

> I'm still figuring out why we were creating a transaction for a single
> statement, since SQLite creates implicit transactions for any write).
A single statement could have multiple sets of data to be bound to it for inserting multiple tuples.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #5)
> > I'm still figuring out why we were creating a transaction for a single
> > statement, since SQLite creates implicit transactions for any write).
> A single statement could have multiple sets of data to be bound to it for
> inserting multiple tuples.

this clarifies me a bunch of things and makes a lot of sense. Still, even if it has multiple sets of data, the fact it needs a transaction depends on whether the statement is going to do a write, so yeah, merging the 2 things is the way to go.
Thanks.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Looks like I can't use the thread wedger, because the transaction is created in the main execution runnable, and destroyed in notifyComplete (still inside the same runnable though, since the notify runnable is spawned later).
This means using the wedger I'd stop just before transaction need is evaluated or just after the transaction has been destroyed :(

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

maybe sqlite3_commit_hook and a counter?
or failing that, co-opt the logging mechanism and have it use strstr on BEGIN/COMMIT and count that way?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #8)
> maybe sqlite3_commit_hook and a counter?
Yes I was about to start looking into this (I'm digging into the sqlite api documentation)

> or failing that, co-opt the logging mechanism and have it use strstr on
> BEGIN/COMMIT and count that way?

may work, will consider as second option. thanks!

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Ok, here is the patch, the test fails correctly if I return wrong results from needsTransaction() (like always false), it accounts 2 transactions rather than 1, one for each statement.
The only drawback(?) I could tell is that I had to remove nsString from mozStorageConnection.h, since I need to get the native connection and I can't include an internal string api header. Luckily looks like it was unused but it's to take into account for future.

Asuth, if you want to review instead of Shawn, feel free to, I'd love to have this in before the Aurora merge.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Ehr, looks like this patch causes a failure in Places preventive maintenance executing a VACUUM.
I see there was a fix for VACUUM and readonly in SQLite 3.7.5, since locally I have 3.7.6.2 for testing, I'm going to see if going back to 3.7.5 solves it.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

no luck, I suspect the explicit immediate transaction is creating some problem :(

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 533919 [details] [diff] [review]
patch v1.1

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

(In reply to comment #12)
> no luck, I suspect the explicit immediate transaction is creating some
> problem :(
Along with my suggested refactoring, you could then count the number of statements that need a transaction, and if that number is great than one, return true, otherwise, return false.

r=sdwilsh with comments addressed.

::: storage/src/mozStorageAsyncStatementExecution.cpp
@@ +578,5 @@
> +  // Check if any of the statements needs a transaction and wrap them in one,
> +  // when needed.  Notice that even a single write statement would be wrapped in
> +  // an implicit transaction by SQLite, so it doesn't need any special handling.
> +  for (PRUint32 i = 0; i < mStatements.Length(); ++i) {
> +    if (mStatements[i].needsTransaction()) {

I've always pushed to keep run simple, and this is making it a bit more complex than I'd like.  Can you pull this out into a method like bool statementsNeedTransaction() on the class please?

::: storage/src/mozStorageStatementData.h
@@ +154,1 @@
>    {

MOZ_ASSERT(!NS_IsMainThread())
(at least I think this is safe to do)

::: storage/test/test_asyncStatementExecution_transaction.cpp
@@ +59,5 @@
> +  // -- complete the execution
> +  asyncSpin->SpinUntilCompleted();
> +
> +  // -- uninstall the transaction commit hook.
> +  ::sqlite3_commit_hook(static_cast<Connection *>(aDB)->GetNativeConnection(),

You don't need to call GetNativeConnection.  We have a sqlite3* operator on the class, so all you need to do is *static_cast<Connection *>(aDB).

@@ +248,5 @@
> +  check_transaction(db, stmts, NS_ARRAY_LENGTH(stmts), true);
> +}
> +
> +/**
> + * Test that executing a single AsyncStatement doesn't create a transaction.

read-only would be good to mention here

@@ +269,5 @@
> +  check_transaction(db, stmts, NS_ARRAY_LENGTH(stmts), false);
> +}
> +
> +/**
> + * Test that executing a single Statement doesn't create a transaction.

and here

@@ +342,5 @@
> +  test_MultipleWriteStatements,
> +  test_SingleAsyncReadStatement,
> +  test_SingleReadStatement,
> +  test_SingleAsyncWriteStatement,
> +  test_SingleWriteStatement,

You should also write a test (for AsyncStatement and Statement) that tests one read-only statement with multiple sets of bound parameters, and one write statement with multiple sets of bound parameters.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #13)
> Comment on attachment 533919 [details] [diff] [review] [review]
> patch v1.1
> 
> Review of attachment 533919 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> (In reply to comment #12)
> > no luck, I suspect the explicit immediate transaction is creating some
> > problem :(
> Along with my suggested refactoring, you could then count the number of
> statements that need a transaction, and if that number is great than one,
> return true, otherwise, return false.

This wouldn't work if I have only 1 statement but it has multiple binding params, unless instead of returning true, false from needsTransaction I return a int (1 for common statements, N for statements with N binding params).

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> This wouldn't work if I have only 1 statement but it has multiple binding
> params, unless instead of returning true, false from needsTransaction I return
> a int (1 for common statements, N for statements with N binding params).
Whoops.  I thought you were still doing that in your patch, but I see you are not.  We should still be doing that if it isn't read-only (which makes needs transaction even more complicated!).

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #15)
> (In reply to comment #14)
> Whoops.  I thought you were still doing that in your patch, but I see you
> are not.  We should still be doing that if it isn't read-only (which makes
> needs transaction even more complicated!).

There was no point, since I was not expecting an explicit transaction to create problems to VACUUM. Changing the return value from a bool to an int (number of implicit statements in need of a transaction), it should be easy to avoid an explicit transaction when an implicit one will be created.
Will try this and post results.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

This should address all comments, I'd prefer having a second look on it.
I had to remove a subtest from test_connection_executeAsync.js that was looking for a transaction is some fancy way, with my changes it wouldn't work, and my test does a better check.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

A doubt I have regard the case getSqliteStatement(&stmt) fails.
Iin this case i proceed checking mParamsArray, but maybe I should rather(?):

rc = getSqliteStatement(&stmt)
if (rc != SQLITE_OK || readonly)
  return 0;

and consider it not in need of a transaction (since invalid)

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 534518 [details] [diff] [review]
patch v1.2

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

r=sdwilsh

::: storage/src/mozStorageAsyncStatementExecution.cpp
@@ +534,5 @@
>    mozIStoragePendingStatement
>  )
>  
> +bool
> +AsyncExecuteStatements::statementsNeedTransaction()

I'd like to see a comment in the cpp file here explaining what we use to determine what needs a transaction like we had before.

::: storage/src/mozStorageStatementData.h
@@ +163,5 @@
> +    sqlite3_stmt *stmt;
> +    if (SQLITE_OK == getSqliteStatement(&stmt) &&
> +        ::sqlite3_stmt_readonly(stmt)) {
> +      return 0;
> +    }

Your suspicion in comment 18 is correct, but you'll have to do it a bit differently than you suggest.  Probably something like:
[get stmt]
[if error, return]
[if read-only, return]
[return size of mParams]

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

added the comment and addressed returning 0 if getSqliteStatement fails.
Landed in Places for baking:
http://hg.mozilla.org/projects/places/rev/36bf5b4f5e0a

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/36bf5b4f5e0a