rowenc: error when encoding NULLs for PK columns

[postamar]

This commit adds a validation check to rowenc.EncodeIndexKey to have it
return an error when it encodes a NULL value for a primary key column.

Fixes #69867.

Release note: None

[cockroach-teamcity]

This change is 

[jordanlewis]

Interesting fix! One thing I don't understand - how do we avoid this situation under normal circumstances without this check? It seems to me that we're adding a check at a very low level of the system, which in and of itself isn't bad (defense in depth), but are we missing a higher level of validation that normally exists in all cases besides this CREATE TABLE AS scenario that this PR is designed to solve?

[ajwerner]

One thing I don't understand - how do we avoid this situation under normal circumstances without this check?

I had a similar question. It turns out that in the implicit transaction case where we use a job, we end up relying on a similarly low-level check. An alternative approach might be to validate the non-nil properties of the columns by running a validation query. That would have cost and complexity and, thus, wouldn't be as suitable for backport.

I still favor such a check and will probably want us to plan one in the declarative schema changer.

[ajwerner]

Tagging @cockroachdb/sql-queries for a review. The current thinking is that we should merge this approach and backport it and then file a separate issue to do higher level constraint checking.

[mgartner]

FWIW, NOT NULL is enforced in mutations here:

cockroach/pkg/sql/update.go

Line 425 in dfd5205

func enforceLocalColumnConstraints(row tree.Datums, cols []catalog.Column) error {

[mgartner]

Looks find to me, though it does seem a bit low level. I wonder if CREATE TABLE t (...) AS ... should just be syntactic sugar for:

BEGIN;
CREATE TABLE t (...);
INSERT INTO t ...;
COMMIT;

Then the mutation logic would enforce NOT NULL constraints.

[mgartner]

Don't we mark all PK columns as non-nullable in the descriptor?

[postamar]

Weird things happen with CREATE TABLE ... AS ... unfortunately. It's sad.

[postamar]

Thanks for the review.

I wonder if CREATE TABLE t (...) AS ... should just be syntactic sugar for BEGIN ... CREATE ... INSERT ... COMMIT.

You're completely correct and that's exactly were we're headed at with the new schema changer.

[postamar]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)