Create meta table atomically.

Create meta table atomically.

http://crbug.com/111376 happened because a crash between creation and
population of the meta table could leave a meta table returning 0 for
the version numbers, even though no client ever stored 0 for the
versions.  This makes creation and population atomic.

Additionally, version numbers are restricted to positive values, to
prevent this case from being masked in the future.

BUG=116306
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125557

[Scott Hess - ex-Googler, 2012-03-06 22:18:33]

Erik, for making sure this matches findings WRT the Cookies problem, and that my
change to the test is appropriate.  And as an owner in that code.

Ben, that this is appropriate for DownloadDatabase.

Brett, as history owner, and to spank if this not an appropriate interface
change.

[benjhayden, 2012-03-06 22:34:34]

LGTM
Thanks!

Longer term, should HistoryDatabase have-a DownloadDatabase instead of the
current is-a relationship? Or should there be a baser base class for this kind
of method? Or should DownloadDatabase move out of HistoryDatabase and grow into
a real database with its own file? Or are there good reasons for the current
situation that I just haven't picked up yet?
I'm planning on doing some more work on DownloadDatabase at some point, so I'm
happy to fix it if people think it needs fixing.

https://chromiumcodereview.appspot.com/9592026/diff/1/sql/meta_table.cc
File sql/meta_table.cc (right):

https://chromiumcodereview.appspot.com/9592026/diff/1/sql/meta_table.cc#newco...
sql/meta_table.cc:40: // Make sure the table is created an populated atomically.
s/ an / and /

[brettw, 2012-03-06 23:23:33]

Rubber stamp LGTM, got too many reviews to care.

[erikwright (departed), 2012-03-07 14:57:34]

I'm concerned about how the transaction here will interact with a transaction
that the client might want to use.

For example, what is the suggested strategy for making sure that if the meta
table shows version 3, the full correct version 3 schema is present?

Presumably it should be:

BEGIN TRANSACTION
... create tables ...
... set current version ...
COMMIT

This CL does not support that.

I would suggest at least allowing a sql::Transaction* to be passed in. If it is
passed and open, use it (do not commit). Otherwise create, begin, and commit a
transaction in Init.

https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:164:
ASSERT_TRUE(db.Execute("DELETE FROM meta"));
I'd prefer if MetaTable either exposed a 'Delete' method or exposed a kTableName
static member.

[Scott Hess - ex-Googler, 2012-03-07 15:24:27]

On 2012/03/07 14:57:34, erikwright wrote:
> I'm concerned about how the transaction here will interact with a transaction
> that the client might want to use.

sql::Transaction implements fake nesting.  If an inner transaction's Commit is
not called, then the outer transaction will be flagged so that its Commit fails.
 So if the caller wants to make MetaTable::Init() part of a transaction with the
actual table setup, then just put all of that in a sql::Transaction.

> For example, what is the suggested strategy for making sure that if the meta
> table shows version 3, the full correct version 3 schema is present?

Even without that, our versions are entirely arbitrary anyhow, you could just
Init to version 1 with no schema, and have an upgrade flow handle it.  I would
actually prefer if our code never changed the initial create statement once
shipped, since the database is empty that doesn't really make anything faster,
and it does make things more error-prone.

https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:164:
ASSERT_TRUE(db.Execute("DELETE FROM meta"));
On 2012/03/07 14:57:34, erikwright wrote:
> I'd prefer if MetaTable either exposed a 'Delete' method or exposed a
kTableName
> static member.

I would not be happy exposing such information as part of a non-testing API. 
Exposing it as part of a testing API would effectively mean friending this test,
which to me feels more broken than just going direct.  I think the main goal
should be that this test should break if someone changes the internals of how
sql::MetaTable works, which I think happens here, which will happen, here.

[erikwright (departed), 2012-03-07 15:42:00]

On 2012/03/07 15:24:27, shess wrote:
> On 2012/03/07 14:57:34, erikwright wrote:
> > I'm concerned about how the transaction here will interact with a
transaction
> > that the client might want to use.
> 
> sql::Transaction implements fake nesting.  If an inner transaction's Commit is
> not called, then the outer transaction will be flagged so that its Commit
fails.
>  So if the caller wants to make MetaTable::Init() part of a transaction with
the
> actual table setup, then just put all of that in a sql::Transaction.

Great. That's not documented in sql/transaction.h. I will submit a CL R=you to
fix that.

> 
> > For example, what is the suggested strategy for making sure that if the meta
> > table shows version 3, the full correct version 3 schema is present?
> 
> Even without that, our versions are entirely arbitrary anyhow, you could just
> Init to version 1 with no schema, and have an upgrade flow handle it.  I would
> actually prefer if our code never changed the initial create statement once
> shipped, since the database is empty that doesn't really make anything faster,
> and it does make things more error-prone.

I like that approach, but in that case perhaps you should actually remove the
arguments to Init and then document this as the preferred approach:

