fix: improve delete queries for janitor command #2540

flavioleggio · 2021-05-20T15:55:26Z

Related issue

Proposed changes

Improve delete queries by separating the data extraction from actual delete.
Extraction is made with a configurable limit, using --limit flag, then deletes are made in batch mode with a configurable batch size, using --batch-size flag.
I chose default values for limit and batch size of respectively 100000 records and 100 records.

Checklist

I have read the contributing guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further comments

I chose to remove the transaction in cleanup for login and consent requests, which I think could be part of the problem with table locking.

CLAassistant · 2021-05-20T15:55:31Z

All committers have signed the CLA.

Benehiko

I think this is looking good! :)
Great job!

Benehiko · 2021-05-20T16:07:10Z

cmd/janitor.go

@@ -52,6 +52,8 @@ Janitor can be used in several ways.
 		RunE: cli.NewHandler().Janitor.RunE,
 		Args: cli.NewHandler().Janitor.Args,
 	}
+	cmd.Flags().Int(cli.Limit, 10000, "Limits the number of records retrieved from database for deletion.")


So this limit here is 10K instead of 100k as suggested in the comment?

Yes, my mistake. Should I fix the comment or change the default value in the config? What do you think about this?

I think you can adjust the defaults to what has worked for you

Actually, my limit was 1 million records, but it takes an hour for the select only and delete is a long process (not affecting database resources nor service though). I tought that a lower default would be more reasonable for common use cases.

Then we stick with the lower limits :)

Well then, we just have to decide between 10k and 100k.
In my database setup a single 100 batch delete for consent (the slowest table, I guess because of many cascade deletes) takes 1 second or so. A default limit of 10k would take a total of 100 seconds, while a limit of 100k would take 1000 seconds (i.e. almost 17 minutes). We could stick with one of these (I would take the 100k limit, if I have to choose) or get a value in the middle.

@aeneasr what is your opinion on this?

persistence/sql/persister_consent.go

cmd/janitor.go

Improve delete queries by separating the data extraction from actual delete. Extraction is made with a configurable limit, using --limit flag, then deletes are made in batch mode with a configurable batch size, using --batch-size flag. Default value for limit is 100000 records and default value for batch size is 100 records.

This uses LEFT JOIN to select also login and consent requests which did not result in a complete authentication, i.e. user requested login but timed out or user logged in and timed out at consent.

This splits in two independent SELECTs the extraction of login and consent requests eligible for deletion. This solves a bug in the single SELECT causing deletion of consent requests where matching login requests were eligible for deletion and vice versa. With independent SELECTs we keep consent requests even if matching login request gets deleted and vice versa.

This adds a check in janitor command handler to ensure that user is not passing wrong values for limit anch batch flags. This also adds tests for these command line arguments.

oauth2/handler.go

oauth2/fosite_store_helpers.go

aeneasr

Great job @flavioleggio ! I have some ideas here :)

aeneasr · 2021-05-27T16:54:47Z

persistence/sql/persister_consent.go

+		FROM %[1]s
+		LEFT JOIN %[2]s ON %[1]s.challenge = %[2]s.challenge
+		WHERE (
+			(%[2]s.challenge IS NULL)


Can challenge be null here?

Yes, LEFT JOIN selects records matching in both tables and records only present in left table: in that case, column from right tables will we NULL, so that check should mean "give me unhandled requests".

I see, thank you for the explanation. I'll keep this comment open so that I can re-check the SQL query when I'm back in the office to ensure nothing gets deleted that shouldn't be deleted.

Did you have a chance to check the query?

persistence/sql/persister_consent.go

persistence/sql/persister_oauth2.go

This fix avoids unexpected behaviors if the janitor cli is executed in parallel. In that case, if we do not order records to delete, we might incur in duplicate deletes.

Benehiko

I think this is looking quite good! Nice job @flavioleggio :)

I just have one request on the tests, maybe there should be a test to verify setting the limit and the batch size to confirm it actually does work, meaning if we have 5 rows in the database and you limit to 2, then we should have 3 rows still in the database - assuming the rows were all invalid/expired. Since testing the batch size isn't really possible on increments, e.g. on batch size 1 we get X, on batch size 2 we get XY. I think we should just have a test setting it to 0 and another setting it to an arbitrary number to verify 0 does not delete anything and some number above 0 does delete the rows according to the limit size.

aeneasr · 2021-06-28T10:07:19Z

Let me know when I should take a final look :)

aeneasr · 2021-07-14T08:48:51Z

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

flavioleggio · 2021-07-17T13:30:12Z

Looks like there are problems with the dependency check with some transitive dependencies. The command states the following:

pkg:golang/github.com/gobuffalo/packr@1.22.0
1 known vulnerabilities affecting installed version

