partition: make ExchangePartition follow check constraints(part1) #46021

jiyfhust · 2023-08-12T14:34:58Z

What problem does this PR solve?

Issue Number: close #45922

Problem Summary:

What is changed and how it works?

Part1:
only verify records by check constraints.
prohibiting insert or update records which not satisfy check constraints is not included.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2023-08-12T14:35:08Z

Hi @jiyfhust. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiprow · 2023-08-12T14:35:23Z

Hi @jiyfhust. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mjonss · 2023-08-16T19:28:10Z

/ok-to-test

codecov · 2023-08-16T19:48:19Z

Codecov Report

Merging #46021 (c57a3a2) into master (8b15111) will increase coverage by 0.3514%.
Report is 50 commits behind head on master.
The diff coverage is 85.5172%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46021        +/-   ##
================================================
+ Coverage   73.0847%   73.4362%   +0.3514%     
================================================
  Files          1335       1359        +24     
  Lines        397994     412370     +14376     
================================================
+ Hits         290873     302829     +11956     
- Misses        88382      90995      +2613     
+ Partials      18739      18546       -193

Flag	Coverage Δ
integration	`32.4563% <7.5862%> (?)`
unit	`73.7961% <85.5172%> (+0.7113%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9913% <ø> (ø)`
parser	`85.3266% <ø> (+0.3394%)`	⬆️
br	`49.4444% <ø> (-3.4981%)`	⬇️

jiyfhust · 2023-08-17T00:38:42Z

/test check-dev

tiprow · 2023-08-17T00:39:14Z

@jiyfhust: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jiyfhust · 2023-08-20T12:26:30Z

It is ready for review now. @mjonss

ddl/partition.go

mjonss · 2023-08-16T19:47:28Z

ddl/partition.go

