Skip to content
This repository has been archived by the owner on Sep 27, 2019. It is now read-only.

Catalog updates for checkpoints #1383

Merged
merged 18 commits into from
Jun 18, 2018
Merged

Catalog updates for checkpoints #1383

merged 18 commits into from
Jun 18, 2018

Conversation

ksaito7
Copy link
Contributor

@ksaito7 ksaito7 commented May 25, 2018 •
edited
Loading

This PR updates some issues related to catalog stuff for checkpoints.

- Related Issues

  1. Add a column length in ColumnCatalogObject and pg_attribute #1319 Add column length in ColumnCatalogObject and pg_attribute
  2. Catalog OID collision in checkpoint recovery  #1320 Catalog OID collision in checkpoint recovery
  3. Unexpected catalog object might be obtained from CatalogCache #1373 Unexpected catalog object might be obtained from CatalogCache
  4. Can't recover tile group from LayoutCatalog in checkpoint recovery #1374 Can't recover tile group from LayoutCatalog in checkpoint recovery

- Fix Points

  1. Add column_length feild within pg_attribute and ColumnCatalogObject, and modify related functions in ColumnCatalog.
  2. Add UpdateOid() function within catalogs using oid_ in order to avoid collision between catalog values added by system and users. And add OID_FOR_USER_OFFSET (= 10000) variable within catalog_default.cpp to be used for UpdateOid() function. These are called in last of bootstraps of Catalog and SystemCatalog.
  3. Add database oid/name argument within GetChachedTableObject() function and GetChachedIndexObject() function in order to avoid to acquire an unexpected table from other database. Also fix related functions, GetTableObject() in TableCatalog and GetIndexObject() in IndexCatalog.
  4. Modify Layout stuff to recover layout information for default layout of DataTable and layout of TileGroup.
    1. Modify Layout constructor Layout(num_columns, layout_type) to set appropriate layout oid.
    2. Modify CreateTable() and CreateLayout() in Catalog class to insert all Layout information including row layout and column layout into pg_layout, LayoutCatalog. These avoid to insert same layout. And also modify DropLayout() in Catalog to recreate default layout in pg_catalog in case that it is deleted.
    3. Add default_layout_oid field within pg_table and TableCatalogObject, and modify related functions in TableCatalog. To support for updating default layout, add UpdateDefaultLayoutOid() function within TableCatalog. It is used in CreateDefaultLayout() function and DropLayout() function.

@apavlo apavlo requested review from gvos94 and poojanilangekar May 25, 2018 20:08
@coveralls
Copy link

coveralls commented May 26, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 77.111% when pulling 05e042c on ksaito7:catalog_update into e53e5b4 on cmu-db:master.

@apavlo apavlo requested a review from db-ol June 5, 2018 16:53
@ksaito7 ksaito7 mentioned this pull request Jun 8, 2018
Closed
db-ol
db-ol suggested changes Jun 16, 2018
View reviewed changes
Copy link
Contributor

@db-ol db-ol left a comment

Choose a reason for hiding this comment

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

The logic solves the problems. Remove the dead code, use macro names to make it more readable, and don't introduce changes if unnecessary.

src/catalog/catalog.cpp Outdated
@@ -169,22 +169,22 @@ void Catalog::BootstrapSystemCatalogs(storage::Database *database,
// pg_database record is shared across different databases
system_catalogs->GetTableCatalog()->InsertTable(
DATABASE_CATALOG_OID, DATABASE_CATALOG_NAME, CATALOG_SCHEMA_NAME,
CATALOG_DATABASE_OID, pool_.get(), txn);
CATALOG_DATABASE_OID, 0, pool_.get(), txn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with its macro name ROW_STORE_OID. The same below.

Copy link
Contributor Author

@ksaito7 ksaito7 Jun 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I moved macros defined in layout.h into catalog_defaults.h and modify all raw values with these macros.

src/catalog/catalog.cpp Outdated
system_catalogs->GetTableCatalog()->InsertTable(
SCHEMA_CATALOG_OID, SCHEMA_CATALOG_NAME, CATALOG_SCHEMA_NAME,
database_oid, pool_.get(), txn);
database_oid, 0, pool_.get(), txn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with its macro name ROW_STORE_OID.

src/catalog/catalog.cpp Outdated
system_catalogs->GetTableCatalog()->InsertTable(
TABLE_CATALOG_OID, TABLE_CATALOG_NAME, CATALOG_SCHEMA_NAME, database_oid,
pool_.get(), txn);
0, pool_.get(), txn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with its macro name ROW_STORE_OID.

src/catalog/catalog.cpp Outdated
system_catalogs->GetTableCatalog()->InsertTable(
INDEX_CATALOG_OID, INDEX_CATALOG_NAME, CATALOG_SCHEMA_NAME, database_oid,
pool_.get(), txn);
0, pool_.get(), txn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with its macro name ROW_STORE_OID.

src/catalog/catalog.cpp Outdated
system_catalogs->GetTableCatalog()->InsertTable(
COLUMN_CATALOG_OID, COLUMN_CATALOG_NAME, CATALOG_SCHEMA_NAME,
database_oid, pool_.get(), txn);
database_oid, 0, pool_.get(), txn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with its macro name ROW_STORE_OID.

test/catalog/catalog_test.cpp Outdated
// Change default layout.
// Check the first default layout
auto first_default_layout = table->GetDefaultLayout();
EXPECT_EQ(0, first_default_layout->GetOid());
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with its macro name ROW_STORE_OID.

test/catalog/catalog_test.cpp
auto default_layout_oid = default_layout->GetOid();
EXPECT_EQ(default_layout_oid, table->GetDefaultLayout()->GetOid());
EXPECT_FALSE(default_layout->IsColumnStore());
EXPECT_FALSE(default_layout->IsRowStore());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test if its type is HYBRID directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there was not a function to access the layout type directly. These two EXPECT_FALSE tests match HYBRID actually.

Copy link
Contributor

@db-ol db-ol Jun 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about just adding a function like IsHybridStore() to make it more clear?

test/catalog/catalog_test.cpp

// Check the created layout
EXPECT_FALSE(other_layout->IsColumnStore());
EXPECT_FALSE(other_layout->IsRowStore());
Copy link
Contributor

Choose a reason for hiding this comment

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

The same question as before. Can we test if its type is HYBRID directly?

test/catalog/catalog_test.cpp Outdated
EXPECT_TRUE(table->GetDefaultLayout().IsRowStore());
EXPECT_NE(default_layout, table->GetDefaultLayout());
EXPECT_TRUE(table->GetDefaultLayout()->IsRowStore());
EXPECT_EQ(0, table->GetDefaultLayout()->GetOid());
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the macro name of 0.

test/catalog/catalog_test.cpp Outdated
EXPECT_EQ(nullptr,
pg_layout->GetLayoutWithOid(table_oid, default_layout_oid, txn));
EXPECT_EQ(0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace 0 with nullptr.

@ksaito7
Copy link
Contributor Author

ksaito7 commented Jun 17, 2018

@aaron-tian Thanks for a lot of comments! I modified them and reply each comments. Please check them again.

@db-ol
Copy link
Contributor

db-ol commented Jun 17, 2018

Thanks for the nice work. IsHybridStore() could be added and the type of GetColumnOffset() could be changed if you think it's a good idea, all the others LGTM.

@ksaito7
Copy link
Contributor Author

ksaito7 commented Jun 18, 2018

I update them you mentioned. thanks.

@tli2 tli2 merged commit 4d61826 into cmu-db:master Jun 18, 2018
@tli2 tli2 mentioned this pull request Jun 18, 2018
Closed
@ksaito7 ksaito7 mentioned this pull request Jun 21, 2018
Open
mtunique pushed a commit to mtunique/peloton that referenced this pull request Apr 16, 2019
@tli2
Merge pull request cmu-db#1383 from ksaito7/catalog_update
931fa92 
Catalog updates for checkpoints
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@db-ol db-ol db-ol approved these changes

@gvos94 gvos94 Awaiting requested review from gvos94

@poojanilangekar poojanilangekar Awaiting requested review from poojanilangekar

Assignees
No one assigned
Labels
accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ksaito7 @coveralls @db-ol @tli2 @apavlo