/cockroach

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-2.1: sql: assorted mutation fixes #31280

Merged
merged 10 commits into from Oct 13, 2018

Conversation

Reviewers

@jordanlewis jordanlewis

@BramGruneir BramGruneir

@cockroachdb/sql-ccl-prs sql-ccl-prs

@cockroachdb/sql-rest-prs sql-rest-prs

@cockroachdb/sql-execution-prs sql-execution-prs

@cockroachdb/sql-planning-prs sql-planning-prs

@cockroachdb/sql-async-prs sql-async-prs

Assignees
No one assigned
Labels
None yet
Projects
  
Finished (milestone r2.1) in SQL Front-end, Lang & Semantics
Milestone
No milestone
4 participants
@knz @cockroach-teamcity @jordanlewis @BramGruneir
@knz
Member

knz commented Oct 11, 2018

Backport 10/10 commits from #31222.

/cc @cockroachdb/release

knz added some commits Oct 10, 2018

@knz
sql: fix the schema type check for INSERT/UPSERT 
The types of the input data for a mutation (the data source clause for
INSERT/UPSERT, the result of SET RHS for UPDATE) must match the types
of the columns being mutated.

Prior to this patch, this check was properly done early by UPDATE,
however it was not done at the right time for INSERT/UPSERT.

The code for INSERT/UPSERT was relying on an assertion check very deep
down in the encoding of datums to KV values. This assertion check was
never meant to be an input validation check, but because Go does not
really give us semantic assertions nobody noticed the check was
abused.

This patch clarifies the situation by introducing
`pgerror.NewAssertionErrorf` and using it there as appropriate. Later
patches can introduce more uses of pgerror.NewAssertionErrorf where
appropriate (e.g. everywhere we already use CodeInternalError).

In addition, the proper check for INSERT/UPSERT is introduced at the
right place, early during semantic analysis.

Release note (bug fix): CockroachDB will no longer fail in unexpected
ways or write invalid data when the type of input values provided to
INSERT/UPSERT does not match the type of the target columns.
c35092b
@knz
sql: produce the right pg error codes for column constraint failures 
Release note: None
f750e45
@knz
sql: ensure UPDATE checks column constraints before CHECK constraints 
This is required for postgres compatibility: the CHECK expressions
must be able to assume that the column data is already valid according
to the column type and nullability constraint.

The code for INSERT/UPSERT was already valid, only UPDATE needed to be
fixed. However this patch provides the corresponding test for all 3
mutations.

Release note (bug fix): UPDATE now verifies the column constraints
before CHECK constraints, for compatibility with PostgreSQL.
cd20f42
@knz
sql: add missing mutation check tests 
The mutations must validate column constraints before they perform FK
checks. There were no clear tests for this. This patch adds the
missing tests.

Release note: None
99a7bbb
@knz
sql: prevent UPDATE RETURNING from seeing non-public columns 
Prior to this patch, it was possible to use a newly added but not yet
visible column in UPDATE RETURNING. This patch fixes it.

INSERT/UPSERT/DELETE were not affected, however this patch adds the
corresponding tests for all 4 mutation statements -- this aspect
was not tested before.

Release note (bug fix): It is not possible any more to use
not-fully-added-yet columns in the RETURNING clause of UPDATE
statements.
85be94b
@knz
sql: fix the generation of an error message 
Release note: None
cd9e57f
@knz
sql: add a missing comment 
Release note: None
da762c0
@knz
sql: prevent INSERT DO UPDATE from silently setting computed columns 
Release note (bug fix): CockroachDB does no longer incorrectly and
silently accept a computed column on the left side of the assignment
in a ON CONFLICT DO UPDATE SET clause.
04e62d1
@knz
sql: improve an error message 
Release note: None
3402740
@knz
sql: prevent INSERT DO UPDATE SET from assigning mutation columns 
This is a band aid, a refactoring of the upsert pipeline is required
for a more robust solution.

Release note (bug fix): CockroachDB does no longer incorrectly and
silently accept a not-fully-added-yet column on the left side of the
assignment in a ON CONFLICT DO UPDATE SET clause.
Loading status checks…
f6c884d

@knz knz requested review from jordanlewis and BramGruneir Oct 11, 2018

@knz knz requested review from cockroachdb/sql-async-prs as code owners Oct 11, 2018

@knz knz added this to Triage in SQL Front-end, Lang & Semantics via automation Oct 11, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 11, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 11, 2018

This change is Reviewable

@knz knz moved this from Triage to Current milestone in SQL Front-end, Lang & Semantics Oct 11, 2018

@knz

This comment has been minimized.

Show comment
Hide comment
@knz

knz Oct 12, 2018

Member

do you reckon I can merge this before the weekend?
Member

knz commented Oct 12, 2018

do you reckon I can merge this before the weekend?
@BramGruneir

BramGruneir approved these changes Oct 12, 2018
View changes

:lgtm:

Reviewed 5 of 9 files at r1, 1 of 3 files at r2, 4 of 6 files at r5, 2 of 3 files at r6, 2 of 2 files at r9, 2 of 2 files at r10.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@knz knz merged commit a44d231 into cockroachdb:release-2.1 Oct 13, 2018
2 checks passed

2 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
license/cla Contributor License Agreement is signed.
Details

@knz knz deleted the knz:backport2.1-31222 branch Oct 13, 2018

@knz knz moved this from Current milestone to Finished (milestone r2.1) in SQL Front-end, Lang & Semantics Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment