From 6f68bd3907164c404842a54b217fe048ebf80f93 Mon Sep 17 00:00:00 2001 From: shess Date: Thu, 4 Feb 2016 11:29:44 -0800 Subject: [sql] Remove misleading AutoRecoverTable() parameter. |extend_columns| was intended to be used in the case where the target schema differed slightly from the schema of the table being recovered. Unfortunately, it actually implemented part of the solution for when the target has fewer columns and described it as the solution for when the target has more columns. Remove the unnecessary code and parameter. [In general, table schema only add new columns, removing is more infrequent.] BUG=none Review URL: https://codereview.chromium.org/1666473003 Cr-Commit-Position: refs/heads/master@{#373586} --- sql/recovery.cc | 9 --------- sql/recovery.h | 9 +++------ sql/recovery_unittest.cc | 49 +++++++++++++++++++++++++++++------------------- 3 files changed, 33 insertions(+), 34 deletions(-) (limited to 'sql') diff --git a/sql/recovery.cc b/sql/recovery.cc index 5e101b1..5c4223c 100644 --- a/sql/recovery.cc +++ b/sql/recovery.cc @@ -349,7 +349,6 @@ void Recovery::Shutdown(Recovery::Disposition raze) { } bool Recovery::AutoRecoverTable(const char* table_name, - size_t extend_columns, size_t* rows_recovered) { // Query the info for the recovered table in database [main]. std::string query( @@ -459,14 +458,6 @@ bool Recovery::AutoRecoverTable(const char* table_name, if (pk_column_count == 1 && !rowid_decl.empty()) create_column_decls[rowid_ofs] = rowid_decl; - // Additional columns accept anything. - // TODO(shess): ignoreN isn't well namespaced. But it will fail to - // execute in case of conflicts. - for (size_t i = 0; i < extend_columns; ++i) { - create_column_decls.push_back( - base::StringPrintf("ignore%" PRIuS " ANY", i)); - } - std::string recover_create(base::StringPrintf( "CREATE VIRTUAL TABLE temp.recover_%s USING recover(corrupt.%s, %s)", table_name, diff --git a/sql/recovery.h b/sql/recovery.h index c03ebb2..2c191cf 100644 --- a/sql/recovery.h +++ b/sql/recovery.h @@ -118,9 +118,8 @@ class SQL_EXPORT Recovery { // in database [main]. Data is copied using INSERT OR REPLACE, so // duplicates overwrite each other. // - // |extend_columns| allows recovering tables which have excess - // columns relative to the target schema. The recover virtual table - // treats more data than specified as a sign of corruption. + // If the source table has fewer columns than the target, the target + // DEFAULT value will be used for those columns. // // Returns true if all operations succeeded, with the number of rows // recovered in |*rows_recovered|. @@ -134,9 +133,7 @@ class SQL_EXPORT Recovery { // // TODO(shess): Flag for INSERT OR REPLACE vs IGNORE. // TODO(shess): Handle extended table names. - bool AutoRecoverTable(const char* table_name, - size_t extend_columns, - size_t* rows_recovered); + bool AutoRecoverTable(const char* table_name, size_t* rows_recovered); // Setup a recover virtual table at temp.recover_meta, reading from // corrupt.meta. Returns true if created. diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc index 11f2a87..73d146f 100644 --- a/sql/recovery_unittest.cc +++ b/sql/recovery_unittest.cc @@ -429,7 +429,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTable) { ExecuteWithResults(recovery->db(), kTempSchemaSql, "|", "\n")); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(2u, rows); // Test that any additional temp tables were cleaned up. @@ -452,7 +452,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTable) { // TODO(shess): Should this failure implicitly lead to Raze()? size_t rows = 0; - EXPECT_FALSE(recovery->AutoRecoverTable("y", 0, &rows)); + EXPECT_FALSE(recovery->AutoRecoverTable("y", &rows)); sql::Recovery::Unrecoverable(std::move(recovery)); } @@ -509,7 +509,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithDefault) { ASSERT_TRUE(recovery->db()->Execute(final_schema.c_str())); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(4u, rows); ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery))); @@ -545,7 +545,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableNullFilter) { ASSERT_TRUE(recovery->db()->Execute(kFinalSchema)); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(1u, rows); ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery))); @@ -584,7 +584,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithRowid) { ASSERT_TRUE(recovery->db()->Execute(kCreateSql)); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(2u, rows); ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery))); @@ -629,7 +629,7 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithCompoundKey) { ASSERT_TRUE(recovery->db()->Execute(kCreateSql)); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(3u, rows); ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery))); @@ -642,29 +642,40 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableWithCompoundKey) { ASSERT_EQ(orig_data, ExecuteWithResults(&db(), kXSql, "|", "\n")); } -// Test |extend_columns| support. -TEST_F(SQLRecoveryTest, AutoRecoverTableExtendColumns) { +// Test recovering from a table with fewer columns than the target. +TEST_F(SQLRecoveryTest, AutoRecoverTableMissingColumns) { const char kCreateSql[] = "CREATE TABLE x (id INTEGER PRIMARY KEY, t0 TEXT)"; + const char kAlterSql[] = "ALTER TABLE x ADD COLUMN t1 TEXT DEFAULT 't'"; ASSERT_TRUE(db().Execute(kCreateSql)); ASSERT_TRUE(db().Execute("INSERT INTO x VALUES (1, 'This is')")); ASSERT_TRUE(db().Execute("INSERT INTO x VALUES (2, 'That was')")); - // Save aside a copy of the original schema and data. + // Generate the expected info by faking a table to match what recovery will + // create. const std::string orig_schema(GetSchema(&db())); const char kXSql[] = "SELECT * FROM x ORDER BY 1"; - const std::string orig_data(ExecuteWithResults(&db(), kXSql, "|", "\n")); + std::string expected_schema; + std::string expected_data; + { + ASSERT_TRUE(db().BeginTransaction()); + ASSERT_TRUE(db().Execute(kAlterSql)); - // Modify the table to add a column, and add data to that column. - ASSERT_TRUE(db().Execute("ALTER TABLE x ADD COLUMN t1 TEXT")); - ASSERT_TRUE(db().Execute("UPDATE x SET t1 = 'a test'")); - ASSERT_NE(orig_schema, GetSchema(&db())); - ASSERT_NE(orig_data, ExecuteWithResults(&db(), kXSql, "|", "\n")); + expected_schema = GetSchema(&db()); + expected_data = ExecuteWithResults(&db(), kXSql, "|", "\n"); + db().RollbackTransaction(); + } + + // Following tests are pointless if the rollback didn't work. + ASSERT_EQ(orig_schema, GetSchema(&db())); + + // Recover the previous version of the table into the altered version. { scoped_ptr recovery = sql::Recovery::Begin(&db(), db_path()); ASSERT_TRUE(recovery->db()->Execute(kCreateSql)); + ASSERT_TRUE(recovery->db()->Execute(kAlterSql)); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 1, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(2u, rows); ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery))); } @@ -672,8 +683,8 @@ TEST_F(SQLRecoveryTest, AutoRecoverTableExtendColumns) { // Since the database was not corrupt, the entire schema and all // data should be recovered. ASSERT_TRUE(Reopen()); - ASSERT_EQ(orig_schema, GetSchema(&db())); - ASSERT_EQ(orig_data, ExecuteWithResults(&db(), kXSql, "|", "\n")); + ASSERT_EQ(expected_schema, GetSchema(&db())); + ASSERT_EQ(expected_data, ExecuteWithResults(&db(), kXSql, "|", "\n")); } // Recover a golden file where an interior page has been manually modified so @@ -697,7 +708,7 @@ TEST_F(SQLRecoveryTest, Bug387868) { ASSERT_TRUE(recovery->db()->Execute(kCreateSql)); size_t rows = 0; - EXPECT_TRUE(recovery->AutoRecoverTable("x", 0, &rows)); + EXPECT_TRUE(recovery->AutoRecoverTable("x", &rows)); EXPECT_EQ(43u, rows); // Successfully recovered. -- cgit v1.1