Skip to content

[fix](load) add lock for runtime_state->tablet_commit_infos #48709

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.

Sign up for GitHub

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

Jump to bottom
Merged
liaoxin01 merged 3 commits into from
Mar 7, 2025
Merged

[fix](load) add lock for runtime_state->tablet_commit_infos #48709

liaoxin01 merged 3 commits into from
Mar 7, 2025

Conversation

@kaijchen
Copy link
Member

@kaijchen kaijchen commented Mar 5, 2025

What problem does this PR solve?

Issue Number: CIR-14718

Problem Summary:

Lock before read/write tablet_commit_infos vector, to fix use-after-free.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

  • Behavior changed:

    • No.
    • Yes.

  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen
Copy link
Contributor

hello-stephen commented Mar 5, 2025

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@kaijchen
Copy link
Member Author

kaijchen commented Mar 5, 2025

run buildall

@dataroaring dataroaring added usercase Important user case type label dev/2.1.x dev/3.0.x labels Mar 5, 2025
@kaijchen
Copy link
Member Author

kaijchen commented Mar 5, 2025

run buildall

dataroaring
dataroaring approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2025

PR approved by at least one committer and no changes requested.

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Mar 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2025

PR approved by anyone and no changes requested.

@doris-robot
Copy link

doris-robot commented Mar 5, 2025

TPC-H: Total hot run time: 32866 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 035fe10bd0707b702c7dfcbc3841a2adee55e6f8, data reload: false

------ Round 1 ----------------------------------
q1	17590	5294	5144	5144
q2	2048	302	166	166
q3	10443	1283	751	751
q4	10305	1062	558	558
q5	8559	2427	2347	2347
q6	192	169	136	136
q7	922	765	611	611
q8	9336	1325	1224	1224
q9	5043	4861	4897	4861
q10	6810	2337	1877	1877
q11	514	274	259	259
q12	350	354	219	219
q13	17771	3685	3101	3101
q14	229	233	214	214
q15	533	507	476	476
q16	622	623	575	575
q17	576	863	358	358
q18	6858	6532	6359	6359
q19	1827	968	549	549
q20	308	320	188	188
q21	2771	2257	1910	1910
q22	1076	1044	983	983
Total cold run time: 104683 ms
Total hot run time: 32866 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5223	5165	5160	5160
q2	242	331	236	236
q3	2198	2675	2335	2335
q4	1464	1886	1427	1427
q5	4255	4129	4149	4129
q6	211	164	129	129
q7	2000	1875	1768	1768
q8	2643	2657	2572	2572
q9	7308	7210	7270	7210
q10	3045	3197	2804	2804
q11	589	497	493	493
q12	657	737	602	602
q13	3488	3939	3253	3253
q14	278	297	281	281
q15	532	464	479	464
q16	642	690	640	640
q17	1143	1673	1316	1316
q18	7845	7641	7525	7525
q19	807	868	944	868
q20	2025	2047	1866	1866
q21	5457	4971	4695	4695
q22	1128	1076	1060	1060
Total cold run time: 53180 ms
Total hot run time: 50833 ms

@doris-robot
Copy link

doris-robot commented Mar 5, 2025

TPC-DS: Total hot run time: 191925 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 035fe10bd0707b702c7dfcbc3841a2adee55e6f8, data reload: false

