Skip to content
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: add a virtual index on the pg_catalog.pg_type.OID #51374

Merged
merged 1 commit into from
Jul 17, 2020

Conversation

ekalinin
Copy link
Contributor

@ekalinin ekalinin commented Jul 13, 2020

Fixes #49208

Release note (performance improvement): scans over virtual
table pg_type by OID column have improved performance in common cases.

@ekalinin ekalinin requested a review from a team as a code owner July 13, 2020 14:21
@blathers-crl
Copy link

blathers-crl bot commented Jul 13, 2020

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Jul 13, 2020
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ekalinin ekalinin force-pushed the virtual-index-for-pg_type-oid branch from c316fd0 to c332751 Compare July 13, 2020 14:23
@blathers-crl
Copy link

blathers-crl bot commented Jul 13, 2020

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@rohany
Copy link
Contributor

rohany commented Jul 13, 2020

Hey @ekalinin, thanks for your contribution! However, this doesn't do what we want. Right now, your virtual index is ignoring the input constraint value and generating all rows of the table instead. You should be using the input constraint to return only the row of the oid requested.

@ekalinin
Copy link
Contributor Author

ekalinin commented Jul 15, 2020

Hey @ekalinin, thanks for your contribution! However, this doesn't do what we want. Right now, your virtual index is ignoring the input constraint value and generating all rows of the table instead. You should be using the input constraint to return only the row of the oid requested.

Thanks for response, @rohany !
Could you help me with this?

I'm trying to use the input constraint value like this:

	indexes: []virtualIndex{
		{
			partial: false,
			populate: func(ctx context.Context, constraint tree.Datum, p *planner, db *sqlbase.ImmutableDatabaseDescriptor,
				addRow func(...tree.Datum) error) (bool, error) {
				oid := tree.MustBeDOid(constraint)
				fmt.Printf("--> populate index: oid: %+v\n", oid)
				tbl, err := p.LookupTableByID(ctx, sqlbase.ID(oid.DInt))
				if err != nil {
					fmt.Printf("--> populate index: lookup err=%+v\n", err)
					if sqlbase.IsUndefinedRelationError(err) {
						fmt.Printf("--> populate index: IsUndefinedRelationError\n")
						return false, nil
					}
					return false, err
				}
  // ...

and getting an error:

➜ ./cockroach demo 
root@127.0.0.1:34233/movr> select * from pg_type where oid = 1000;
--> populate index: oid: 1000
--> populate index: lookup err=relation "[1000]" does not exist

@rohany
Copy link
Contributor

rohany commented Jul 15, 2020

You're on the right track! You'll first want to check if the input oid is one of the predefined type oids. If not, then you'll want to look up the oid's TypeDescriptor. Note that the oid of a type doesn't directly correspond to its descriptor ID. You have to use types.UserDefinedTypeOIDToID to do the conversion.

@blathers-crl
Copy link

blathers-crl bot commented Jul 16, 2020

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@ekalinin ekalinin force-pushed the virtual-index-for-pg_type-oid branch 2 times, most recently from 542f2a8 to 17a4103 Compare July 16, 2020 21:02
@ekalinin
Copy link
Contributor Author

You're on the right track! You'll first want to check if the input oid is one of the predefined type oids. If not, then you'll want to look up the oid's TypeDescriptor. Note that the oid of a type doesn't directly correspond to its descriptor ID. You have to use types.UserDefinedTypeOIDToID to do the conversion.

Hey @rohany! Thanks for the hints! I hope i understood them correctly.
PTAL.

pkg/sql/opt/exec/execbuilder/testdata/catalog Show resolved Hide resolved
pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
@ekalinin ekalinin force-pushed the virtual-index-for-pg_type-oid branch 2 times, most recently from ef7f435 to 971e343 Compare July 16, 2020 21:42
Copy link
Contributor

@rohany rohany left a comment

Choose a reason for hiding this comment

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

Looking good! Just 2 more small requests.

pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
@ekalinin ekalinin force-pushed the virtual-index-for-pg_type-oid branch from 971e343 to e222791 Compare July 17, 2020 05:35
pkg/sql/pg_catalog.go Outdated Show resolved Hide resolved
Release note (performance improvement): scans over virtual
table pg_type by OID column have improved performance in common cases.
@ekalinin ekalinin force-pushed the virtual-index-for-pg_type-oid branch from e222791 to d85f407 Compare July 17, 2020 18:21
@rohany
Copy link
Contributor

rohany commented Jul 17, 2020

Thanks for the contribution! This is ready to go.

bors r=rohany

@craig
Copy link
Contributor

craig bot commented Jul 17, 2020

Build succeeded

@craig craig bot merged commit 7cb22cd into cockroachdb:master Jul 17, 2020
@ekalinin ekalinin deleted the virtual-index-for-pg_type-oid branch August 11, 2020 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: add a virtual index on the OID column of pg_catalog.pg_type
3 participants