Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plan: get correct columns #5818

Merged
merged 5 commits into from Feb 11, 2018

Conversation

Projects
None yet
4 participants
@zimulala
Copy link
Member

zimulala commented Feb 7, 2018

Use the correct columns in PhysicalIndexScan's ToPB function.
I need use the same columns in function buildDataSource and ToPB.

@zimulala zimulala added the status/WIP label Feb 7, 2018

@zimulala zimulala removed the status/WIP label Feb 8, 2018

@zimulala

This comment has been minimized.

Copy link
Member Author

zimulala commented Feb 8, 2018

@zimulala zimulala referenced this pull request Feb 8, 2018

Closed

*: Add debug log #5807

@shenli shenli added the priority/P1 label Feb 8, 2018

@@ -114,6 +114,9 @@ type TableInfo struct {
// Now it only uses for compatibility with the old version that already uses this field.
OldSchemaID int64 `json:"old_schema_id,omitempty"`

publicColumns []*ColumnInfo
writableColumns []*ColumnInfo

This comment has been minimized.

Copy link
@coocood

coocood Feb 8, 2018

Member

The two fields are not needed.

@@ -172,6 +175,45 @@ func (t *TableInfo) GetPkColInfo() *ColumnInfo {
return nil
}

// Cols returns the columns of the table in public state.
func (t *TableInfo) Cols() []*ColumnInfo {
if len(t.publicColumns) > 0 {

This comment has been minimized.

Copy link
@shenli

shenli Feb 8, 2018

Member

Will this field be modified by multiple goroutines?

func (s *testStateChangeSuite) TestUpdateOrDelete(c *C) {
sqls := make([]string, 2)
sqls[0] = "delete from t where c2 = 'a'"
sqls[1] = "update t set c2 = 'c2_update' where c2 = 'a'"

This comment has been minimized.

Copy link
@shenli

shenli Feb 8, 2018

Member

Will this update use index scan?

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 8, 2018

Author Member

Yes.
The delete SQL can't use index in a statement directly, so these two statements use drop state table to have the index scan.

This comment has been minimized.

Copy link
@coocood

coocood Feb 8, 2018

Member

We can add index hint to update statement.

"update t use index(c2) set c2 = 'c2_update' where c2 = 'a'"

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 9, 2018

Author Member

So how to deal with delete statement?

@zimulala

This comment has been minimized.

Copy link
Member Author

zimulala commented Feb 8, 2018

/run-all-tests tidb-test=release-1.0 tidb-private-test=release-1.0 tikv=release-1.0

@zimulala

This comment has been minimized.

Copy link
Member Author

zimulala commented Feb 8, 2018

_, err = se.Execute("use test_db_state")
c.Assert(err, IsNil)
callback.OnJobUpdatedExported = func(job *model.Job) {
if job.SchemaState == prevState || checkErr != nil || times >= 3 {

This comment has been minimized.

Copy link
@coocood

coocood Feb 8, 2018

Member

Why limit the times?

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 9, 2018

Author Member

It is used for excluding the effects of other DDL statements.
Make the testing stable.

@coocood

This comment has been minimized.

Copy link
Member

coocood commented Feb 8, 2018

The original problem is the mismatch of the column position in slice and the column offset.

A cleaner way to fix this issue is to change createColumnInfo in ddl/column.go, we can append the non-public column to the end first, and when the column is public, change the position and offset of the newly added column.

@coocood

This comment has been minimized.

Copy link
Member

coocood commented Feb 8, 2018

If we append the new column to the end in non-public state, we don't need this PR #3754

@@ -89,14 +89,18 @@ func (p *PhysicalTableScan) ToPB(ctx context.Context) (*tipb.Executor, error) {
// ToPB implements PhysicalPlan ToPB interface.
func (p *PhysicalIndexScan) ToPB(ctx context.Context) (*tipb.Executor, error) {
columns := make([]*model.ColumnInfo, 0, p.schema.Len())
tableColumns := p.Table.Cols()
if ctx.GetSessionVars().StmtCtx.InUpdateStmt {

This comment has been minimized.

Copy link
@coocood

coocood Feb 8, 2018

Member

We don't need to check it and get writable columns because all the columns in an index must be public.

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 9, 2018

Author Member

Yes, now the column in an index must be public.

@@ -48,6 +48,7 @@ func (s *testStateChangeSuite) SetUpSuite(c *C) {
s.store, err = tikv.NewMockTikvStore()
c.Assert(err, IsNil)
tidb.SetSchemaLease(s.lease)

This comment has been minimized.

Copy link
@winkyao

winkyao Feb 9, 2018

Member

remove line 50?

This comment has been minimized.

Copy link
@zimulala

zimulala Feb 9, 2018

Author Member

Why?

This comment has been minimized.

Copy link
@winkyao

winkyao Feb 9, 2018

Member

never mind, I read it wrong.

@zimulala

This comment has been minimized.

Copy link
Member Author

zimulala commented Feb 9, 2018

@coocood
I see. But the PR change a little. If update createColumnInfo, I'm afraid the impact of larger. I hope to solve this bug first, then change createColumnInfo.

@coocood

This comment has been minimized.

Copy link
Member

coocood commented Feb 9, 2018

LGTM

@winkyao
Copy link
Member

winkyao left a comment

LGTM

@winkyao

This comment has been minimized.

Copy link
Member

winkyao commented Feb 9, 2018

@shenli PTAL

@@ -172,6 +172,22 @@ func (t *TableInfo) GetPkColInfo() *ColumnInfo {
return nil
}

// Cols returns the columns of the table in public state.
func (t *TableInfo) Cols() []*ColumnInfo {

This comment has been minimized.

Copy link
@shenli

shenli Feb 9, 2018

Member

The implementation is not efficient. Could you do some pre-calculation?

@shenli

This comment has been minimized.

Copy link
Member

shenli commented Feb 11, 2018

LGTM
We need to optimize this latter.

@shenli

shenli approved these changes Feb 11, 2018

@shenli shenli merged commit fafc39d into pingcap:release-1.0 Feb 11, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on release-1.0 at 72.469%
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.