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

sql: set mutation columns to nullable while backfilling #32585

Merged
merged 4 commits into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@vivekmenezes
Copy link
Contributor

vivekmenezes commented Nov 26, 2018

Also added the missing UPSERT tests.

The better way to fix this in the future is to always set
mutable columns to non-nullable in the yet to be introduced
ImmutableTableDescriptor (the opposite of MutableTableDescriptor)

fixes #32530

Release note (sql change): Fix crash in UPSERT in the middle of
a schema change adding a non-nullable column.

@vivekmenezes vivekmenezes requested a review from jordanlewis Nov 26, 2018

@vivekmenezes vivekmenezes requested review from cockroachdb/sql-async-prs as code owners Nov 26, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 26, 2018

This change is Reviewable

@vivekmenezes vivekmenezes requested a review from knz Nov 26, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 26, 2018

filed away future cleanup #32586

// Even if the column is non-nullable it can be null in the
// middle of a schema change while it's being baxckfilled
// to a non-nullable value.
col.Nullable = true

This comment has been minimized.

@knz

knz Nov 26, 2018

Member

@vivekmenezes I think we need to extend this change. Let me explain.

The change you are making here appears to help the row reader to not be confused when it sees a null value in a non-nullable column. I have some comments about this (see point 2 below) but let's keep it at that for now.

  1. The main issue here is that changing this property will create a semantic problem for row writers -- it is important that a client app receives a SQL error if they attempt to insert a NULL value into a non-NULLable column. With your change here, I believe this check is not done any more.

So I think the first order of business is to verify (with tests) that NULL inserts into non-NULLable columns currently undergoing a backfill are still/properly rejected by SQL errors.

  1. regarding the point at the top, with the row readers. In any case it is invalid for a SQL client to receive a NULL value as response for a query to a non-NULL column. We must not allow this to happen in any case -- this is a plain correctness error, a silent data error (S-0).

I wonder if your change here makes it more or less likely to encounter this situation. If the column is marked as nullable in the descriptor, and there is a query to read a row in the process of being added, the correct behavior is this:

2.a) the query must not see the column at all -- it must operate on the table as if the column did not exist
2.b) in any case, there must not be any NULL value visible by the query

In contrast, the following behavior would be incorrect:

2.c) the query sees the column
2.d) the query sees a NULL value in the column

What would you say in response to my comments here?

This comment has been minimized.

@vivekmenezes

vivekmenezes Nov 26, 2018

Contributor

@knz we get around this problem by always not allowing a column to be provided with a value in sql when it is not public. So specifically, when a column is being backfilled, all DML statements are only inserting defaults/computed columns (internal values) into the column. I know this is tested but I'll add in additional tests that try to insert NULL values.

But you bring up an important point that affects #32586 that I'll add to that issue.

regarding 2. since the column is non-public it will never be seen by a SELECT.

This comment has been minimized.

@knz

knz Nov 26, 2018

Member

Ok thanks.

What happens if the user runs SHOW CREATE during the txn where the column was added? Is the new column hidden in the output of SHOW CREATE? If not, is the nullability affected?

This comment has been minimized.

@knz

knz Nov 26, 2018

Member

Also to ensure the new added column is not visible it will be interesting to ensure:

  1. we have a test that the following statement groups each produce an error:
CREATE TABLE t (x INT PRIMARY KEY);
BEGIN;
ALTER TABLE t ADD COLUMN y INT NOT NULL DEFAULT 123;
INSERT INTO t (x) VALUES (1) ON CONFLICT DO UPDATE SET x = t.y + 1; -- this must error with "t.y is not public"
COMMIT;
  1. that the behavior with stored columns that depend on both new columns and existing columns is clear:
CREATE TABLE t (x INT PRIMARY KEY);
BEGIN;
ALTER TABLE t ADD COLUMN y INT NOT NULL DEFAULT 123,
              ADD COLUMN z INT AS (x + y) STORED;
INSERT INTO t (x) VALUES (2); --  see note below
COMMIT;

In this example the column z must be updated both by the backfill and the insert, so that the final value of z is properly equal to 125.

@vivekmenezes
Copy link
Contributor

vivekmenezes left a comment

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


pkg/sql/sqlbase/structured.go, line 1904 at r2 (raw file):

Previously, knz (kena) wrote…

Also to ensure the new added column is not visible it will be interesting to ensure:

  1. we have a test that the following statement groups each produce an error:
CREATE TABLE t (x INT PRIMARY KEY);
BEGIN;
ALTER TABLE t ADD COLUMN y INT NOT NULL DEFAULT 123;
INSERT INTO t (x) VALUES (1) ON CONFLICT DO UPDATE SET x = t.y + 1; -- this must error with "t.y is not public"
COMMIT;
  1. that the behavior with stored columns that depend on both new columns and existing columns is clear:
CREATE TABLE t (x INT PRIMARY KEY);
BEGIN;
ALTER TABLE t ADD COLUMN y INT NOT NULL DEFAULT 123,
              ADD COLUMN z INT AS (x + y) STORED;
INSERT INTO t (x) VALUES (2); --  see note below
COMMIT;

In this example the column z must be updated both by the backfill and the insert, so that the final value of z is properly equal to 125.

I've beefed up the tests. PTAL. Thanks!

@vivekmenezes vivekmenezes removed the request for review from jordanlewis Nov 26, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 27, 2018

@knz I'd love to merge this soon so that we can declare this fixed! Thanks!

@knz
Copy link
Member

knz left a comment

Thank you for pinging me. I have reviewed this again, see my comments below.

Reviewed 3 of 3 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/schema_changer_test.go, line 2241 at r4 (raw file):

	var wg sync.WaitGroup
	wg.Add(1)
	go func() {

I like the SQL in this test but I do not understand the architecture of the test.

It seems to me like we want the test to issue the schema change concurrently with each of the SQL statements below. Right now the test is non-deterministic and probably completes the schema change very early, before the first few SQL statements below are executed.

What confidence can we create that all the SQL statements below are executed concurrently with a schema change?

Is there a way to block the SC transaction and see all these statements:

A) assert that there is a mutation present in the descriptor (if there is no mutation, the test is not testing anything)

B) that despite the mutation the behavior is correct


pkg/sql/sqlbase/structured.go, line 1902 at r4 (raw file):

				col := *c
				// Even if the column is non-nullable it can be null in the
				// middle of a schema change while it's being baxckfilled
  1. nit: typo

  2. why do this in FindReadableColumnById? Is this because you only want the Nullable property to be set on a copy (so the desc is unaffected)?

If so, how can we be sure that the columns are not accessed in other ways than via FindReadableColumnByID?

In particular:

  • makeUpdaterWithoutCascader() (row/writer.go) iterates on Mutations directly and is probably sensitive to having the Nullable bit set properly
  • ditto deleter, etc
  • ProcessDefaultColumns / processColumnSet (default_exprs.go) iterate on Mutations directly but need to have appropriate nullability information for computing default values

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:upsertfail branch from 2e68cd8 to 2af33ee Nov 27, 2018

@vivekmenezes
Copy link
Contributor

vivekmenezes left a comment

Thanks for taking a look. PTAL

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


pkg/sql/schema_changer_test.go, line 2241 at r4 (raw file):

Previously, knz (kena) wrote…

I like the SQL in this test but I do not understand the architecture of the test.

It seems to me like we want the test to issue the schema change concurrently with each of the SQL statements below. Right now the test is non-deterministic and probably completes the schema change very early, before the first few SQL statements below are executed.

What confidence can we create that all the SQL statements below are executed concurrently with a schema change?

Is there a way to block the SC transaction and see all these statements:

A) assert that there is a mutation present in the descriptor (if there is no mutation, the test is not testing anything)

B) that despite the mutation the behavior is correct

The schema changer is blocked from executing through testing knobs. I can do both A and B here. Keep in mind we do test behavior in many tests in descriptor_mutations_test.go. This test is for testing that none of these statements will crash.

Added both A and B


pkg/sql/sqlbase/structured.go, line 1902 at r4 (raw file):

makeUpdaterWithoutCascader
I've modified this to introduce a FindInactiveColumnById() method and moved the code to set the appropriate column to nullable to the writer.go code.

@knz
Copy link
Member

knz left a comment

Thank you for the changes. The test looks good.
I think some other test needs to be adapted (see CI failure)

LGTM modulo double-checking the rest of the code in row/writer.go - perhaps the deleter needs the same change.

Reviewed 3 of 3 files at r6.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/sqlbase/structured.go, line 1902 at r4 (raw file):

Previously, vivekmenezes wrote…

makeUpdaterWithoutCascader
I've modified this to introduce a FindInactiveColumnById() method and moved the code to set the appropriate column to nullable to the writer.go code.

Thank you. Can you double check the other writers in the same file. I think also the deleter is affected.

@vivekmenezes
Copy link
Contributor

