Remove expired data from the database, ISSUE-952 #2406

abador · 2022-04-19T12:12:20Z

Cleaning up the database from old data and flows

Related issue(s)

#952

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code 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

abador · 2022-04-19T12:13:10Z

Hope It's enough :)

zepatrik

Thanks for this PR, I have some ideas on how to further improve it 😉

zepatrik · 2022-04-19T12:21:32Z

persistence/sql/persister_continuity.go

+func (p *Persister) DeleteExpiredContinuitySessions(ctx context.Context, expiresAt time.Time, limit int) error {
+	// #nosec G201
+	err := p.GetConnection(ctx).RawQuery(fmt.Sprintf(
+		"DELETE FROM %s WHERE expires_at <= ? LIMIT ?",


Please see gobuffalo/pop#658 and specifically https://github.com/gobuffalo/pop/pull/658/files#diff-5d204b98ec636a0fe75d32bf558fc3c06ff540eb1010ede6144e788c0c3aa57aR131-R140

this will be fun. Removing limits in our case will be tricky (or we'll just cleanup with our version where we use limits Wikia#56 and then just run this versionlater on ). As long as we run it more frequently this shouldn't be an issue

This will be fun.

Is limit-less deletion for an established system a big performance impact? If so, we should probably add some kind of limit, e.g. using a sub-query, that works on all DBs.

Yeah this is tricky since we didn't have this ability from the start and we can't remove everything all at once, even with using the dates like in the pr. When switching from one system to another we created a lot of sessions/flows etc at the same time. Dropping those records can cause a big replication lag when the delete query hits that moment in time. Using limit helps us mitigate the problem a little and drops them in batches. This way we don't have to run this job as often and there is no need to monitor this as much

persistence/sql/persister.go

cmd/cleanup/sql.go

zepatrik

Nice, this looks pretty good already 👍
I have some small suggestions how to improve it further 😉

zepatrik · 2022-04-25T09:12:04Z

cmd/cliclient/cleanup.go

+			fmt.Println(cmd.UsageString())
+			fmt.Println("")
+			fmt.Println("When using flag -e, environment variable DSN must be set")
+			os.Exit(1)


As tests will always fail if you call os.Exit we instead use cobra.Command.RunE and propagate errors up. As in this case you already printed the error message, you can just return cmdx.FailSilently(cmd). It will silence the standard cobra output and results in an error exit code being set.

I think I did what you had in mind

cmd/cliclient/cleanup.go

persistence/sql/persister.go

zepatrik · 2022-04-25T09:19:30Z

persistence/sql/persister_continuity.go

+	err := p.GetConnection(ctx).RawQuery(fmt.Sprintf(
+		"DELETE FROM %s WHERE expires_at <= ?",
+		new(continuity.Container).TableName(ctx),
+	),
+		expiresAt,
+	).Exec()


Suggested change

err := p.GetConnection(ctx).RawQuery(fmt.Sprintf(

"DELETE FROM %s WHERE expires_at <= ?",

new(continuity.Container).TableName(ctx),

),

expiresAt,

).Exec()

err := p.GetConnection(ctx)

.Where("expires_at <= ?", expiresAt)

.Delete(new(continuity.Container)),

I left the raw query since it's done the same way everywhere else. Shouldn't this be done in a follow-up with all other cases?

The problem with this query is that it will be extremely slow as it is unbound. Check out this PR for batch-based processing: https://github.com/grantzvolsky/hydra/pull/2/files#diff-6034803b09ef5017e3aa7d3082827dadb4503e6ee3a56853d49edd419a75f864

Co-authored-by: Patrik <zepatrik@users.noreply.github.com>

abador · 2022-04-28T06:27:23Z

@zepatrik I left a few comments. If it's ok to leave it this way please resolve the conversations

aeneasr

@abador looks like there are a couple of compile errors

abador · 2022-04-29T08:56:54Z

@abador looks like there are a couple of compile errors

I've seen them on the master branch. I'll merge master and check if they're gone

abador · 2022-04-29T09:10:33Z

Ok it didn't help:/ I see similar errors in this pr: https://github.com/ory/kratos/runs/6133235832?check_suite_focus=true

codecov · 2022-04-29T12:39:24Z

Codecov Report

Merging #2406 (c011208) into master (93bf1e2) will increase coverage by 0.05%.
The diff coverage is 84.28%.

@@            Coverage Diff             @@
##           master    #2406      +/-   ##
==========================================
+ Coverage   76.56%   76.61%   +0.05%     
==========================================
  Files         316      319       +3     
  Lines       17603    17813     +210     
==========================================
+ Hits        13477    13647     +170     
- Misses       3192     3225      +33     
- Partials      934      941       +7

Impacted Files	Coverage Δ
session/persistence.go	`0.00% <ø> (ø)`
cmd/cliclient/cleanup.go	`31.03% <31.03%> (ø)`
persistence/sql/persister.go	`73.28% <64.70%> (-3.01%)`	⬇️
cmd/cleanup/sql.go	`96.00% <96.00%> (ø)`
cmd/cleanup/root.go	`100.00% <100.00%> (ø)`
cmd/root.go	`75.00% <100.00%> (+1.08%)`	⬆️
continuity/test/persistence.go	`100.00% <100.00%> (ø)`
driver/config/config.go	`82.17% <100.00%> (+0.11%)`	⬆️
persistence/sql/persister_continuity.go	`91.42% <100.00%> (+5.06%)`	⬆️
persistence/sql/persister_login.go	`100.00% <100.00%> (ø)`
... and 7 more

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 93bf1e2...c011208. Read the comment docs.

zepatrik

Nice, this looks very good now 👍 I just have some ideas how to improve it further, and a few questions.

cmd/cleanup/sql.go

continuity/test/persistence.go

continuity/persistence.go

zepatrik · 2022-05-17T09:00:10Z

internal/driver.go

+			config.ViperKeyDatabaseCleanupSleepTables:        1 * time.Minute,
+			config.ViperKeyDatabaseCleanupBatchSize:          100,


Those defaults will be loaded from the config schema, so there should be no need to add them here.

driver/config/config.go

cmd/cliclient/cleanup.go

zepatrik · 2022-05-17T09:23:28Z

persistence/sql/persister.go

+	}
+	time.Sleep(wait)
+
+	p.r.Logger().Println("Successfully cleaned up the SQL database!")


At this point we did not fully clean the database but rather purged batchSize from each of the tables. If there are more expired rows, they would not have been deleted. The output should reflect that as we will have to re-run the cleanup multiple times until all data are truly cleaned.

persistence/sql/persister_continuity.go

abador · 2022-05-27T09:04:45Z

@aeneasr and @zepatrik please take a look when you'll have a chance

persistence/sql/persister_login.go

aeneasr · 2022-06-10T13:55:14Z

persistence/sql/persister_continuity.go

@@ -55,7 +55,7 @@ func (p *Persister) DeleteContinuitySession(ctx context.Context, id uuid.UUID) e
 func (p *Persister) DeleteExpiredContinuitySessions(ctx context.Context, expiresAt time.Time, limit int) error {
 	// #nosec G201
 	err := p.GetConnection(ctx).RawQuery(fmt.Sprintf(
-		"DELETE FROM %s WHERE id in (SELECT id FROM (SELECT id FROM %s c WHERE expires_at <= ? and nid = ? ORDER BY expires_at ASC LIMIT %d ) AS s )",
+		"DELETE FROM %s WHERE id IN (SELECT id FROM %s WHERE expires_at <= ? and nid = ? ORDER BY expires_at ASC LIMIT %d )",


We can revert this now :)

aeneasr · 2022-06-20T08:49:40Z

Thank you so much for this great contribution!

frederikhors · 2023-01-05T16:34:43Z

Is there any docs about this tool? @abador, @aeneasr?

Closes ory#952 Co-authored-by: Patrik <zepatrik@users.noreply.github.com>

abador requested review from aeneasr and zepatrik as code owners April 19, 2022 12:12

zepatrik requested changes Apr 19, 2022

View reviewed changes

Remove expired data from the database, ISSUE-952

df0f175

abador force-pushed the issue-952-cleanup-db branch from 0b815c7 to df0f175 Compare April 22, 2022 08:29

abador added 2 commits April 22, 2022 12:59

Use flag instead of config, ISSUE-952

84beaa3

Remove limit in delete statements, ISSUE-952

961ee35

zepatrik requested changes Apr 25, 2022

View reviewed changes

abador and others added 2 commits April 27, 2022 09:56

Update cmd/cliclient/cleanup.go

afcf973

Co-authored-by: Patrik <zepatrik@users.noreply.github.com>

CR fixes, ISSUE-952

5e2a0a6

aeneasr requested a review from zepatrik April 28, 2022 21:16

aeneasr requested changes Apr 28, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into issue-952-cleanup-db

a571e3b

Delete using subselect, ISSUE-952

6448602

abador force-pushed the issue-952-cleanup-db branch from 6558e19 to 6448602 Compare May 17, 2022 08:36

Merge branch 'master' into issue-952-cleanup-db

2321574

zepatrik reviewed May 17, 2022

View reviewed changes

abador force-pushed the issue-952-cleanup-db branch 3 times, most recently from 1909ac8 to d615d80 Compare May 25, 2022 09:38

Delete using subselect, ISSUE-952

0580b63

abador force-pushed the issue-952-cleanup-db branch from db4c862 to 0580b63 Compare May 26, 2022 08:04

Remove defaults, ISSUE-952

c011208

aeneasr self-requested a review June 4, 2022 08:53

aeneasr reviewed Jun 9, 2022

View reviewed changes

persistence/sql/persister_login.go Show resolved Hide resolved

aeneasr reviewed Jun 10, 2022

View reviewed changes

abador force-pushed the issue-952-cleanup-db branch from d910a1e to c011208 Compare June 13, 2022 06:34

abador requested review from aeneasr and zepatrik June 14, 2022 14:14

aeneasr merged commit 29d6376 into ory:master Jun 20, 2022

harnash deleted the issue-952-cleanup-db branch July 8, 2022 13:29

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023

feat(cli): helper for cleaning up stale records (ory#2406)

f9068f3

Closes ory#952 Co-authored-by: Patrik <zepatrik@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove expired data from the database, ISSUE-952 #2406

Remove expired data from the database, ISSUE-952 #2406

abador commented Apr 19, 2022

abador commented Apr 19, 2022

zepatrik left a comment

zepatrik Apr 19, 2022

abador Apr 22, 2022

zepatrik Apr 25, 2022

abador Apr 27, 2022 •

edited

Loading

zepatrik left a comment

zepatrik Apr 25, 2022

abador Apr 27, 2022

zepatrik Apr 25, 2022

abador Apr 26, 2022

aeneasr Apr 28, 2022

abador commented Apr 28, 2022

aeneasr left a comment

abador commented Apr 29, 2022

abador commented Apr 29, 2022 •

edited

Loading

codecov bot commented Apr 29, 2022 •

edited

Loading

zepatrik left a comment

zepatrik May 17, 2022

zepatrik May 17, 2022

abador commented May 27, 2022

aeneasr Jun 10, 2022

abador Jun 13, 2022

aeneasr commented Jun 20, 2022

frederikhors commented Jan 5, 2023

		config.ViperKeyDatabaseCleanupSleepTables: 1 * time.Minute,
		config.ViperKeyDatabaseCleanupBatchSize: 100,

Remove expired data from the database, ISSUE-952 #2406

Remove expired data from the database, ISSUE-952 #2406

Conversation

abador commented Apr 19, 2022

Related issue(s)

Checklist

Further Comments

abador commented Apr 19, 2022

zepatrik 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

abador Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

zepatrik 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

abador commented Apr 28, 2022

aeneasr left a comment

Choose a reason for hiding this comment

abador commented Apr 29, 2022

abador commented Apr 29, 2022 • edited Loading

codecov bot commented Apr 29, 2022 • edited Loading

Codecov Report

zepatrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abador commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Jun 20, 2022

frederikhors commented Jan 5, 2023

abador Apr 27, 2022 •

edited

Loading

abador commented Apr 29, 2022 •

edited

Loading

codecov bot commented Apr 29, 2022 •

edited

Loading