sql: add checks after all referenced columns have been backfilled #35300

thoszhang · 2019-03-01T04:34:38Z

Previously, if a column was added with a check constraint that also referenced
another column that was public, writes to that public column would erroneously
fail (and, in the worst case, result in a panic) while the column being added
was not yet public. With this change, the schema changer will now wait to add
the check constraint to the table descriptor until after all the columns that
were added in the same transaction have been backfilled.

A new optional field has been added to ConstraintToValidate for the check
constraint itself, so that it can be added to the table descriptor at the
correct step in the schema change process.

I ended up adding this field to the existing mutation instead of creating a new
type of mutation to add the constraint to the descriptor, since it ultimately
seemed to me that a mutation that simply adds a schema element in its backfill
step would be too inconsistent with what mutations are, especially since all
the mutations for a single transaction are basically executed at the same time.
To support NOT VALID in the future, we could add more flags to the protobuf to
indicate that either the addition of the constraint or the validation should be
skipped, so that they can be executed separately.

Fixes #35258, fixes #35193

Release note: None

cockroach-teamcity · 2019-03-01T04:34:45Z

This change is

pkg/sql/backfill.go

andy-kimball · 2019-03-04T19:01:38Z

pkg/sql/sqlbase/structured.go

@@ -584,6 +584,25 @@ func (desc *TableDescriptor) AllNonDropIndexes() []*IndexDescriptor {
 	return indexes
 }

+// AllPublicAndNonPublicChecks returns all the checks, including those being
+// added in the mutations.
+func (desc *TableDescriptor) AllPublicAndNonPublicChecks() []*TableDescriptor_CheckConstraint {


Need a better name here, since it sounds the same as the existing AllChecks method. I wouldn't know which to use.

I updated it so that constraints are either "active" (should be enforced for writes) or "inactive" (queued but invisible to writes). I'm open to better suggestions.

Seems reasonable.

thoszhang · 2019-03-05T18:06:18Z

This is ready for another look.

vivekmenezes

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @lucy-zhang, and @vivekmenezes)

pkg/sql/backfill.go, line 208 at r2 (raw file):

			return err
		}
		version = desc.Version

Add a comment here

pkg/sql/backfill.go, line 332 at r2 (raw file):

	return desc, nil
}

Both the above two functions have nothing to do with backfilling. I'd suggest calling them before the backfiller is called. Can you call them from RunStateMachineBeforeBackfill? Seems like you can for dropChecksInRollback. perhaps we can call addChecks() in schemachanger.done()

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @lucy-zhang, and @vivekmenezes)

pkg/sql/backfill.go, line 332 at r2 (raw file):

Previously, vivekmenezes wrote…

Both the above two functions have nothing to do with backfilling. I'd suggest calling them before the backfiller is called. Can you call them from RunStateMachineBeforeBackfill? Seems like you can for dropChecksInRollback. perhaps we can call addChecks() in schemachanger.done()

Yeah, dropChecksInRollback() could be moved to RunStateMachineBeforeBackfill. The problem is that addChecks() has to be called before the check is validated. So if we moved addChecks() to schemachanger.done(), validation would have to happen in done() as well, which seems undesirable.

We also can't call addChecks() before the column backfill is complete. If we do, then the constraint evaluator will see NULL values in the non-public column, even for columns which have default/computed values. In practice this might be fine for most checks, which will always pass if the expression evaluates to NULL, so we'll just let allow those writes and then find all violations at the end. But ultimately I don't think it's a good idea to have an "active" constraint that's being enforced on a column that's not fully backfilled yet. (There's a test here with an explicit NOT NULL condition that demonstrates this problem: https://github.com/cockroachdb/cockroach/pull/35300/files#diff-e38d0cfdbd3babbd39d2de3495f5ed86R1104). A similar issue exists for FKs: we don't want to add the backreference visible to updates/deletes on the referenced columns until the index on the referencing columns is fully backfilled.

thoszhang

This is ready for another look. The changes were:

restoring the function call to drop checks that failed validation to reverseMutations(), where it was originally, since it didn't actually seem like there was a good place to put it in any other step of the schema changer
rebasing to incorporate sql: rearchitecture ALTER TABLE RENAME, add support for renaming constraints #35091 (to rename constraints)
adding more logic tests for schema change rollbacks and for the original bug in the case of computed columns
adding more comments

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @lucy-zhang, and @vivekmenezes)

