tabledesc: forbid computed columns from having DEFAULT expressions

[postamar]

Previously, we didn't have a table descriptor validation check to ensure
that computed columns didn't also have DEFAULT expressions. It is
therefore possible that clusters and cluster backups exist in production
which contain tables with such columns. This commit therefore fixes this
bug by adding the missing validation check, and by removing default
expressions from computed columns when necessary after deserializing
a descriptor protobuf.

Fixes #72881.

Release note (sql change, bug fix): fixes a bug which allowed computed
columns to also have DEFAULT expressions.

[cockroach-teamcity]

This change is 

[ajwerner]

Thoughts on adding parallel testing for ON UPDATE? If so, do it as a separate commit which you don't backport to 21.1.backport has tools to pick commits.

Reviewed 1 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/sql/logictest/testdata/logic_test/computed, line 1054 at r1 (raw file):

statement error computed column "v" cannot also have a DEFAULT expression
ALTER TABLE t69327 ALTER COLUMN v SET DEFAULT 'foo'

what's the pgcode here? Are we only catching this with validation? Ideally not.

[postamar]

Sure, I can add a test for ON UPDATE in a separate commit. It wouldn't hurt.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/sql/logictest/testdata/logic_test/computed, line 1054 at r1 (raw file):

Previously, ajwerner wrote…

statement error computed column "v" cannot also have a DEFAULT expression
ALTER TABLE t69327 ALTER COLUMN v SET DEFAULT 'foo'

what's the pgcode here? Are we only catching this with validation? Ideally not.

No, because it's one of these cases I don't like where the validation function serves a dual purpose of (1) actually validating the descriptor and (2) validating the statement's effects. For some reason I don't understand, a lot of the errors which can be thrown in this way simply don't have pgcodes. If we care about pgcodes let's please address that in a separate issue and PR I feel this is out of scope here.

[ajwerner]

Reviewed 1 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/sql/logictest/testdata/logic_test/computed, line 1054 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

No, because it's one of these cases I don't like where the validation function serves a dual purpose of (1) actually validating the descriptor and (2) validating the statement's effects. For some reason I don't understand, a lot of the errors which can be thrown in this way simply don't have pgcodes. If we care about pgcodes let's please address that in a separate issue and PR I feel this is out of scope here.

At this point, I’m of the mindset that have two such checks is fine. The validation level check ought not to carry a pgcode and ought to be a defense in depth mechanism that additionally protects us in cases like restore and upgrades. The statement time checks give better errors to the user and carry the intention. It is duplication, but it’s duplication that I think has served us well thus far.

Fine with not doing it now. Were the randomized schema change workload in good shape, it’d reveal the missing pgcode.

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

[postamar]

Thanks for the review!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 5b4ce82 to blathers/backport-release-21.1-73090: 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 21.1.x failed. See errors above.

---

error creating merge commit from 5b4ce82 to blathers/backport-release-21.2-73090: 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 21.2.x failed. See errors above.

---

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