executor: ignore foreign key error in UPDATE/INSERT/DELETE ignore
#56682
Conversation
ti-chi-bot
bot
added
release-note-none
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #56682 +/- ##
=================================================
- Coverage 73.3422% 56.6417% -16.7005%
=================================================
Files 1630 1761 +131
Lines 450364 640407 +190043
=================================================
+ Hits 330307 362738 +32431
- Misses 99779 253438 +153659
- Partials 20278 24231 +3953
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/executor/delete.go
Outdated
| if e.ignoreErr { | ||
| fkToBeCheckedRows[0] = toBeCheckedRow{row: datumRow, ignored: false} | ||
| err := checkFKIgnoreErr(ctx, e.Ctx(), e.fkChecks[tbl.Meta().ID], fkToBeCheckedRows) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // meets an error, skip this row. | ||
| if fkToBeCheckedRows[0].ignored { | ||
| datumRow = datumRow[:0] | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
I prefer to keep it outside. If we move it inside the removeRow, we'll need to handle the error carefully to distinguish the error, the ignored error and success. (And also, these logic are also duplicated in insert / update, and it's also not easy to have an easy-to-understand abstration for all of them).
Instead, I change the declaration / implementation of checkFKIgnoreErr to integrate the declaration and usage of the fkToBeCheckedRows[0] usage, and make it easier to understand / use:
func checkFKIgnoreErr(ctx context.Context, sctx sessionctx.Context, fkChecks []*FKCheckExec, row []types.Datum) (bool, error)Now, only the row is needed, and it'll return whether an error has been ignored, or there is an unignorable error. The usage can be simpler:
if e.ignoreErr {
ignored, err := checkFKIgnoreErr(ctx, e.Ctx(), e.fkChecks[tbl.Meta().ID], datumRow)
if err != nil {
return err
}
// meets an error, skip this row.
if ignored {
datumRow = datumRow[:0]
continue
}
}
| if e.ignoreErr { | ||
| ignored, err := checkFKIgnoreErr(ctx, e.Ctx(), e.fkChecks[id], val.handleVal) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
If return from here, will ignore the error?
There was a problem hiding this comment.
Yes. It's expected. See the implementation of checkFKIgnoreErr and fkc.checkRows. If the row breaks the foreign key constraint, a warning will be appended and the function checkFKIgnoreErr will return true, nil. If the row has a wrong format, or it meets some network issues (or other errors not related to the foreign key), it'll return the error as false, err.
Previously, we already have an existing implementation of INSERT ignore. This PR keeps the same behavior with it.
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crazycs520, hawkingrei The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #56678, close #56681, close #39712
Problem Summary:
In the following cases, the foreign key error should be omitted and placed in the warning:
INSERT IGNORE ... ON DUPLICATE UPDATE ...DELETE IGNORE ...UPDATE IGNORE ...What changed and how does it work?
Introduce a function
checkFKIgnoreErrto check rows explicitly, and use this function to check each rows in these three cases.Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.