You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/10/12 15:21:40 UTC

[GitHub] [hive] deniskuzZ opened a new pull request #2716: Hive 25346

deniskuzZ opened a new pull request #2716:
URL: https://github.com/apache/hive/pull/2716


   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
     2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
     3. Ensure you have added or run the appropriate tests for your PR: 
     4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX:  Your PR title ...'.
     5. Be sure to keep the PR description updated to reflect all changes.
     6. Please write your PR title to summarize what this PR proposes.
     7. If possible, provide a concise example to reproduce the issue for a faster review.
   
   -->
   
   ### What changes were proposed in this pull request?
   <!--
   Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. 
   If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
     1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
     2. If you fix some SQL features, you can provide some references of other DBMSes.
     3. If there is design documentation, please add the link.
     4. If there is a discussion in the mailing list, please add the link.
   -->
   
   
   ### Why are the changes needed?
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you propose a new API, clarify the use case for a new API.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   
   
   ### Does this PR introduce _any_ user-facing change?
   <!--
   Note that it means *any* user-facing change including all aspects such as the documentation fix.
   If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
   If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
   If no, write 'No'.
   -->
   
   
   ### How was this patch tested?
   <!--
   If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
   If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
   If tests were not added, please describe why they were not added and/or why it was difficult to add.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730944551



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -591,7 +593,7 @@ public void testDBMetrics() throws Exception {
 
     txnHandler.cleanTxnToWriteIdTable();
     runAcidMetricService();
-    Assert.assertEquals(2,
+    Assert.assertEquals(3,

Review comment:
       Thx for the explanation




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731677269



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -79,6 +79,7 @@
   @Before
   public void setUp() throws Exception {
     MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.METRICS_ENABLED, true);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       Maybe in another PR we should create a parametrized tests to test both cases




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731677683



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -542,6 +543,7 @@ public void testDBMetrics() throws Exception {
     String tblName = "dcamc";
     Table t = newTable(dbName, tblName, false);
 
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, false);

Review comment:
       Cool




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730785493



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -591,7 +593,7 @@ public void testDBMetrics() throws Exception {
 
     txnHandler.cleanTxnToWriteIdTable();
     runAcidMetricService();
-    Assert.assertEquals(2,
+    Assert.assertEquals(3,

Review comment:
       Why is this change?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730891209



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -529,19 +531,22 @@ public void cleanTxnToWriteIdTable() throws MetaException {
         // If there are no txns which are currently open or aborted in the system, then current value of
         // max(TXNS.txn_id) could be min_uncommitted_txnid.
         String s = "SELECT MIN(\"RES\".\"ID\") AS \"ID\" FROM (" +
-            "SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\" " +
-            "UNION " +
-            "SELECT MIN(\"WS_COMMIT_ID\") AS \"ID\" FROM \"WRITE_SET\" " +
-            "UNION " +
-            "SELECT MIN(\"TXN_ID\") AS \"ID\" FROM \"TXNS\" WHERE \"TXN_STATE\" = " + TxnStatus.ABORTED +
-            " OR \"TXN_STATE\" = " + TxnStatus.OPEN +
-            ") \"RES\"";
+          " SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\"" +
+          (useMinHistoryLevel ? "" :
+          "   UNION" +
+          " SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\"") +

Review comment:
       1-liner would be even less readable




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731630710



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -520,6 +520,8 @@ public void cleanTxnToWriteIdTable() throws MetaException {
       ResultSet rs = null;
 
       try {
+        long minTxnIdSeenOpen = findMinTxnIdSeenOpen();

Review comment:
       it's gonna run it only in case of MIN_HISTORY present + it doesn't affect ACID performance, only the housekeeper part. ps: originally it was not embedded into a single query as well.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730786874



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -529,19 +531,22 @@ public void cleanTxnToWriteIdTable() throws MetaException {
         // If there are no txns which are currently open or aborted in the system, then current value of
         // max(TXNS.txn_id) could be min_uncommitted_txnid.
         String s = "SELECT MIN(\"RES\".\"ID\") AS \"ID\" FROM (" +
-            "SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\" " +
-            "UNION " +
-            "SELECT MIN(\"WS_COMMIT_ID\") AS \"ID\" FROM \"WRITE_SET\" " +
-            "UNION " +
-            "SELECT MIN(\"TXN_ID\") AS \"ID\" FROM \"TXNS\" WHERE \"TXN_STATE\" = " + TxnStatus.ABORTED +
-            " OR \"TXN_STATE\" = " + TxnStatus.OPEN +
-            ") \"RES\"";
+          " SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\"" +
+          (useMinHistoryLevel ? "" :
+          "   UNION" +
+          " SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\"") +

Review comment:
       I ha problems finding the relevant `)`. Maybe put these into a single line?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730789044



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -520,6 +520,8 @@ public void cleanTxnToWriteIdTable() throws MetaException {
       ResultSet rs = null;
 
       try {
+        long minTxnIdSeenOpen = findMinTxnIdSeenOpen();

Review comment:
       Could we add this to the query below as well?
   Previously the goal was to reduce the number of SQL queries so we can have better performance




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730796373



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {
           acquireTxnLock(stmt, false);
           commitId = getHighWaterMark(stmt);
 
-          if (!rqst.isExclWriteEnabled()) {
+        } else if (txnType.get() != TxnType.READ_ONLY && !isReplayedReplTxn) {
+          String writeSetInsertSql = "INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\"," +
+            "   \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
+            " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" ";
+
+          if (isUpdateOrDelete(stmt, conflictSQLSuffix)) {
+            isUpdateDelete = 'Y';

Review comment:
       Any changes below this block? I would guess no, but the github UI does not help, and I might have overlooked something.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731623472



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -542,6 +543,7 @@ public void testDBMetrics() throws Exception {
     String tblName = "dcamc";
     Table t = newTable(dbName, tblName, false);
 
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, false);

Review comment:
       because in Petya's backward compatibility change we have restored the creation MIN_HISTORY table, so all tests are now running with MIN_HISTORY enabled code path.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730955860



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {

Review comment:
       I think this `if` clause causes behavioural change, and the `else` path will not be executer for `COMPACTION` queries, but it was executed before. But I could be wrong, just testing my understanding




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731633760



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {

Review comment:
       however, that would introduce few more levels of nesting: +2




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730944209



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
##########
@@ -64,7 +65,8 @@ public void setUp() throws Exception {
     ctx = new Context(conf);
     driver = new Driver(new QueryState.Builder().withHiveConf(conf).nonIsolated().build());
     driver2 = new Driver(new QueryState.Builder().withHiveConf(conf).build());
-    conf.setBoolVar(HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    HiveConf.setBoolVar(conf, HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       Shall we use this new config instead of the `TxnHandler.useMinHistoryLevel` then? I would hate to have two different methods to handle this config. Also If in the future we would like to make sure that this config is changed in several connected deployments together, it would be good to need to change it only a single well defined place




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731632635



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -520,6 +520,8 @@ public void cleanTxnToWriteIdTable() throws MetaException {
       ResultSet rs = null;
 
       try {
+        long minTxnIdSeenOpen = findMinTxnIdSeenOpen();

Review comment:
       also if you check TxnHandler open txn, backport removed the optimization of embedding minOpenTxnIdWaterMark and now it is executed as a separate query. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731669207



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -520,6 +520,8 @@ public void cleanTxnToWriteIdTable() throws MetaException {
       ResultSet rs = null;
 
       try {
+        long minTxnIdSeenOpen = findMinTxnIdSeenOpen();

Review comment:
       Cool




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731676792



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {

Review comment:
       Cool




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ merged pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ merged pull request #2716:
URL: https://github.com/apache/hive/pull/2716


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731689100



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {

Review comment:
       refactored




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730895015



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -520,6 +520,8 @@ public void cleanTxnToWriteIdTable() throws MetaException {
       ResultSet rs = null;
 
       try {
+        long minTxnIdSeenOpen = findMinTxnIdSeenOpen();

Review comment:
       that is how it was implemented originally + It would add overhead only when MIN_HISTORY is present.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730904031



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {
           acquireTxnLock(stmt, false);
           commitId = getHighWaterMark(stmt);
 
-          if (!rqst.isExclWriteEnabled()) {
+        } else if (txnType.get() != TxnType.READ_ONLY && !isReplayedReplTxn) {
+          String writeSetInsertSql = "INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\"," +
+            "   \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
+            " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" ";
+
+          if (isUpdateOrDelete(stmt, conflictSQLSuffix)) {
+            isUpdateDelete = 'Y';

Review comment:
       `else block` that adds logic to handle INSERTs




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730782273



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
##########
@@ -64,7 +65,8 @@ public void setUp() throws Exception {
     ctx = new Context(conf);
     driver = new Driver(new QueryState.Builder().withHiveConf(conf).nonIsolated().build());
     driver2 = new Driver(new QueryState.Builder().withHiveConf(conf).build());
-    conf.setBoolVar(HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    HiveConf.setBoolVar(conf, HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       If I remember correctly in other places we depend on `TxnHandler.useMinHistoryLevel`. We might want to stick to a specific solution.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730792092



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {

Review comment:
       Could we use try-with-resorces here for the statement and the dbConnection?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730899455



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {

Review comment:
       we could, however, it would introduce 1 more level of nesting, I would prefer to keep it like this. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730902109



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {
+        dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
+        stmt = dbConn.createStatement();
 
-    } finally {
-      closeStmt(stmt);
-      closeDbConn(dbConn);
+        String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +

Review comment:
       ````
   (useMinHistoryLevel ? "" :
             "   AND \"COMMITTED\".\"WS_OPERATION_TYPE\" != " + OperationType.INSERT) +
   ````
   in a case when MIN_HISTORY is not available, we will add entries for INSERTs into the WRITE_SET table (required for snapshot) - those shouldn't participate in conflict check 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731628641



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -529,19 +531,22 @@ public void cleanTxnToWriteIdTable() throws MetaException {
         // If there are no txns which are currently open or aborted in the system, then current value of
         // max(TXNS.txn_id) could be min_uncommitted_txnid.
         String s = "SELECT MIN(\"RES\".\"ID\") AS \"ID\" FROM (" +
-            "SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\" " +
-            "UNION " +
-            "SELECT MIN(\"WS_COMMIT_ID\") AS \"ID\" FROM \"WRITE_SET\" " +
-            "UNION " +
-            "SELECT MIN(\"TXN_ID\") AS \"ID\" FROM \"TXNS\" WHERE \"TXN_STATE\" = " + TxnStatus.ABORTED +
-            " OR \"TXN_STATE\" = " + TxnStatus.OPEN +
-            ") \"RES\"";
+          " SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\"" +
+          (useMinHistoryLevel ? "" :
+          "   UNION" +
+          " SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\"") +

Review comment:
       i can change, but for me original version is more readable




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731667658



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
##########
@@ -64,7 +65,8 @@ public void setUp() throws Exception {
     ctx = new Context(conf);
     driver = new Driver(new QueryState.Builder().withHiveConf(conf).nonIsolated().build());
     driver2 = new Driver(new QueryState.Builder().withHiveConf(conf).build());
-    conf.setBoolVar(HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    HiveConf.setBoolVar(conf, HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       Thanks for the info




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731636011



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {

Review comment:
       compaction txn should only be handled by in ``xnType.get() == TxnType.COMPACTION`` section

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {

Review comment:
       compaction txn should only be handled by in ``txnType.get() == TxnType.COMPACTION`` section




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730792887



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {
+        dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
+        stmt = dbConn.createStatement();
 
-    } finally {
-      closeStmt(stmt);
-      closeDbConn(dbConn);
+        String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +

Review comment:
       Could you please highlight the changes other than formatting here?
   I see an outer try-catch for retrying. Anything else?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730951996



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -79,6 +79,7 @@
   @Before
   public void setUp() throws Exception {
     MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.METRICS_ENABLED, true);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       Don't we want to run these tests for both `MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL` value?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730952267



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -542,6 +543,7 @@ public void testDBMetrics() throws Exception {
     String tblName = "dcamc";
     Table t = newTable(dbName, tblName, false);
 
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, false);

Review comment:
       Why is this needed to be specifically turned off?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730946864



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {
           acquireTxnLock(stmt, false);
           commitId = getHighWaterMark(stmt);
 
-          if (!rqst.isExclWriteEnabled()) {
+        } else if (txnType.get() != TxnType.READ_ONLY && !isReplayedReplTxn) {
+          String writeSetInsertSql = "INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\"," +
+            "   \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
+            " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" ";
+
+          if (isUpdateOrDelete(stmt, conflictSQLSuffix)) {
+            isUpdateDelete = 'Y';

Review comment:
       Thx




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730906969



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {

Review comment:
       probably, why? those would have diff txn type
   ````
       boolean isReplayedReplTxn = TxnType.REPL_CREATED.equals(rqst.getTxn_type());
       boolean isHiveReplTxn = rqst.isSetReplPolicy() && TxnType.DEFAULT.equals(rqst.getTxn_type());
   ````




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730794690



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1444,76 +1444,84 @@ public void commitTxn(CommitTxnRequest rqst)
 
         String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
                 OperationType.UPDATE + "," + OperationType.DELETE + ")";
-
         long tempCommitId = generateTemporaryId();
-        if (txnType.get() != TxnType.READ_ONLY
-                && !isReplayedReplTxn
-                && isUpdateOrDelete(stmt, conflictSQLSuffix)) {
-
-          isUpdateDelete = 'Y';
-          //if here it means currently committing txn performed update/delete and we should check WW conflict
-          /**
-           * "select distinct" is used below because
-           * 1. once we get to multi-statement txns, we only care to record that something was updated once
-           * 2. if {@link #addDynamicPartitions(AddDynamicPartitions)} is retried by caller it may create
-           *  duplicate entries in TXN_COMPONENTS
-           * but we want to add a PK on WRITE_SET which won't have unique rows w/o this distinct
-           * even if it includes all of its columns
-           *
-           * First insert into write_set using a temporary commitID, which will be updated in a separate call,
-           * see: {@link #updateWSCommitIdAndCleanUpMetadata(Statement, long, TxnType, Long, long)}}.
-           * This should decrease the scope of the S4U lock on the next_txn_id table.
-           */
-          Savepoint undoWriteSetForCurrentTxn = dbConn.setSavepoint();
-          stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
-                          " SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\", " + tempCommitId + ", \"TC_OPERATION_TYPE\" " + conflictSQLSuffix);
 
-          /**
-           * This S4U will mutex with other commitTxn() and openTxns().
-           * -1 below makes txn intervals look like [3,3] [4,4] if all txns are serial
-           * Note: it's possible to have several txns have the same commit id.  Suppose 3 txns start
-           * at the same time and no new txns start until all 3 commit.
-           * We could've incremented the sequence for commitId as well but it doesn't add anything functionally.
-           */
+        if (txnType.get() == TxnType.COMPACTION) {

Review comment:
       Are compaction transactions replicated?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731625563



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -79,6 +79,7 @@
   @Before
   public void setUp() throws Exception {
     MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.METRICS_ENABLED, true);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       probably, not sure if we want to tackle it here, cause it was running only in MIN_HISTORY enabled mode




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731628028



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
##########
@@ -64,7 +65,8 @@ public void setUp() throws Exception {
     ctx = new Context(conf);
     driver = new Driver(new QueryState.Builder().withHiveConf(conf).nonIsolated().build());
     driver2 = new Driver(new QueryState.Builder().withHiveConf(conf).build());
-    conf.setBoolVar(HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    HiveConf.setBoolVar(conf, HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       i think, it should only be handled via the config, I don't see any direct usages of TxnHandler.useMinHistoryLevel in tests




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730958048



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {
+        dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
+        stmt = dbConn.createStatement();
 
-    } finally {
-      closeStmt(stmt);
-      closeDbConn(dbConn);
+        String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +

Review comment:
       Thanks for the explanation




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730945488



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -529,19 +531,22 @@ public void cleanTxnToWriteIdTable() throws MetaException {
         // If there are no txns which are currently open or aborted in the system, then current value of
         // max(TXNS.txn_id) could be min_uncommitted_txnid.
         String s = "SELECT MIN(\"RES\".\"ID\") AS \"ID\" FROM (" +
-            "SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\" " +
-            "UNION " +
-            "SELECT MIN(\"WS_COMMIT_ID\") AS \"ID\" FROM \"WRITE_SET\" " +
-            "UNION " +
-            "SELECT MIN(\"TXN_ID\") AS \"ID\" FROM \"TXNS\" WHERE \"TXN_STATE\" = " + TxnStatus.ABORTED +
-            " OR \"TXN_STATE\" = " + TxnStatus.OPEN +
-            ") \"RES\"";
+          " SELECT MAX(\"TXN_ID\") + 1 AS \"ID\" FROM \"TXNS\"" +
+          (useMinHistoryLevel ? "" :
+          "   UNION" +
+          " SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\"") +

Review comment:
       I was refering to something like this:
   ```
    (useMinHistoryLevel ? "" : "   UNION SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\"") +
   ```
   
   Isn't this better?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730953258



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -520,6 +520,8 @@ public void cleanTxnToWriteIdTable() throws MetaException {
       ResultSet rs = null;
 
       try {
+        long minTxnIdSeenOpen = findMinTxnIdSeenOpen();

Review comment:
       `findMinTxnIdSeenOpen()` adds a new query. Is this intened?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730888296



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/lockmgr/DbTxnManagerEndToEndTestBase.java
##########
@@ -64,7 +65,8 @@ public void setUp() throws Exception {
     ctx = new Context(conf);
     driver = new Driver(new QueryState.Builder().withHiveConf(conf).nonIsolated().build());
     driver2 = new Driver(new QueryState.Builder().withHiveConf(conf).build());
-    conf.setBoolVar(HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    HiveConf.setBoolVar(conf, HiveConf.ConfVars.TXN_WRITE_X_LOCK, false);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       TxnHandler.useMinHistoryLevel is package-protected and is set during the initialization based on TXN_USE_MIN_HISTORY_LEVEL config value. I can't find any tests that access TxnHandler.useMinHistoryLevel directly.  




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730890374



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -591,7 +593,7 @@ public void testDBMetrics() throws Exception {
 
     txnHandler.cleanTxnToWriteIdTable();
     runAcidMetricService();
-    Assert.assertEquals(2,
+    Assert.assertEquals(3,

Review comment:
       result of snapshot change as we should be relying on WS_TXNID and not WS_COMMITID




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] pvary commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r730957276



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1617,42 +1625,52 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
    * @return max Id for the conflicting transaction, if any, otherwise -1
    * @throws MetaException
    */
+  @RetrySemantics.ReadOnly
   public long getLatestTxnIdInConflict(long txnid) throws MetaException {
-    Connection dbConn = null;
-    Statement stmt = null;
-
     try {
-      dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
-      stmt = dbConn.createStatement();
+      Connection dbConn = null;
+      Statement stmt = null;
 
-      String writeConflictQuery = "SELECT MAX(\"COMMITTED\".\"WS_TXNID\")" +
-        " FROM \"WRITE_SET\" \"COMMITTED\" " +
-        "   INNER JOIN (" +
-        "SELECT DISTINCT \"TC_DATABASE\", \"TC_TABLE\", \"TC_PARTITION\", \"TC_TXNID\" " +
-        " FROM \"TXN_COMPONENTS\"  " +
-        "   WHERE \"TC_TXNID\" = " + txnid +
-        "     AND \"TC_OPERATION_TYPE\" IN (" + OperationType.UPDATE + "," + OperationType.DELETE + ")) \"CUR\" " +
-        "   ON \"COMMITTED\".\"WS_DATABASE\" = \"CUR\".\"TC_DATABASE\" " +
-        "     AND \"COMMITTED\".\"WS_TABLE\" = \"CUR\".\"TC_TABLE\" " +
-        //For partitioned table we always track writes at partition level (never at table)
-        //and for non partitioned - always at table level, thus the same table should never
-        //have entries with partition key and w/o
-        "     AND (\"COMMITTED\".\"WS_PARTITION\" = \"CUR\".\"TC_PARTITION\" OR " +
-        "       \"CUR\".\"TC_PARTITION\" IS NULL) " +
-        " WHERE \"CUR\".\"TC_TXNID\" <= \"COMMITTED\".\"WS_COMMIT_ID\""; //txns overlap
-
-      LOG.debug("Going to execute query: <" + writeConflictQuery + ">");
-      ResultSet rs = stmt.executeQuery(writeConflictQuery);
-      return rs.next() ? rs.getLong(1) : -1;
-
-    } catch (Exception e) {
-      throw new MetaException(StringUtils.stringifyException(e));
+      try {

Review comment:
       Maybe wrong, but the `finally` is only there for closing the `dbConn` and the `stmt`. We can skip that with try-with-resources.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org


[GitHub] [hive] deniskuzZ commented on a change in pull request #2716: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation

Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2716:
URL: https://github.com/apache/hive/pull/2716#discussion_r731695358



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java
##########
@@ -79,6 +79,7 @@
   @Before
   public void setUp() throws Exception {
     MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.METRICS_ENABLED, true);
+    MetastoreConf.setBoolVar(conf, MetastoreConf.ConfVars.TXN_USE_MIN_HISTORY_LEVEL, true);

Review comment:
       https://issues.apache.org/jira/browse/HIVE-25623




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org