Extremely slow while invalidating all login sessions of a certain user

[rickwang7712]

Describe the bug

Hundreds of thousands of records in PostgreSQL could cost more than 10 minutes to invalidate all login sessions of a certain user.

Reproducing the bug

Steps to reproduce the behavior:

1. Run the Hydra up with PostgreSQL
2. Keep stressing login with a client until there are more than 20000 records.
3. Start using api " invalidate all login sessions of a certain user".
4. Wait and you will find it's likely not gonna finish.

Server logs

No error logs in this case.

Server configuration

services:
hydra-migrate:
depends_on:
- postgresd
image: ${HYDRA_VERSION}
environment:
- DSN=postgres://${DB_USER}:${DB_PASSWORD}@${DB_HOST}:5432/${DB_NAME}?sslmode=disable
command:
migrate sql -e --yes
restart: on-failure
hydra:
depends_on:
- hydra-migrate
image: ${HYDRA_VERSION}
volumes:
- .:/config
ports:
- ${HYDRA_PUBLIC_PORT}:4444 # Public port
- ${HYDRA_ADMIN_PORT}:4445 # Admin port
command:
serve all --dangerous-force-http --config /config/config.yml
environment:
- LOG_LEVEL=debug
- URLS_SELF_ISSUER=${HYDRA_URLS_SELF_ISSUER} # must set to public port
- URLS_CONSENT=${HYDRA_URLS_CONSENT}
- URLS_LOGIN=${HYDRA_URLS_LOGIN}
- URLS_LOGOUT=${HYDRA_URLS_LOGOUT}
- DSN=postgres://${DB_USER}:${DB_PASSWORD}@${DB_HOST}:5432/${DB_NAME}?sslmode=disable
- SECRETS_SYSTEM=${SECRETS_SYSTEM}
restart: unless-stopped
postgresd:
image: postgres:9.6
volumes:
- ${DB_VOLUME}:/var/lib/postgresql/data
ports:
- "5433:5432"
environment:
- POSTGRES_USER=${DB_USER}
- POSTGRES_PASSWORD=${DB_PASSWORD}
- POSTGRES_DB=${DB_NAME}

Expected behavior

The request should finish in less a minute under this kind of amount.

Environment

• Version: Hydra-v1.0.9
• Environment: Docker on Ubuntu-16.04

Additional context

I already figured out that there are 6 indices needed to be add:

table hydra_oauth2_authentication_session
	subject  -- hydra_auth2_authentication_session_sub_idx

table hydra_oauth2_authentication_request
	login_session_id -> hydra_oauth2_authentication_session.id  (ON DELETE CASCADE) -- hydra_auth2_authentication_request_login_session_id_idx
	challenge -> hydra_oauth2_authentication_request.challenge

table hydra_oauth2_authentication_request_handled
	challenge -> hydra_oauth2_authentication_request.challenge (ON DELETE CASCADE) -- hydra_oauth2_authentication_request_handled_challenge_idx

table hydra_oauth2_consent_request
	login_session_id -> hydra_oauth2_authentication_session.id (ON DELETE SET NULL) -- hydra_oauth2_consent_request_login_session_id_idx
	login_challenge -> hydra_oauth2_authentication_request.challenge (ON DELETE SET NULL) -- hydra_oauth2_consent_request_login_challenge_idx

My fix patch is ready to send a pull request: rickwang7712@8b98807
Hope I can make contribution this time.

[aeneasr]

Thank you for the report and the work put into your PR.

I checked the code, and we we only query for subject:

hydra/consent/manager_sql.go

Line 145 in 9dbf8b5

m.DB.Rebind("DELETE FROM hydra_oauth2_authentication_session WHERE subject=?"),

I haven't checked a full SQL dump but by scanning quickly through the code it appears that the subject column does indeed not have a key, which causes the issues you're experiencing. This however requires only a minor modification along the lines of:

// mysql
CREATE INDEX hydra_oauth2_authentication_session_sub_idx ON hydra_oauth2_authentication_session (subject);

// postgres (and cockroach i think)
CREATE INDEX hydra_oauth2_authentication_session_sub_idx ON hydra_oauth2_authentication_session (subject);

Please note that the migration system is filebased. New migrations have to be added in a new file that has a larger number than existing ones. In this case, you would create 13.sql in directories cockroach,mysql, postgres as well as 13_test.sql in the tests directory where you would simply copy the contents of 12_test.sql and replace any occurrence of 12 with 13.

[aeneasr]

