Lazy Schema Change #342

yashNaN · 2019-04-08T20:38:24Z

For the design overview please take a look at the DESIGN.md file. Most of our modifications are localized to sql_table.cpp and sql_table.h . We have added tests in sql_table_test.cpp and catalog_test_util.h, and we have benchmarks in sql_table_benchmark.cpp

…o resolve multiple schemas

…jectedRow from different block

codeworm96 · 2019-05-11T18:27:49Z

test/storage/sql_table_test.cpp

-  storage::TupleSlot row_slot = table.EndRowAndInsert();
+  table.StartInsertRow();
+  table.SetIntColInRow(catalog::col_oid_t(0), 100);
+  table.SetVarcharColInRow(catalog::col_oid_t(1), "name");


You should also use longer varlen values to test for varlen entries which are not inlined.

codeworm96 · 2019-05-11T18:53:03Z

src/include/catalog/schema.h

      TERRIER_ASSERT(attr_size_ == VARLEN_COLUMN, "This constructor is meant for VARLEN columns.");
      TERRIER_ASSERT(type_ != type::TypeId::INVALID, "Attribute type cannot be INVALID.");
+      SetDefault(default_value);


This line is problematic. The most significant bit of VARLEN_COLUMN is set to 1. You should not use that directly as the size for memcpy in SetDefault, which will cause scary memory issues.

codeworm96 · 2019-05-11T19:00:38Z

test/storage/sql_table_test.cpp

@@ -38,62 +39,134 @@ class SqlTableRW {
      cols_.emplace_back(name, type, 255, nullable, oid);
  }

