{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25410","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25410/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25410/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25410/events","html_url":"https://github.com/cockroachdb/cockroach/pull/25410","id":322059052,"number":25410,"title":" sql: run schema changes after CREATE TABLE in txn","user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars1.githubusercontent.com/u/11774212?v=4","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":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-05-10T19:45:32Z","updated_at":"2018-05-16T19:27:42Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/25410","html_url":"https://github.com/cockroachdb/cockroach/pull/25410","diff_url":"https://github.com/cockroachdb/cockroach/pull/25410.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/25410.patch"},"body":"Includes a commit from #25362 and should be reviewed after that change."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/389637522","html_url":"https://github.com/cockroachdb/cockroach/pull/25410#issuecomment-389637522","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/25410","id":389637522,"user":{"login":"mjibson","id":41181,"avatar_url":"https://avatars1.githubusercontent.com/u/41181?v=4","gravatar_id":"","url":"https://api.github.com/users/mjibson","html_url":"https://github.com/mjibson","followers_url":"https://api.github.com/users/mjibson/followers","following_url":"https://api.github.com/users/mjibson/following{/other_user}","gists_url":"https://api.github.com/users/mjibson/gists{/gist_id}","starred_url":"https://api.github.com/users/mjibson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mjibson/subscriptions","organizations_url":"https://api.github.com/users/mjibson/orgs","repos_url":"https://api.github.com/users/mjibson/repos","events_url":"https://api.github.com/users/mjibson/events{/privacy}","received_events_url":"https://api.github.com/users/mjibson/received_events","type":"User","site_admin":false},"created_at":"2018-05-16T19:27:42Z","updated_at":"2018-05-16T19:27:42Z","author_association":"MEMBER","body":"\n\n\n\nReview status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.\n\n---\n\n*[pkg/sql/backfill.go, line 621 at r3](https://reviewable.io/reviews/cockroachdb/cockroach/25410#-LCeOQGs6YYwl7ZBOnit:-LCeOQGs6YYwl7ZBOniu:b3mq71q) ([raw file](https://github.com/cockroachdb/cockroach/blob/256826e25a1fe95d13f21adb924b864886952872/pkg/sql/backfill.go#L621)):*\n> ```Go\n> \t}\n> \n> \tif len(tableDesc.Mutations) == 0 {\n> ```\n\nnit: this block can be removed\n\n---\n\n*[pkg/sql/backfill.go, line 625 at r3](https://reviewable.io/reviews/cockroachdb/cockroach/25410#-LCeP9EvB4Y4s3INq6oS:-LCeP9EvB4Y4s3INq6oT:b-g051td) ([raw file](https://github.com/cockroachdb/cockroach/blob/256826e25a1fe95d13f21adb924b864886952872/pkg/sql/backfill.go#L625)):*\n> ```Go\n> \t}\n> \n> \tfor len(tableDesc.Mutations) > 0 {\n> ```\n\nWhy does this line need to be here? Can we just range over tableDesc.Mutations? I don't see how this loop would ever run more than once.\n\n---\n\n*[pkg/sql/table.go, line 458 at r3](https://reviewable.io/reviews/cockroachdb/cockroach/25410#-LCeJXQF5Wde0Ldfl0PF:-LCeJXQF5Wde0Ldfl0PG:bonn4x7) ([raw file](https://github.com/cockroachdb/cockroach/blob/256826e25a1fe95d13f21adb924b864886952872/pkg/sql/table.go#L458)):*\n> ```Go\n> \n> func (tc *TableCollection) isCreatedTable(id sqlbase.ID) bool {\n> \tif tc.createdTables == nil {\n> ```\n\nNo need for this if block. Code is clearer without it.\n\n---\n\n*[pkg/sql/table.go, line 511 at r3](https://reviewable.io/reviews/cockroachdb/cockroach/25410#-LCeMrv96rxCd6Z9_Zm3:-LCeMrv96rxCd6Z9_Zm4:bcgjvre) ([raw file](https://github.com/cockroachdb/cockroach/blob/256826e25a1fe95d13f21adb924b864886952872/pkg/sql/table.go#L511)):*\n> ```Go\n> \tdbID sqlbase.ID, tn *tree.TableName, required bool,\n> ) (refuseFurtherLookup bool, table *sqlbase.TableDescriptor, err error) {\n> \t// Walk latest to earliest.\n> ```\n\nWhy this change?\n\n---\n\n*[pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 138 at r2](https://reviewable.io/reviews/cockroachdb/cockroach/25410#-LCA_aVEYeGz94rLLEdS-r2-138:-LCeG4BsDY48mVgYOtMA:bs75l5v) ([raw file](https://github.com/cockroachdb/cockroach/blob/fb08c2a516cc7b1cfa267e9883e7f3bdd41982ca/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn#L138)):*\n<details><summary><i>Previously, vivekmenezes wrote…</i></summary><blockquote>\n\nI don't believe that's necessary right?\n</blockquote></details>\n\nSQL can return data in any order if an ORDER BY is not specified. See https://github.com/cockroachdb/cockroach/issues/24223. Or you can use the rowsort flag in the logictest.\n\n---\n\n*[pkg/sql/logictest/testdata/logic_test/schema_change_in_txn, line 235 at r3](https://reviewable.io/reviews/cockroachdb/cockroach/25410#-LCeGmCX6m8fPxVCzW65:-LCeGmCX6m8fPxVCzW66:b-fk1twb) ([raw file](https://github.com/cockroachdb/cockroach/blob/256826e25a1fe95d13f21adb924b864886952872/pkg/sql/logictest/testdata/logic_test/schema_change_in_txn#L235)):*\n> ```\n> \n> statement ok\n> COMMIT\n> ```\n\nYou can run a `COMMIT` after a statement with an error? I thought you had to run ROLLBACK in this case. TIL.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io/reviews/cockroachdb/cockroach/25410)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"mjibson"}}