Also as a side note, iirc, you don't need extra keys for any things that are foreign keys, or unique keys, as the RDBMS already has indices for faster lookup for those. So changes such as this one do not actually make a difference :)

[rickwang7712]

Hi @aeneasr, Thanks for your reply!

I did test what you said about "only adding index of subject on table". In fact this is the first index I tried to add.
But the result is still awful as it was at first. The delete action is just like infinite.
I think the concept you described of "RDBMS already has indices for faster lookup" did not happen?

This image are the pgadmin explain analyze results(using query like DELETE FROM hydra_oauth2_authentication_session WHERE subject='12850' AND authenticated_at='2019-11-17 21:44:21.457057'), the left-side one is the result of only adding subject's index; the right-side one is the result of adding indices in my PR.
And we can see there is a huge difference of time cost on table hydra_oauth2_authentication_request and hydra_oauth2_consent_request.

The difference of the real delete command(the same user, start from the same database content) is like 5 seconds vs. ? minutes(I didn't really wait until it finish, it's just too long)

Here are some references on other forums talked about this symptom:
https://dba.stackexchange.com/questions/37034/very-slow-delete-in-postgresql-workaround
https://stackoverflow.com/questions/970562/postgres-and-indexes-on-foreign-keys-and-primary-keys

And there is a statement said: "Postgres doesn’t automatically create indexes on the referencing side of foreign key relationships." on this article.
Does it mean the concept of RDBMS you said isn't applied in PostgreSQL?

[aeneasr]

Thank you for the links, I didn't know that cascading deletes on foreign keys or unique fields don't actually have fast lookup indices! In that case, you're absolutely right, of course.

This appears to be database specific. MySQL for example appears to create index keys automatically on foreign keys (but only InnoDB!): https://stackoverflow.com/questions/304317/does-mysql-index-foreign-key-columns-automatically

But then again, this only works when you're doing CREATE TABLE and not ALTER TABLE in versions 5.1 and 5.5. So this behavior isn't even consistent there!

In that case your changes make sense and we'd definitively accept a the patch as a PR! For it to be merged though, all SQL files would have to go to into a new file as I described in my previous comment :)

Thank you!

[rickwang7712]

Hi @aeneasr , Is it OK that I also add indices for other foreign keys? My co-worker said there are other indices should be added which will be useful while revoking consent sessions.
If yes, do I need to open a new issue?
Thanks!

[rickwang7712]

Hi @aeneasr ,
Here are the current status of foreign keys in postgreSQL:

Foreign keys already have indices or have been set as primary key:

• hydra_oauth2_access_client_id_fk (hydra_oauth2_access_client_id_idx)
• hydra_oauth2_authentication_request_client_id_fk (hydra_oauth2_authentication_request_cid_idx)
• hydra_oauth2_code_client_id_fk (hydra_oauth2_code_client_id_idx)
• hydra_oauth2_authentication_request_handled_challenge_fk (hydra_oauth2_authentication_request_handled_pkey)
• hydra_oauth2_consent_request_client_id_fk (hydra_oauth2_consent_request_cid_idx)
• hydra_oauth2_consent_request_handled_challenge_fk (hydra_oauth2_consent_request_handled_pkey)
• hydra_oauth2_obfuscated_authentication_session_client_id_fk (hydra_oauth2_obfuscated_authentication_session_pkey)
• hydra_oauth2_oidc_client_id_fk (hydra_oauth2_oidc_client_id_idx)
• hydra_oauth2_pkce_client_id_fk (hydra_oauth2_pkce_client_id_idx)
• hydra_oauth2_refresh_client_id_fk (hydra_oauth2_refresh_client_id_idx)

==========================================================
Indices need to be added this time for saving performance while revoking login sessions:

• subject
• hydra_oauth2_authentication_request_login_session_id_fk
• hydra_oauth2_consent_request_login_challenge_fk
• hydra_oauth2_consent_request_login_session_id_fk

==========================================================
Other foreign keys which do not have indices:

• hydra_oauth2_access_challenge_id_fk
• hydra_oauth2_code_challenge_id_fk
• hydra_oauth2_logout_request_client_id_fk
• hydra_oauth2_oidc_challenge_id_fk
• hydra_oauth2_pkce_challenge_id_fk
• hydra_oauth2_refresh_challenge_id_fk

I think we also need to create indices for these foreign keys, otherwise the performance will drop again while during the delete query.
If you agree, I will split up my fix as 2 commits for this PR, one is for revoking login sessions, one is for the rest.

[aeneasr]

Yes, that sounds perfect - thank you! :)