cmu-db / peloton

The Self-Driving Database Management System
http://pelotondb.io
Apache License 2.0
2.04k stars 622 forks source link

Namespace(Schema) support + Catalog refactoring #1302

Closed mengranwo closed 6 years ago

mengranwo commented 6 years ago

1. catalog refactoring

2. Namespace(Schema) support

default_database=#CREATE TABLE foo(a int);
create table under default namespace "public"

default_database=#CREATE SCHEMA private;
default_database=#CREATE TABLE private.foo(a int);
create table with same name "foo" under different namespace "private"
default_database=#DROP SCHEMA private;
delete record from pg_namespace catalog table and drop table private.foo
DataTable *table = catalog::Catalog::GetInstance()->GetTableWithName(
    database_name, schema_name, table_name, txn);

TableCatalogObject *table_object = catalog::Catalog::GetInstance()->GetTableObject(
    database_name, schema_name, table_name, txn);

3. To-Do

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 77.481% when pulling 781b6d75e1c0e405e7fd397216431e432d7537ed on camellyx:master into ff5b583720c9d0df4c16155089616d0d322fdf6f on cmu-db:master.

mengranwo commented 6 years ago

Hey @poojanilangekar!
Thank you for your reviews and I've made changes according to your suggestion. The following is a summary to what I've modified and answers to some of your question.

  • delete all the commented out line
  • Add java doc style documentation if necessary
  • Add CATALOG_TABLES_COUNT variable into catalog_defaults.h to get rid of the magic number 8
  • The reason why we change the visibility of the catalog contractors from private to public is that: before this PR each catalog table is a global singleton (thus the constructor must be private), but now each database has their own set of catalog tables (a.k.a system_catalogs.h), when creating a new system catalog object, we need to create multiple catalog tables(pg_table, pg_index, etc), thus the constructors must be public now
  • In all the test cases, we require you to use lowercase database_name. Cause many test cases invoke catalog::CreateDatabase(database_name) directly, and we use create sql to build secondary catalog tables(e.g CREATE TABLE database_name.pg_trigger(a int);) which means that this sql statement will pass through parser--> binder-->optimizer-->planner-->executor.And binder/optimizer will convert whatever name to be lowercase. We need to keep both name in sync, otherwise "can't find database" exception will be thrown.
  • PR #1286 is adding TempSchema
  • I'll push the schema test cases during weekend.
poojanilangekar commented 6 years ago

@tcm-marcel, The same issue again. Travis build is not triggered. Do you think the bug is because of some error in our configuration or a bug on their end?

camellyx commented 6 years ago

@poojanilangekar Thanks for your comments. We are working on adding more tests, if possible, once we have time.

poojanilangekar commented 6 years ago

@camellyx Can this PR be merged in for now? You can add more tests in the subsequent PR. If this gets merged in and I merge in the layout PR #1327, then we can start work on schema change. Or do you want to wait? Let me know what works for you.

mengranwo commented 6 years ago

@poojanilangekar I think we are ready to go.

poojanilangekar commented 6 years ago

Thanks @mengranwo! @pervazea @apavlo Can you please merge this in?

nwang57 commented 6 years ago

@mengranwo I think there are typos in src/include/catalog/catalog_defaults.h and other places. DEFUALT_SCHEMA_OID and DEFUALT_SCHEMA_NAME which should be DEFAULT_SCHEMA_OID and DEFAULT_SCHEMA_NAME