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

release-23.2: sql: Allow dropping enum value when referenced by UDF #121237

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Mar 27, 2024

Backport 2/2 commits from #118780.
fixes #120930

/cc @cockroachdb/release


This PR extends the current validation code executed
while dropping an enum value to also check for the value
being referenced in a UDF. Additionally, we re-enable
testing for alterTypeDropValue within the random
schema change workload.

Prior to dropping an enum value, we confirm that the value
is unused in tables, constraints, indices etc. However,
when we added support for UDFs, we did not extend the
scope of this to also check for an enum value being
referenced in a UDF. This introduced a bug where we were
unable to drop an unused enum value if some other value
from within the enum was being referenced by a UDF.
For example, if an enum 'e' contained values {'1', '2', '3'},
with the value '1' being referenced in a UDF, we were
unable to drop the values '2' and '3' as well.
This PR fixes this bug by expanding the usage check for
enum values to also include UDFs. With this fix, we can drop
an enum value as long as the value itself is unreferenced.

The first commit address this bug and and refactors some of
the common code into its own methods to avoid code duplication.
It extends the existing test suite to test for multiple scenarios
when an enum value is dropped.

The second commit re-enables testing for alterTypeDropValue
within the random schema workload. This was disabled
due to the bug which is being addressed in the first commit.

Epic: none

Fixes: #115612, #114844

Release note (bug fix): Fix an existing bug where we are
unable to drop an unused value from an enum if the enum is being
referenced in a UDF. With this bug fix, we can drop a value
from an enum as long as the value is not being referenced
by a UDF. Note, the enum can still be referenced by a UDF.
We only allow a value to be dropped if the value is not being
referenced by any other data element including UDFs.

Release justification: a low risk bug fix for an issue would prevent users from being able to modify enums used by UDFs

This commit  extends the current validation code executed
while dropping an enum value to also check for the value
being referenced in a UDF.

Prior to dropping an enum value, we confirm that the value
is unused in tables, constraints, indices etc. However,
when we added support for UDFs, we did not extend the
scope of this to also check for an enum value being
referenced in a UDF. This introduced a bug where we were
unable to drop an unused enum value if some other value
from within the enum was  being referenced by a UDF.
For example, if an enum 'e' contained values {'1', '2', '3'},
with the value '1' being referenced in a UDF, we were
unable to drop the values '2' and '3' as well.
This PR fixes this bug by expanding the usage check for
enum values to also include UDFs. With this fix, we can drop
an enum value as long as the value itself is unreferenced.

Additionally, the PR also refactors some of the common code into its
own methods to avoid code duplication. Finally, it extends the existing
test suite to test for multiple scenarios when an enum value
is dropped.

Fixes: cockroachdb#115612
@fqazi fqazi requested a review from a team March 27, 2024 19:37
@fqazi fqazi requested a review from a team as a code owner March 27, 2024 19:37
@fqazi fqazi requested review from DarrylWong and renatolabs and removed request for a team March 27, 2024 19:37
Copy link

blathers-crl bot commented Mar 27, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL and one additional
    TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Mar 27, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rimadeodhar rimadeodhar left a comment

Choose a reason for hiding this comment

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

:lgtm:. Did you have to make any major changes to the backport manually or were it just minor conflicts?

Reviewed 1 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong and @renatolabs)

@rimadeodhar
Copy link
Collaborator

Thanks for taking care of the backport!

@fqazi
Copy link
Collaborator Author

fqazi commented Mar 27, 2024

:lgtm:. Did you have to make any major changes to the backport manually or were it just minor conflicts?

Reviewed 1 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong and @renatolabs)

Just minor conflicts related to imports and a minor difference in the workload

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! can you submit this on #db-backports-point-releases ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants