sql: move schema change backfill logic to sql/backfill by vivekmenezes · Pull Request #25362 · cockroachdb/cockroach

vivekmenezes · 2018-05-08T19:46:36Z

The distsql backfill calls the sql/backfill code.

The future plan is to also call backfill within a transaction
that runs schema changes in the same transaction as a
CREATE TABLE. Since the CREATE TABLE in a transaction
is not visible to other transactions until the transaction
commits, the schema change can run the backfill without
incrementing the version of the table descriptor.

related to #24626

Release note: None

cockroach-teamcity · 2018-05-08T19:46:43Z

This change is

andreimatei · 2018-05-09T23:21:05Z

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved.

pkg/sql/sqlbase/backfill.go, line 24 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/internal/client"
	"github.com/cockroachdb/cockroach/pkg/roachpb"
	"github.com/cockroachdb/cockroach/pkg/sql/scrub"

Depending on SQL/scrub from sqlbase seems funky to me. You sure sqlbase is the right package for this? Wouldn't a new package be better?

Comments from Reviewable

vivekmenezes · 2018-05-10T11:37:14Z

it's really a refactor to allow use of distsql processor code on a local execution path. One option is to create sql/processors and put it there?

madelynnblue · 2018-05-14T17:06:39Z

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion.

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

		encRow, _, _, err := ib.fetcher.NextRow(ctx)
		if err != nil {
			err = scrub.UnwrapScrubError(err)

To address andrei's comment: I think we can just outright remove this check, and thus the package import. It was included in all calls of the above function (fetcher.NextRow) but we know that we are never doing anything scrub related in the backfiller, and thus can never produce that error.

Comments from Reviewable

vivekmenezes · 2018-05-14T18:55:39Z

I've moved the common backfill code to sql/backfill and have gotten rid of the scrub dependency.

The distsql backfill calls the sql/backfill code. The future plan is to also call backfill within a transaction that runs schema changes in the same transaction as a CREATE TABLE. Since the CREATE TABLE in a transaction is not visible to other transactions until the transaction commits, the schema change can run the backfill without incrementing the version of the table descriptor and running the schema change state machine. related to cockroachdb#24626 Release note: None

vivekmenezes · 2018-05-14T20:35:00Z

bors r+

25362: sql: move schema change backfill logic to sql/backfill r=vivekmenezes a=vivekmenezes The distsql backfill calls the sql/backfill code. The future plan is to also call backfill within a transaction that runs schema changes in the same transaction as a CREATE TABLE. Since the CREATE TABLE in a transaction is not visible to other transactions until the transaction commits, the schema change can run the backfill without incrementing the version of the table descriptor. related to #24626 Release note: None 25476: acceptance: remove Finagle test r=a-robinson a=tschottdorf It does do away with the issue #8332, though I'm avoiding the Github keyword to close it to test whether bors will still auto-close the issue due to the mention in the actual commit. Release note: None Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com> Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>

craig · 2018-05-14T21:06:04Z

Build succeeded

GitHub CI (Cockroach)

24883: dep: Bump grpc-go version r=a-robinson a=a-robinson And pull in new packages -- in particular, the encoding/proto package isn't needed for compilation but is needed at runtime. Release note: None --------------------- We should wait to merge this until they've cut the 1.12 release tag so that we aren't just at an arbitrary commit in their git history but I'm sending out the PR now so that I (or whoever would have done this) don't have to deal with debugging the missing encoding/proto package when it comes time to merge this. As tested in #17370 (comment), this gives a 5-10% boost in whole-cluster throughput and improved tail latencies when run with a highly concurrent workload. It appears to have little performance effect for lower-concurrency workloads. 25410: sql: run schema changes after CREATE TABLE in txn r=vivekmenezes a=vivekmenezes Includes a commit from #25362 and should be reviewed after that change. 25612: util: fix `retry.WithMaxAttempts` context cancelled before run. r=windchan7 a=windchan7 If context gets cancelled right after `retry.WithMaxAttempts` runs, the function passed to it will never gets run. Now `retry.WithMaxAttempts` will at least run the function once otherwise an error will be returned. Making this change because there are places such as `show_cluster_setting.go` require the passed in function to be run at least once. Otherwise there will be seg fault. Fixes: #25600. Fixes: #25603. Fixes: #25570. Fixes: #25567. Fixes: #25566. Fixes: #25511. Fixes: #25485. Release note: None 25625: storage: Adding testing knob to disable automatic lease renewals r=a-robinson a=a-robinson In order to fix the test flakes caused by automatic lease renewals Fixes #25537 Fixes #25540 Fixes #25568 Fixes #25573 Fixes #25576 Fixes #25589 Fixes #25594 Fixes #25599 Fixes #25605 Fixes #25620 Release note: None Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com> Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com> Co-authored-by: Victor Chen <victor@cockroachlabs.com>

vivekmenezes requested review from a team and madelynnblue May 8, 2018 19:46

vivekmenezes force-pushed the backfill branch 2 times, most recently from 15cec60 to 5145f74 Compare May 9, 2018 01:28

vivekmenezes force-pushed the backfill branch 2 times, most recently from c2000ba to d8488b3 Compare May 10, 2018 15:22

vivekmenezes mentioned this pull request May 10, 2018

sql: run schema changes after CREATE TABLE in txn #25410

Merged

vivekmenezes force-pushed the backfill branch from d8488b3 to 26dab8d Compare May 14, 2018 18:42

vivekmenezes requested a review from a team May 14, 2018 18:42

madelynnblue approved these changes May 14, 2018

View reviewed changes

vivekmenezes force-pushed the backfill branch from 26dab8d to 614b198 Compare May 14, 2018 20:01

vivekmenezes changed the title ~~sql: move schema change backfill logic to sqlbase~~ sql: move schema change backfill logic to sql/backfill May 14, 2018

craig bot merged commit 614b198 into cockroachdb:master May 14, 2018

vivekmenezes deleted the backfill branch May 15, 2018 01:18

gpaul mentioned this pull request Sep 19, 2018

sql: explicit index added by ORM after FK creates redundant index with the FK's _auto_ index #30402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: move schema change backfill logic to sql/backfill#25362

sql: move schema change backfill logic to sql/backfill#25362
craig[bot] merged 1 commit intocockroachdb:masterfrom
vivekmenezes:backfill

vivekmenezes commented May 8, 2018 •

edited

Loading

Uh oh!

cockroach-teamcity commented May 8, 2018

Uh oh!

andreimatei commented May 9, 2018

Uh oh!

vivekmenezes commented May 10, 2018

Uh oh!

madelynnblue commented May 14, 2018

Uh oh!

vivekmenezes commented May 14, 2018

Uh oh!

vivekmenezes commented May 14, 2018

Uh oh!

craig bot commented May 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

vivekmenezes commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented May 8, 2018

Uh oh!

andreimatei commented May 9, 2018

Uh oh!

vivekmenezes commented May 10, 2018

Uh oh!

madelynnblue commented May 14, 2018

Uh oh!

vivekmenezes commented May 14, 2018

Uh oh!

vivekmenezes commented May 14, 2018

Uh oh!

craig bot commented May 14, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vivekmenezes commented May 8, 2018 •

edited

Loading