Skip to content

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Oct 3, 2025

This commit fixes a bug that was caused by prematurely re-synthesizing
computed columns after building a BEFORE trigger.

Fixes #154672

Release note (bug fix): A bug has been fixed that caused internal errors
for INSERT .. ON CONFLICT .. DO UPDATE statements when the target
table had both a computed column and a BEFORE trigger. This bug has
been present since triggers were introduced in v24.3.0.

This commit fixes a bug that was caused by prematurely re-synthesizing
computed columns after  building a `BEFORE` trigger.

Fixes cockroachdb#154672

Release note (bug fix): A bug has been fixed that caused internal errors
for `INSERT .. ON CONFLICT .. DO UPDATE` statements when the target
table had both a computed column and a `BEFORE` trigger. This bug has
been present since triggers were introduced in v24.3.0.
@mgartner mgartner requested review from DrewKimball and a team October 3, 2025 19:52
@mgartner mgartner requested a review from a team as a code owner October 3, 2025 19:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner
Copy link
Collaborator Author

mgartner commented Oct 3, 2025

This feels a bit hacky, but gets the job done. Probably worth thinking if there is a better way to fix this.

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

It seems that targetCols is only used to keep track of the columns directly being inserted into or being set by an update. I think the right way to fix it is actually to have addSynthesizedComputedCols skip appending to targetCols when called for a trigger. What do you think?

I think the mutationBuilder could use some refactoring. That field is really only meant to be used "locally" while building the input for an insert or an update (and is unset after that's done), but that's not really clear from the comments.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: v25.2.6: addTargetColsForUpdate cannot be called more than once
3 participants