Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-07 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/
---

(Updated May 7, 2020, 2:49 p.m.)


Review request for hive, Marton Bod, Peter Varga, and Peter Vary.


Changes
---

removed not related change


Bugs: HIVE-23349
https://issues.apache.org/jira/browse/HIVE-23349


Repository: hive-git


Description
---

2 concurrent MERGE INSERT operations generate duplicates due to lack of locking.
MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or EXCL_WRITE 
if hive.txn.write.xlock=false;

create table target (a int, b int) stored as orc TBLPROPERTIES 
('transactional'='true')");
insert into target values (1,2), (3,4)
create table source (a int, b int)
execute in parallel:

insert into source values (5,6), (7,8)

PS:

Current patch doesn cover following scenario:
1) T1 (merge-insert) opens txns & records snapshot;
2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
3) T2 commits it's changes and unlocks T1; 
4) T1 locks snapshot and validates txn list, however only txns with txnId lower 
than T1's is beeing considered, T2 changes are ignored -> duplicates are 
generated.


merge-insert/merge-insert scenario could be fixed by leveraging write-write 
conflict check mechanism. We just need to set operation type for merge-insert 
to update.
However it won't solve issue with merge-insert/insert. 

We should consider moving locking before snapshot generation, current snapshot 
re-check mechanism doesn't handle described scenarios.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
  ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
3ffdcec528 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
b435e79c3c 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
1687425bcb 
  ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 


Diff: https://reviews.apache.org/r/72470/diff/5/

Changes: https://reviews.apache.org/r/72470/diff/4-5/


Testing
---

Manual, added number of merge related test scenarios into TestDbTxnManager2, 
modified explain_locks.q


Thanks,

Denys Kuzmenko

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-07 Thread Peter Vary via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220673
---


Ship it!




Ship It!

- Peter Vary


On máj. 7, 2020, 1:21 du, Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated máj. 7, 2020, 1:21 du)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/4/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-07 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/
---

(Updated May 7, 2020, 1:21 p.m.)


Review request for hive, Marton Bod, Peter Varga, and Peter Vary.


Bugs: HIVE-23349
https://issues.apache.org/jira/browse/HIVE-23349


Repository: hive-git


Description
---

2 concurrent MERGE INSERT operations generate duplicates due to lack of locking.
MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or EXCL_WRITE 
if hive.txn.write.xlock=false;

create table target (a int, b int) stored as orc TBLPROPERTIES 
('transactional'='true')");
insert into target values (1,2), (3,4)
create table source (a int, b int)
execute in parallel:

insert into source values (5,6), (7,8)

PS:

Current patch doesn cover following scenario:
1) T1 (merge-insert) opens txns & records snapshot;
2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
3) T2 commits it's changes and unlocks T1; 
4) T1 locks snapshot and validates txn list, however only txns with txnId lower 
than T1's is beeing considered, T2 changes are ignored -> duplicates are 
generated.


merge-insert/merge-insert scenario could be fixed by leveraging write-write 
conflict check mechanism. We just need to set operation type for merge-insert 
to update.
However it won't solve issue with merge-insert/insert. 

We should consider moving locking before snapshot generation, current snapshot 
re-check mechanism doesn't handle described scenarios.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
  ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
3ffdcec528 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
b435e79c3c 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
1687425bcb 
  ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 


Diff: https://reviews.apache.org/r/72470/diff/4/

Changes: https://reviews.apache.org/r/72470/diff/3-4/


Testing
---

Manual, added number of merge related test scenarios into TestDbTxnManager2, 
modified explain_locks.q


Thanks,

Denys Kuzmenko

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-05 Thread Marton Bod 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220632
---


Ship it!




Ship It!

- Marton Bod


On May 5, 2020, 8:33 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 5, 2020, 8:33 a.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/3/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-05 Thread Peter Varga via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220631
---


Ship it!




Ship It!

- Peter Varga


On May 5, 2020, 8:33 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 5, 2020, 8:33 a.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/3/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-05 Thread Denys Kuzmenko via Review Board 


> On May 5, 2020, 7:20 a.m., Peter Varga wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Context.java
> > Lines 1198 (patched)
> > 
> >
> > Do you need a separate field for that, you could just check the 
> > operation type?

I actually started with checking operation type and ended up with the new 
field. Reverted back to the original approach.


- Denys


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220620
---


On May 5, 2020, 8:33 a.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 5, 2020, 8:33 a.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/3/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-05 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/
---

(Updated May 5, 2020, 8:33 a.m.)


Review request for hive, Marton Bod, Peter Varga, and Peter Vary.


Changes
---

Removed isMerge property


