{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13428","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13428/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13428/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13428/events","html_url":"https://github.com/cockroachdb/cockroach/pull/13428","id":205630908,"number":13428,"title":"sql: perform index backfill using distsql","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":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.githubusercontent.com/u/16544120?v=3","gravatar_id":"","url":"https://api.github.com/users/RaduBerinde","html_url":"https://github.com/RaduBerinde","followers_url":"https://api.github.com/users/RaduBerinde/followers","following_url":"https://api.github.com/users/RaduBerinde/following{/other_user}","gists_url":"https://api.github.com/users/RaduBerinde/gists{/gist_id}","starred_url":"https://api.github.com/users/RaduBerinde/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RaduBerinde/subscriptions","organizations_url":"https://api.github.com/users/RaduBerinde/orgs","repos_url":"https://api.github.com/users/RaduBerinde/repos","events_url":"https://api.github.com/users/RaduBerinde/events{/privacy}","received_events_url":"https://api.github.com/users/RaduBerinde/received_events","type":"User","site_admin":false},"assignees":[{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.githubusercontent.com/u/16544120?v=3","gravatar_id":"","url":"https://api.github.com/users/RaduBerinde","html_url":"https://github.com/RaduBerinde","followers_url":"https://api.github.com/users/RaduBerinde/followers","following_url":"https://api.github.com/users/RaduBerinde/following{/other_user}","gists_url":"https://api.github.com/users/RaduBerinde/gists{/gist_id}","starred_url":"https://api.github.com/users/RaduBerinde/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RaduBerinde/subscriptions","organizations_url":"https://api.github.com/users/RaduBerinde/orgs","repos_url":"https://api.github.com/users/RaduBerinde/repos","events_url":"https://api.github.com/users/RaduBerinde/events{/privacy}","received_events_url":"https://api.github.com/users/RaduBerinde/received_events","type":"User","site_admin":false}],"milestone":null,"comments":6,"created_at":"2017-02-06T16:06:20Z","updated_at":"2017-02-09T00:36:43Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/13428","html_url":"https://github.com/cockroachdb/cockroach/pull/13428","diff_url":"https://github.com/cockroachdb/cockroach/pull/13428.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/13428.patch"},"body":"One major area of testing that needs to be covered is a test that brings up a cluster with a few nodes, creates a table with range leases spread across the cluster, and runs an index backfill. I'd like to add a test and a benchmark that does this. This seems like a test/framework that would be good for distsql in general.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/13428)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/278509507","html_url":"https://github.com/cockroachdb/cockroach/pull/13428#issuecomment-278509507","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13428","id":278509507,"user":{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.githubusercontent.com/u/16544120?v=3","gravatar_id":"","url":"https://api.github.com/users/RaduBerinde","html_url":"https://github.com/RaduBerinde","followers_url":"https://api.github.com/users/RaduBerinde/followers","following_url":"https://api.github.com/users/RaduBerinde/following{/other_user}","gists_url":"https://api.github.com/users/RaduBerinde/gists{/gist_id}","starred_url":"https://api.github.com/users/RaduBerinde/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RaduBerinde/subscriptions","organizations_url":"https://api.github.com/users/RaduBerinde/orgs","repos_url":"https://api.github.com/users/RaduBerinde/repos","events_url":"https://api.github.com/users/RaduBerinde/events{/privacy}","received_events_url":"https://api.github.com/users/RaduBerinde/received_events","type":"User","site_admin":false},"created_at":"2017-02-09T00:36:43Z","updated_at":"2017-02-09T00:36:43Z","body":"Great work, this is pretty cool!\n\nOn the planning part - I don't understand why we are creating this dummy `backfillNode`. We could just pass whatever we need to `createIndexBackfillers` and call that directly (it could even live in backfill.go). We can split `PlanAndRun` into the planning part and the running part so we can just run the `physicalPlan` returned by `createIndexBackfillers` directly.\n\n---\n\nReview status: 0 of 21 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 42 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV8Ja-jpJIMIieMp19:-KcV8Ja-jpJIMIieMp1A:bgkmim6) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L42)):*\n> ```Go\n> \t// a number of individual index row updates that can be scattered over\n> \t// many ranges.\n> \tindexBackfillChunkSize = 100\n> ```\n\nIMO this should be part of the `BackfillerSpec`, it should be up to whoever instantiates this processor to decide.\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 57 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV9J7ALtPTucuPHmsT:-KcV9J7ALtPTucuPHmsU:b-iepi0x) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L57)):*\n> ```Go\n> \toutputCols []int\n> \n> \t// Last row returned by the rowFetcher; it has one entry per table column.\n> ```\n\nincorrect comment\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 98 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV9SycjpIO_zu8G12f:-KcV9SycjpIO_zu8G12g:b47xgv3) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L98)):*\n> ```Go\n> }\n> \n> func (ib *indexBackfiller) init(outputCols []int) error {\n> ```\n\nCan you add a comment explaining what outputCols is and how it is encoded? (there is a distinction between values <= numCols and > numCols).\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 108 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVFRQqfSVP0LaNGpU2:-KcVFRQri0HHj2n-t0pg:buge5f1) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L108)):*\n> ```Go\n> \t\tcols = append(cols, desc.Columns...)\n> \t\tfor _, c := range ib.outputCols {\n> \t\t\tif c >= numCols+len(desc.Mutations) {\n> ```\n\nI would pull out this check to a separate loop before the`if` so we don't duplicate it below\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 126 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVA0s0bjwcBPGo3izg:-KcVA0s0bjwcBPGo3izh:b-5kxfoe) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L126)):*\n> ```Go\n> \n> \t// Figure out which columns we need: the output columns plus any other\n> \t// columns used by the filter expression.\n> ```\n\ncopy-paste-o in comment\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 134 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVEi-CPg47MFh5CRHZ:-KcVEi-CPg47MFh5CRH_:b278u3v) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L134)):*\n> ```Go\n> \t\t}\n> \t\tif c < numCols {\n> \t\t\tvalNeededForCol[c] = true\n> ```\n\nis this `c` ?\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 136 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVAJNMu-gftpzVBEk7:-KcVAJNMu-gftpzVBEk8:bd9cf5s) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L136)):*\n> ```Go\n> \t\t\tvalNeededForCol[c] = true\n> \t\t} else {\n> \t\t\tvalNeededForCol[numCols+idx] = true\n> ```\n\nSomething is off here. In one case we use the `outputCols[i]`, in the other we use a counter..\n\nI believe `valNeededForCol` corresponds 1-to-1 to `colIdxMap` so we should just set them all to true no?\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 157 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV7Ve4UWVFHwx4oPq7:-KcV7Ve4UWVFHwx4oPq8:b64w03f) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L157)):*\n> ```Go\n> \t}\n> \n> \toutRow := ib.rowAlloc.AllocRow(len(ib.outputCols))\n> ```\n\nDo we need to alloc a row every time, or could we reuse the same row?\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 227 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV7eLjp4U7q46xdVJR:-KcV7eLkKpQnnBO_0Ew8:bg8th68) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L227)):*\n> ```Go\n> \t\t\t\trowVals = make(parser.Datums, len(encRow))\n> \t\t\t}\n> \t\t\tvar da sqlbase.DatumAlloc\n> ```\n\nThe point of DatumAlloc is to amortize the cost of Datum allocation for many rows, it should be declared outside of the loop (or even in `ib`).\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 232 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV7o7uzOrUE-UCBweb:-KcV7o7uzOrUE-UCBwec:b-2w7ena) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L232)):*\n> ```Go\n> \t\t\t}\n> \n> \t\t\tif err := sqlbase.EncodeSecondaryIndexes(\n> ```\n\nIdeally we should make a version of this that takes `EncDatum`s. It would not only avoid allocations, but also decode-encode cycles in some cases (e.g. anything that is part of the PK). Similar to how the joinreader uses `MakeKeyFromEncDatums` (https://github.com/cockroachdb/cockroach/blob/cd8053b502ccab720fab07616acffd02ae0b69c1/pkg/sql/distsqlrun/joinreader.go#L100).\n\nYou can leave a TODO though.\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 250 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVCV8HHEYepix0FNYR:-KcVCV8HHEYepix0FNYS:b-y2z9wo) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L250)):*\n> ```Go\n> \t\t}\n> \t\t// Have we processed all the rows?\n> \t\tif done = numRows < chunkSize; done {\n> ```\n\nI would pull this out of the `if`, this syntax is usually to restrict the scope of something, but here it's the opposite, we are relying on `done`'s value afterward.\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 341 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVCu_qroiMsEfKtN35:-KcVCu_qroiMsEfKtN36:bxcf4tg) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L341)):*\n> ```Go\n> \t\t}\n> \n> \t\tfor i, sp := range tableDesc.Mutations[mutationIdx].ResumeSpan {\n> ```\n\nDefinitely need a comment explaining what we are doing here\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 349 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVDvNdxn6dkzepC-2H:-KcVDvNdxn6dkzepC-2I:b-nd2p0e) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L349)):*\n> ```Go\n> \t\t\t\tmaybeRemoveCheckpoint := func(idx int) {\n> \t\t\t\t\tif resume.Key == nil {\n> \t\t\t\t\t\ttableDesc.Mutations[mutationIdx].ResumeSpan =\n> ```\n\nwe should do a `m := &tableDesc.Mutations[mutationIdx]` at the beginning to avoid repeating this over and over\n\n---\n\n*[pkg/sql/distsqlrun/indexbackfiller.go, line 357 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVDjTRxXciO45Vq_R2:-KcVDjTRxXciO45Vq_R3:b-qxsw8u) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/indexbackfiller.go#L357)):*\n> ```Go\n> \t\t\t\t\ttableDesc.Mutations[mutationIdx].ResumeSpan =\n> \t\t\t\t\t\tappend(tableDesc.Mutations[mutationIdx].ResumeSpan[:i],\n> \t\t\t\t\t\t\tappend(spans, tableDesc.Mutations[mutationIdx].ResumeSpan[i+1:]...)...)\n> ```\n\nI would pull this inner `append` out.\n\n---\n\n*[pkg/sql/distsqlrun/processors.proto, line 329 at r7](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcV8el2IF5F61LToi5F:-KcV8el2IF5F61LToi5G:b-yyei13) ([raw file](https://github.com/cockroachdb/cockroach/blob/79b14d00843b04363a0b62315497ad02d679808c/pkg/sql/distsqlrun/processors.proto#L329)):*\n> ```protobuf\n> // A backfiller performs KV operations to retrieve rows for a table and\n> // backfills the new indexes/columns contained in the table descriptor.\n> message BackfillerSpec {\n> ```\n\nThe comment should also mention that the processor checkpoints its progress by updating the table descriptor in the database. Also, that it doesn't emit any rows and does not support any post-processing (by post-processing I mean stuff in `PostProcessSpec`).\n\n---\n\n*[pkg/sql/sqlbase/structured.proto, line 315 at r5](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-KcVD4VWr9mohhXf9krd:-KcVD4VWr9mohhXf9kre:b-cayz8s) ([raw file](https://github.com/cockroachdb/cockroach/blob/66ccadc29b11f3e4817279a092a477f804edc636/pkg/sql/sqlbase/structured.proto#L315)):*\n> ```protobuf\n>   // or deleting data. They occasionally checkpoint Spans so that the\n>   // processing can resume in the event of a node failure. \n>   repeated roachpb.Span resume_span = 6 [(gogoproto.nullable) = false];\n> ```\n\nNeed to mention exactly what the spans mean (areas of table that are not processed yet?). Are the spans non-overlapping?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/13428#-:-KcVGv9lIhUkFD78suP3:b-nsp9bx)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"RaduBerinde"}}