+	pCons := getWriteableConstraintExpr(pt.Constraints)
+	nCons := getWriteableConstraintExpr(nt.Constraints)
+	if len(pCons) > 0 {
+		if err := verifyFunc(nschemaName, nt.Name.L, "", pCons); err != nil {


Would it be possible to combine this with the verification in checkExchangePartitionRecordValidation so we can do a single scan of the non-partitioned table, and fail at the first row that neither match the partition or the partitioned tables condition?

ddl/tests/partition/db_partition_test.go

ddl/partition.go

jiyfhust · 2023-09-05T00:44:37Z

PTAL @mjonss

mjonss · 2023-09-13T16:06:48Z

ddl/partition.go

@@ -2446,6 +2453,21 @@ func (w *worker) onExchangeTablePartition(d *ddlCtx, t *meta.Meta, job *model.Jo
 			job.State = model.JobStateRollingback
 			return ver, errors.Trace(err)
 		}
+		if variable.EnableCheckConstraint.Load() {


I would prefer if the check is combined with the checkExchangePartitionRecordValidation above, to only do a single scan of the non-partitioned table.

jiyfhust · 2023-09-16T06:52:47Z

/retest

jiyfhust · 2023-09-18T04:07:32Z

/retest

mjonss

Please create a followup issue, to test null handling in validation, since I assume there may be hidded issues in non-list cases?

mjonss · 2023-09-20T21:20:58Z

ddl/partition.go

+			buf.WriteString("mod(")
+			buf.WriteString(pi.Expr)
+			buf.WriteString(", %?) != %?")


Are there any tests that validates NULLs for Hash partitions? (not blocking this PR, just curious)

mjonss · 2023-09-20T22:06:33Z

ddl/partition.go

+	if variable.EnableCheckConstraint.Load() {
+		ncc, ok := ntbl.(CheckConstraintTable)
+		if !ok {
+			return errors.Errorf("exchange partition process assert table partition failed")
+		}
+		nCons := ncc.WritableConstraint()
+		if len(nCons) > 0 {
+			buf.Reset()
+			buf.WriteString("select 1 from %n.%n partition(%n) where ")
+			buf.WriteString(genConstraintCondition(nCons))
+			buf.WriteString(" limit 1")
+			err = verifyFunc(buf.String(), pschemaName, pt.Name.L, partitionName)
+			if err != nil {
+				return errors.Trace(err)
+			}
+		}


More of a note, a future improvement may be to run the validations in parallel.

ti-chi-bot · 2023-09-21T06:58:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, tiancaiamao

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:

~~OWNERS~~ [mjonss,tiancaiamao]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2023-09-21T06:58:22Z

[LGTM Timeline notifier]

Timeline:

2023-09-19 17:07:33.913456549 +0000 UTC m=+623619.881044600: ☑️ agreed by tiancaiamao.
2023-09-21 06:58:21.491198577 +0000 UTC m=+759867.458786626: ☑️ agreed by mjonss.

jiyfhust · 2023-09-21T08:20:00Z

Please create a followup issue, to test null handling in validation, since I assume there may be hidded issues in non-list cases?

Issue created #47167.

jiyfhust · 2023-09-21T08:24:36Z

/retest

jiyfhust · 2023-09-22T03:04:24Z

/retest

wuhuizuo · 2023-09-22T03:26:08Z

/retest

make ExchangePartition follow check constraints

c963c7c

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed labels Aug 12, 2023

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 12, 2023

fix

9eaee22

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 16, 2023

fix

865aa53

mjonss reviewed Aug 20, 2023

View reviewed changes

jiyfhust added 2 commits August 21, 2023 13:05

fix

5e7a1b9

fix

35e06c9

jiyfhust requested a review from mjonss August 31, 2023 11:02

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Sep 13, 2023

mjonss reviewed Sep 13, 2023

View reviewed changes

Merge branch 'pingcap:master' into exchangepartition_check_part1

261500f

ti-chi-bot bot added do-not-merge/needs-tests-checked and removed do-not-merge/needs-tests-checked labels Sep 15, 2023

fix

f9e53cd

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 16, 2023

fix

d47ff82

fix

c57a3a2

tiancaiamao approved these changes Sep 19, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 19, 2023

mjonss approved these changes Sep 21, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 21, 2023

jiyfhust mentioned this pull request Sep 21, 2023

Exchange List Partition add validition check tests for null value handling and default partition #47167

Closed

ti-chi-bot bot merged commit cfa9ffd into pingcap:master Sep 22, 2023

jiyfhust deleted the exchangepartition_check_part1 branch September 22, 2023 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

partition: make ExchangePartition follow check constraints(part1) #46021

partition: make ExchangePartition follow check constraints(part1) #46021

jiyfhust commented Aug 12, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 12, 2023

tiprow bot commented Aug 12, 2023

mjonss commented Aug 16, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

jiyfhust commented Aug 17, 2023

tiprow bot commented Aug 17, 2023

jiyfhust commented Aug 20, 2023

mjonss Aug 16, 2023

jiyfhust commented Sep 5, 2023

mjonss Sep 13, 2023

jiyfhust commented Sep 16, 2023

jiyfhust commented Sep 18, 2023

mjonss left a comment

mjonss Sep 20, 2023

mjonss Sep 20, 2023

ti-chi-bot bot commented Sep 21, 2023

ti-chi-bot bot commented Sep 21, 2023

jiyfhust commented Sep 21, 2023

jiyfhust commented Sep 21, 2023

jiyfhust commented Sep 22, 2023

wuhuizuo commented Sep 22, 2023

partition: make ExchangePartition follow check constraints(part1) #46021

partition: make ExchangePartition follow check constraints(part1) #46021

Conversation

jiyfhust commented Aug 12, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Aug 12, 2023

tiprow bot commented Aug 12, 2023

mjonss commented Aug 16, 2023

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

jiyfhust commented Aug 17, 2023

tiprow bot commented Aug 17, 2023

jiyfhust commented Aug 20, 2023

mjonss Aug 16, 2023

Choose a reason for hiding this comment

jiyfhust commented Sep 5, 2023

mjonss Sep 13, 2023

Choose a reason for hiding this comment

jiyfhust commented Sep 16, 2023

jiyfhust commented Sep 18, 2023

mjonss left a comment

Choose a reason for hiding this comment

mjonss Sep 20, 2023

Choose a reason for hiding this comment

mjonss Sep 20, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 21, 2023

ti-chi-bot bot commented Sep 21, 2023

[LGTM Timeline notifier]

jiyfhust commented Sep 21, 2023

jiyfhust commented Sep 21, 2023

jiyfhust commented Sep 22, 2023

wuhuizuo commented Sep 22, 2023

jiyfhust commented Aug 12, 2023 •

edited

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading