{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6552","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6552/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6552/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6552/events","html_url":"https://github.com/cockroachdb/cockroach/pull/6552","id":153359985,"number":6552,"title":"sql: terminate schema change if it violating integrity constraints.","user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars.githubusercontent.com/u/11774212?v=3","gravatar_id":"","url":"https://api.github.com/users/vivekmenezes","html_url":"https://github.com/vivekmenezes","followers_url":"https://api.github.com/users/vivekmenezes/followers","following_url":"https://api.github.com/users/vivekmenezes/following{/other_user}","gists_url":"https://api.github.com/users/vivekmenezes/gists{/gist_id}","starred_url":"https://api.github.com/users/vivekmenezes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vivekmenezes/subscriptions","organizations_url":"https://api.github.com/users/vivekmenezes/orgs","repos_url":"https://api.github.com/users/vivekmenezes/repos","events_url":"https://api.github.com/users/vivekmenezes/events{/privacy}","received_events_url":"https://api.github.com/users/vivekmenezes/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"andreimatei","id":377201,"avatar_url":"https://avatars.githubusercontent.com/u/377201?v=3","gravatar_id":"","url":"https://api.github.com/users/andreimatei","html_url":"https://github.com/andreimatei","followers_url":"https://api.github.com/users/andreimatei/followers","following_url":"https://api.github.com/users/andreimatei/following{/other_user}","gists_url":"https://api.github.com/users/andreimatei/gists{/gist_id}","starred_url":"https://api.github.com/users/andreimatei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreimatei/subscriptions","organizations_url":"https://api.github.com/users/andreimatei/orgs","repos_url":"https://api.github.com/users/andreimatei/repos","events_url":"https://api.github.com/users/andreimatei/events{/privacy}","received_events_url":"https://api.github.com/users/andreimatei/received_events","type":"User","site_admin":false},"milestone":null,"comments":3,"created_at":"2016-05-06T01:23:05Z","updated_at":"2016-05-09T18:46:45Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/6552","html_url":"https://github.com/cockroachdb/cockroach/pull/6552","diff_url":"https://github.com/cockroachdb/cockroach/pull/6552.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/6552.patch"},"body":"The original code would declare the schema change as unviable\r\nif it hit any error including transient timeouts. This change now\r\nexplictly terminates the schema change only if it hits integrity\r\nconstraints. Furthermore the schema change is retried for all other\r\nerrors with the exception of the \"table not found\" error.\r\n\r\nWhenever an integrity constraint error is hit, the schema change\r\nis first put into reverse, and then backed out. If an error is\r\nseen while backing out, the process is stopped to be continued\r\nlater via the asynchronous schema change manager.\r\n\r\nfixes #6333 #6432\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/6552)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/217953187","html_url":"https://github.com/cockroachdb/cockroach/pull/6552#issuecomment-217953187","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6552","id":217953187,"user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars.githubusercontent.com/u/11774212?v=3","gravatar_id":"","url":"https://api.github.com/users/vivekmenezes","html_url":"https://github.com/vivekmenezes","followers_url":"https://api.github.com/users/vivekmenezes/followers","following_url":"https://api.github.com/users/vivekmenezes/following{/other_user}","gists_url":"https://api.github.com/users/vivekmenezes/gists{/gist_id}","starred_url":"https://api.github.com/users/vivekmenezes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vivekmenezes/subscriptions","organizations_url":"https://api.github.com/users/vivekmenezes/orgs","repos_url":"https://api.github.com/users/vivekmenezes/repos","events_url":"https://api.github.com/users/vivekmenezes/events{/privacy}","received_events_url":"https://api.github.com/users/vivekmenezes/received_events","type":"User","site_admin":false},"created_at":"2016-05-09T18:46:45Z","updated_at":"2016-05-09T18:46:45Z","body":"\n\n\n\n\n\nReview status: 1 of 7 files reviewed at latest revision, 27 unresolved discussions, some commit checks pending.\n\n---\n\n*[sql/errors.go, line 119 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5aG3r1xFHAyOdD4bt:-KHL2Xf8LJtYDUfQ6Wwr:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/errors.go#L119)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit: s/column/columnName/?</details>\ndone\n\n---\n\n*[sql/errors.go, line 123 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5aRtTyCvf0ktNtOU3:-KHL2xDLRWxKSztlMzdZ:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/errors.go#L123)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> pg gives a better error: `null value in column \"id\" violates not-null constraint`</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 167 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5bswaxfTwTFJ_y1tZ:-KHL35xa5y3RODJshQZv:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L167)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/schemaChangeRetryError/isSchemaChangeRetryError/g</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 168 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5ag-LJO5e3EVshK43:-KHL37BeLpuG7cowMh6i:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L168)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit: i like the type-switch style better like you have in isIntegrityConstraintError\n> \n> ```\n> switch err.(type) {\n>   case *roachpb.DescriptorNotFoundError:\n>     return false\n>   default:\n>     return isIntegrityConstraitError(err)\n> }\n> ```</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 242 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5azW1NyFwEvCvAx1w:-KHL3SWOyHn8rlodyXc7:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L242)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/fail/failed/</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 244 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5b0MW-8TdviciQx3j:-KHL3T95LvksBUtFSV8-:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L244)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> \"accomodate\" doesn't feel like the right word here. Perhaps \"that may be resolved by retrying\"?</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 247 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5bDSCqrTXMzsYfvVL:-KHL3bEFCImI-ag624-I:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L247)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/constraint/constraint violation/</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 249 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5bKSkmEtMEIzIu8xu:-KHL3cv6o2lesL80Fqz3:-382938660) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L249)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> sounds scary. does this get logged?</details>\nIt gets logged in session.go. Its actually not scary because the async path takes care of node failures and we do test that a good state is left behind \n\n---\n\n*[sql/schema_changer.go, line 255 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5bOQMVJRjjVefKIbf:-KHL5gG0bneq1FhKed4E:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L255)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> scope this more narrowly.</details>\ndone\n\n---\n\n*[sql/schema_changer.go, line 409 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5bg5BrHowAmcGOotq:-KHL5UBT9CswgQ9Y8mqt:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer.go#L409)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> put this in the caller and log the actual error - it's not always an integrity constraint violation (as evidenced by the existence of `schemaChangeRetryError`)</details>\ndone\n\n---\n\n*[sql/schema_changer_test.go, line 726 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5cMlMyGSDytOVw3NC:-KHL7brnO2n4xWZHH0Mj:-584183475) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L726)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit: why not composite literal?</details>\nto allow us to reassign SchemaChangersStartBackfillNotification later\n\n---\n\n*[sql/schema_changer_test.go, line 730 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5cJ_SO0lb2emMwqk9:-KHL5qsz46bPH2bFJGNU:-1763578873) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L730)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> is this thread safe?</details>\nit doesn't have to be thread safe because it's called in a specific order.\n\n---\n\n*[sql/schema_changer_test.go, line 744 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5cfJPFFp4-Pl14SpQ:-KHL8-TsKgMxTUovb6t4:1963367944) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L744)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit: `k` appears not to be used. can you use `CREATE TABLE t.test (v INT)`?</details>\nwe need k to maintain a primary key on the table, besides it's a kv table\n\n---\n\n*[sql/schema_changer_test.go, line 765 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dPhR3fPjWVW5Ohzx:-KHLIvV1XzoLNXRn7LSu:-379376322) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L765)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> what if attempts is 3 or 1? be more specific please.</details>\nit now logs the number of attempts\n\n---\n\n*[sql/schema_changer_test.go, line 808 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5d6gARNBEg5b5Gmte:-KHLJMvTONkTnVr6HNV8:-837966666) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L808)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit: you can scope this more narrowly with\n> ```\n> defer func (backFillChunkSize int) {\n>   csql.IndexBackfillChunkSize = backfillChunkSize\n> }(csql.IndexBackfillChunkSize)\n> ```\n> \n> really don't feel strongly about this one.</details>\nnice. Done\n\n---\n\n*[sql/schema_changer_test.go, line 814 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dKXZP84pFA-C3OmI:-KHLJPJV5PhiMyMNFA_a:-524025497) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L814)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> is this thread safe?</details>\nagain, the retries occur in a specific order and there isn't a need for any locking.\n\n---\n\n*[sql/schema_changer_test.go, line 833 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dU_i_BP_hXJBWWup:-KHLKbvD_kH5ZX--NjS4:679869031) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L833)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> mention the expected outcome</details>\nis that better?\n\n---\n\n*[sql/schema_changer_test.go, line 837 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dZJcplh4QGFT399A:-KHLL14bFIw9jE3S38YA:-467915377) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L837)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> \"not found\" is way too broad</details>\nfixed\n\n---\n\n*[sql/schema_changer_test.go, line 860 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dhYpLqd0N1wpefkz:-KHLUYks9wod87Xj0ytG:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L860)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> mention the expected outcome.</details>\ndone\n\n---\n\n*[sql/schema_changer_test.go, line 865 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dki2PthG_JkCU3EW:-KHLUXdUhfXpE22Bqrfr:677588487) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L865)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> mention the expected outcome.</details>\ndone\n\n---\n\n*[sql/schema_changer_test.go, line 868 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552#-KH5dqoED-nJGRymnAUY:-KHLnQzeVachuhYRsNN-:-616844176) ([raw file](https://github.com/cockroachdb/cockroach/blob/ff0f9707b6bc493aafee22b9538a7c4c137bba87/sql/schema_changer_test.go#L868)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> can you also test the reversal to make sure this gets cleaned up? perhaps this is tested elsewhere? if so, where?</details>\nDone. I had to export disableAsyncSchemaChangeExec to allow us to turn it on and then off later when we require the async path to run and clean up the garbage left behind.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/6552)*\n<!-- Sent from Reviewable.io -->\n"}}