-
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: disallow using cluster_logical_timestamp as column default when backfilling #98696
sql: disallow using cluster_logical_timestamp as column default when backfilling #98696
Conversation
e54db54
to
4b7aa6e
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 @Xiang-Gu)
pkg/sql/backfill/backfill.go
line 380 at r1 (raw file):
if errors.Is(err, colexecerror.ErrNilTxnAccessedInColBackfill) { // Issue #98269; Cannot use `cluster_logical_timestamp()` as column default value. return roachpb.Key{}, pgerror.WithCandidateCode(err, pgcode.FeatureNotSupported)
Consider using unimplemented.NewWithIssue
instead.
pkg/sql/sem/eval/context.go
line 501 at r1 (raw file):
func (ec *Context) GetClusterTimestamp() (*tree.DDecimal, error) { if ec.Txn == nil { return nil, colexecerror.ErrNilTxnAccessedInColBackfill
GetClusterTimestamp()
can be called outside of col backfill, right? It seems like this error is misnamed. Maybe something like ErrNilTxnInClusterContext
instead.
4b7aa6e
to
1c93ab6
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 @rharding6373)
pkg/sql/backfill/backfill.go
line 380 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Consider using
unimplemented.NewWithIssue
instead.
Done.
pkg/sql/sem/eval/context.go
line 501 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
GetClusterTimestamp()
can be called outside of col backfill, right? It seems like this error is misnamed. Maybe something likeErrNilTxnInClusterContext
instead.
Done.
…backfilling Previously, `ADD COLUMN ... DEFAULT cluster_logical_timestamp()` would crash the node and leave the table in a corrupt state. The root cause is a nil pointer dereference. This commit fixed it by returning an unimplemented error and hence disallow using this builtin function as default value when backfilling. Release note (bug fix): fixed a bug as detailed in cockroachdb#98269.
1c93ab6
to
796cf66
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 @rharding6373)
pkg/sql/backfill/backfill.go
line 380 at r1 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
Done.
It turns out I cannot. The base error is defined and I can only decorate it with a FeatureNotSupported pgcode. Plus, I don't think creating a issue for this is necessary; my original comments refers to the issue that required this fix. I've updated the comments.
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.
Reviewed 1 of 8 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rharding6373)
TFTR! bors r+ |
👎 Rejected by code reviews |
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! 1 of 0 LGTMs obtained (waiting on @Xiang-Gu)
bors r+ |
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 796cf66 to blathers/backport-release-22.1-98696: 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 796cf66 to blathers/backport-release-22.2-98696: 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 dev-inf. |
Previously,
ADD COLUMN ... DEFAULT cluster_logical_timestamp()
would crash the node and leave the table in a corrupt state. The root cause is a nil pointer dereference. This commit fixed it by returning an unimplemented error and hence disallow using this builtin function as default value when backfilling.Fixes: #98269
Release note (bug fix): fixed a bug as detailed in #98269.