fix: consent cockroachdb perfomance issue with zigzag join query

[lopezator]

Add an index over subject and client_id in order to avoid the
(sometimes) underperformant zigzag join query.

Closes #1789
Related to #1755 cockroachdb/cockroach#47179

After applying the proposed index, the query plan now avoids the zigzag join query, reducing the query times from 50s to 2s in worst cases in my tests.

	distributed	true
	vectorized	false
sort		
 │	order	-requested_at
 └── render		
      └── lookup-join		
           │	table	hydra_oauth2_consent_request_handled@primary
           │	type	inner
           │	equality	(challenge) = (challenge)
           │	equality cols are key	
           │	parallel	
           │	pred	"(@9 = '{}') AND (@7 = true)"
           └── filter		
                │	filter	skip = false
                └── index-join		
                     │	table	hydra_oauth2_consent_request@primary
                     └── scan		
	table	hydra_oauth2_consent_request@hydra_oauth2_consent_request_client_id_subject_idx
	spans	"/""myclient""/""my@subject.com""-/""my-client""/""my@subject.com""/PrefixEnd"

[CLAassistant]

All committers have signed the CLA.

[aeneasr]

Thank you! The migrations still need to be improved but since @zepatrik is refactoring that at the moment I think it makes sense to include this in his PR / after his merge!

[zepatrik]

Hey @lopezator
The migration PR is finally done, can you adopt this PR to the new migration format? Basically you just have to split the migration in two *.cockroach.up.sql and *.cockroach.down.sql files.
Please also add empty migrations for the other databases, I'm pretty sure the migrator will be confused when the numbers don't match (files without the database name in it will be applied to all databases without specific files)

[lopezator]

@zepatrik What would be exactly the file naming convention? Something related to date? I see that every migration file starts by 2019

[zepatrik]

You can either use gobuffalo's soda cli to generate the files or just copy an existing one and increase the number. It is usually of the format YYYYMMDDHHMMSS_name(.database)?.(up|down).(sql|fizz)

[lopezator]

Ok!

Do you prefer empty migrations for postgres and mysql or are you ok on adding the index for all the databases?

I think that this index might not add as much value as it adds for cockroachdb, but as long as there are queries on that table using both fields in the where clause this could be beneficial for them also.

Let me know what you think.

[zepatrik]

Think you can apply it to all or @aeneasr ?

[aeneasr]

apply for all should be ok!

[lopezator]

apply for all should be ok!

Cool!

PTAL @zepatrik

[zepatrik]

right let me fix that I know what the problem is

[zepatrik]

It's actually not a problem with the test.
@lopezator you have to apply your migration after all others, just put the current date/time there.
Or use soda generate sql consent-request-index.
If the migration for all databases is the same you should be able to omit the database name in the file completely and just have one up and one down.

[zepatrik]

The question remains: why did the mysql test pass??

[lopezator]

The question remains: why did the mysql test pass??

Good question 😅

PTAL @zepatrik

[zepatrik]

LGTM @aeneasr

[aeneasr]

Thank you both for the great work!

[lopezator]

You are welcome @aeneasr !