From d9bb9f2bed41cd737f5f8b43c399ada9d80e3f61 Mon Sep 17 00:00:00 2001 From: Alexis King Date: Mon, 16 Dec 2019 03:07:03 -0600 Subject: [PATCH] Alter the type of hdb_catalog columns that reference SQL identifiers As explained in the note included in the diff, this can lead to dramatically better query plans, and it seems to be especially important for versions of Postgres <12. --- server/src-rsr/initialise.sql | 27 +++++++++++++++++++------- server/src-rsr/migrations/28_to_29.sql | 24 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/server/src-rsr/initialise.sql b/server/src-rsr/initialise.sql index 59c0ef2d28e..940fea21745 100644 --- a/server/src-rsr/initialise.sql +++ b/server/src-rsr/initialise.sql @@ -9,11 +9,24 @@ CREATE TABLE hdb_catalog.hdb_version ( CREATE UNIQUE INDEX hdb_version_one_row ON hdb_catalog.hdb_version((version IS NOT NULL)); +/* Note [Reference system columns using type name] +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +While working on #3394, I (Alexis) discovered that Postgres seems to sometimes generate very bad +query plans when joining against the system catalogs if we store things like table/schema names +using type `text` rather than type `name`, the latter of which is used internally. The two types are +compatible in the sense that Postgres will willingly widen `name` to `type` automatically, but +`name`s are restricted to 64 bytes. + +Using `name` for ordinary user data would be a deep sin, but using it to store references to actual +Postgres identifiers makes a lot of sense, so using `name` in those places is alright. And by doing +so, we make Postgres much more likely to take advantage of certain indexes that can significantly +improve query performance. */ + CREATE TABLE hdb_catalog.hdb_table ( - table_schema TEXT, - table_name TEXT, - configuration JSONB, + table_schema name, -- See Note [Reference system columns using type name] + table_name name, + configuration jsonb, is_system_defined boolean default false, is_enum boolean NOT NULL DEFAULT false, @@ -22,8 +35,8 @@ CREATE TABLE hdb_catalog.hdb_table CREATE TABLE hdb_catalog.hdb_relationship ( - table_schema TEXT, - table_name TEXT, + table_schema name, -- See Note [Reference system columns using type name] + table_name name, rel_name TEXT, rel_type TEXT CHECK (rel_type IN ('object', 'array')), rel_def JSONB NOT NULL, @@ -36,8 +49,8 @@ CREATE TABLE hdb_catalog.hdb_relationship CREATE TABLE hdb_catalog.hdb_permission ( - table_schema TEXT, - table_name TEXT, + table_schema name, -- See Note [Reference system columns using type name] + table_name name, role_name TEXT, perm_type TEXT CHECK(perm_type IN ('insert', 'select', 'update', 'delete')), perm_def JSONB NOT NULL, diff --git a/server/src-rsr/migrations/28_to_29.sql b/server/src-rsr/migrations/28_to_29.sql index 76dcfd57919..3b7d4f44964 100644 --- a/server/src-rsr/migrations/28_to_29.sql +++ b/server/src-rsr/migrations/28_to_29.sql @@ -1,6 +1,30 @@ DROP VIEW hdb_catalog.hdb_table_info_agg; DROP VIEW hdb_catalog.hdb_column; +-- we need to drop this view temporarily so we can change the type of hdb_permission columns +DROP VIEW hdb_catalog.hdb_permission_agg; + +-- See Note [Reference system columns using type name] in initialise.sql +ALTER TABLE hdb_catalog.hdb_table + ALTER COLUMN table_schema TYPE name, + ALTER COLUMN table_name TYPE name; +ALTER TABLE hdb_catalog.hdb_relationship + ALTER COLUMN table_schema TYPE name, + ALTER COLUMN table_name TYPE name; +ALTER TABLE hdb_catalog.hdb_permission + ALTER COLUMN table_schema TYPE name, + ALTER COLUMN table_name TYPE name; + +-- this just recreates the view we dropped above with no actual changes +CREATE VIEW hdb_catalog.hdb_permission_agg AS +SELECT + table_schema, + table_name, + role_name, + json_object_agg(perm_type, perm_def) as permissions +FROM hdb_catalog.hdb_permission +GROUP BY table_schema, table_name, role_name; + CREATE VIEW hdb_catalog.hdb_table_info_agg AS SELECT schema.nspname AS table_schema,