vivekmenezes left a comment

The test failure is #32671

TFTR!

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


pkg/sql/sqlbase/structured.go, line 1902 at r4 (raw file):

Previously, knz (kena) wrote…

Thank you. Can you double check the other writers in the same file. I think also the deleter is affected.

We do have DELETE in the unittests. The reason why DELETE is not affected is because it doesn't read any rows when performing its operations, and when using an expression it can only use columns that are public.

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:upsertfail branch from 2af33ee to 809f11f Nov 28, 2018

@knz

This comment has been minimized.

Copy link
Member

knz commented Nov 28, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 28, 2018

@knz I don't believe we allow such FK relationships to exist. Let me add a test for that

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:upsertfail branch from 4a7a0f9 to 90b5fa5 Nov 28, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 28, 2018

@knz I've added two more commits to this PR. The first one fixes a minor bug and the other checks that we cannot add FK relationships over columns that are inactive. PTAL

@knz

knz approved these changes Nov 29, 2018

Copy link
Member

knz left a comment

:lgtm: nice work! 💖

Reviewed 3 of 3 files at r8, 2 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 29, 2018

TFTR!

vivekmenezes added some commits Nov 21, 2018

sql: set mutation columns to nullable while backfilling
Also added the missing UPSERT tests. Add tests for INSERT
and DELETE.

The better way to fix this in the future is to always set
mutable columns to non-nullable in the yet to be introduced
ImmutableTableDescriptor (the opposite of MutableTableDescriptor)

fixes #32530

Release note (sql change): Fix crash in UPSERT in the middle of
a schema change adding a non-nullable column.
sql: allow ADD NOT NULL computed columns
Release note (sql change): fix error when configuring NOT NULL
computed columns.

@vivekmenezes vivekmenezes force-pushed the vivekmenezes:upsertfail branch from 90b5fa5 to 764af4f Nov 29, 2018

@vivekmenezes

This comment has been minimized.

Copy link
Contributor

vivekmenezes commented Nov 29, 2018

bors r+

craig bot pushed a commit that referenced this pull request Nov 29, 2018

Merge #32277 #32585 #32700
32277: exec: implement and plan decimal/float average columnar operator r=asubiotto a=asubiotto

First three commits are #32138

When experimental_vectorize = true, if an average is performed on an
ordered decimal column, the columnar decimal average aggregator is now
planned.

```
root@:26257/tpch> SET experimental_vectorize=true;
SET

Time: 705µs

root@:26257/tpch> SELECT AVG(l_quantity) FROM lineitem;
avg
+-----------------------+
 25.507967136654827397
(1 row)

Time: 6.232818s

root@:26257/tpch> SET experimental_vectorize=false;
SET

Time: 673µs

root@:26257/tpch> SELECT AVG(l_quantity) FROM lineitem;
avg
+-----------------------+
 25.507967136654827397
(1 row)

Time: 7.661776s
```

This is not a crazy improvement but keep in mind that it seems that the scan performance dominates here (around 6s when run with EXPLAIN ANALYZE).

Microbenchmarks of the average operator are included in the relevant commit message.

Looking into adding logic tests for general columnar aggregations.

This PR is useful for the queries we care about but there are two nits I'd like to fix in the long term:
1) Can't use AggregatorSpec_FUNC because doing so would create an import cycle. We have been wanting to extract the specs into their separate packages but although I gave it a shot, it was taking longer than I wanted it to.
2) Template the average operator. Something I can potentially do in this PR but this is useful as it is. I just need to figure out what the preferred way of templating arbitrary type to decimal conversion is.

32585: sql: set mutation columns to nullable while backfilling r=vivekmenezes a=vivekmenezes

Also added the missing UPSERT tests.

The better way to fix this in the future is to always set
mutable columns to non-nullable in the yet to be introduced
ImmutableTableDescriptor (the opposite of MutableTableDescriptor)

fixes #32530

Release note (sql change): Fix crash in UPSERT in the middle of
a schema change adding a non-nullable column.

32700: kv: log the results of range lookups r=andreimatei a=andreimatei

Release note: None

Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 29, 2018

Build succeeded

@craig craig bot merged commit 764af4f into cockroachdb:master Nov 29, 2018

3 checks passed

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

@vivekmenezes vivekmenezes deleted the vivekmenezes:upsertfail branch Nov 29, 2018

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

@knz knz moved this from Triage to Finished (m2.2-2) in SQL Front-end, Lang & Semantics Dec 4, 2018

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