query1	1402	1062	1006	1006
query2	6163	1896	1856	1856
query3	11141	4630	4650	4630
query4	53297	25662	23385	23385
query5	5243	576	484	484
query6	360	205	190	190
query7	4969	523	290	290
query8	327	249	254	249
query9	6027	2558	2560	2558
query10	438	329	254	254
query11	15299	15134	14872	14872
query12	158	110	106	106
query13	1093	522	398	398
query14	10848	6966	6540	6540
query15	201	195	180	180
query16	7046	679	514	514
query17	1099	707	555	555
query18	1507	407	319	319
query19	242	191	158	158
query20	126	132	123	123
query21	214	127	105	105
query22	4590	4433	4392	4392
query23	33891	33313	33390	33313
query24	5685	2435	2466	2435
query25	462	478	397	397
query26	743	277	161	161
query27	1846	507	338	338
query28	2773	2499	2440	2440
query29	602	550	434	434
query30	267	208	198	198
query31	872	848	816	816
query32	71	59	63	59
query33	453	348	323	323
query34	754	864	505	505
query35	809	843	748	748
query36	930	1016	905	905
query37	117	101	86	86
query38	4216	4382	4291	4291
query39	1548	1444	1421	1421
query40	210	115	104	104
query41	53	55	50	50
query42	129	107	104	104
query43	491	508	498	498
query44	1303	834	810	810
query45	181	176	163	163
query46	852	1023	640	640
query47	1843	1847	1783	1783
query48	389	415	328	328
query49	731	512	426	426
query50	716	766	431	431
query51	4288	4310	4210	4210
query52	113	107	89	89
query53	233	256	189	189
query54	502	492	416	416
query55	86	81	82	81
query56	272	283	251	251
query57	1171	1189	1112	1112
query58	239	248	235	235
query59	2765	2798	2658	2658
query60	292	271	260	260
query61	125	118	121	118
query62	739	751	700	700
query63	239	192	190	190
query64	1752	1027	717	717
query65	4547	4527	4448	4448
query66	740	449	289	289
query67	15776	15651	15281	15281
query68	6568	870	508	508
query69	531	294	296	294
query70	1188	1093	1082	1082
query71	509	281	266	266
query72	5556	3632	3859	3632
query73	1403	750	345	345
query74	8883	9103	8701	8701
query75	3827	3155	2727	2727
query76	4189	1162	735	735
query77	652	379	275	275
query78	10185	10081	9325	9325
query79	5089	814	570	570
query80	710	508	448	448
query81	486	266	218	218
query82	770	124	94	94
query83	256	167	151	151
query84	283	93	71	71
query85	790	359	309	309
query86	387	319	302	302
query87	4351	4423	4396	4396
query88	3589	2146	2148	2146
query89	474	313	277	277
query90	1904	187	187	187
query91	143	145	112	112
query92	68	62	60	60
query93	3011	1044	571	571
query94	657	450	303	303
query95	345	265	258	258
query96	472	560	273	273
query97	3347	3396	3233	3233
query98	225	202	201	201
query99	1434	1396	1240	1240
Total cold run time: 300888 ms
Total hot run time: 191925 ms

@doris-robot
Copy link

doris-robot commented Mar 5, 2025

ClickBench: Total hot run time: 31.47 s 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 035fe10bd0707b702c7dfcbc3841a2adee55e6f8, data reload: false

query1	0.04	0.04	0.03
query2	0.07	0.03	0.04
query3	0.23	0.07	0.07
query4	1.62	0.10	0.10
query5	0.55	0.54	0.55
query6	1.18	0.72	0.72
query7	0.02	0.02	0.01
query8	0.04	0.04	0.04
query9	0.58	0.54	0.53
query10	0.57	0.58	0.58
query11	0.16	0.10	0.11
query12	0.14	0.11	0.11
query13	0.61	0.60	0.60
query14	2.79	2.71	2.79
query15	0.92	0.88	0.84
query16	0.38	0.39	0.38
query17	1.01	1.04	1.02
query18	0.21	0.19	0.19
query19	1.97	1.80	1.89
query20	0.02	0.02	0.01
query21	15.35	0.87	0.54
query22	0.77	1.23	0.99
query23	14.70	1.38	0.61
query24	6.55	2.42	1.01
query25	0.55	0.16	0.15
query26	0.62	0.17	0.13
query27	0.05	0.06	0.05
query28	9.39	0.83	0.43
query29	12.58	3.98	3.31
query30	0.24	0.10	0.07
query31	2.81	0.60	0.38
query32	3.23	0.55	0.46
query33	2.94	3.05	3.01
query34	15.82	5.16	4.53
query35	4.58	4.57	4.60
query36	0.66	0.49	0.48
query37	0.09	0.07	0.06
query38	0.05	0.04	0.04
query39	0.04	0.02	0.03
query40	0.17	0.14	0.14
query41	0.08	0.03	0.03
query42	0.03	0.02	0.02
query43	0.04	0.03	0.03
Total cold run time: 104.45 s
Total hot run time: 31.47 s

@doris-robot
Copy link

doris-robot commented Mar 5, 2025

BE UT Coverage Report

Increment line coverage 0.00% (0/45) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 45.94% (12278/26727)
Line Coverage 35.38% (103649/292922)
Region Coverage 34.55% (53092/153674)
Branch Coverage 30.24% (26885/88904)

