sql: include partial index predicates in pg_catalog and pg builtins #53967
Conversation
|
This change is |
| ---- | ||
| CREATE INDEX pg_indexdef_partial_idx ON test.public.pg_indexdef_test USING btree (a ASC) WHERE a > 0 | ||
|
|
||
| # TODO(mgartner): To match Postgres, "::" casts should be appended to constants |
There was a problem hiding this comment.
i noticed this too, for other pg_catalog columns. i have a change here (#53965) that does it, and it should be easy to do here too
pkg/sql/pg_catalog.go
Outdated
| // | ||
| // TODO(mgartner): Avoid the parsing he predicate expression twice. It | ||
| // is parsed in schemaexpr.FormatExprForDisplay and again here. | ||
| formattedPred, err := schemaexpr.FormatExprForDisplayWithoutTypeAnnotations(ctx, table, index.Predicate, p.SemaCtx()) |
There was a problem hiding this comment.
just a bit confused -- why can't formattedPred be used directly as the return value?
There was a problem hiding this comment.
This is confusing because this function converts an IndexDescriptor to a tree.CreateIndex. The tree.CreateIndex.Predicate field is a tree.Expr not a string. So we have to parse it again.
This is definitely not an ideal solution, but I think it's the most minimal one for now. In the long-term, I think it would be prudent to consider whether this conversion back into a tree.CreateIndex AST node is the right way to go about this. At the very least, the many code paths for formatting indexes should converge as much as possible.
There was a problem hiding this comment.
LGTM, just had a small question. also depending on if this PR or #53965 is merged first, one of us will have to accommodate the others changes. which order of merging would you prefer? also, can i rope you into reviewing mine? 😄
|
Let's get yours in first. I can rebase afterwards. I'll review it now. |
|
#53965 is merged, so if you rebase now, you should be able to address some of the TODOs you made by using |
This commit updates the `regression_46799` subtest in `pg_catalog` logic tests so that hard-coded relids are not used. Previously, every time a new relation was added in the test file, this test would have to be rewritten with new relids. Determining the new relid was a nuisance. Now, this test should not require changes when other relations are added or removed elsewhere in the file. Release justification: This is a test-only, non-production code change. Release note: None
mgartner
force-pushed
the
53843-include-partial-index-pred-in-pg-catalog
branch
2 times, most recently
from
0a4cd3e
to
45df2ce
Compare
|
@rafiss I've updated this so that casts are now included for string types, and I've removed |
There was a problem hiding this comment.
excellent, lgtm!
Reviewed 2 of 5 files at r5, 4 of 4 files at r6.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rafiss)
mgartner
changed the title
This commit adds the partial index predicate expression to the formatted `indexdef` column of `pg_catalog.pg_indexes`. Also, the predicate is now included in the output of the `pg_get_indexdef` builtin function. Including the predicate is critical for compatibility with Postgres ORMs. Release justification: This is a low-risk change to new functionality, partial indexes. Release note: None
This commit updates `pg_catalog.pg_constraint.condef` to include the predicate of unique partial index constraints. Also, the predicate is now included in the output of `SHOW CONSTRAINTS` and `pg_get_constraintdef()`. Release justification: This is a low-risk change to new functionality, partial indexes. Release note: None
mgartner
force-pushed
the
53843-include-partial-index-pred-in-pg-catalog
branch
from
45df2ce
to
fa8fac3
Compare
|
bors r=rafiss |
|
Build succeeded: |
logictest: make pg_catalog 46799 regression test more robust
This commit updates the
regression_46799subtest inpg_cataloglogictests so that hard-coded relids are not used. Previously, every time a
new relation was added in the test file, this test would have to be
rewritten with new relids. Determining the new relid was a nuisance.
Now, this test should not require changes when other relations are added
or removed elsewhere in the file.
Release justification: This is a test-only, non-production code change.
Release note: None
sql: include partial index predicate in pg_catalog.pg_indexes
This commit adds the partial index predicate expression to the formatted
indexdefcolumn ofpg_catalog.pg_indexes. Also, the predicate is nowincluded in the output of the
pg_get_indexdefbuiltin function.Including the predicate is critical for compatibility with Postgres
ORMs.
Fixes #53843
Release justification: This is a low-risk change to new functionality,
partial indexes.
Release note: None
pkg: include partial index predicate expressions in SHOW CONSTRAINTS
This commit updates
pg_catalog.pg_constraint.condefto include thepredicate of unique partial index constraints. Also, the predicate is
now included in the output of
SHOW CONSTRAINTSandpg_get_constraintdef().Fixes #53908
Release justification: This is a low-risk change to new functionality,
partial indexes.
Release note: None