Bugs: HIVE-23349
https://issues.apache.org/jira/browse/HIVE-23349


Repository: hive-git


Description
---

2 concurrent MERGE INSERT operations generate duplicates due to lack of locking.
MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or EXCL_WRITE 
if hive.txn.write.xlock=false;

create table target (a int, b int) stored as orc TBLPROPERTIES 
('transactional'='true')");
insert into target values (1,2), (3,4)
create table source (a int, b int)
execute in parallel:

insert into source values (5,6), (7,8)

PS:

Current patch doesn cover following scenario:
1) T1 (merge-insert) opens txns & records snapshot;
2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
3) T2 commits it's changes and unlocks T1; 
4) T1 locks snapshot and validates txn list, however only txns with txnId lower 
than T1's is beeing considered, T2 changes are ignored -> duplicates are 
generated.


merge-insert/merge-insert scenario could be fixed by leveraging write-write 
conflict check mechanism. We just need to set operation type for merge-insert 
to update.
However it won't solve issue with merge-insert/insert. 

We should consider moving locking before snapshot generation, current snapshot 
re-check mechanism doesn't handle described scenarios.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
  ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
3ffdcec528 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
b435e79c3c 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
1687425bcb 
  ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 


Diff: https://reviews.apache.org/r/72470/diff/3/

Changes: https://reviews.apache.org/r/72470/diff/2-3/


Testing
---

Manual, added number of merge related test scenarios into TestDbTxnManager2, 
modified explain_locks.q


Thanks,

Denys Kuzmenko

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-05 Thread Peter Varga via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220620
---



Looks good, just one question.


ql/src/java/org/apache/hadoop/hive/ql/Context.java
Lines 1198 (patched)


Do you need a separate field for that, you could just check the operation 
type?


- Peter Varga


On May 4, 2020, 4:33 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 4, 2020, 4:33 p.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/2/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-04 Thread Denys Kuzmenko via Review Board 


> On May 4, 2020, 3:21 p.m., Marton Bod wrote:
> > Looks good to me, just a couple of questions.

Thank you for the review!


> On May 4, 2020, 3:21 p.m., Marton Bod wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
> > Lines 2226 (patched)
> > 
> >
> > nit: the .getLockid() part is a leftover?

removed.


> On May 4, 2020, 3:21 p.m., Marton Bod wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
> > Lines 2241 (patched)
> > 
> >
> > maybe worth running the same tests with 'false' too?

added.


- Denys


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220595
---


On May 4, 2020, 4:33 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 4, 2020, 4:33 p.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/2/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-04 Thread Denys Kuzmenko via Review Board 


> On May 4, 2020, 3:21 p.m., Marton Bod wrote:
> > ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
> > Lines 2231 (patched)
> > 
> >
> > nit: this appears in a few places, might make sense to extract

i would keep it simple in a tests.


- Denys


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220595
---


On May 4, 2020, 4:33 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 4, 2020, 4:33 p.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/2/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-04 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/
---

(Updated May 4, 2020, 4:33 p.m.)


Review request for hive, Marton Bod, Peter Varga, and Peter Vary.


Changes
---

Uploaded new patch.


Bugs: HIVE-23349
https://issues.apache.org/jira/browse/HIVE-23349


Repository: hive-git


Description
---

2 concurrent MERGE INSERT operations generate duplicates due to lack of locking.
MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or EXCL_WRITE 
if hive.txn.write.xlock=false;

create table target (a int, b int) stored as orc TBLPROPERTIES 
('transactional'='true')");
insert into target values (1,2), (3,4)
create table source (a int, b int)
execute in parallel:

insert into source values (5,6), (7,8)

PS:

Current patch doesn cover following scenario:
1) T1 (merge-insert) opens txns & records snapshot;
2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
3) T2 commits it's changes and unlocks T1; 
4) T1 locks snapshot and validates txn list, however only txns with txnId lower 
than T1's is beeing considered, T2 changes are ignored -> duplicates are 
generated.


merge-insert/merge-insert scenario could be fixed by leveraging write-write 
conflict check mechanism. We just need to set operation type for merge-insert 
to update.
However it won't solve issue with merge-insert/insert. 

We should consider moving locking before snapshot generation, current snapshot 
re-check mechanism doesn't handle described scenarios.


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
  ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
3ffdcec528 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
b435e79c3c 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
1687425bcb 
  ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 


Diff: https://reviews.apache.org/r/72470/diff/2/

Changes: https://reviews.apache.org/r/72470/diff/1-2/


Testing
---

Manual, added number of merge related test scenarios into TestDbTxnManager2, 
modified explain_locks.q


Thanks,

Denys Kuzmenko

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-04 Thread Marton Bod 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/#review220595
---



Looks good to me, just a couple of questions.


ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
Lines 2226 (patched)


nit: the .getLockid() part is a leftover?



ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
Lines 2231 (patched)


nit: this appears in a few places, might make sense to extract



ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
Lines 2241 (patched)


maybe worth running the same tests with 'false' too?


- Marton Bod


On May 4, 2020, 1:32 p.m., Denys Kuzmenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72470/
> ---
> 
> (Updated May 4, 2020, 1:32 p.m.)
> 
> 
> Review request for hive, Marton Bod, Peter Varga, and Peter Vary.
> 
> 
> Bugs: HIVE-23349
> https://issues.apache.org/jira/browse/HIVE-23349
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> 2 concurrent MERGE INSERT operations generate duplicates due to lack of 
> locking.
> MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
> doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or 
> EXCL_WRITE if hive.txn.write.xlock=false;
> 
> create table target (a int, b int) stored as orc TBLPROPERTIES 
> ('transactional'='true')");
> insert into target values (1,2), (3,4)
> create table source (a int, b int)
> execute in parallel:
> 
> insert into source values (5,6), (7,8)
> 
> PS:
> 
> Current patch doesn cover following scenario:
> 1) T1 (merge-insert) opens txns & records snapshot;
> 2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
> 3) T2 commits it's changes and unlocks T1; 
> 4) T1 locks snapshot and validates txn list, however only txns with txnId 
> lower than T1's is beeing considered, T2 changes are ignored -> duplicates 
> are generated.
> 
> 
> merge-insert/merge-insert scenario could be fixed by leveraging write-write 
> conflict check mechanism. We just need to set operation type for merge-insert 
> to update.
> However it won't solve issue with merge-insert/insert. 
> 
> We should consider moving locking before snapshot generation, current 
> snapshot re-check mechanism doesn't handle described scenarios.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
>   ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
> 3ffdcec528 
>   
> ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
>  b435e79c3c 
>   ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
> 1687425bcb 
>   ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 
> 
> 
> Diff: https://reviews.apache.org/r/72470/diff/1/
> 
> 
> Testing
> ---
> 
> Manual, added number of merge related test scenarios into TestDbTxnManager2, 
> modified explain_locks.q
> 
> 
> Thanks,
> 
> Denys Kuzmenko
> 
>

Re: Review Request 72470: ACID: Concurrent MERGE INSERT operations produce duplicates

2020-05-04 Thread Denys Kuzmenko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72470/
---

(Updated May 4, 2020, 1:32 p.m.)


Review request for hive, Marton Bod, Peter Varga, and Peter Vary.


Bugs: HIVE-23349
https://issues.apache.org/jira/browse/HIVE-23349


Repository: hive-git


Description (updated)
---

2 concurrent MERGE INSERT operations generate duplicates due to lack of locking.
MERGE INSERT is treated as regular INSERT, it acquires SHARED_READ lock that 
doesn't prevent other INSERTs. We should use EXCLUSIVE lock here or EXCL_WRITE 
if hive.txn.write.xlock=false;

create table target (a int, b int) stored as orc TBLPROPERTIES 
('transactional'='true')");
insert into target values (1,2), (3,4)
create table source (a int, b int)
execute in parallel:

insert into source values (5,6), (7,8)

PS:

Current patch doesn cover following scenario:
1) T1 (merge-insert) opens txns & records snapshot;
2) T2 (insert/merge-insert) opens txns, records snapshot & locks it;
3) T2 commits it's changes and unlocks T1; 
4) T1 locks snapshot and validates txn list, however only txns with txnId lower 
than T1's is beeing considered, T2 changes are ignored -> duplicates are 
generated.


merge-insert/merge-insert scenario could be fixed by leveraging write-write 
conflict check mechanism. We just need to set operation type for merge-insert 
to update.
However it won't solve issue with merge-insert/insert. 

We should consider moving locking before snapshot generation, current snapshot 
re-check mechanism doesn't handle described scenarios.


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/Context.java 9f59d4cea3 
  ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java c1f94d165b 
  ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java 998c05e37d 
  ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java deaab89c1f 
  ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java 
3ffdcec528 
  
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java 
b435e79c3c 
  ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java 
1687425bcb 
  ql/src/test/results/clientpositive/llap/explain_locks.q.out d62f6ccafd 


Diff: https://reviews.apache.org/r/72470/diff/1/


Testing
---

Manual, added number of merge related test scenarios into TestDbTxnManager2, 
modified explain_locks.q


Thanks,

Denys Kuzmenko