vivekmenezes

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @lucy-zhang)

pkg/sql/backfill.go, line 186 at r3 (raw file):

				if !m.Rollback {
					panic("trying to drop constraint through schema changer outside of a rollback")
				}

while I see the value of having a panic here I think it's better to return an error.

pkg/sql/backfill.go, line 269 at r3 (raw file):

			return nil
		},
		func(txn *client.Txn) error {

I believe you can use nil instead of using a dummy function

pkg/sql/sqlbase/structured.go, line 605 at r3 (raw file):

	}
	return checks
}

Its better to construct the list active and inactive checks once in NewImmutableTableDescriptor() and return it here.

Previously, if a column was added with a check constraint that also referenced another column that was public, writes to that public column would erroneously fail (and, in the worst case, result in a panic) while the column being added was not yet public. With this change, the schema changer will now wait to add the check constraint to the table descriptor until after all the columns that were added in the same transaction have been backfilled. A new optional field has been added to `ConstraintToValidate` for the check constraint itself, so that it can be added to the table descriptor at the correct step in the schema change process. I ended up adding this field to the existing mutation instead of creating a new type of mutation to add the constraint to the descriptor, since it ultimately seemed to me that a mutation that simply adds a schema element in its backfill step would be too inconsistent with what mutations are, especially since all the mutations for a single transaction are basically executed at the same time. To support NOT VALID in the future, we could add more flags to the protobuf to indicate that either the addition of the constraint or the validation should be skipped, so that they can be executed separately. Release note: None

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @lucy-zhang, and @vivekmenezes)

pkg/sql/backfill.go, line 186 at r3 (raw file):

Previously, vivekmenezes wrote…

while I see the value of having a panic here I think it's better to return an error.

Done.

pkg/sql/backfill.go, line 269 at r3 (raw file):

Previously, vivekmenezes wrote…

I believe you can use nil instead of using a dummy function

Done.

pkg/sql/sqlbase/structured.go, line 605 at r3 (raw file):

Previously, vivekmenezes wrote…

Its better to construct the list active and inactive checks once in NewImmutableTableDescriptor() and return it here.

I made this a TableDescriptor method because it only gets called when when schema changes are being performed and it doesn't make sense to use ImmutableTableDescriptor. This is also consistent with methods like AllNonDropIndexes() above, though I agree there is some duplicated logic which is not ideal.

vivekmenezes

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @lucy-zhang)

thoszhang · 2019-03-13T15:17:42Z

bors r+

35300: sql: add checks after all referenced columns have been backfilled r=lucy-zhang a=lucy-zhang Previously, if a column was added with a check constraint that also referenced another column that was public, writes to that public column would erroneously fail (and, in the worst case, result in a panic) while the column being added was not yet public. With this change, the schema changer will now wait to add the check constraint to the table descriptor until after all the columns that were added in the same transaction have been backfilled. A new optional field has been added to `ConstraintToValidate` for the check constraint itself, so that it can be added to the table descriptor at the correct step in the schema change process. I ended up adding this field to the existing mutation instead of creating a new type of mutation to add the constraint to the descriptor, since it ultimately seemed to me that a mutation that simply adds a schema element in its backfill step would be too inconsistent with what mutations are, especially since all the mutations for a single transaction are basically executed at the same time. To support NOT VALID in the future, we could add more flags to the protobuf to indicate that either the addition of the constraint or the validation should be skipped, so that they can be executed separately. Fixes #35258, fixes #35193 Release note: None 35682: engineccl/mvcc: fix time-bound iterator's interaction with moving intents r=nvanbenschoten a=nvanbenschoten Fixes #34819. 349ff61 introduced a workaround for #28358 into MVCCIncrementalIterator. This workaround created a second (non-time-bound) iterator to verify possibly-phantom MVCCMetadata keys during iteration. We found in #34819 that it is necessary for correctness that sanityIter be created before the original iter. This is because the provided Reader that both iterators are created from may not be a consistent snapshot, so the two iterators could end up observing different information. The hack around sanityCheckMetadataKey only works properly if all possible discrepancies between the two iterators lead to intents and values falling outside of the timestamp range **from the time-bound iterator's perspective**. This allows us to simply ignore discrepancies that we notice in advance(). This commit makes this change. It also adds a test that failed regularly before the fix under stress and no longer fails after the fix. Release note: None 35689: roachtest: add large node kv tests and batching kv tests r=nvanbenschoten a=nvanbenschoten This commit adds support for running `kv` with a `--batch` parameter. It then adds the following new roachtest configurations: - kv0/enc=false/nodes=3/batch=16 - kv95/enc=false/nodes=3/batch=16 - kv0/enc=false/nodes=3/cpu=96 - kv95/enc=false/nodes=3/cpu=96 - kv50/enc=false/nodes=4/cpu=96/batch=64 The last test is currently skipped because of #34241. I confirmed that it triggers the corresponding assertion on both AWS and GCE. My request for more m5d.24xlarge quota just succeeded, but I may need to request more quota for n1-highcpu-96 VMs for these to run nightly. Release note: None Co-authored-by: Lucy Zhang <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>

craig · 2019-03-13T15:50:38Z

Build succeeded

GitHub CI (Cockroach)

thoszhang requested review from dt and a team March 1, 2019 04:34

thoszhang force-pushed the constraint-mutation branch 2 times, most recently from f773dea to ba23f2a Compare March 2, 2019 00:08

dt approved these changes Mar 4, 2019

View reviewed changes

pkg/sql/backfill.go Outdated Show resolved Hide resolved

pkg/sql/backfill.go Show resolved Hide resolved

thoszhang requested review from vivekmenezes and andy-kimball March 4, 2019 18:25

andy-kimball mentioned this pull request Mar 4, 2019

opt: Fix panic when CHECK expression depends on delete-only column #35310

Closed

andy-kimball reviewed Mar 4, 2019

View reviewed changes

thoszhang force-pushed the constraint-mutation branch 4 times, most recently from f032bd6 to b2fecda Compare March 5, 2019 18:01

vivekmenezes suggested changes Mar 6, 2019

View reviewed changes

thoszhang commented Mar 6, 2019

View reviewed changes

thoszhang mentioned this pull request Mar 11, 2019

opt: panic on insert when a column is being dropped and a check constraint is present #35611

Closed

thoszhang force-pushed the constraint-mutation branch from b2fecda to 8badb35 Compare March 12, 2019 00:50

thoszhang commented Mar 12, 2019

View reviewed changes

vivekmenezes suggested changes Mar 12, 2019

View reviewed changes

thoszhang force-pushed the constraint-mutation branch from 8badb35 to be946fd Compare March 13, 2019 02:08

thoszhang commented Mar 13, 2019

View reviewed changes

vivekmenezes approved these changes Mar 13, 2019

View reviewed changes

craig bot merged commit be946fd into cockroachdb:master Mar 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add checks after all referenced columns have been backfilled #35300

sql: add checks after all referenced columns have been backfilled #35300

thoszhang commented Mar 1, 2019 •

edited

Loading

cockroach-teamcity commented Mar 1, 2019

andy-kimball Mar 4, 2019

thoszhang Mar 5, 2019

andy-kimball Mar 5, 2019

thoszhang commented Mar 5, 2019

vivekmenezes left a comment

thoszhang left a comment

thoszhang left a comment

vivekmenezes left a comment

thoszhang left a comment

vivekmenezes left a comment

thoszhang commented Mar 13, 2019

craig bot commented Mar 13, 2019

sql: add checks after all referenced columns have been backfilled #35300

sql: add checks after all referenced columns have been backfilled #35300

Conversation

thoszhang commented Mar 1, 2019 • edited Loading

cockroach-teamcity commented Mar 1, 2019

andy-kimball Mar 4, 2019

Choose a reason for hiding this comment

thoszhang Mar 5, 2019

Choose a reason for hiding this comment

andy-kimball Mar 5, 2019

Choose a reason for hiding this comment

thoszhang commented Mar 5, 2019

vivekmenezes left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

vivekmenezes left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

vivekmenezes left a comment

Choose a reason for hiding this comment

thoszhang commented Mar 13, 2019

craig bot commented Mar 13, 2019

Build succeeded

thoszhang commented Mar 1, 2019 •

edited

Loading