Skip to content

sql: add cache for jobs to avoid querying system.jobs to find existing jobs#50848

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
RichardJCai:cache_jobs
Jul 2, 2020
Merged

sql: add cache for jobs to avoid querying system.jobs to find existing jobs#50848
craig[bot] merged 1 commit intocockroachdb:masterfrom
RichardJCai:cache_jobs

Conversation

@RichardJCai
Copy link
Copy Markdown
Contributor

Currently, we perform a loop through all existing jobs which involves
querying a system table to check if there is an existing job for the current
table undergoing a schema change. To avoid this query, we can use a cache
to map TableIDs to jobs.

Fixes #50165, #50783

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

GRANT ALL ON * TO TEST
----
160
50
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🏎️

@RichardJCai RichardJCai force-pushed the cache_jobs branch 2 times, most recently from b1b4f39 to 83a6476 Compare July 1, 2020 16:05
@RichardJCai RichardJCai marked this pull request as ready for review July 1, 2020 16:06
@RichardJCai RichardJCai changed the title wip: sql: add cache for jobs to avoid querying system table to find existing jobs sql: add cache for jobs to avoid querying system.jobs to find existing jobs Jul 1, 2020
@RichardJCai RichardJCai requested review from a team July 1, 2020 16:07
@rohany
Copy link
Copy Markdown
Contributor

rohany commented Jul 1, 2020

Would we backport something like this? I'm not sure.

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea looks good to me. I think we would backport this to 20.1, it's a major regression and does not touch any durable state. The pointer map should get fixed. Otherwise just the naming nit from me.

Reviewed 1 of 4 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @rohany)


pkg/sql/conn_executor.go, line 942 at r2 (raw file):

		jobs jobsCollection

		// jobsCache is a map of TableIDs to Jobs.

nit: maybe call this schemaChangeJobCache

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @rohany)


pkg/sql/planner.go, line 75 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Nvm I was being dumb, the pointer is necessary because we actually want the map to be reset by the call to resetExtraTxnState since it resets the cache. Without the pointer the cache doesn't get cleared when a txn is committed

Perhaps a better option is to clear the map rather than constructing a new one in resetExtraTxnState.

golang/go#20138

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Jul 1, 2020

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @rohany)

pkg/sql/planner.go, line 75 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Nvm I was being dumb, the pointer is necessary because we actually want the map to be reset by the call to resetExtraTxnState since it resets the cache. Without the pointer the cache doesn't get cleared when a txn is committed

Perhaps a better option is to clear the map rather than constructing a new one in resetExtraTxnState.

golang/go#20138

Good call, I'll do this instead.

…ng job

Currently, we perform a loop through all existing jobs which involves
querying a system table to check if there is an existing job for the current
table undergoing a schema change. To avoid this query, we can use a cache
to map TableIDs to jobs.

Fixes cockroachdb#50165, cockroachdb#50783

Release note: None
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewed 1 of 5 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RichardJCai and @rohany)

@RichardJCai
Copy link
Copy Markdown
Contributor Author

TFTR!

@RichardJCai
Copy link
Copy Markdown
Contributor Author

bors r+

@RichardJCai
Copy link
Copy Markdown
Contributor Author

bors r=ajwerner

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 2, 2020

Build failed (retrying...)

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 2, 2020

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v20.1.1 Regression: Grant on all tables is Very Slow

4 participants