Skip to content
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

Cherry Pick of cache speed up and additional performance improvements #5128

Merged
merged 15 commits into from
Sep 24, 2024

Conversation

tmessi
Copy link
Member

@tmessi tmessi commented Sep 24, 2024

This cherry-picks the changes from #5126 to main.

jefferai and others added 15 commits September 24, 2024 14:18
A query parameter `max_result_set_size` can be specified to set a
per-request limit, with `-1` meaning all values. If there were more
entries than the max size used for the request, an `incomplete` boolean
is set in the response.

(cherry picked from commit 3cdd078)
By default, we will use a persistent cache for the
client cache. This will allow us to keep the cache
across restarts of the server and it will also
reduce the amount of time it takes to start the
server while reducing the amount of memory used.

This change also includes a validation of the cache
schema version. If the schema version is different
from the one expected, the cache will be
reset/recreated.

(cherry picked from commit cf8a51f)
This allows getting a list of scope IDs known by the cache via cached
targets and sessions. It is not refreshed from the controller. The name
contains `implicit` in case we ever want e.g. `scopes`.

(cherry picked from commit c0a318c)
This ensures we don't have competing update processes running. When doing the
inline RefreshForSearch, a specific error will be returned so we can hint in
the API that we're in the middle of a refresh.

(cherry picked from commit 5477609)
Fix ensures that the TestServer returned uses the
correct directory for writing its sqlite files,
which was created via t.TempDir()

(cherry picked from commit 85f9104)
* Update Go API to support pagination
* Implement pagination of targets in cache

(cherry picked from commit cd25fad)
* internal/clientcache: add -force-reset-schema flag

* clientcache: stream list pages directly to DB

---------

Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
(cherry picked from commit ccea8a4)
This removes the Cartesian product in the DB for GrantsForUser in favor
of returning the actual grant scope information and dealing with it in
application code.

(cherry picked from commit 913cd53)
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.

(cherry picked from commit 263a96e)
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.

(cherry picked from commit 5577b3c)
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.

(cherry picked from commit 3569b36)
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.

(cherry picked from commit 32d4163)
(cherry picked from commit 51a2b20)
Copy link

Database schema diff between main and tmessi-cherry-pick-cache-speedup @ 632d048

To understand how these diffs are generated and some limitations see the
documentation of the script.

Functions

diff --git a/.schema-diff/funcs_81ac8c52102bd803666f3ffb9ec050d3adc882b4/bulk_insert_deleted_ids.sql b/.schema-diff/funcs_81ac8c52102bd803666f3ffb9ec050d3adc882b4/bulk_insert_deleted_ids.sql
new file mode 100644
index 000000000..f3ab51448
--- /dev/null
+++ b/.schema-diff/funcs_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_30145b1f9c971586424bc81154e4111840c48f89/get_deletion_tables.sql b/.schema-diff/funcs_30145b1f9c971586424bc81154e4111840c48f89/get_deletion_tables.sql
deleted file mode 100644
index 1dd0bd0fc..000000000
--- a/.schema-diff/funcs_30145b1f9c971586424bc81154e4111840c48f89/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
---
-

Tables

Unchanged

Views

diff --git a/.schema-diff/views_81ac8c52102bd803666f3ffb9ec050d3adc882b4/deletion_table.sql b/.schema-diff/views_81ac8c52102bd803666f3ffb9ec050d3adc882b4/deletion_table.sql
new file mode 100644
index 000000000..bc5e281c0
--- /dev/null
+++ b/.schema-diff/views_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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
+--
+

Triggers

diff --git a/.schema-diff/triggers_30145b1f9c971586424bc81154e4111840c48f89/session insert_deleted_id.sql b/.schema-diff/triggers_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session bulk_insert_deleted_ids.sql
similarity index 65%
rename from .schema-diff/triggers_30145b1f9c971586424bc81154e4111840c48f89/session insert_deleted_id.sql
rename to .schema-diff/triggers_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session bulk_insert_deleted_ids.sql
index 45a7cb382..e0a8e6584 100644
--- a/.schema-diff/triggers_30145b1f9c971586424bc81154e4111840c48f89/session insert_deleted_id.sql	
+++ b/.schema-diff/triggers_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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');
 
 
 --

Indexes

diff --git a/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/credential_vault_credential_session_id_ix.sql b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/credential_vault_credential_session_id_ix.sql
new file mode 100644
index 000000000..b6253c680
--- /dev/null
+++ b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_81ac8c52102bd803666f3ffb9ec050d3adc882b4/recording_connection_session_id_ix.sql b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/recording_connection_session_id_ix.sql
new file mode 100644
index 000000000..681a2ad7c
--- /dev/null
+++ b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_auth_token_id_ix.sql b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_auth_token_id_ix.sql
new file mode 100644
index 000000000..20d6b6cb4
--- /dev/null
+++ b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_state_state_terminated_start_time_ix.sql b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_state_state_terminated_start_time_ix.sql
new file mode 100644
index 000000000..bc476fe34
--- /dev/null
+++ b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_target_id_ix.sql b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_target_id_ix.sql
new file mode 100644
index 000000000..43d402d96
--- /dev/null
+++ b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_worker_protocol_session_id_ix.sql b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/session_worker_protocol_session_id_ix.sql
new file mode 100644
index 000000000..5b6540c88
--- /dev/null
+++ b/.schema-diff/indexes_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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
+--
+

Constraints

diff --git a/.schema-diff/constraints_30145b1f9c971586424bc81154e4111840c48f89/auth_oidc_managed_group_member_account_pkey.sql b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/auth_oidc_managed_group_member_account_pkey.sql
index cd06fda0f..3b2496f55 100644
--- a/.schema-diff/constraints_30145b1f9c971586424bc81154e4111840c48f89/auth_oidc_managed_group_member_account_pkey.sql
+++ b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_30145b1f9c971586424bc81154e4111840c48f89/iam_group_member_user_pkey.sql b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/iam_group_member_user_pkey.sql
index 427d16687..06e2a9e9f 100644
--- a/.schema-diff/constraints_30145b1f9c971586424bc81154e4111840c48f89/iam_group_member_user_pkey.sql
+++ b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_30145b1f9c971586424bc81154e4111840c48f89/iam_group_role_pkey.sql b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/iam_group_role_pkey.sql
index bd1a1c7e6..d0698a994 100644
--- a/.schema-diff/constraints_30145b1f9c971586424bc81154e4111840c48f89/iam_group_role_pkey.sql
+++ b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_30145b1f9c971586424bc81154e4111840c48f89/iam_managed_group_role_pkey.sql b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/iam_managed_group_role_pkey.sql
index 62a432318..c08ce772b 100644
--- a/.schema-diff/constraints_30145b1f9c971586424bc81154e4111840c48f89/iam_managed_group_role_pkey.sql
+++ b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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_30145b1f9c971586424bc81154e4111840c48f89/iam_user_role_pkey.sql b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/iam_user_role_pkey.sql
index a880ca387..9fd817cf7 100644
--- a/.schema-diff/constraints_30145b1f9c971586424bc81154e4111840c48f89/iam_user_role_pkey.sql
+++ b/.schema-diff/constraints_81ac8c52102bd803666f3ffb9ec050d3adc882b4/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 Constraints

Unchanged

@tmessi tmessi merged commit f431d1e into main Sep 24, 2024
66 checks passed
@tmessi tmessi deleted the tmessi-cherry-pick-cache-speedup branch September 24, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants