ddl, planner: avoid running queries on the non-Public state table #55891

YangKeao · 2024-09-06T01:43:51Z

What problem does this PR solve?

Issue Number: close #55813

Problem Summary:

When the table is in WriteOnly state (and hasn't been synchronized to other tidb instances), it'll cause inconsistency:

WriteOnly TiDB	None TiDB
begin
insert into child values(1,1)
	begin
	delete from parent where id=1; // it blocks until prev txn commit
commit
	commit
select * from child; // we can see 1 row
	select * from parent;// empty result

What changed and how does it work?

Make the table invisible when it's in WriteOnly/DeleteOnly state. Previously, the users can see/write/delete any data for any table without considering the state. I think it's because the key range of the table is not affected by newly written things, so the DROP TABLE always has a good behavior.

In conclusion, the behavior of foreign key in WriteOnly(orDeleteOnly` is that:

The table is not visible for any explicit DML (or any query? Maybe only DML is enough 🤔, but make it always invisible is clearer).
The table will be modified by implicit modification caused by ON DELETE CASCADE or ON UPDATE CASCADE

For more detail, see #55813

This PR also modified the state from WriteOnly to DeleteOnly. Both state have the same effect, but using DeleteOnly is more suitable considering the semantic.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

TODO

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

codecov · 2024-09-06T01:55:44Z

Codecov Report

Attention: Patch coverage is 83.33333% with 9 lines in your changes missing coverage. Please review.

Project coverage is 57.0961%. Comparing base (c32c7b2) to head (6f8d876).
Report is 3 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55891         +/-   ##
=================================================
- Coverage   73.3374%   57.0961%   -16.2414%     
=================================================
  Files          1638       1789        +151     
  Lines        453738     640626     +186888     
=================================================
+ Hits         332760     365773      +33013     
- Misses       100578     249754     +149176     
- Partials      20400      25099       +4699

Flag	Coverage Δ
integration	`39.4287% <59.2592%> (?)`
unit	`72.5585% <83.3333%> (+0.0051%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.1458% <ø> (+17.1469%)`	⬆️

YangKeao · 2024-09-06T01:58:02Z

/hold

I'm doing some manual test.

YangKeao · 2024-09-09T09:05:34Z

Another concern is that: when the table is in WriteOnly state, should it show in the show tables? The current behavior is that it'll show in the show tables, and I didn't change the behavior in this PR.

YangKeao · 2024-09-09T09:06:09Z

/retest

YangKeao · 2024-09-10T06:44:50Z

/unhold

YangKeao · 2024-09-10T15:39:01Z

/retest

crazycs520 · 2024-09-10T15:54:59Z

pkg/planner/core/preprocess.go

@@ -1634,6 +1634,10 @@ func (p *preprocessor) handleTableName(tn *ast.TableName) {
 			return
 		}
 	}
+	if err := p.checkTableVisible(tableInfo); err != nil {


I am not sure about this modification, please ask the DDL owner to take a look.

REST LGTM.

YangKeao · 2024-09-26T08:53:20Z

/retest

YangKeao · 2024-09-26T14:31:22Z

/retest

ti-chi-bot · 2024-09-27T06:07:55Z

[LGTM Timeline notifier]

Timeline:

2024-09-25 11:18:49.51997864 +0000 UTC m=+1651199.260402579: ☑️ agreed by wjhuang2016.
2024-09-27 06:07:54.302597583 +0000 UTC m=+1805344.043021522: ☑️ agreed by tiancaiamao.

YangKeao · 2024-10-08T15:43:20Z

/retest

YangKeao · 2024-10-12T06:23:37Z

/test pull-br-integration-test

tiprow · 2024-10-12T06:23:43Z

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

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-br-integration-test

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-sigs/prow repository.

pkg/planner/core/preprocess.go

Signed-off-by: Yang Keao <[email protected]>

ti-chi-bot · 2024-10-29T08:29:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, hawkingrei, tiancaiamao, winoros, wjhuang2016

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~~ [AilinKid,hawkingrei,tiancaiamao,winoros,wjhuang2016]
~~pkg/ddl/OWNERS~~ [wjhuang2016]
~~pkg/infoschema/OWNERS~~ [wjhuang2016]
~~pkg/meta/OWNERS~~ [wjhuang2016]
~~pkg/planner/OWNERS~~ [AilinKid,hawkingrei,winoros]

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 bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed sig/planner SIG: Planner size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2024

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/needs-triage-completed labels Sep 6, 2024

YangKeao force-pushed the fix-55813 branch from cfaa8ff to 870b5ae Compare September 9, 2024 09:06

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Sep 10, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2024

YangKeao force-pushed the fix-55813 branch 2 times, most recently from 6ad00e8 to 8a2368b Compare September 10, 2024 10:23

YangKeao changed the title ~~ddl, planner: avoid running queries on the WriteOnly state table~~ ddl, planner: avoid running queries on the non-Public state table Sep 10, 2024

YangKeao force-pushed the fix-55813 branch 3 times, most recently from f45e8f6 to 551b811 Compare September 10, 2024 13:36

crazycs520 reviewed Sep 10, 2024

View reviewed changes

YangKeao requested a review from wjhuang2016 September 11, 2024 06:33

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2024

YangKeao force-pushed the fix-55813 branch 3 times, most recently from 1e665ef to 91702c1 Compare September 12, 2024 07:32

ti-chi-bot bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 12, 2024

YangKeao force-pushed the fix-55813 branch from 3280646 to a4ccd29 Compare September 26, 2024 12:29

tiancaiamao approved these changes Sep 27, 2024

View reviewed changes

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

YangKeao requested a review from 0xPoe September 27, 2024 06:50

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2024

YangKeao mentioned this pull request Oct 8, 2024

Foreign Key GA #55861

Open

17 tasks

YangKeao force-pushed the fix-55813 branch from a4ccd29 to 40c625e Compare October 8, 2024 08:11

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2024

YangKeao force-pushed the fix-55813 branch from 40c625e to 9a7c093 Compare October 29, 2024 05:44

hawkingrei self-requested a review October 29, 2024 07:14

winoros reviewed Oct 29, 2024

View reviewed changes

pkg/planner/core/preprocess.go Show resolved Hide resolved

YangKeao requested a review from winoros October 29, 2024 07:24

YangKeao added 4 commits October 29, 2024 15:24

avoid running queries on the WriteOnly state table

5ea7913

Signed-off-by: Yang Keao <[email protected]>

include DeleteOnly/WriteOnly state table in the infoschema

b3426f9

Signed-off-by: Yang Keao <[email protected]>

don't return tables in non-Public state in information_schema

1256780

Signed-off-by: Yang Keao <[email protected]>

check visible in tryLockMDLAndUpdateSchemaIfNecessary

6f8d876

Signed-off-by: Yang Keao <[email protected]>

YangKeao force-pushed the fix-55813 branch from 9a7c093 to 6f8d876 Compare October 29, 2024 07:24

winoros approved these changes Oct 29, 2024

View reviewed changes

ti-chi-bot bot added the approved label Oct 29, 2024

hawkingrei approved these changes Oct 29, 2024

View reviewed changes

AilinKid approved these changes Oct 29, 2024

View reviewed changes

ti-chi-bot bot merged commit e1c44f7 into pingcap:master Oct 29, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ddl, planner: avoid running queries on the non-Public state table #55891

ddl, planner: avoid running queries on the non-Public state table #55891

Uh oh!

YangKeao commented Sep 6, 2024 •

edited

Loading

Uh oh!

codecov bot commented Sep 6, 2024 •

edited

Loading

Uh oh!

YangKeao commented Sep 6, 2024

Uh oh!

YangKeao commented Sep 9, 2024

Uh oh!

YangKeao commented Sep 9, 2024

Uh oh!

YangKeao commented Sep 10, 2024

Uh oh!

YangKeao commented Sep 10, 2024

Uh oh!

crazycs520 Sep 10, 2024

Uh oh!

YangKeao commented Sep 26, 2024

Uh oh!

YangKeao commented Sep 26, 2024

Uh oh!

ti-chi-bot bot commented Sep 27, 2024

Uh oh!

YangKeao commented Oct 8, 2024

Uh oh!

YangKeao commented Oct 12, 2024

Uh oh!

tiprow bot commented Oct 12, 2024

Uh oh!

Uh oh!

ti-chi-bot bot commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

ddl, planner: avoid running queries on the non-Public state table #55891

ddl, planner: avoid running queries on the non-Public state table #55891

Uh oh!

Conversation

YangKeao commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

codecov bot commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

YangKeao commented Sep 6, 2024

Uh oh!

YangKeao commented Sep 9, 2024

Uh oh!

YangKeao commented Sep 9, 2024

Uh oh!

YangKeao commented Sep 10, 2024

Uh oh!

YangKeao commented Sep 10, 2024

Uh oh!

crazycs520 Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

YangKeao commented Sep 26, 2024

Uh oh!

YangKeao commented Sep 26, 2024

Uh oh!

ti-chi-bot bot commented Sep 27, 2024

[LGTM Timeline notifier]

Uh oh!

YangKeao commented Oct 8, 2024

Uh oh!

YangKeao commented Oct 12, 2024

Uh oh!

tiprow bot commented Oct 12, 2024

Uh oh!

Uh oh!

ti-chi-bot bot commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

YangKeao commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading