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.

Sign up for GitHub

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

Jump to bottom

sql/schemachanger: ensure dropping descriptors are not taken OFFLINE #86626

Closed

ajwerner opened this issue Aug 23, 2022 · 0 comments · Fixed by #86639

Assignees

Labels

branch-release-22.2 C-bug release-blocker T-sql-foundations

Contributor

ajwerner commented Aug 23, 2022 •

edited by cockroach-jira-scripts

Loading

Describe the problem

In #83915 we moved descriptors to transition through OFFLINE before being dropped. This was done for two reasons:

To enable us to drop descriptors in the same transaction where something which might fail occurs
To fix a bug in the existing TXN_DROPPED state whereby virtual tables would see descriptors which should not be seen.

Note that TXN_DROPPED did not preclude (1). The problem with OFFLINE is that it is not backwards compatible, and the removal of TXN_DROPPED is not forwards compatible.

Jira issue: CRDB-18846

The text was updated successfully, but these errors were encountered:

ajwerner added C-bug release-blocker branch-release-22.2 labels

ajwerner self-assigned this

blathers-crl bot added the T-sql-schema-deprecated label

ajwerner mentioned this issue

sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE #86639

Merged

Xiang-Gu pushed a commit to Xiang-Gu/cockroach that referenced this issue


          sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE

5e962dc

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today
support, but doing that would require an overhaul of the dependency rules
which,at this point in the cycle, feels risky.

Fixes cockroachdb#86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

Xiang-Gu pushed a commit to Xiang-Gu/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

e3596eb

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

Xiang-Gu pushed a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE

f5e74cc

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today
support, but doing that would require an overhaul of the dependency rules
which,at this point in the cycle, feels risky.

Fixes cockroachdb#86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

Xiang-Gu pushed a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

ab3df2b

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

ajwerner added a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE

a3b4068

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today
support, but doing that would require an overhaul of the dependency rules
which,at this point in the cycle, feels risky.

Fixes cockroachdb#86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

ajwerner added a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

cff1905

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

Xiang-Gu pushed a commit to Xiang-Gu/cockroach that referenced this issue


          sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE

74fa543

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today
support, but doing that would require an overhaul of the dependency rules
which,at this point in the cycle, feels risky.

Fixes cockroachdb#86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

Xiang-Gu pushed a commit to Xiang-Gu/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

90747dc

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

postamar pushed a commit to postamar/cockroach that referenced this issue


          sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE

825ff48

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today
support, but doing that would require an overhaul of the dependency rules
which,at this point in the cycle, feels risky.

Fixes cockroachdb#86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

postamar pushed a commit to postamar/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

a836f74

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

postamar pushed a commit to postamar/cockroach that referenced this issue


          sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE

923f7f2

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today
support, but doing that would require an overhaul of the dependency rules
which,at this point in the cycle, feels risky.

Fixes cockroachdb#86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

postamar pushed a commit to postamar/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

e70db5e

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

ajwerner added a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

6a7477d

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

ajwerner added a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

2ada0c2

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

ajwerner added a commit to ajwerner/cockroach that referenced this issue


          sql/schemachanger: enforce online schema change invariants with dep r…

affdfee

…ules

Previously we had implicit rules in opgen which were used to enforce
sequencing between transitions. This was brittle and made it hard to
collapse certain edges. Now that we can represent a not-join, we can
add the rules we want selectively, only when descriptors are not being
dropped.

Fixes cockroachdb#86691
Fixes cockroachdb#86626

Release justification: fixes a bug

Release note: None

craig bot pushed a commit that referenced this issue


          Merge #86639 #87151 #87175

86639: sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE r=postamar a=ajwerner

The first two commits enable synthetic descriptors to be used during transaction
execution to influence the virtual tables.

We added the `OFFLINE` and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the `OFFLINE` state is not backwards compatible,
and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors
straight to `DROPPED` in the pre-commit case for all the cases we today support,
but doing that would require an overhaul of the dependency rules which, at this
point in the cycle, feels risky.

Fixes #86626

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

87151: sql: plumb isCopy down to event log r=rafiss a=otan

Previously, we relied on the `txn` being filled to log `COPY`
statements. This is however inaccurate - `txn` may be nil if `COPY` is
the first statement encountered. This meant that the first COPY
encountered would not be logged.

Instead, plumb down `isCopy` and use `timeutil.Now()` if it is a COPY
related statement.

Release justification: logging related critical fix

Release note: None

87175: sql: fix COPY internal error in optimizer r=cucaroach a=cucaroach

Statistics builder code to evaluate distinct count had a typo and would
crash if there were more rows than columns.  Fix bug and rewrite to be a
little clearer.

Fixes: #87011

Release justification: low-risk high priority fix to new funcationality
Release note: None


Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Tommy Reilly <[email protected]>

craig bot closed this as completed in

c302570

exalate-issue-sync bot added T-sql-foundations and removed T-sql-schema-deprecated labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment