feat: introduce 'pg_catalog.pg_type' #4332
Conversation
…on_schema and pg_catalog
WalkthroughThe changes introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
src/catalog/src/kvbackend/manager.rs (1)
305-305: Documentation update for system tablesThe documentation update reflects the addition of
pg_catalogtables, ensuring that the documentation is consistent with the new functionality.- /// - public.numbers - /// - information_schema.{tables} + /// - public.numbers + /// - information_schema.{tables} + /// - pg_catalog.{tables}
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (28)
- src/catalog/src/kvbackend/manager.rs (5 hunks)
- src/catalog/src/lib.rs (1 hunks)
- src/catalog/src/memory/manager.rs (3 hunks)
- src/catalog/src/system_schema.rs (1 hunks)
- src/catalog/src/system_schema/information_schema.rs (5 hunks)
- src/catalog/src/system_schema/information_schema/cluster_info.rs (1 hunks)
- src/catalog/src/system_schema/information_schema/information_memory_table.rs (2 hunks)
- src/catalog/src/system_schema/information_schema/key_column_usage.rs (1 hunks)
- src/catalog/src/system_schema/information_schema/partitions.rs (1 hunks)
- src/catalog/src/system_schema/information_schema/region_peers.rs (1 hunks)
- src/catalog/src/system_schema/information_schema/schemata.rs (1 hunks)
- src/catalog/src/system_schema/information_schema/tables.rs (1 hunks)
- src/catalog/src/system_schema/memory_table.rs (8 hunks)
- src/catalog/src/system_schema/memory_table/table_columns.rs (1 hunks)
- src/catalog/src/system_schema/memory_table/tables.rs (1 hunks)
- src/catalog/src/system_schema/pg_catalog.rs (1 hunks)
- src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs (1 hunks)
- src/catalog/src/system_schema/pg_catalog/table_names.rs (1 hunks)
- src/common/catalog/src/consts.rs (2 hunks)
- tests/cases/standalone/common/create/create_database.result (1 hunks)
- tests/cases/standalone/common/create/create_database_opts.result (3 hunks)
- tests/cases/standalone/common/information_schema/tables.result (1 hunks)
- tests/cases/standalone/common/show/show_databases_tables.result (2 hunks)
- tests/cases/standalone/common/system/information_schema.result (6 hunks)
- tests/cases/standalone/common/system/information_schema.sql (1 hunks)
- tests/cases/standalone/common/system/pg_catalog.result (1 hunks)
- tests/cases/standalone/common/system/pg_catalog.sql (1 hunks)
- tests/cases/standalone/common/view/create.result (1 hunks)
Files skipped from review due to trivial changes (10)
- src/catalog/src/system_schema/information_schema/key_column_usage.rs
- src/catalog/src/system_schema/information_schema/partitions.rs
- src/catalog/src/system_schema/information_schema/schemata.rs
- src/catalog/src/system_schema/information_schema/tables.rs
- src/catalog/src/system_schema/pg_catalog/table_names.rs
- tests/cases/standalone/common/create/create_database.result
- tests/cases/standalone/common/create/create_database_opts.result
- tests/cases/standalone/common/information_schema/tables.result
- tests/cases/standalone/common/show/show_databases_tables.result
- tests/cases/standalone/common/system/information_schema.sql
Additional comments not posted (46)
tests/cases/standalone/common/system/pg_catalog.sql (1)
1-4: SQL commands are correct.The commands correctly test the creation and selection from the
pg_catalogschema.tests/cases/standalone/common/system/pg_catalog.result (1)
1-31: Expected results are correct.The results correctly reflect the error and the contents of the
pg_typetable.src/catalog/src/system_schema/memory_table/table_columns.rs (1)
1-50: Macro and test are correct.The
memory_table_colsmacro correctly initializes and populates columns. The test verifies its correctness.src/catalog/src/system_schema/memory_table/tables.rs (1)
1-79: Functions and tests are correct.The functions correctly create column schemas for various data types. The tests verify their correctness.
src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs (1)
24-59: LGTM! Verify the usage of the macromemory_table_cols!.The function
pg_type_schema_columnscorrectly defines columns and their types for thepg_typetable in thepg_catalogschema.However, ensure the macro
memory_table_cols!is used correctly and consistently throughout the codebase.Verification successful
LGTM!
The macro
memory_table_cols!is correctly defined and consistently used in the codebase.
- Definition in
src/catalog/src/system_schema/memory_table/table_columns.rs- Usage in
src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs- Usage in
src/catalog/src/system_schema/memory_table/table_columns.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the macro `memory_table_cols!`. # Test: Search for the macro usage. Expect: Consistent usage throughout the codebase. rg --type rust --word memory_table_colsLength of output: 479
Script:
#!/bin/bash # Description: Verify the usage of the macro `memory_table_cols!`. # Test: Search for the macro usage. Expect: Consistent usage throughout the codebase. rg --type rust --word-regexp memory_table_colsLength of output: 418
Script:
#!/bin/bash # Extract the definition of the macro `memory_table_cols!` and its usages in the specified files. # Extract the macro definition echo "Macro Definition in table_columns.rs:" rg --type rust --context 10 --word-regexp "macro_rules! memory_table_cols" src/catalog/src/system_schema/memory_table/table_columns.rs # Extract the macro usage in pg_catalog_memory_table.rs echo -e "\nMacro Usage in pg_catalog_memory_table.rs:" rg --type rust --context 10 --word-regexp "memory_table_cols" src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs # Extract the macro usage in table_columns.rs echo -e "\nMacro Usage in table_columns.rs:" rg --type rust --context 10 --word-regexp "memory_table_cols" src/catalog/src/system_schema/memory_table/table_columns.rsLength of output: 3410
src/catalog/src/lib.rs (2)
34-37: LGTM!The module declaration for
system_schemaand the re-export ofinformation_schemafor legacy compatibility are correct.
Line range hint
40-89:
LGTM!The trait
CatalogManagerdefines various async methods for managing catalogs, schemas, and tables. The return types and method signatures are appropriate.src/catalog/src/system_schema/pg_catalog.rs (4)
47-51: LGTM!The struct
PGCatalogProvidercorrectly includes fields for catalog name, catalog manager, and tables. The use ofWeakfor the catalog manager is appropriate.
79-87: LGTM!The
newfunction correctly initializes a newPGCatalogProviderinstance and callsbuild_tablesto initialize the tables.
89-97: LGTM!The
build_tablesfunction correctly initializes the tables for thePGCatalogProviderby iterating overMEMORY_TABLES. The note regarding security rules is a good practice.
62-76: LGTM!The
setup_memory_tablemacro correctly sets up memory tables for thepg_catalogschema. The use of thepastecrate is appropriate.src/common/catalog/src/consts.rs (1)
99-104: LGTM!The constants related to the
pg_catalogschema are correctly defined. The use of a mask for table IDs ensures uniqueness.src/catalog/src/system_schema.rs (4)
15-18: Module declarations align with the new structure.The new module declarations for
information_schema,memory_table, andpg_catalogalign with the reorganization and new additions mentioned in the PR summary.
37-61: Introduction ofSystemSchemaProvidertrait is well-defined.The methods within the
SystemSchemaProvidertrait for managing system tables are well-defined and align with the overall goal of managing system tables.
63-94: Introduction ofSystemSchemaProviderInnertrait is clear and necessary.The methods within the
SystemSchemaProviderInnertrait provide necessary internal operations for managing system schemas.
97-164: Introduction ofSystemTabletrait and its implementations is well-structured.The
SystemTabletrait, associated types, and implementations are well-structured and necessary for defining and managing system tables.src/catalog/src/system_schema/memory_table.rs (3)
15-17: Module declarations align with the new structure.The new module declarations for
table_columnsandtablesalign with the reorganization and new additions mentioned in the PR summary.
Line range hint
37-60:
Introduction ofMemoryTablestruct and its methods is well-structured.The
MemoryTablestruct and its methods provide necessary functionality for managing in-memory tables.
107-138: Implementation ofSystemTableforMemoryTableis well-structured.The implementation of
SystemTableforMemoryTablealigns with theSystemTabletrait and provides necessary functionality for in-memory tables.src/catalog/src/system_schema/information_schema.rs (4)
17-18: Module declarations align with the new structure.The new module declarations for
information_memory_tableandkey_column_usagealign with the reorganization and new additions mentioned in the PR summary.
108-114: Implementation ofSystemSchemaProviderforInformationSchemaProvideris well-structured.The implementation of
SystemSchemaProviderforInformationSchemaProvideraligns with theSystemSchemaProvidertrait and provides necessary functionality for managing information schema tables.
Line range hint
115-182:
Implementation ofSystemSchemaProviderInnerforInformationSchemaProvideris well-structured.The implementation of
SystemSchemaProviderInnerforInformationSchemaProvideraligns with theSystemSchemaProviderInnertrait and provides necessary internal operations for managing information schema tables.
258-280: Compatibility implementation for legacyinformation_schemacode is well-structured.The compatibility implementation ensures backward compatibility with legacy
information_schemacode.src/catalog/src/system_schema/information_schema/region_peers.rs (1)
43-43: Implementation ofInformationSchemaRegionPeersis well-structured.The implementation of
InformationSchemaRegionPeersand related traits for managing region peers information is detailed and aligns with the overall goal of managing region peers information.src/catalog/src/system_schema/information_schema/cluster_info.rs (1)
44-44: Importing new modules frominformation_schemaThe import of
utils,InformationTable, andPredicatesfrominformation_schemais necessary to support the new functionality introduced for managing cluster info.src/catalog/src/kvbackend/manager.rs (9)
22-22: Addition ofPG_CATALOG_NAMEconstantThe addition of the
PG_CATALOG_NAMEconstant is necessary for the newpg_catalogschema.
50-51: ImportingPGCatalogProviderThe import of
PGCatalogProviderfromsystem_schema::pg_catalogis necessary for managing the newpg_catalogschema.
96-99: Initialization ofpg_catalog_providerinKvBackendCatalogManagerThe initialization of
pg_catalog_providerin theKvBackendCatalogManagerconstructor is essential for supporting the newpg_catalogschema.
311-311: Addition ofpg_catalog_providertoSystemCatalogstructThe addition of
pg_catalog_providerto theSystemCatalogstruct is necessary for managing the newpg_catalogschema.
315-320: Schema names method updateThe
schema_namesmethod now includesPG_CATALOG_NAME, allowing the system to recognize thepg_catalogschema.
324-330: Table names method updateThe
table_namesmethod now includes handling forPG_CATALOG_NAME, enabling the system to manage tables within thepg_catalogschema.
335-335: Schema exists method updateThe
schema_existsmethod now checks for thepg_catalogschema, ensuring the system can verify the existence of this schema.
343-344: Table exists method updateThe
table_existsmethod now includes a check for tables within thepg_catalogschema, ensuring proper management of these tables.
360-362: Table method updateThe
tablemethod now includes handling for thepg_catalogschema, ensuring that the system can retrieve tables from this schema.src/catalog/src/memory/manager.rs (4)
23-24: Addition ofPG_CATALOG_NAMEconstantThe addition of the
PG_CATALOG_NAMEconstant is necessary for the newpg_catalogschema.
32-32: ImportingSystemSchemaProviderThe import of
SystemSchemaProvideris necessary to support the new system schema changes.
178-183: Registeringpg_catalogschemaThe registration of the
pg_catalogschema in thewith_default_setupmethod ensures that the memory catalog manager recognizes and can manage this new schema.
207-207: Update tocatalog_exist_syncmethodThe update to the
catalog_exist_syncmethod is necessary to ensure that thepg_catalogschema is recognized by the system.src/catalog/src/system_schema/information_schema/information_memory_table.rs (3)
18-24: Importing necessary modulesThe import of
Schema,SchemaRef, andVectorReffromdatatypes::schemaanddatatypes::vectorsis necessary for defining the schemas and columns for the new tables.
21-24: Importing table names and memory table utilitiesThe import of
table_namesand memory table utilities is necessary for defining the schemas and columns for the new tables.
30-30: Defining schemas and columns for new tablesThe function
get_schema_columnsdefines the schemas and columns for various new tables in theinformation_schema. This change is necessary to support the new functionality.tests/cases/standalone/common/view/create.result (1)
97-97: Addition ofpg_typetable inpg_catalogschema looks good.The new entry for the
pg_typetable in thepg_catalogschema is consistent with the existing entries.tests/cases/standalone/common/system/information_schema.result (4)
45-45: Addition ofpg_typetable entry inpg_catalogschema is appropriate.The addition of this entry aligns with the objective of supporting
pg_catalog.
391-393: Addition of columns forpg_typetable inpg_catalogschema is appropriate.The columns
oid,typlen, andtypnameare standard for thepg_typetable and their inclusion aligns with the PR objective.
449-449: Addition of filter to excludepg_catalogschema is appropriate.This change ensures that the query does not return tables from the
pg_catalogschema, aligning with the PR objective.
463-463: Addition of filter to excludepg_catalogschema is appropriate.This change ensures that the query does not return tables from the
pg_catalogschema, aligning with the PR objective.
src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs
Outdated
Show resolved
Hide resolved
J0HN50N133
changed the title
|
@tisonkun can you review on this? |
|
These cases failed: I think it's mainly we add a new table. Please update them correspondingly. |
github-actions
bot
added
docs-required
Fixed. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/catalog/src/kvbackend/manager.rs (5 hunks)
- tests-integration/src/tests/instance_test.rs (3 hunks)
Additional comments not posted (10)
src/catalog/src/kvbackend/manager.rs (8)
22-22: Import change looks good.The addition of
PG_CATALOG_NAMEis necessary to support the newpg_catalogschema.
50-50: Import change looks good.The addition of
PGCatalogProvideris necessary to support the newpg_catalogschema.
51-51: Import change looks good.The addition of
SystemSchemaProvideris necessary to support the new system schema handling.
92-100: Changes toKvBackendCatalogManager::newlook good.Adding
pg_catalog_cacheand initializingpg_catalog_providerare necessary to support the newpg_catalogschema.
311-311: Changes toSystemCataloglook good.Adding
pg_catalog_cacheis necessary to support caching for the newpg_catalogschema.
315-315: Changes toSystemCataloglook good.Adding
pg_catalog_provideris necessary to support the newpg_catalogschema.
319-324: Changes toSystemCatalog::schema_nameslook good.Adding
PG_CATALOG_NAMEto the list of schema names is necessary to support the newpg_catalogschema.
328-331: Changes toSystemCatalog::table_nameslook good.Adding support for
pg_catalogin thetable_namesmethod is necessary to support the newpg_catalogschema.tests-integration/src/tests/instance_test.rs (2)
465-465: Verify the inclusion ofpg_catalogin the database listings.Ensure that the
pg_catalogschema is correctly included in the output of the "show databases" command.Verification successful
Verify that
pg_catalogis included in the test assertions for "show databases" command.Check the test assertions to ensure that
pg_catalogis included in the expected output.
Verified inclusion of
pg_catalogin the database listings.The integration test correctly includes
pg_catalogin the expected output of the "show databases" command.
tests-integration/src/tests/instance_test.rs
- Line 465: The expected output includes
pg_catalog.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `pg_catalog` is included in the output of the "show databases" command. # Test: Execute the command and check the output. Expect: `pg_catalog` should be listed. rg --type rust 'show databases' -A 5Length of output: 2056
Script:
#!/bin/bash # Description: Search for test assertions that check the output of the "show databases" command in the `instance_test.rs` file. # We will look for patterns that define expected output to include `pg_catalog`. rg --type rust 'expected' tests-integration/src/tests/instance_test.rs -A 5Length of output: 14982
1903-1903: Verify the inclusion ofpg_catalogin the database listings.Ensure that the
pg_catalogschema is correctly included in the output of the "show databases" command in a different context.Verification successful
The
pg_catalogschema is correctly included in the expected output of the "show databases" command.The test case in
tests-integration/src/tests/instance_test.rsconfirms thatpg_catalogis part of the expected output.
- Location in the codebase:
tests-integration/src/tests/instance_test.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `pg_catalog` is included in the output of the "show databases" command in a different context. # Test: Execute the command and check the output. Expect: `pg_catalog` should be listed. rg --type rust 'show databases' -A 5Length of output: 2056
Script:
#!/bin/bash # Description: Verify that `pg_catalog` is included in the expected output of the "show databases" command in the test cases. # Search for the expected output in the test cases. rg 'execute_sql\(&instance, "show databases"\)' -A 5 tests-integration/src/tests/instance_test.rsLength of output: 336
Script:
#!/bin/bash # Description: Verify that `pg_catalog` is included in the expected output of the "show databases" command in the test cases. # Search for the complete expected output in the test cases. rg 'let expected =' -A 10 tests-integration/src/tests/instance_test.rsLength of output: 16591
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4332 +/- ##
==========================================
- Coverage 85.24% 84.96% -0.28%
==========================================
Files 1068 1073 +5
Lines 191277 191406 +129
==========================================
- Hits 163046 162629 -417
- Misses 28231 28777 +546 |
src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs
Outdated
Show resolved
Hide resolved
|
@J0HN50N133 @killme2008 @evenyag Let's try to push forward this PR together. It's the first step for pg_catalog and changing the system table file structure. If we keep it open for a long time, it will conflict with every following changes to information_schema, which introduces unnecessary burden to keep track on. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- src/catalog/src/lib.rs (1 hunks)
- src/catalog/src/system_schema/information_schema.rs (5 hunks)
- src/catalog/src/system_schema/information_schema/information_memory_table.rs (2 hunks)
- src/catalog/src/system_schema/information_schema/views.rs (1 hunks)
- src/catalog/src/system_schema/memory_table.rs (8 hunks)
- src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs (1 hunks)
- src/common/catalog/src/consts.rs (2 hunks)
- tests/cases/standalone/common/show/show_databases_tables.result (2 hunks)
- tests/cases/standalone/common/system/information_schema.result (6 hunks)
- tests/cases/standalone/common/system/information_schema.sql (1 hunks)
- tests/cases/standalone/common/view/create.result (1 hunks)
Files skipped from review due to trivial changes (1)
- src/catalog/src/system_schema/information_schema/views.rs
Files skipped from review as they are similar to previous changes (9)
- src/catalog/src/system_schema/information_schema.rs
- src/catalog/src/system_schema/information_schema/information_memory_table.rs
- src/catalog/src/system_schema/memory_table.rs
- src/catalog/src/system_schema/pg_catalog/pg_catalog_memory_table.rs
- src/common/catalog/src/consts.rs
- tests/cases/standalone/common/show/show_databases_tables.result
- tests/cases/standalone/common/system/information_schema.result
- tests/cases/standalone/common/system/information_schema.sql
- tests/cases/standalone/common/view/create.result
Additional comments not posted (2)
src/catalog/src/lib.rs (2)
34-34: Modulesystem_schemaadded.The
system_schemamodule is correctly defined and integrated.
35-37: Re-exportinformation_schemafor legacy compatibility.The
information_schemamodule is correctly re-exported fromsystem_schema. The TODO comment suggests migrating to the new path later.
|
This looks awesome from my side. It's great as a start of introducing postgres metatables. It also contains some refactoring to make system schema API more friendly. There are some non-blocking issues we can think about next:
|
|
Thanks for your contribution! Please go ahead :D |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#3560
What's changed and what's your intention?
Refactor the structure of
src/catalogadapting to bothpg_catalogandinformation_schema. Add one of the most simplepg_catalogtable:pg_typesupportChecklist
Summary by CodeRabbit
pg_catalogschema support, adding PostgreSQL catalog functionality and enhancing schema management capabilities.pg_catalog, ensuring consistent database visibility and schema reporting.pg_catalogin database listings and schema queries.