opt: fix trigger+computed column internal error

[mgartner]

This commit fixes a bug that was caused by incorrectly modifying the set
and list of target columns when re-projecting 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.

[cockroach-teamcity]

This change is 

[mgartner]

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

[DrewKimball]

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: complete! 0 of 0 LGTMs obtained

[mgartner]

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 like it. I tried something similar on Friday, but I forgot why I abandoned it. Let me try again.

[DrewKimball]

Thanks!

@DrewKimball reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[mgartner]

I think this assumes that table columns have ascending IDs... which is true right now, but maybe not something we want to rely on.

Here's one place where we assume that the order of columns in targetColList is the same as the order of columns in mb.outScope after projectSynthesizedComputedCols returns:

cockroach/pkg/sql/opt/optbuilder/insert.go

Lines 699 to 701 in b845bd8

	for i := range mb.outScope.cols {
	inCol := &mb.outScope.cols[i]
	ord := mb.tabID.ColumnOrdinal(mb.targetColList[i])

The need for targetColList is dubious... Maybe we can get rid of it in the near future...

In the meantime I'm going to attempt a safer fix.

[mgartner]

Ok, third time's the charm. PTAL!

[DrewKimball]

@DrewKimball reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained

[mgartner]

TFTRs!

bors r+

[craig]

Build succeeded:

• lint
• check_generated_code
• examples_orms
• linux_amd64_build
• docker_image_amd64
• local_roachtest
• linux_amd64_fips_build
• unit_tests
• acceptance
• local_roachtest_fips

[blathers-crl]

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

---

Issue #154672: branch-release-24.3, branch-release-25.3, branch-release-25.4.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}