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: populate indexprs column of pg_catalog.pg_index #72238

Merged
merged 1 commit into from Nov 1, 2021

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Oct 29, 2021

sql: populate indexprs column of pg_catalog.pg_index

This commit also fixes the formatting of user-defined types in the
indexdef column of pg_catalog.pg_indexes and the indpred column of
pg_catalog.pg_index.

Release note (bug fix): The indexprs column of pg_catalog.pg_index
is now populated with string representations of every expression element
in the index. If the index is not an expression index, indexprs is
NULL. The indexdef column of pg_catalog.pg_indexes and the
indpred column of pg_catalog.pg_index now correctly display
user-defined types.

@mgartner mgartner requested a review from rafiss October 29, 2021 16:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rafiss rafiss 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 @mgartner and @rafiss)


pkg/sql/pg_catalog.go, line 1710 at r1 (raw file):

						if col.IsExpressionIndexColumn() {
							colIDs = append(colIDs, 0)
							exprs = append(exprs, fmt.Sprintf("(%s)", col.GetComputeExpr()))

we potentially might want to be using schemaexpr.FormatExprForDisplay() -- take a look at the other usages in this file. I forget the exact reasoning, but I believe you worked on that function before, so hopefully together we can remember :)


pkg/sql/pg_catalog.go, line 1750 at r1 (raw file):

					indpred := tree.DNull
					if index.IsPartial() {
						indpred = tree.NewDString(index.GetPredicate())

maybe schemaexpr.FormatExprForDisplay() here too?

@rafiss rafiss added the backport-21.2.x 21.2 is EOL label Oct 29, 2021
This commit also fixes the formatting of user-defined types in the
`indexdef` column of `pg_catalog.pg_indexes` and the `indpred` column of
`pg_catalog.pg_index`.

Release note (bug fix): The `indexprs` column of `pg_catalog.pg_index`
is now populated with string representations of every expression element
in the index. If the index is not an expression index, `indexprs` is
`NULL`. The `indexdef` column of `pg_catalog.pg_indexes` and the
`indpred` column of `pg_catalog.pg_index` now correctly display
user-defined types.
Copy link
Collaborator Author

@mgartner mgartner 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 @rafiss)


pkg/sql/pg_catalog.go, line 1710 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we potentially might want to be using schemaexpr.FormatExprForDisplay() -- take a look at the other usages in this file. I forget the exact reasoning, but I believe you worked on that function before, so hopefully together we can remember :)

Good point. We need to use this to correct print user-defined types. I've added some tests cases to highlight this.

This also led me to stumble upon this bug, which I won't try to fix in this PR, if that's ok with you: #72241


pkg/sql/pg_catalog.go, line 1750 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

maybe schemaexpr.FormatExprForDisplay() here too?

Done.

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm! and thanks for uncovering that other issue

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

@mgartner
Copy link
Collaborator Author

mgartner commented Nov 1, 2021

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 1, 2021

Build succeeded:

@craig craig bot merged commit ce0b24e into cockroachdb:master Nov 1, 2021
@blathers-crl
Copy link

blathers-crl bot commented Nov 1, 2021

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 072cb20 to blathers/backport-release-21.2-72238: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-21.2.x 21.2 is EOL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants