Skip to content

sql: add a virtual index on the pg_catalog.pg_type.OID#51374

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
ekalinin:virtual-index-for-pg_type-oid
Jul 17, 2020
Merged

sql: add a virtual index on the pg_catalog.pg_type.OID#51374
craig[bot] merged 1 commit intocockroachdb:masterfrom
ekalinin:virtual-index-for-pg_type-oid

Conversation

@ekalinin
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

@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
Copy Markdown
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.

@ekalinin ekalinin force-pushed the virtual-index-for-pg_type-oid branch from 971e343 to e222791 Compare July 17, 2020 05:35
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
Copy Markdown
Contributor

rohany commented Jul 17, 2020

Thanks for the contribution! This is ready to go.

bors r=rohany

@craig
Copy link
Copy Markdown
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