New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: use table leases in planner.searchAndQualifyDatabase #13635

Merged
merged 1 commit into from Feb 17, 2017

Conversation

Projects
None yet
4 participants
@petermattis
Contributor

petermattis commented Feb 16, 2017

Use table leases to retrieve table descriptors in
planner.searchAndQualifyDatabase. This avoids 2 KV reads (1 for the
database descriptor, 1 for the table descriptor) for queries that use an
unqualified table name.

Fixes #13632


This change is Reviewable

@RaduBerinde

This comment has been minimized.

Show comment
Hide comment
@RaduBerinde

RaduBerinde Feb 16, 2017

Member

LGTM, I just saw the issue and was looking at the code

Member

RaduBerinde commented Feb 16, 2017

LGTM, I just saw the issue and was looking at the code

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Feb 16, 2017

Contributor

I don't know the sql code well enough anymore to know if there are implications to use a table lease instead of a non-cached descriptor here. Definitely would appreciate the reviewers to sanity check that the code paths which call QualifyWithDatabase can use a cached descriptor.

Contributor

petermattis commented Feb 16, 2017

I don't know the sql code well enough anymore to know if there are implications to use a table lease instead of a non-cached descriptor here. Definitely would appreciate the reviewers to sanity check that the code paths which call QualifyWithDatabase can use a cached descriptor.

@petermattis petermattis requested a review from knz Feb 16, 2017

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Feb 16, 2017

Contributor

@RaduBerinde Do you know this code well enough to know if using a cached descriptor (i.e. a table lease) is kosher in all of the call paths? I don't and I haven't audited the code yet.

Contributor

petermattis commented Feb 16, 2017

@RaduBerinde Do you know this code well enough to know if using a cached descriptor (i.e. a table lease) is kosher in all of the call paths? I don't and I haven't audited the code yet.

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Feb 16, 2017

Contributor

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/sql/table.go, line 531 at r1 (raw file):

		// has its timestamps set correctly so mustGetTableDesc will fetch with the
		// correct timestamp.
		descFunc = p.mustGetTableOrViewDesc

Should this be getTableOrViewDesc? I'm not clear on the difference between that method and mustGetTableOrViewDesc.


Comments from Reviewable

Contributor

petermattis commented Feb 16, 2017

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/sql/table.go, line 531 at r1 (raw file):

		// has its timestamps set correctly so mustGetTableDesc will fetch with the
		// correct timestamp.
		descFunc = p.mustGetTableOrViewDesc

Should this be getTableOrViewDesc? I'm not clear on the difference between that method and mustGetTableOrViewDesc.


Comments from Reviewable

@a-robinson

LGTM

Show outdated Hide outdated pkg/sql/table.go
@a-robinson

This comment has been minimized.

Show comment
Hide comment
@a-robinson

a-robinson Feb 16, 2017

Member

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


pkg/sql/table.go, line 531 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Should this be getTableOrViewDesc? I'm not clear on the difference between that method and mustGetTableOrViewDesc.

You shouldn't need the must version. All it's doing compared to the non-must version is adding an error that promptly gets ignored by this code.


Comments from Reviewable

Member

a-robinson commented Feb 16, 2017

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


pkg/sql/table.go, line 531 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Should this be getTableOrViewDesc? I'm not clear on the difference between that method and mustGetTableOrViewDesc.

You shouldn't need the must version. All it's doing compared to the non-must version is adding an error that promptly gets ignored by this code.


Comments from Reviewable

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Feb 16, 2017

Contributor

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


pkg/sql/table.go, line 531 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

You shouldn't need the must version. All it's doing compared to the non-must version is adding an error that promptly gets ignored by this code.

Ah, I see. Switched to the non-must version. Note that I cribbed this code from planner.getTableScanOrViewPlan which uses the must version.


Comments from Reviewable

Contributor

petermattis commented Feb 16, 2017

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


pkg/sql/table.go, line 531 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

You shouldn't need the must version. All it's doing compared to the non-must version is adding an error that promptly gets ignored by this code.

Ah, I see. Switched to the non-must version. Note that I cribbed this code from planner.getTableScanOrViewPlan which uses the must version.


Comments from Reviewable

@RaduBerinde

This comment has been minimized.

Show comment
Hide comment
@RaduBerinde

RaduBerinde Feb 16, 2017

Member

It's used in two places - for views and for getDataSource. The former sets avoidCachedDescriptors so it should be unaffected. The latter also uses getTableScanOrViewPlan which uses a lease so it seems fine to me.


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


Comments from Reviewable

Member

RaduBerinde commented Feb 16, 2017

It's used in two places - for views and for getDataSource. The former sets avoidCachedDescriptors so it should be unaffected. The latter also uses getTableScanOrViewPlan which uses a lease so it seems fine to me.


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


Comments from Reviewable

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Feb 17, 2017

Contributor

It's used in two places - for views and for getDataSource.

Ah, confusingly there is also TableName.QualifyWithDatabase which was fouling my simplistic grep'ing. Seems safe to me for the two uses.


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

Contributor

petermattis commented Feb 17, 2017

It's used in two places - for views and for getDataSource.

Ah, confusingly there is also TableName.QualifyWithDatabase which was fouling my simplistic grep'ing. Seems safe to me for the two uses.


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

sql: use table leases in planner.searchAndQualifyDatabase
Use table leases to retrieve table descriptors in
planner.searchAndQualifyDatabase. This avoids 2 KV reads (1 for the
database descriptor, 1 for the table descriptor) for queries that use an
unqualified table name.

Fixes #13632
@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Feb 17, 2017

Member

Yes, all the code paths that use this can (and probably should) use table leases. It's all SQL name resolution which only looks at the schema visible from the current transaction.
I wrote this code and I was assuming the whole time that the underlying methods were using leases, and I didn't check.

Member

knz commented Feb 17, 2017

Yes, all the code paths that use this can (and probably should) use table leases. It's all SQL name resolution which only looks at the schema visible from the current transaction.
I wrote this code and I was assuming the whole time that the underlying methods were using leases, and I didn't check.

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Feb 17, 2017

Member

The change looks kosher to me btw.


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

Member

knz commented Feb 17, 2017

The change looks kosher to me btw.


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@petermattis petermattis merged commit 0f8aa7a into cockroachdb:master Feb 17, 2017

3 checks passed

ci/teamcity TeamCity build finished
Details
code-review/reviewable Review complete: 0 of 0 LGTMs obtained
Details
licence/cla Contributor License Agreement is signed.
Details

@petermattis petermattis deleted the petermattis:pmattis/search-and-qualify-database branch Feb 17, 2017

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