@kaijchen kaijchen mentioned this pull request Mar 6, 2025
Merged
liaoxin01
liaoxin01 approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@liaoxin01 liaoxin01 left a comment

Choose a reason for hiding this comment

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

LGTM

@liaoxin01 liaoxin01 merged commit 00ec587 into apache:master Mar 7, 2025
24 of 26 checks passed
kaijchen added a commit to kaijchen/doris that referenced this pull request Mar 10, 2025
@kaijchen
[fix](load) add lock for runtime_state->tablet_commit_infos (apache#4…
36c6d32 
…8709)
@kaijchen kaijchen mentioned this pull request Mar 10, 2025
Merged
dataroaring pushed a commit that referenced this pull request Mar 10, 2025
@kaijchen
[fix](load) add lock for runtime_state->tablet_commit_infos (#48709) (#…
9c39a34 
…48732)

backport #48709
yiguolei pushed a commit that referenced this pull request Mar 11, 2025
@kaijchen
[fix](load) add lock for runtime_state->tablet_commit_infos (#48709) (#…
7ca00c1 
…48850)

backport #48709
@kaka11chen kaka11chen mentioned this pull request Apr 7, 2025
Merged
16 tasks
morningman pushed a commit that referenced this pull request Apr 8, 2025
@kaka11chen
[fix](multi-catalog) Fix multi-thread issue in hive/iceberg writer co…
d3a7e2f 
…mmit meta-info to fe. (#49842)

### What problem does this PR solve?

Related PR: #48709

Problem Summary:

Refer to the changes in #48709. In hive/iceberg writer, the information
related to `partiton_update_info` and `iceberg_commit_data` will be
submitted to FE for processing. However, there will be multiple threads
accessing the related vectors, so thread-safe processing is required.
github-actions bot pushed a commit that referenced this pull request Apr 8, 2025
@kaka11chen
[fix](multi-catalog) Fix multi-thread issue in hive/iceberg writer co…
a09f85a 
…mmit meta-info to fe. (#49842)

### What problem does this PR solve?

Related PR: #48709

Problem Summary:

Refer to the changes in #48709. In hive/iceberg writer, the information
related to `partiton_update_info` and `iceberg_commit_data` will be
submitted to FE for processing. However, there will be multiple threads
accessing the related vectors, so thread-safe processing is required.
kaka11chen added a commit to kaka11chen/doris that referenced this pull request Apr 9, 2025
@kaka11chen
[fix](multi-catalog) Fix multi-thread issue in hive/iceberg writer co…
3994a31 
…mmit meta-info to fe. (apache#49842)

Related PR: apache#48709

Problem Summary:

Refer to the changes in apache#48709. In hive/iceberg writer, the information
related to `partiton_update_info` and `iceberg_commit_data` will be
submitted to FE for processing. However, there will be multiple threads
accessing the related vectors, so thread-safe processing is required.
koarz pushed a commit to koarz/doris that referenced this pull request Jun 4, 2025
@kaijchen @koarz
[fix](load) add lock for runtime_state->tablet_commit_infos (apache#4…
cc8ca3a 
…8709)

Lock before read/write tablet_commit_infos vector, to fix
use-after-free.
koarz pushed a commit to koarz/doris that referenced this pull request Jun 4, 2025
@kaka11chen @koarz
[fix](multi-catalog) Fix multi-thread issue in hive/iceberg writer co…
0e9c7de 
…mmit meta-info to fe. (apache#49842)

### What problem does this PR solve?

Related PR: apache#48709

Problem Summary:

Refer to the changes in apache#48709. In hive/iceberg writer, the information
related to `partiton_update_info` and `iceberg_commit_data` will be
submitted to FE for processing. However, there will be multiple threads
accessing the related vectors, so thread-safe processing is required.
stuliper added a commit to stuliper/doris that referenced this pull request Nov 13, 2025
@stuliper
Revert "[fix](load) add lock for runtime_state->tablet_commit_infos (a…
4d1eb1a 
…pache#48709) …"

This reverts commit 7ca00c1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liaoxin01 liaoxin01 liaoxin01 approved these changes

@dataroaring dataroaring dataroaring approved these changes

@gavinchou gavinchou Awaiting requested review from gavinchou gavinchou is a code owner

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by one committer. dev/2.1.9-merged dev/3.0.5-merged reviewed usercase Important user case type label

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kaijchen @hello-stephen @doris-robot @liaoxin01 @dataroaring @yiguolei