From fe97bc9749293807b4111ca82bb8085949fa455f Mon Sep 17 00:00:00 2001 From: Philip Lykke Carlsen Date: Tue, 30 Jan 2024 23:27:07 +0100 Subject: [PATCH] Only get types and procedures from unqualified schemas As an auxiliary improvement, use the `regclass` etc. types to refer to catalog table entries instead of hardcoding oids. Incidentally this fixes fetching of table and column comments on Cockroach. --- .../src/configuration/version2.rs | 17 ++- .../src/configuration/version2.sql | 139 +++++++++++------- ...schema_tests__schema_test__get_schema.snap | 19 +++ ...ation_tests__get_configuration_schema.snap | 8 +- ...tests__get_rawconfiguration_v2_schema.snap | 8 +- ...v2_initial_configuration_is_unchanged.snap | 4 +- ..._openapi__up_to_date_generated_schema.snap | 8 +- .../v2-chinook-ndc-metadata-template.json | 2 +- static/citus/v2-chinook-ndc-metadata.json | 2 +- static/cockroach/v2-chinook-ndc-metadata.json | 26 ++-- static/postgres/v2-chinook-ndc-metadata.json | 2 +- static/yugabyte/v2-chinook-ndc-metadata.json | 2 +- 12 files changed, 160 insertions(+), 77 deletions(-) diff --git a/crates/connectors/ndc-postgres/src/configuration/version2.rs b/crates/connectors/ndc-postgres/src/configuration/version2.rs index b06663857..4f9b02a2f 100644 --- a/crates/connectors/ndc-postgres/src/configuration/version2.rs +++ b/crates/connectors/ndc-postgres/src/configuration/version2.rs @@ -14,8 +14,8 @@ use crate::configuration::version1; use crate::configuration::IsolationLevel; pub use version1::{ - default_comparison_operator_mapping, default_excluded_schemas, default_unqualified_schemas, - ConnectionUri, PoolSettings, ResolvedSecret, + default_comparison_operator_mapping, default_excluded_schemas, ConnectionUri, PoolSettings, + ResolvedSecret, }; const CONFIGURATION_QUERY: &str = include_str!("version2.sql"); @@ -50,6 +50,17 @@ impl RawConfiguration { } } +pub fn default_unqualified_schemas() -> Vec { + vec![ + // For the sake of having the user's tables to appear unqualified by default. + "public".to_string(), + // For the sake of having types and procedures appear unqualified by default. + "pg_catalog".to_string(), + "tiger".to_string(), + // "topology".to_string(), + ] +} + /// Options which only influence how the configuration server updates the configuration #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(rename_all = "camelCase")] @@ -82,7 +93,7 @@ impl Default for ConfigureOptions { fn default() -> ConfigureOptions { ConfigureOptions { excluded_schemas: version1::default_excluded_schemas(), - unqualified_schemas: version1::default_unqualified_schemas(), + unqualified_schemas: default_unqualified_schemas(), comparison_operator_mapping: version1::default_comparison_operator_mapping(), // we'll change this to `Some(MutationsVersions::V1)` when we // want to "release" this behaviour diff --git a/crates/connectors/ndc-postgres/src/configuration/version2.sql b/crates/connectors/ndc-postgres/src/configuration/version2.sql index 567154644..d31181ed5 100644 --- a/crates/connectors/ndc-postgres/src/configuration/version2.sql +++ b/crates/connectors/ndc-postgres/src/configuration/version2.sql @@ -4,13 +4,6 @@ -- The data model of these tables is quite involved and carries with it decades -- of legacy. Supporting notes on this are kept in 'introspection-notes.md'. -- --- TODO: This uses unqualified table (and view) and constraint names. --- We will need to qualify them at some point. This makes the aliases seem --- redundant, but they will change in the future. --- If similar named tables exist in different schemas it is arbitrary --- which one we pick currently! (c.f. Citus schemas 'columnar' and --- 'columnar_internal' which both have a 'chunk' table) - -- When debugging in 'psql', uncomment the lines below to be able to run the -- query with arguments set. @@ -50,6 +43,26 @@ WITH NOT (ns.nspname = ANY ($1)) ), + -- These are the schemas of which types and procedures will be + -- exported unqualified. + unqualified_schemas AS + ( + SELECT DISTINCT + schema_name, + ns.oid as schema_id + FROM + -- Unless 'pg_catalog' (added as a default during version-2) is + -- considered a schema to use unqualified we won't get access to any + -- built-in types or procedures. + -- Therefore it has been put here to preserve behavior. It should be + -- removed once we cut 'version3.sql'. + UNNEST($2 || '{"pg_catalog"}'::text[]) AS t(schema_name) + INNER JOIN + pg_namespace + AS ns + ON (ns.nspname = schema_name) + ), + -- Tables and views etc. are recorded in `pg_class`, see -- https://www.postgresql.org/docs/current/catalog-pg-class.html for its -- schema. @@ -129,8 +142,9 @@ WITH -- (remember that, since 'pg_class' records all tables (and other relations) -- that exist in the database, it also has a record of itself). -- - -- We assume 'classoid' to be stable and will just use literal values rather - -- than actually looking them up in pg_class. + -- Rather than using literal numerical oids in this query we use the special + -- built-in datatype 'regclass' which resolves names to oids automatically. + -- See https://www.postgresql.org/docs/current/datatype-oid.html column_comments AS ( SELECT @@ -146,7 +160,7 @@ WITH FROM pg_description WHERE - classoid = 1259 + classoid = 'pg_catalog.pg_class'::regclass ) AS comm INNER JOIN columns @@ -161,7 +175,7 @@ WITH FROM pg_description WHERE - classoid = 1259 + classoid = 'pg_catalog.pg_class'::regclass AND objsubid = 0 ), @@ -178,6 +192,11 @@ WITH -- typedelim FROM pg_catalog.pg_type AS t + INNER JOIN + -- Until the schema is made part of our model of types we only consider + -- those defined in the public schema. + unqualified_schemas + ON (t.typnamespace = unqualified_schemas.schema_id) WHERE -- We currently filter out pseudo (polymorphic) types, because our schema -- can only deal with monomorphic types. @@ -198,41 +217,42 @@ WITH -- 'r' for range -- 'm' for multi-range ) - AND NOT ( - -- Exclude arrays (see 'array_types' below). - t.typelem != 0 -- whether you can subscript into the type - AND typcategory = 'A' -- The parsers considers this type an array for - -- the purpose of selecting preferred implicit casts. + AND NOT + ( + -- Exclude arrays (see 'array_types' below). + t.typelem != 0 -- whether you can subscript into the type + AND typcategory = 'A' -- The parsers considers this type an array for + -- the purpose of selecting preferred implicit casts. + ) + -- Ignore types that are (primarily) for internal postgres use. + -- This is a good candidate for a configuration option. + AND NOT typname IN + ( + 'aclitem', + 'cid', + 'gidx', + 'name', + 'oid', + 'pg_dependencies', + 'pg_lsn', + 'pg_mcv_list', + 'pg_ndistinct', + 'pg_node_tree', + 'regclass', + 'regcollation', + 'regconfig', + 'regdictionary', + 'regnamespace', + 'regoper', + 'regoperator', + 'regproc', + 'regprocedure', + 'regrole', + 'regtype', + 'tid', + 'xid', + 'xid8' ) - -- Ignore types that are (primarily) for internal postgres use. - -- This is a good candidate for a configuration option. - AND NOT typname IN - ( - 'aclitem', - 'cid', - 'gidx', - 'name', - 'oid', - 'pg_dependencies', - 'pg_lsn', - 'pg_mcv_list', - 'pg_ndistinct', - 'pg_node_tree', - 'regclass', - 'regcollation', - 'regconfig', - 'regdictionary', - 'regnamespace', - 'regoper', - 'regoperator', - 'regproc', - 'regprocedure', - 'regrole', - 'regtype', - 'tid', - 'xid', - 'xid8' - ) ), array_types AS ( @@ -248,6 +268,11 @@ WITH scalar_types AS et ON (et.type_id = t.typelem) + INNER JOIN + -- Until the schema is made part of our model of types we only consider + -- types defined in the public schema. + unqualified_schemas + USING (schema_id) WHERE -- See 'scalar_types' above t.typtype NOT IN @@ -346,8 +371,13 @@ WITH INNER JOIN scalar_types AS ret_type ON (ret_type.type_id = proc.prorettype) + INNER JOIN + -- Until the schema is made part of our model of types we only consider + -- types defined in the public schema. + unqualified_schemas + on (unqualified_schemas.schema_id = proc.pronamespace) WHERE - ret_type.type_name = 'bool' + ret_type.type_id = 'pg_catalog.bool'::regtype -- We check that we only consider procedures which take two regular -- arguments. AND cardinality(proc.proargtypes) = 2 @@ -412,8 +442,13 @@ WITH scalar_types AS t_res ON (op.oprresult = t_res.type_id) + INNER JOIN + -- Until the schema is made part of our model of operators we only consider + -- those defined in the public schema. + unqualified_schemas + ON (unqualified_schemas.schema_id = op.oprnamespace) WHERE - t_res.type_name = 'bool' + t_res.type_id = 'pg_catalog.bool'::regtype ORDER BY op.oprname ), @@ -779,7 +814,7 @@ FROM SELECT jsonb_object_agg( CASE - WHEN s.schema_name = ANY ($2) + WHEN unqualified_schemas.schema_id IS NOT NULL THEN rel.relation_name ELSE s.schema_name || '_' || rel.relation_name END, @@ -803,6 +838,10 @@ FROM relations AS rel + LEFT OUTER JOIN + unqualified_schemas + USING (schema_id) + LEFT OUTER JOIN table_comments AS comm @@ -810,7 +849,7 @@ FROM INNER JOIN schemas AS s - USING (schema_id) + ON (rel.schema_id = s.schema_id) -- Columns INNER JOIN @@ -1095,7 +1134,7 @@ FROM -- -- EXECUTE configuration( -- '{"information_schema", "tiger", "pg_catalog", "topology"}'::varchar[], --- '{}'::varchar[], +-- '{"public", "pg_catalog", "tiger"}'::varchar[], -- '[ -- {"operatorName": "=", "exposedName": "_eq"}, -- {"operatorName": "!=", "exposedName": "_neq"}, diff --git a/crates/tests/databases-tests/src/cockroach/snapshots/databases_tests__cockroach__schema_tests__schema_test__get_schema.snap b/crates/tests/databases-tests/src/cockroach/snapshots/databases_tests__cockroach__schema_tests__schema_test__get_schema.snap index b19982f72..68f54d1d8 100644 --- a/crates/tests/databases-tests/src/cockroach/snapshots/databases_tests__cockroach__schema_tests__schema_test__get_schema.snap +++ b/crates/tests/databases-tests/src/cockroach/snapshots/databases_tests__cockroach__schema_tests__schema_test__get_schema.snap @@ -1130,20 +1130,24 @@ expression: result }, "object_types": { "Album": { + "description": "The record of all albums", "fields": { "AlbumId": { + "description": "The identifier of an album", "type": { "type": "named", "name": "int8" } }, "ArtistId": { + "description": "The id of the artist that authored the album", "type": { "type": "named", "name": "int8" } }, "Title": { + "description": "The title of an album", "type": { "type": "named", "name": "varchar" @@ -1152,14 +1156,17 @@ expression: result } }, "Artist": { + "description": "The record of all artists", "fields": { "ArtistId": { + "description": "The identifier of an artist", "type": { "type": "named", "name": "int8" } }, "Name": { + "description": "The name of an artist", "type": { "type": "nullable", "underlying_type": { @@ -1171,6 +1178,7 @@ expression: result } }, "Customer": { + "description": "The record of all customers", "fields": { "Address": { "type": { @@ -1209,6 +1217,7 @@ expression: result } }, "CustomerId": { + "description": "The identifier of customer", "type": { "type": "named", "name": "int8" @@ -1230,12 +1239,14 @@ expression: result } }, "FirstName": { + "description": "The first name of a customer", "type": { "type": "named", "name": "varchar" } }, "LastName": { + "description": "The last name of a customer", "type": { "type": "named", "name": "varchar" @@ -1905,6 +1916,7 @@ expression: result } }, "pg_extension_spatial_ref_sys": { + "description": "Shows all defined Spatial Reference Identifiers (SRIDs). Matches PostGIS' spatial_ref_sys table.", "fields": { "auth_name": { "type": { @@ -2684,6 +2696,7 @@ expression: result "collections": [ { "name": "Album", + "description": "The record of all albums", "arguments": {}, "type": "Album", "uniqueness_constraints": { @@ -2704,6 +2717,7 @@ expression: result }, { "name": "Artist", + "description": "The record of all artists", "arguments": {}, "type": "Artist", "uniqueness_constraints": { @@ -2717,6 +2731,7 @@ expression: result }, { "name": "Customer", + "description": "The record of all customers", "arguments": {}, "type": "Customer", "uniqueness_constraints": { @@ -2901,6 +2916,7 @@ expression: result }, { "name": "pg_extension_spatial_ref_sys", + "description": "Shows all defined Spatial Reference Identifiers (SRIDs). Matches PostGIS' spatial_ref_sys table.", "arguments": {}, "type": "pg_extension_spatial_ref_sys", "uniqueness_constraints": {}, @@ -3282,6 +3298,7 @@ expression: result "description": "Delete any value on the Album table using the AlbumId key", "arguments": { "AlbumId": { + "description": "The identifier of an album", "type": { "type": "named", "name": "int8" @@ -3298,6 +3315,7 @@ expression: result "description": "Delete any value on the Artist table using the ArtistId key", "arguments": { "ArtistId": { + "description": "The identifier of an artist", "type": { "type": "named", "name": "int8" @@ -3314,6 +3332,7 @@ expression: result "description": "Delete any value on the Customer table using the CustomerId key", "arguments": { "CustomerId": { + "description": "The identifier of customer", "type": { "type": "named", "name": "int8" diff --git a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_configuration_schema.snap b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_configuration_schema.snap index 6813242d0..5bc9fdaf6 100644 --- a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_configuration_schema.snap +++ b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_configuration_schema.snap @@ -213,7 +213,9 @@ expression: schema "columnar_internal" ], "unqualifiedSchemas": [ - "public" + "public", + "pg_catalog", + "tiger" ], "comparisonOperatorMapping": [ { @@ -1325,7 +1327,9 @@ expression: schema "unqualifiedSchemas": { "description": "The names of Tables and Views in these schemas will be returned unqualified. The default setting will set the `public` schema as unqualified.", "default": [ - "public" + "public", + "pg_catalog", + "tiger" ], "type": "array", "items": { diff --git a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_rawconfiguration_v2_schema.snap b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_rawconfiguration_v2_schema.snap index 3925b12ae..722c8117a 100644 --- a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_rawconfiguration_v2_schema.snap +++ b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__get_rawconfiguration_v2_schema.snap @@ -50,7 +50,9 @@ expression: schema "columnar_internal" ], "unqualifiedSchemas": [ - "public" + "public", + "pg_catalog", + "tiger" ], "comparisonOperatorMapping": [ { @@ -819,7 +821,9 @@ expression: schema "unqualifiedSchemas": { "description": "The names of Tables and Views in these schemas will be returned unqualified. The default setting will set the `public` schema as unqualified.", "default": [ - "public" + "public", + "pg_catalog", + "tiger" ], "type": "array", "items": { diff --git a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__postgres_current_only_configure_v2_initial_configuration_is_unchanged.snap b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__postgres_current_only_configure_v2_initial_configuration_is_unchanged.snap index 053f20db7..1d4dab7b3 100644 --- a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__postgres_current_only_configure_v2_initial_configuration_is_unchanged.snap +++ b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__configuration_tests__configuration_tests__postgres_current_only_configure_v2_initial_configuration_is_unchanged.snap @@ -1646,7 +1646,9 @@ expression: default_configuration "columnar_internal" ], "unqualifiedSchemas": [ - "public" + "public", + "pg_catalog", + "tiger" ], "comparisonOperatorMapping": [ { diff --git a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__openapi_tests__openapi__up_to_date_generated_schema.snap b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__openapi_tests__openapi__up_to_date_generated_schema.snap index 1e6fad136..5022b2435 100644 --- a/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__openapi_tests__openapi__up_to_date_generated_schema.snap +++ b/crates/tests/databases-tests/src/postgres/snapshots/databases_tests__postgres__openapi_tests__openapi__up_to_date_generated_schema.snap @@ -201,7 +201,9 @@ expression: generated_schema_json "columnar_internal" ], "unqualifiedSchemas": [ - "public" + "public", + "pg_catalog", + "tiger" ], "comparisonOperatorMapping": [ { @@ -1291,7 +1293,9 @@ expression: generated_schema_json "unqualifiedSchemas": { "description": "The names of Tables and Views in these schemas will be returned unqualified. The default setting will set the `public` schema as unqualified.", "default": [ - "public" + "public", + "pg_catalog", + "tiger" ], "type": "array", "items": { diff --git a/static/aurora/v2-chinook-ndc-metadata-template.json b/static/aurora/v2-chinook-ndc-metadata-template.json index e02020710..c6847fa4a 100644 --- a/static/aurora/v2-chinook-ndc-metadata-template.json +++ b/static/aurora/v2-chinook-ndc-metadata-template.json @@ -2472,7 +2472,7 @@ "columnar", "columnar_internal" ], - "unqualifiedSchemas": ["public"], + "unqualifiedSchemas": ["public", "pg_catalog", "tiger"], "comparisonOperatorMapping": [ { "operatorName": "=", diff --git a/static/citus/v2-chinook-ndc-metadata.json b/static/citus/v2-chinook-ndc-metadata.json index 50fe5c54d..f8689a196 100644 --- a/static/citus/v2-chinook-ndc-metadata.json +++ b/static/citus/v2-chinook-ndc-metadata.json @@ -2456,7 +2456,7 @@ "columnar", "columnar_internal" ], - "unqualifiedSchemas": ["public"], + "unqualifiedSchemas": ["public", "pg_catalog", "tiger"], "comparisonOperatorMapping": [ { "operatorName": "=", diff --git a/static/cockroach/v2-chinook-ndc-metadata.json b/static/cockroach/v2-chinook-ndc-metadata.json index b9129ed6d..7ba54be62 100644 --- a/static/cockroach/v2-chinook-ndc-metadata.json +++ b/static/cockroach/v2-chinook-ndc-metadata.json @@ -18,7 +18,7 @@ "scalarType": "int8" }, "nullable": "nonNullable", - "description": null + "description": "The identifier of an album" }, "ArtistId": { "name": "ArtistId", @@ -26,7 +26,7 @@ "scalarType": "int8" }, "nullable": "nonNullable", - "description": null + "description": "The id of the artist that authored the album" }, "Title": { "name": "Title", @@ -34,7 +34,7 @@ "scalarType": "varchar" }, "nullable": "nonNullable", - "description": null + "description": "The title of an album" } }, "uniquenessConstraints": { @@ -49,7 +49,7 @@ } } }, - "description": null + "description": "The record of all albums" }, "Artist": { "schemaName": "public", @@ -61,7 +61,7 @@ "scalarType": "int8" }, "nullable": "nonNullable", - "description": null + "description": "The identifier of an artist" }, "Name": { "name": "Name", @@ -69,14 +69,14 @@ "scalarType": "varchar" }, "nullable": "nullable", - "description": null + "description": "The name of an artist" } }, "uniquenessConstraints": { "PK_Artist": ["ArtistId"] }, "foreignRelations": {}, - "description": null + "description": "The record of all artists" }, "Customer": { "schemaName": "public", @@ -120,7 +120,7 @@ "scalarType": "int8" }, "nullable": "nonNullable", - "description": null + "description": "The identifier of customer" }, "Email": { "name": "Email", @@ -144,7 +144,7 @@ "scalarType": "varchar" }, "nullable": "nonNullable", - "description": null + "description": "The first name of a customer" }, "LastName": { "name": "LastName", @@ -152,7 +152,7 @@ "scalarType": "varchar" }, "nullable": "nonNullable", - "description": null + "description": "The last name of a customer" }, "Phone": { "name": "Phone", @@ -199,7 +199,7 @@ } } }, - "description": null + "description": "The record of all customers" }, "Employee": { "schemaName": "public", @@ -772,7 +772,7 @@ }, "uniquenessConstraints": {}, "foreignRelations": {}, - "description": null + "description": "Shows all defined Spatial Reference Identifiers (SRIDs). Matches PostGIS' spatial_ref_sys table." } }, "compositeTypes": { @@ -2280,7 +2280,7 @@ "columnar", "columnar_internal" ], - "unqualifiedSchemas": ["public"], + "unqualifiedSchemas": ["public", "pg_catalog", "tiger"], "comparisonOperatorMapping": [ { "operatorName": "=", diff --git a/static/postgres/v2-chinook-ndc-metadata.json b/static/postgres/v2-chinook-ndc-metadata.json index 97e6561b3..7ed796ce2 100644 --- a/static/postgres/v2-chinook-ndc-metadata.json +++ b/static/postgres/v2-chinook-ndc-metadata.json @@ -2773,7 +2773,7 @@ "columnar", "columnar_internal" ], - "unqualifiedSchemas": ["public"], + "unqualifiedSchemas": ["public", "pg_catalog", "tiger"], "comparisonOperatorMapping": [ { "operatorName": "=", diff --git a/static/yugabyte/v2-chinook-ndc-metadata.json b/static/yugabyte/v2-chinook-ndc-metadata.json index 70f39e149..36eae6d92 100644 --- a/static/yugabyte/v2-chinook-ndc-metadata.json +++ b/static/yugabyte/v2-chinook-ndc-metadata.json @@ -2447,7 +2447,7 @@ "columnar", "columnar_internal" ], - "unqualifiedSchemas": ["public"], + "unqualifiedSchemas": ["public", "pg_catalog", "tiger"], "comparisonOperatorMapping": [ { "operatorName": "=",