+  void AddColumn(transaction::TransactionContext *txn, std::string name, type::TypeId type, bool nullable,


AddColumn does not support adding varlen columns. You should also test adding varlen columns to make sure your code is handling varlens correctly.

codeworm96 · 2019-05-11T20:21:57Z

test/storage/sql_table_concurrent_test.cpp

+  const uint32_t num_threads = MultiThreadTestUtil::HardwareConcurrency();
+  common::WorkerPool thread_pool(num_threads, {});
+
+  for (uint32_t iteration = 0; iteration < num_iterations; iteration++) {


Maybe you do not need to repeat the test by yourself. googletest already provides the --gtest_repeat flag and the GTEST_REPEAT environment variable.

We're using the same setup that was already used for the DataTable tests. The repetitions here are useful for increasing the likelihood that CI will error out on race conditions in the concurrent tests without having to make deep modifications to the CMake build scripts.

codeworm96 · 2019-05-11T21:29:58Z

test/include/util/catalog_test_util.h

+    return pr;
+  }
+
+  // It populates a projected row with random bytes. The schema must not contain VarChar


You'd better have some assertions to ensure the schema does not contain VarChar.

codeworm96 · 2019-05-11T21:35:07Z

test/include/util/catalog_test_util.h

+  template <typename Random>
+  static void PopulateRandomRow(storage::ProjectedRow *row, const catalog::Schema &schema,
+                                const storage::ProjectionMap &pr_map, Random *const generator) {
+    for (auto &it : pr_map) {


This algorithm has low efficiency. Why not let the outer loop iterate over the columns and then look up the pr_map?

lmy1229 · 2019-05-11T21:53:20Z

src/storage/sql_table.cpp

+  for (const auto &column : schema.GetColumns()) {
+    auto col_oid = column.GetOid();
+    auto *default_value = column.GetDefault();
+    // Only populate the default values of the columns which are new and have a default value


Is it possible for the user to change the default value of a column? Or else it is treated as a new column (with a different OID)

It should be possible for a user to change the default value of a column. But what is happening here is that if a new column is added and a default value is set for that column on creation then it all tuples within the table should receive that default value for that column. But if a default value is set on an already created column then that default value is only applied to tuples inserted from that point on.

The storage engine only needs to reason about default values that are applied at the time the column is created as these are retroactive (applied to all existing data). Default value changes at any other time would only affect new inserts and have to be handled by the catalog, execution engine, and binder because we are not in a position at this level to reason about that granularity of visibility.

I think that part deserves better documentation. The user of SqlTable needs to know for default values which part is handled by SqlTable and which part they should take care of.

lmy1229 · 2019-05-11T22:01:18Z

src/storage/sql_table.cpp

+
+  // The slot version is not the same as the version_num
+  col_id_t original_column_ids[out_buffer->NumColumns()];
+  ModifyProjectionHeaderForVersion(out_buffer, tables_.Find(version_num)->second, old_dt_version, original_column_ids);


tables_.Find(version_num)->second is repeated several times here, and curr_dt_version is unused. Are you doing this for some concurrent issues?

codeworm96 · 2019-05-13T13:46:26Z

DESIGN.md

+
+## Future Work
+### Pending tasks
+#### Default values


Outdated? You have done some work about default values.

codeworm96 · 2019-05-13T14:30:10Z

src/include/storage/sql_table.h

+        auto next_table = tables_->Find(curr_version_);
+        if (next_table == tables_->CEnd()) {  // next_table does not exist (at end)
+          is_end_ = true;
+          break;


Break the loop with is_end_ set to be true. But immediately after that the assertion checks !is_end_, which is confusing.

codeworm96 · 2019-05-13T14:37:55Z

src/include/storage/sql_table.h

   */
-  catalog::table_oid_t Oid() const { return oid_; }
+  SlotIterator begin(layout_version_t txn_version) const {
+    // common::SpinLatch::ScopedSpinLatch guard(&tables_latch_);


Deadcode. Should be removed.

codeworm96 · 2019-05-13T14:40:31Z

src/include/storage/sql_table.h

   */
-  DataTable::SlotIterator begin() const { return table_.data_table->begin(); }
+  SlotIterator end() const {
+    // common::SpinLatch::ScopedSpinLatch guard(&tables_latch_);


Deadcode. Should be removed.

codeworm96 · 2019-05-13T14:54:07Z

src/include/storage/sql_table.h


  /**
-   * @return the first tuple slot contained in the underlying DataTable
+   * Returns one past the last tuple slot contained in the last data table. Note that this is not an accurate number


Confusing document: What is the number it is talking about? Tuple slot or something else?

codeworm96 · 2019-05-13T14:58:31Z

src/include/storage/sql_table.h

    TERRIER_ASSERT(col_ids.size() == col_oids.size(),
                   "Projection should be the same number of columns as requested col_oids.");
+    TERRIER_ASSERT(tables_.Find(version_num) != tables_.CEnd(), "Table version must exist before insert");


The message seems incorrect. InitializerForProjectedRow does not insert any row.

codeworm96 · 2019-05-13T19:58:17Z

test/include/util/catalog_test_util.h

@@ -33,5 +34,99 @@ struct CatalogTestUtil {
    }
    return catalog::Schema(columns);
  }
+
+  // The same as RandomSchem but it won't generate Varchar Column


typo: RandomSchema?

codeworm96 · 2019-05-13T20:15:37Z

test/include/util/catalog_test_util.h

+  static bool ProjectionListEqual(const catalog::Schema &schema, const RowType1 *const one, const RowType2 *const other,
+                                  const storage::ProjectionMap &map) {
+    if (one->NumColumns() != other->NumColumns()) return false;
+    for (auto &it : map) {


Similar to #342 (comment).

codeworm96 · 2019-05-13T20:45:28Z

src/storage/sql_table.cpp

+    // We should create a buffer of old Projected Row and update in place. We can't just
+    // directly erase the data without creating a redo and update the chain.
+
+    // auto old_pair = InitializerForProjectedRow(redo_col_oids, old_version);


What is these commented out code?

codeworm96 · 2019-05-13T23:02:59Z

src/storage/sql_table.cpp

+SqlTable::~SqlTable() {
+  while (default_value_map_.CBegin() != default_value_map_.CEnd()) {
+    auto pair = *(default_value_map_.CBegin());
+    delete[] pair.second.first;


For varlens that are not inlined, simply delete pair.second.first will leak memory.

codeworm96 · 2019-05-13T23:06:05Z

src/storage/sql_table.cpp

+    const ProjectedRowInitializer &initializer, layout_version_t version) const;
+
+// TODO(Yashwanth): don't copy the entire header, no need for template only take in ColumnIds() and then just modify
+// that when resetting header only have memc py ColumnIds()


typo: memc py

codeworm96 · 2019-05-13T23:14:15Z

src/storage/sql_table.cpp

+    auto *default_value = column.GetDefault();
+    // Only populate the default values of the columns which are new and have a default value
+    if (default_value_map_.Find(col_oid) == default_value_map_.End()) {
+      uint8_t attr_size = column.GetAttrSize();


Note that the results of GetAttrSize() of varlen attributes have the sign bit set. You will allocate a large chunk of memory. This will also leads to out of boundary access in std::memcpy.

codeworm96 · 2019-05-13T23:21:20Z

src/storage/sql_table.cpp

+  auto default_value = pair.first;
+  auto attr_size = pair.second;
+  // TODO(Sai): If this becomes a performance bottleneck, we can move this logic to ModifyProjectionHeaderForVersion
+  storage::StorageUtil::CopyWithNullCheck(default_value, out_buffer, attr_size, pr_map.at(col_oid));


CopyWithNullCheck only does a shallow copy and VarlenEntry has no reference counting. You may need careful reasoning about how this will interact with the GC.

codeworm96 · 2019-05-13T23:26:23Z

src/include/storage/sql_table.h

+  // NOTE: This map only keeps track of the default values specified at column creation
+  // For columns which don't have default value or added later, just set to null
+  // Populating default values into the ProjectedRow inserted later is taken care of by the execution engine
+  DefaultValueMap default_value_map_;


Users can drop columns. So some default values may be no longer needed in the future. You may want to have some way to shrink the DefaultValueMap.

jrolli · 2019-05-29T16:01:03Z

Blocked on #392 which will solidify the correct API for default values.

Yangjun Sheng added 30 commits March 14, 2019 16:05

Add a vector of DataTableVersion

bcbbcaa

Implemented multiversion select

63f80d2

Add functions for changing schemas in Sql Table, overload functions t…

ebb0ef1

…o resolve multiple schemas

Add multi_version_sql_table test

1723b43

Fix memory leak

cf91e19

Implemented multiversion updates, insertion and deletion

a3f1b96

Add mv-schema insert test

c82b7ff

Fix delete bug

9c7d5ef

Add mv-delete test

36b38f3

Merge branch 'master' into schema_change

0f173ee

Add mv-update test.

c85242e

Fix mv Select bug. Fix mv Update bug

66b6aa9

Format code

8717f51

Remove Slot Iterators in SqlTable

2ba3914

Remove single version Select and InitializerForProjectedRow

1d7d2db

Remove printf

2776858

Remove single version Insert and Delete

7506fc9

Remove ASSERT and unused_variables to deal with release version

03402cb

Add string test

01f48cf

Add versions for initializers in sql tables

21b968b

Add utility function for copying from one ProjectedRow to another Pro…

87b6bf3

…jectedRow from different block

Implement multiversion scan in SqlTable

99d95cb

Fix scan bug

5872f3c

Add Scan Test

de11304

Add SqlTable::SlotIterator fix scan logic

8d845f6

Fix bug in CopyProjectionIntoProjection

7ae48ae

Format

504e450

Make ChangeSchema Atomic

a4d7b57

Add sql_table concurrent insert test

6cdee78

Completely remove SqlTable with single DataTable

ea1305a

codeworm96 reviewed May 11, 2019

View reviewed changes

lmy1229 reviewed May 11, 2019

View reviewed changes

yashNaN and others added 2 commits May 12, 2019 18:08

Addressed some pr comments (#40)

8d22128

Fix test comments (#41)

9a7384a

codeworm96 reviewed May 13, 2019

View reviewed changes

DESIGN.md

## Future Work

### Pending tasks

#### Default values

Copy link

Contributor

codeworm96 May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated? You have done some work about default values.

codeworm96 reviewed May 13, 2019

View reviewed changes

jrolli added blocked This issue or pull request is in progress, but dependent on another task being completed first. and removed in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. labels May 29, 2019

jrolli mentioned this pull request Jul 25, 2019

Support updating a table's schema in the catalog #474

Open

jrolli mentioned this pull request Aug 13, 2019

Recovery Manager #459

Merged

13 tasks

mbutrovich closed this Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy Schema Change #342

Lazy Schema Change #342

yashNaN commented Apr 8, 2019

codeworm96 May 11, 2019

codeworm96 May 11, 2019

codeworm96 May 11, 2019

codeworm96 May 11, 2019

jrolli May 12, 2019

codeworm96 May 11, 2019

codeworm96 May 11, 2019

lmy1229 May 11, 2019

yashNaN May 12, 2019

jrolli May 12, 2019

codeworm96 May 13, 2019

lmy1229 May 11, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019

codeworm96 May 13, 2019 •

edited

codeworm96 May 13, 2019

codeworm96 May 13, 2019

jrolli commented May 29, 2019

Lazy Schema Change #342

Lazy Schema Change #342

Conversation

yashNaN commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeworm96 May 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrolli commented May 29, 2019

codeworm96 May 13, 2019 •

edited