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

ddl: migrate part of ddl package code from Execute/ExecRestricted to safe API (1) #22670

Merged
merged 17 commits into from
Feb 5, 2021

Conversation

AilinKid
Copy link
Contributor

@AilinKid AilinKid commented Feb 2, 2021

Signed-off-by: AilinKid 314806019@qq.com

What problem does this PR solve?

Issue Number: part close #pingcap/tidb-test#1152

What is changed and how it works?

What's Changed:

Migrate part of code in ddl package from using Execute/ExecRestricted to safe API

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

  • ddl: migrate part of ddl package code from Execute/ExecRestricted to safe API (1).

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Feb 2, 2021
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need changes.

ddl/util/util.go Outdated Show resolved Hide resolved
ddl/util/util.go Outdated
recordDoneDeletedRangeSQL = `INSERT IGNORE INTO mysql.gc_delete_range_done SELECT * FROM mysql.gc_delete_range WHERE job_id = %? AND element_id = %?`
completeDeleteRangeSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %? AND element_id = %?`
completeDeleteMultiRangesSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %? AND element_id in (%?)`
updateDeleteRangeSQL = `UPDATE mysql.gc_delete_range SET start_key = "%?" WHERE job_id = %? AND element_id = %? AND start_key = "%?"`
Copy link
Contributor

@xhebox xhebox Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the quotes around %?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

ddl/util/util.go Outdated Show resolved Hide resolved
ddl/column.go Outdated
sql := fmt.Sprintf("select 1 from `%s`.`%s` where %s limit 1;", schema.L, table.L, colsStr)
rows, _, err := ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(sql)
sql := "select 1 from %n.%n where " + colsStr + " limit 1;"
stmt, err := ctx.(sqlexec.RestrictedSQLExecutor).ParseWithParams(context.Background(), sql, paramsList...)
Copy link
Contributor

@xhebox xhebox Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not considered safe to use concat and ParseWithParams at the same time. Though this case is safe.

fmt.Sprintf(fmt.Sprintf("%s", "%s")) will report an error.

You may want to check this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good~
the PR link is not right

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, here it is: 22674

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest that you can use fmt.Fprintf.

Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
Signed-off-by: AilinKid <314806019@qq.com>
ddl/util/util.go Outdated
completeDeleteMultiRangesSQL := completeDeleteMultiRangesSQLPrefix + idList + completeDeleteMultiRangesSQLSuffix
_, err := ctx.(sqlexec.SQLExecutor).ExecuteInternal(context.TODO(), completeDeleteMultiRangesSQL, paramIDs...)
var buf strings.Builder
_, err := fmt.Fprintf(&buf, completeDeleteMultiRangesSQL, idList)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just let you know, we have fmt.Fprintf in our errcheck_ignore lists. And I agreed with that. fmt.Fprintf can not throw error on valid input, unless OOM. So you do not need to check that error.

BTW, You could use Fprintf to concat idList & nameList too.

ddl/column.go Outdated Show resolved Hide resolved
ddl/testutil/testutil.go Outdated Show resolved Hide resolved
ddl/util/util.go Outdated Show resolved Hide resolved
.
Signed-off-by: AilinKid <314806019@qq.com>
.
Signed-off-by: AilinKid <314806019@qq.com>
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 4, 2021
@AilinKid
Copy link
Contributor Author

AilinKid commented Feb 4, 2021

/run-all-tests

@AilinKid
Copy link
Contributor Author

AilinKid commented Feb 4, 2021

/run-integration-copr-test

@AilinKid AilinKid requested review from morgo and removed request for morgo February 5, 2021 03:48
@AilinKid
Copy link
Contributor Author

AilinKid commented Feb 5, 2021

ptal @tiancaiamao

@morgo
Copy link
Contributor

morgo commented Feb 5, 2021

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 5, 2021
@AilinKid
Copy link
Contributor Author

AilinKid commented Feb 5, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 5, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 75f7485 into pingcap:master Feb 5, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 25, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0-rc in PR #22928

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 25, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #22929

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Feb 25, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #22930

ti-chi-bot pushed a commit that referenced this pull request Feb 25, 2021
AilinKid added a commit to AilinKid/tidb that referenced this pull request Mar 3, 2021
…safe API (1) (pingcap#22670)

Signed-off-by: AilinKid <314806019@qq.com>
ti-chi-bot pushed a commit that referenced this pull request Mar 3, 2021
ti-chi-bot pushed a commit that referenced this pull request Mar 5, 2021
ti-chi-bot pushed a commit that referenced this pull request Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants