-
Notifications
You must be signed in to change notification settings - Fork 287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Indexes and trigger changes to improve performance #5111
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgaffney
reviewed
Sep 16, 2024
internal/db/schema/migrations/oss/postgres/91/03_indexes_grants_query.up.sql
Outdated
Show resolved
Hide resolved
mgaffney
approved these changes
Sep 16, 2024
mgaffney
approved these changes
Sep 16, 2024
tmessi
force-pushed
the
tmessi-indexes
branch
from
September 16, 2024 18:12
1283e53
to
1abfa09
Compare
jefferai
reviewed
Sep 16, 2024
internal/db/schema/migrations/oss/postgres/91/02_indexes_fk_delete.up.sql
Show resolved
Hide resolved
jefferai
reviewed
Sep 16, 2024
internal/db/schema/migrations/oss/postgres/91/03_indexes_grants_query.up.sql
Show resolved
Hide resolved
jefferai
reviewed
Sep 16, 2024
internal/db/schema/migrations/oss/postgres/91/05_deletion_tables_view.up.sql
Show resolved
Hide resolved
jefferai
reviewed
Sep 16, 2024
internal/db/schema/migrations/oss/postgres/91/05_deletion_tables_view.up.sql
Show resolved
Hide resolved
tmessi
force-pushed
the
tmessi-indexes
branch
from
September 17, 2024 13:29
4964762
to
ed4e4f2
Compare
This adds indexes for a few categories: 1. Foreign keys on the session table. These are set to `null` when the referenced row is deleted. These indexes will help to more efficiently set these `null` values in the case where a target is deleted, or and auth token is deleted. 2. Foreign keys from other tables to the session table. These are either set to `null` or cascade deleted when a session is deleted. These indexes help with these update/deletes when a session is deleted. 3. A multi-column index on session_state. This helps with the query that is used to delete all terminated sessions that have been terminated for over an hour.
When processing a controller API request, system uses a query to fetch the grants for the requesting user. This query requires pulling together information from several tables, and is currently performing many sequential scans to do so. For a number of the tables involved, there are indexes from multi-column primary keys, however, due to the order of the columns in the index, the postgres planner would need to do a full scan of the index, which can be less efficient than a sequential scan, so instead it will perform a sequential scan of the table. This recreates the primary keys while swapping the order of the columns in the primary key definition, and thus the order in the index. By doing so, the planner will not need to perform a full index scan, and will be more likely to use the index when executing the grants query.
When sessions are deleted, a trigger is used to insert records into the session_deleted table. This table is utilized by the sessions list endpoint when using a refresh token to inform a client of the sessions that have been deleted since the last request. We delete sessions in bulk via a controller job to delete sessions that have been terminated over an hour ago, which results in the trigger running a large number of separate insert statements while processing the delete statement. This changes the trigger to run once for the delete statement, instead of for each row, resulting in a single bulk insert statement to the session_deleted table. This new trigger function also avoids the use of `on conflict`. When testing this function, while the single statement was still faster than running multiple inserts, the `on conflict` still added significant overhead, even when there were no conflicts. It should be safe to perform the insert without the `on conflict`, since the same ID should never be deleted more than once if it is successfully deleted.
This function was returning the set of deletion tables. A view seems better suited for this task since it would allow for applying additional filters of the result set. This was particularly necessary to easily make changes to some sqltests due to switching the delete trigger for the session table.
tmessi
force-pushed
the
tmessi-indexes
branch
from
September 17, 2024 14:32
ed4e4f2
to
139e5b9
Compare
This comment has been minimized.
This comment has been minimized.
Database schema diff between To understand how these diffs are generated and some limitations see the Functionsdiff --git a/.schema-diff/funcs_9a3225055225d47d2de2acef5b04338e1acd22c7/bulk_insert_deleted_ids.sql b/.schema-diff/funcs_9a3225055225d47d2de2acef5b04338e1acd22c7/bulk_insert_deleted_ids.sql
new file mode 100644
index 000000000..f3ab51448
--- /dev/null
+++ b/.schema-diff/funcs_9a3225055225d47d2de2acef5b04338e1acd22c7/bulk_insert_deleted_ids.sql
@@ -0,0 +1,46 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+--
+-- name: bulk_insert_deleted_ids(); type: function; schema: public; owner: -
+--
+
+create function public.bulk_insert_deleted_ids() returns trigger
+ language plpgsql
+ as $$
+ begin
+ execute format('insert into %i (public_id, delete_time)
+ select o.public_id, now()
+ from old_table o;',
+ tg_argv[0]);
+ return null;
+ end;
+ $$;
+
+
+--
+-- name: function bulk_insert_deleted_ids(); type: comment; schema: public; owner: -
+--
+
+comment on function public.bulk_insert_deleted_ids() is 'bulk_insert_deleted_ids is a function that inserts records into the table specified by the first trigger argument. it takes the public ids from the set of rows that where deleted and the current timestamp.';
+
+
+--
+-- postgresql database dump complete
+--
+
diff --git a/.schema-diff/funcs_75563599fcb331b67b20040ca78b3fa58d83941f/get_deletion_tables.sql b/.schema-diff/funcs_75563599fcb331b67b20040ca78b3fa58d83941f/get_deletion_tables.sql
deleted file mode 100644
index 1dd0bd0fc..000000000
--- a/.schema-diff/funcs_75563599fcb331b67b20040ca78b3fa58d83941f/get_deletion_tables.sql
+++ /dev/null
@@ -1,44 +0,0 @@
---
--- postgresql database dump
---
-
--- dumped from database version 13.16
--- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
-
-set statement_timeout = 0;
-set lock_timeout = 0;
-set idle_in_transaction_session_timeout = 0;
-set client_encoding = 'utf8';
-set standard_conforming_strings = on;
-select pg_catalog.set_config('search_path', '', false);
-set check_function_bodies = false;
-set xmloption = content;
-set client_min_messages = warning;
-set row_security = off;
-
---
--- name: get_deletion_tables(); type: function; schema: public; owner: -
---
-
-create function public.get_deletion_tables() returns setof name
- language sql
- as $_$
- select c.relname
- from pg_catalog.pg_class c
- where c.relkind in ('r')
- and c.relname operator(pg_catalog.~) '^(.+_deleted)$' collate pg_catalog.default
- and pg_catalog.pg_table_is_visible(c.oid);
- $_$;
-
-
---
--- name: function get_deletion_tables(); type: comment; schema: public; owner: -
---
-
-comment on function public.get_deletion_tables() is 'get_deletion_tables returns a set containing all the deleted table names by looking for all tables that end in _deleted.';
-
-
---
--- postgresql database dump complete
---
- TablesUnchanged Viewsdiff --git a/.schema-diff/views_9a3225055225d47d2de2acef5b04338e1acd22c7/deletion_table.sql b/.schema-diff/views_9a3225055225d47d2de2acef5b04338e1acd22c7/deletion_table.sql
new file mode 100644
index 000000000..bc5e281c0
--- /dev/null
+++ b/.schema-diff/views_9a3225055225d47d2de2acef5b04338e1acd22c7/deletion_table.sql
@@ -0,0 +1,32 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+--
+-- name: deletion_table; type: view; schema: public; owner: -
+--
+
+create view public.deletion_table as
+ select c.relname as tablename
+ from pg_class c
+ where ((c.relkind = 'r'::"char") and (c.relname ~ ('^(.+_deleted)$'::text collate "default")) and pg_table_is_visible(c.oid));
+
+
+--
+-- postgresql database dump complete
+--
+ Triggersdiff --git a/.schema-diff/triggers_75563599fcb331b67b20040ca78b3fa58d83941f/session insert_deleted_id.sql b/.schema-diff/triggers_9a3225055225d47d2de2acef5b04338e1acd22c7/session bulk_insert_deleted_ids.sql
similarity index 65%
rename from .schema-diff/triggers_75563599fcb331b67b20040ca78b3fa58d83941f/session insert_deleted_id.sql
rename to .schema-diff/triggers_9a3225055225d47d2de2acef5b04338e1acd22c7/session bulk_insert_deleted_ids.sql
index 45a7cb382..e0a8e6584 100644
--- a/.schema-diff/triggers_75563599fcb331b67b20040ca78b3fa58d83941f/session insert_deleted_id.sql
+++ b/.schema-diff/triggers_9a3225055225d47d2de2acef5b04338e1acd22c7/session bulk_insert_deleted_ids.sql
@@ -17,10 +17,10 @@ set client_min_messages = warning;
set row_security = off;
--
--- name: session insert_deleted_id; type: trigger; schema: public; owner: -
+-- name: session bulk_insert_deleted_ids; type: trigger; schema: public; owner: -
--
-create trigger insert_deleted_id after delete on public.session for each row execute function public.insert_deleted_id('session_deleted');
+create trigger bulk_insert_deleted_ids after delete on public.session referencing old table as old_table for each statement execute function public.bulk_insert_deleted_ids('session_deleted');
-- Indexesdiff --git a/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/credential_vault_credential_session_id_ix.sql b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/credential_vault_credential_session_id_ix.sql
new file mode 100644
index 000000000..b6253c680
--- /dev/null
+++ b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/credential_vault_credential_session_id_ix.sql
@@ -0,0 +1,31 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+set default_tablespace = '';
+
+--
+-- name: credential_vault_credential_session_id_ix; type: index; schema: public; owner: -
+--
+
+create index credential_vault_credential_session_id_ix on public.credential_vault_credential using btree (session_id);
+
+
+--
+-- postgresql database dump complete
+--
+
diff --git a/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/recording_connection_session_id_ix.sql b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/recording_connection_session_id_ix.sql
new file mode 100644
index 000000000..681a2ad7c
--- /dev/null
+++ b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/recording_connection_session_id_ix.sql
@@ -0,0 +1,31 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+set default_tablespace = '';
+
+--
+-- name: recording_connection_session_id_ix; type: index; schema: public; owner: -
+--
+
+create index recording_connection_session_id_ix on public.recording_connection using btree (session_id);
+
+
+--
+-- postgresql database dump complete
+--
+
diff --git a/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_auth_token_id_ix.sql b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_auth_token_id_ix.sql
new file mode 100644
index 000000000..20d6b6cb4
--- /dev/null
+++ b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_auth_token_id_ix.sql
@@ -0,0 +1,31 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+set default_tablespace = '';
+
+--
+-- name: session_auth_token_id_ix; type: index; schema: public; owner: -
+--
+
+create index session_auth_token_id_ix on public.session using btree (auth_token_id);
+
+
+--
+-- postgresql database dump complete
+--
+
diff --git a/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_state_state_terminated_start_time_ix.sql b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_state_state_terminated_start_time_ix.sql
new file mode 100644
index 000000000..bc476fe34
--- /dev/null
+++ b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_state_state_terminated_start_time_ix.sql
@@ -0,0 +1,31 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+set default_tablespace = '';
+
+--
+-- name: session_state_state_terminated_start_time_ix; type: index; schema: public; owner: -
+--
+
+create index session_state_state_terminated_start_time_ix on public.session_state using btree (state, start_time) where (state = 'terminated'::text);
+
+
+--
+-- postgresql database dump complete
+--
+
diff --git a/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_target_id_ix.sql b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_target_id_ix.sql
new file mode 100644
index 000000000..43d402d96
--- /dev/null
+++ b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_target_id_ix.sql
@@ -0,0 +1,31 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+set default_tablespace = '';
+
+--
+-- name: session_target_id_ix; type: index; schema: public; owner: -
+--
+
+create index session_target_id_ix on public.session using btree (target_id);
+
+
+--
+-- postgresql database dump complete
+--
+
diff --git a/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_worker_protocol_session_id_ix.sql b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_worker_protocol_session_id_ix.sql
new file mode 100644
index 000000000..5b6540c88
--- /dev/null
+++ b/.schema-diff/indexes_9a3225055225d47d2de2acef5b04338e1acd22c7/session_worker_protocol_session_id_ix.sql
@@ -0,0 +1,31 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.16
+-- dumped by pg_dump version 14.13 (ubuntu 14.13-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+set default_tablespace = '';
+
+--
+-- name: session_worker_protocol_session_id_ix; type: index; schema: public; owner: -
+--
+
+create index session_worker_protocol_session_id_ix on public.session_worker_protocol using btree (session_id);
+
+
+--
+-- postgresql database dump complete
+--
+ Constraintsdiff --git a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/auth_oidc_managed_group_member_account_pkey.sql b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/auth_oidc_managed_group_member_account_pkey.sql
index cd06fda0f..3b2496f55 100644
--- a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/auth_oidc_managed_group_member_account_pkey.sql
+++ b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/auth_oidc_managed_group_member_account_pkey.sql
@@ -1,2 +1,2 @@
-- name: auth_oidc_managed_group_member_account auth_oidc_managed_group_member_account_pkey; type: constraint; schema: public; owner: -
- add constraint auth_oidc_managed_group_member_account_pkey primary key (managed_group_id, member_id);
+ add constraint auth_oidc_managed_group_member_account_pkey primary key (member_id, managed_group_id);
diff --git a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_group_member_user_pkey.sql b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_group_member_user_pkey.sql
index 427d16687..06e2a9e9f 100644
--- a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_group_member_user_pkey.sql
+++ b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_group_member_user_pkey.sql
@@ -1,2 +1,2 @@
-- name: iam_group_member_user iam_group_member_user_pkey; type: constraint; schema: public; owner: -
- add constraint iam_group_member_user_pkey primary key (group_id, member_id);
+ add constraint iam_group_member_user_pkey primary key (member_id, group_id);
diff --git a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_group_role_pkey.sql b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_group_role_pkey.sql
index bd1a1c7e6..d0698a994 100644
--- a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_group_role_pkey.sql
+++ b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_group_role_pkey.sql
@@ -1,2 +1,2 @@
-- name: iam_group_role iam_group_role_pkey; type: constraint; schema: public; owner: -
- add constraint iam_group_role_pkey primary key (role_id, principal_id);
+ add constraint iam_group_role_pkey primary key (principal_id, role_id);
diff --git a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_managed_group_role_pkey.sql b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_managed_group_role_pkey.sql
index 62a432318..c08ce772b 100644
--- a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_managed_group_role_pkey.sql
+++ b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_managed_group_role_pkey.sql
@@ -1,2 +1,2 @@
-- name: iam_managed_group_role iam_managed_group_role_pkey; type: constraint; schema: public; owner: -
- add constraint iam_managed_group_role_pkey primary key (role_id, principal_id);
+ add constraint iam_managed_group_role_pkey primary key (principal_id, role_id);
diff --git a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_user_role_pkey.sql b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_user_role_pkey.sql
index a880ca387..9fd817cf7 100644
--- a/.schema-diff/constraints_75563599fcb331b67b20040ca78b3fa58d83941f/iam_user_role_pkey.sql
+++ b/.schema-diff/constraints_9a3225055225d47d2de2acef5b04338e1acd22c7/iam_user_role_pkey.sql
@@ -1,2 +1,2 @@
-- name: iam_user_role iam_user_role_pkey; type: constraint; schema: public; owner: -
- add constraint iam_user_role_pkey primary key (role_id, principal_id);
+ add constraint iam_user_role_pkey primary key (principal_id, role_id); Foreign Key ConstraintsUnchanged |
jefferai
approved these changes
Sep 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds several performance related changes in the form of indexes,
and changes to a delete trigger to run more efficiently. These
improvements should help with several operations, notably:
requests.
See the individual commits for additional details.