meta_table_.Init();
if (meta_table_.version < 1) {
  transaction.Begin();
  CreateSchemaV1();
  meta_table_.setVersion(1);
  transaction.End();
}
if (meta_table_.version == 1) {
  ...
}

> 
>
https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
> File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
> chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:164:
> ASSERT_TRUE(db.Execute("DELETE FROM meta"));
> On 2012/03/07 14:57:34, erikwright wrote:
> > I'd prefer if MetaTable either exposed a 'Delete' method or exposed a
> kTableName
> > static member.
> 
> I would not be happy exposing such information as part of a non-testing API. 
> Exposing it as part of a testing API would effectively mean friending this
test,
> which to me feels more broken than just going direct.  I think the main goal
> should be that this test should break if someone changes the internals of how
> sql::MetaTable works, which I think happens here, which will happen, here.

Ultimately, I don't think that the test for the SQLitePersistenCookieStore
should even know that there IS a meta table in the database, so I understand
where you are coming from, though I still think these options are better than
duplicating the table name here. However I don't disagree enough to try to
persuade you (I'd rather refactor my code and tests so that I don't need to test
this in this way).

LGTM.

[Scott Hess - ex-Googler, 2012-03-08 00:01:45]

On 2012/03/07 15:42:00, erikwright wrote:
> On 2012/03/07 15:24:27, shess wrote:
> > On 2012/03/07 14:57:34, erikwright wrote:
> > > I'm concerned about how the transaction here will interact with a
> transaction
> > > that the client might want to use.
> > 
> > sql::Transaction implements fake nesting.  If an inner transaction's Commit
is
> > not called, then the outer transaction will be flagged so that its Commit
> fails.
> >  So if the caller wants to make MetaTable::Init() part of a transaction with
> the
> > actual table setup, then just put all of that in a sql::Transaction.
> 
> Great. That's not documented in sql/transaction.h. I will submit a CL R=you to
> fix that.
> 
> > 
> > > For example, what is the suggested strategy for making sure that if the
meta
> > > table shows version 3, the full correct version 3 schema is present?
> > 
> > Even without that, our versions are entirely arbitrary anyhow, you could
just
> > Init to version 1 with no schema, and have an upgrade flow handle it.  I
would
> > actually prefer if our code never changed the initial create statement once
> > shipped, since the database is empty that doesn't really make anything
faster,
> > and it does make things more error-prone.
> 
> I like that approach, but in that case perhaps you should actually remove the
> arguments to Init and then document this as the preferred approach:
> 
> meta_table_.Init();
> if (meta_table_.version < 1) {
>   transaction.Begin();
>   CreateSchemaV1();
>   meta_table_.setVersion(1);
>   transaction.End();
> }
> if (meta_table_.version == 1) {
>   ...
> }

Yeah, basically.  Plus it would be nice to have sugar for testing purposes, to
make certain that tests handle every possible starting point.  Plus you could
wrap the version-setting and transaction into a single scoper to enforce correct
usage - or you could use callbacks matching each version transition.  If you
want to be even more pedantic, you could make it so that DDL can only be run by
calling through the MetaTable interface.

Probably a pretty deep rabbit hole :-).


>
https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
> > File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9592026/diff/1/chrome/browser/net/sqli...
> > chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:164:
> > ASSERT_TRUE(db.Execute("DELETE FROM meta"));
> > On 2012/03/07 14:57:34, erikwright wrote:
> > > I'd prefer if MetaTable either exposed a 'Delete' method or exposed a
> > kTableName
> > > static member.
> > 
> > I would not be happy exposing such information as part of a non-testing API.

> > Exposing it as part of a testing API would effectively mean friending this
> test,
> > which to me feels more broken than just going direct.  I think the main goal
> > should be that this test should break if someone changes the internals of
how
> > sql::MetaTable works, which I think happens here, which will happen, here.
> 
> Ultimately, I don't think that the test for the SQLitePersistenCookieStore
> should even know that there IS a meta table in the database, so I understand
> where you are coming from, though I still think these options are better than
> duplicating the table name here. However I don't disagree enough to try to
> persuade you (I'd rather refactor my code and tests so that I don't need to
test
> this in this way).

I think it's exactly the opposite - we're testing that a specific starting
position existing in the world is handled correctly.  If the test were to rely
on SqlitePersistentCookieStore or MetaTable to set things up, then it's possible
that it will end up testing starting points which don't actually represent
anything interesting to test.

Even so, creating the database from scratch at this point gives me the willies. 
Long-term, it would be appropriate for the test code to contain snapshots of
databases with different schema which contain data, to make sure we're testing
the upgrade transitions.  But that's a bigger change than this one, and I
wouldn't want to do it in incremental bits.

> LGTM.

Thanks.

[commit-bot: I haz the power, 2012-03-08 00:03:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/9592026/1

[commit-bot: I haz the power, 2012-03-08 04:53:51]

Change committed as 125557