-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: Define more columns in pg_catalog.pg_statistic_ext
#93274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls also take a review from @rafiss
Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown)
-- commits
line 7 at r1:
looks to me like a bug fix worthy of a release note.
pkg/sql/pg_catalog.go
line 3465 at r1 (raw file):
for _, row := range rows { tableID := tree.MustBeDInt(row[0]) name := tree.MustBeDString(row[1])
Using MustBeDString
for the name forces a heap allocation when you convert it back to a Datum below.
You could simply refer to row[1]
in the addRow call below instead.
pkg/sql/pg_catalog.go
line 3488 at r1 (raw file):
schemaOid(statSchema), // stxnamespace tree.DNull, // stxowner tree.NewDInt(-1), // stxstattarget
You can allocate this DInt just once out of the loop.
6058fc2
to
0ad6ab3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rafiss)
Previously, knz (Raphael 'kena' Poss) wrote…
looks to me like a bug fix worthy of a release note.
Done.
pkg/sql/pg_catalog.go
line 3465 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Using
MustBeDString
for the name forces a heap allocation when you convert it back to a Datum below.
You could simply refer torow[1]
in the addRow call below instead.
Done.
pkg/sql/pg_catalog.go
line 3488 at r1 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
You can allocate this DInt just once out of the loop.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, with one nit!
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @knz)
pkg/sql/pg_catalog.go
line 3453 at r2 (raw file):
schema: vtable.PgCatalogStatisticExt, populate: func(ctx context.Context, p *planner, db catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { query := `SELECT "tableID", name, "columnIDs", "statisticID", '{d}'::CHAR[] FROM system.table_statistics;`
nit: the {d}
value should be cast to "char"[]
also, can you add a comment explaining what d
means here (based on the PG docs)
`pg_catalog.pg_statistic_ext` is now populated with more data. Release note (bug fix): The `stxnamespace`, `stxkind` and `stxstattarget` columns are now defined in `pg_statistics_ext`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice fix! i'm adding a backport label as well
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @knz)
bors r=rafiss |
Build failed (retrying...): |
Build failed (retrying...): |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 5fb4c60 to blathers/backport-release-22.1-93274: 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 22.1.x failed. See errors above. error creating merge commit from 5fb4c60 to blathers/backport-release-22.2-93274: 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 22.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Epic: CRDB-23454
Fixes: #88108
Fixes: #93430
This commit makes sure the
stxnamespace
,stxkind
andstxstattarget
columns are defined inpg_statistics_ext
.Release note: The
stxnamespace
,stxkind
andstxstattarget
columns are now defined inpg_statistics_ext
. Alsopg_statistics_ext
no longer crashes when stxname is null.