I tried with a simple go mod tidy and also upgrading transitive dependencies, but that upgraded them all and caused even more errors. Can someone give me a pointer on this field?

UPDATE
I upgraded only the indirect packr dependency, resulting in this line in go.mod
github.com/gobuffalo/packr v1.30.1 // indirect
This made dependency check job pass in tests.

codecov · 2021-07-17T13:57:31Z

Codecov Report

Merging #2540 (12661f2) into master (a8675dd) will increase coverage by 0.26%.
The diff coverage is 91.46%.

@@            Coverage Diff             @@
##           master    #2540      +/-   ##
==========================================
+ Coverage   52.47%   52.73%   +0.26%     
==========================================
  Files         234      234              
  Lines       13905    13995      +90     
==========================================
+ Hits         7296     7380      +84     
- Misses       5986     5989       +3     
- Partials      623      626       +3

Impacted Files	Coverage Δ
oauth2/handler.go	`67.01% <0.00%> (ø)`
persistence/sql/persister_consent.go	`78.40% <82.60%> (-2.06%)`	⬇️
cmd/cli/handler_janitor.go	`77.90% <86.66%> (+2.90%)`	⬆️
persistence/sql/persister_oauth2.go	`82.86% <95.00%> (+2.15%)`	⬆️
cmd/janitor.go	`100.00% <100.00%> (ø)`
internal/testhelpers/janitor_test_helper.go	`100.00% <100.00%> (ø)`
oauth2/fosite_store_helpers.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8675dd...12661f2. Read the comment docs.

Benehiko

nice job! @flavioleggio
The changes make sense to me and from my side I don't have any major suggestions or change requests :)

As for the dependency issues, I'm not too sure. Maybe @aeneasr knows something about it?

oauth2/fosite_store_helpers.go

aeneasr · 2021-07-23T14:51:09Z

Nice, thank you! I will review it next week together with @Benehiko :)

Benehiko

LGTM! :)

aeneasr

Just have to verify one thing, otherwise this looks good! :)

aeneasr · 2021-08-04T15:46:22Z

Congratulations, thank you for the hard work @flavioleggio !!!

aeneasr · 2021-08-17T08:28:03Z

Looks like we got a flaky test :/

https://app.circleci.com/pipelines/github/ory/hydra/2826/workflows/fa46fe2b-54e0-453c-a0f8-96be01aff245/jobs/32749?invite=true#step-128-49

aeneasr · 2022-02-03T17:05:01Z

@flavioleggio would you be open to investigate the test flake? It's unfortunately plaguing almost 50% of runs with a failure

flavioleggio · 2022-02-03T19:16:49Z

I'll take a look this weekend, hopefully. If I can't find an answer on my own I will ask for some help on slack chat, maybe, but I will be in touch.

aeneasr · 2022-02-03T19:38:37Z

Thank you!!

vinckr · 2022-02-09T20:03:15Z

Hello @flavioleggio
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
Don't stress about the test flake, I am sure it will get sorted out.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

Benehiko reviewed May 20, 2021

View reviewed changes

flavioleggio changed the title ~~perf(janitor): improve delete queries~~ perf: improve delete queries May 20, 2021

flavioleggio changed the title ~~perf: improve delete queries~~ perf: improve delete queries for janitor command May 20, 2021

flavioleggio added 3 commits May 20, 2021 22:28

fix(janitor): include unhandled requests in login and consent cleanup

8b6cb9e

This uses LEFT JOIN to select also login and consent requests which did not result in a complete authentication, i.e. user requested login but timed out or user logged in and timed out at consent.

flavioleggio force-pushed the janitor-slow-queries branch from 58a8177 to 1c2f4fe Compare May 20, 2021 20:32

flavioleggio added 2 commits May 20, 2021 23:12

refactor(janitor): remove unuseful logs

d5bd252

Merge remote-tracking branch 'upstream/master' into janitor-slow-queries

1b27346

flavioleggio force-pushed the janitor-slow-queries branch from 1c2f4fe to 1b27346 Compare May 20, 2021 21:13

flavioleggio changed the title ~~perf: improve delete queries for janitor command~~ fix: improve delete queries for janitor command May 21, 2021

test: ensure that user is passing limit and batch size greater than 0

d55622b

This adds a check in janitor command handler to ensure that user is not passing wrong values for limit anch batch flags. This also adds tests for these command line arguments.

flavioleggio commented May 21, 2021

View reviewed changes

oauth2/handler.go Show resolved Hide resolved

flavioleggio commented May 21, 2021

View reviewed changes

oauth2/fosite_store_helpers.go Show resolved Hide resolved

flavioleggio force-pushed the janitor-slow-queries branch 2 times, most recently from 2ab5910 to 86bda51 Compare May 21, 2021 22:22

docs(janitor): add info on limit and batch deletes

b5b0791

flavioleggio force-pushed the janitor-slow-queries branch from 86bda51 to b5b0791 Compare May 21, 2021 22:23

aeneasr reviewed May 27, 2021

View reviewed changes

aeneasr mentioned this pull request Jun 4, 2021

Need way to housekeep records in DB #1574

Closed

8 tasks

flavioleggio added 5 commits June 25, 2021 11:06

Merge branch 'master' into janitor-slow-queries

e526182

fix: --batch-size must be smaller than --limit

254a271

fix(janitor): ensure that records to delete are selected in order

744b1c5

This fix avoids unexpected behaviors if the janitor cli is executed in parallel. In that case, if we do not order records to delete, we might incur in duplicate deletes.

refactor: generate a query multiple times with a single format string

e77b610

Merge branch 'master' into janitor-slow-queries

8157887

flavioleggio marked this pull request as ready for review June 25, 2021 13:28

fix: run format on versions.json

84a7794

Benehiko reviewed Jun 28, 2021

View reviewed changes

aeneasr marked this pull request as draft July 14, 2021 08:48

flavioleggio added 2 commits July 16, 2021 18:16

fix: use only pop RawQuery and make batchSize=1 work

0ff8afe

Merge branch 'master' into janitor-slow-queries

9103f42

flavioleggio force-pushed the janitor-slow-queries branch 2 times, most recently from c7f79c5 to 9103f42 Compare July 17, 2021 13:20

fix: upgrade github.com/gobuffalo/packr

b54d3e4

flavioleggio marked this pull request as ready for review July 17, 2021 13:58

Benehiko reviewed Jul 18, 2021

View reviewed changes

oauth2/fosite_store_helpers.go Outdated Show resolved Hide resolved

refactor: loop over requests in flush tokens validation

12661f2

Benehiko approved these changes Aug 3, 2021

View reviewed changes

aeneasr reviewed Aug 4, 2021

View reviewed changes

aeneasr merged commit 6ea0bf8 into ory:master Aug 4, 2021

flavioleggio deleted the janitor-slow-queries branch August 6, 2021 13:43

flavioleggio mentioned this pull request Feb 5, 2022

fix(janitor): flaky test for flush-consent-request-limit case #2967

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve delete queries for janitor command #2540

fix: improve delete queries for janitor command #2540

flavioleggio commented May 20, 2021 •

edited

CLAassistant commented May 20, 2021 •

edited

Benehiko left a comment

Benehiko May 20, 2021

flavioleggio May 20, 2021

Benehiko May 21, 2021

flavioleggio May 21, 2021

Benehiko May 21, 2021

flavioleggio May 21, 2021

flavioleggio May 21, 2021 •

edited

aeneasr left a comment

aeneasr May 27, 2021

flavioleggio May 27, 2021 •

edited

aeneasr May 28, 2021

flavioleggio Jun 25, 2021

Benehiko left a comment

aeneasr commented Jun 28, 2021

aeneasr commented Jul 14, 2021

flavioleggio commented Jul 17, 2021 •

edited

codecov bot commented Jul 17, 2021 •

edited

Benehiko left a comment

aeneasr commented Jul 23, 2021

Benehiko left a comment

aeneasr left a comment

aeneasr commented Aug 4, 2021

aeneasr commented Aug 17, 2021

aeneasr commented Feb 3, 2022

flavioleggio commented Feb 3, 2022

aeneasr commented Feb 3, 2022

vinckr commented Feb 9, 2022

fix: improve delete queries for janitor command #2540

fix: improve delete queries for janitor command #2540

Conversation

flavioleggio commented May 20, 2021 • edited

Related issue

Proposed changes

Checklist

Further comments

CLAassistant commented May 20, 2021 • edited

Benehiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavioleggio May 21, 2021 • edited

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavioleggio May 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko left a comment

Choose a reason for hiding this comment

aeneasr commented Jun 28, 2021

aeneasr commented Jul 14, 2021

flavioleggio commented Jul 17, 2021 • edited

codecov bot commented Jul 17, 2021 • edited

Codecov Report

Benehiko left a comment

Choose a reason for hiding this comment

aeneasr commented Jul 23, 2021

Benehiko left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented Aug 4, 2021

aeneasr commented Aug 17, 2021

aeneasr commented Feb 3, 2022

flavioleggio commented Feb 3, 2022

aeneasr commented Feb 3, 2022

vinckr commented Feb 9, 2022

flavioleggio commented May 20, 2021 •

edited

CLAassistant commented May 20, 2021 •

edited

flavioleggio May 21, 2021 •

edited

flavioleggio May 27, 2021 •

edited

flavioleggio commented Jul 17, 2021 •

edited

codecov bot commented Jul 17, 2021 •

edited