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/07/29 11:31:49 UTC
[GitHub] [hive] zchovan opened a new pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
zchovan opened a new pull request #2547:
URL: https://github.com/apache/hive/pull/2547
initial test fixes
Change-Id: Ieb4f922d1e1957538cbeda2d410a167d18993724
<!--
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] deniskuzZ edited a comment on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ edited a comment on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-916945444
@zchovan thank you for the benchmarks!
Last thing, to be on a safe side, could you please record INSERTS only if min_history is not available + use old logic for writeId removal (cleanTxnToWriteIdTable) based on min_history if available. @pvary, what do you think?
````
if (!useMinHistoryLevel && isInsert(stmt, txnid)) {
....
}
````
--
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] github-actions[bot] commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-974730512
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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 pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-895894458
> > Would be great if you could run the HMS benchmark and see if that has affected the commit step performance. See if the index on opType improves the situation.
>
> There is no benchmark currently for the commitTxn() call.
Could we create one or at least perform similar to https://issues.apache.org/jira/browse/HIVE-23104?focusedCommentId=17083005&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17083005 test?
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679788733
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ 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\" " + insertFilter);
+ acquireTxnLock(stmt, false);
Review comment:
Why do you need to acquire locks here and generate 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] deniskuzZ edited a comment on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ edited a comment on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-916945444
@zchovan thank you for the benchmarks!
Last thing, to be on a safe side, could you please record INSERTS only if min_history is not available + use old logic for writeId removal based on min_history if available. @pvary, what do you think?
````
if (!useMinHistoryLevel && isInsert(stmt, txnid)) {
....
}
````
--
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 pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-916945444
@zchovan thank you for the benchmarks!
Last thing, to be on a safe side, could you please record INSERTS only if min_history is not available + use old logic for writeId removal based on min_history if available. @pvary, what do you think?
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679771294
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1442,7 +1443,9 @@ public void commitTxn(CommitTxnRequest rqst)
}
String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
- OperationType.UPDATE + "," + OperationType.DELETE + ")";
+ OperationType.UPDATE + "," + OperationType.DELETE + "," + OperationType.INSERT + ")";
+ String insertFilter = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid +
+ " AND \"TC_OPERATION_TYPE\" IN (" + OperationType.INSERT + ")";
Review comment:
Why not simply '=' instead of IN
--
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] zchovan commented on a change in pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r685874054
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
Review comment:
The row count would most likely be higher in the WRITE_SET table, so selects could possibly see slower response times.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707184594
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
````What happens with the WRITE_SET```` - nothing, it's only cleaned by the AcidHouseKeeperThread:
````
performTask(txnHandler::performWriteSetGC, "Cleaning obsolete write set entries");
performTask(txnHandler::cleanTxnToWriteIdTable, "Cleaning obsolete TXN_TO_WRITE_ID entries");
````
--
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 pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-889770605
Would be great if you could run the HMS benchmark and see if that has affected the commit step performace. See if the index on opType improves the. situation.
--
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] zchovan commented on a change in pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r680821564
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ 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\" " + insertFilter);
+ acquireTxnLock(stmt, false);
Review comment:
it's required for later when ` updateWSCommitIdAndCleanUpMetadata(stmt, txnid, txnType.get(), commitId, tempCommitId);` is called
--
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 pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-896692730
> > > > Would be great if you could run the HMS benchmark and see if that has affected the commit step performance. See if the index on opType improves the situation.
> > >
> > >
> > > There is no benchmark currently for the commitTxn() call.
> >
> >
> > Could we create one or at least perform similar to https://issues.apache.org/jira/browse/HIVE-23104?focusedCommentId=17083005&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17083005 test?
>
> Based on the linked comment it is not clear to me what exactly was tested and how or what tools were used for it or what environment the tests were run in. I found no mention of the above details in the jira comments.
You are not limited in a set of tools or approaches, free to choose any.
This patch changes the way how INSERTs are handled at the commit step, so it would make sense to test the throughput of INSERT/UPDATE operations in multithreaded env.
AFAIK in the above JIRA JMH tool was used and testing was done on a local env.
We cannot merge this PR until we know the performance effect of the proposed 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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r685868894
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
Review comment:
how would it affect the 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] deniskuzZ commented on a change in pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679773817
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
Review comment:
We should ignore repl as well. Please extract "txnType.get() != TxnType.READ_ONLY && !rqst.isSetReplPolicy()" nand handle inserts and updates there.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679777118
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
Review comment:
do we have an index on TC_OPERATION_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] zchovan commented on a change in pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r685785276
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ stmt.executeUpdate("INSERT INTO \"WRITE_SET\" (\"WS_DATABASE\", \"WS_TABLE\", \"WS_PARTITION\", \"WS_TXNID\", \"WS_COMMIT_ID\", \"WS_OPERATION_TYPE\")" +
Review comment:
no, we don't
--
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] kgyrtkirk commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-975356703
merged as #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] kgyrtkirk closed pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
kgyrtkirk closed pull request #2547:
URL: https://github.com/apache/hive/pull/2547
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679788733
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1513,6 +1516,11 @@ public void commitTxn(CommitTxnRequest rqst)
} else if (txnType.get() == TxnType.COMPACTION) {
acquireTxnLock(stmt, false);
commitId = getHighWaterMark(stmt);
+ } else if (txnType.get() != TxnType.READ_ONLY && isInsert(stmt, insertFilter)) {
+ 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\" " + insertFilter);
+ acquireTxnLock(stmt, false);
Review comment:
Why do you need to acquire locks here?
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679775113
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1608,6 +1616,14 @@ private boolean isUpdateOrDelete(Statement stmt, String conflictSQLSuffix) throw
}
}
+ private boolean isInsert(Statement stmt, String insertFilter) throws SQLException, MetaException {
Review comment:
this method has a specific purpose, why not define filter inside?
--
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] zchovan commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-895817651
> Would be great if you could run the HMS benchmark and see if that has affected the commit step performace. See if the index on opType improves the. situation.
There is no benchmark currently for the commitTxn() call.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707184594
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
````What happens with the WRITE_SET when a compaction happened on this table```` - nothing, it's only cleaned by the AcidHouseKeeperThread:
````
performTask(txnHandler::performWriteSetGC, "Cleaning obsolete write set entries");
performTask(txnHandler::cleanTxnToWriteIdTable, "Cleaning obsolete TXN_TO_WRITE_ID entries");
````
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r710919584
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1446,70 +1446,75 @@ public void commitTxn(CommitTxnRequest rqst)
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.
- */
- acquireTxnLock(stmt, false);
- commitId = getHighWaterMark(stmt);
+ if (txnType.get() != TxnType.READ_ONLY && !isReplayedReplTxn && txnType.get() != TxnType.COMPACTION) {
+ if (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);
- if (!rqst.isExclWriteEnabled()) {
/**
- * see if there are any overlapping txns that wrote the same element, i.e. have a conflict
- * Since entire commit operation is mutexed wrt other start/commit ops,
- * committed.ws_commit_id <= current.ws_commit_id for all txns
- * thus if committed.ws_commit_id < current.ws_txnid, transactions do NOT overlap
- * For example, [17,20] is committed, [6,80] is being committed right now - these overlap
- * [17,20] committed and [21,21] committing now - these do not overlap.
- * [17,18] committed and [18,19] committing now - these overlap (here 18 started while 17 was still running)
+ * 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.
*/
- try (ResultSet rs = checkForWriteConflict(stmt, txnid)) {
- if (rs.next()) {
- //found a conflict, so let's abort the txn
- String committedTxn = "[" + JavaUtils.txnIdToString(rs.getLong(1)) + "," + rs.getLong(2) + "]";
- StringBuilder resource = new StringBuilder(rs.getString(3)).append("/").append(rs.getString(4));
- String partitionName = rs.getString(5);
- if (partitionName != null) {
- resource.append('/').append(partitionName);
- }
- String msg = "Aborting [" + JavaUtils.txnIdToString(txnid) + "," + commitId + "]" + " due to a write conflict on " + resource +
- " committed by " + committedTxn + " " + rs.getString(7) + "/" + rs.getString(8);
- //remove WRITE_SET info for current txn since it's about to abort
- dbConn.rollback(undoWriteSetForCurrentTxn);
- LOG.info(msg);
- //todo: should make abortTxns() write something into TXNS.TXN_META_INFO about this
- if (abortTxns(dbConn, Collections.singletonList(txnid), false, isReplayedReplTxn) != 1) {
- throw new IllegalStateException(msg + " FAILED!");
+ acquireTxnLock(stmt, false);
+ commitId = getHighWaterMark(stmt);
+
+ if (!rqst.isExclWriteEnabled()) {
+ /**
+ * see if there are any overlapping txns that wrote the same element, i.e. have a conflict
+ * Since entire commit operation is mutexed wrt other start/commit ops,
+ * committed.ws_commit_id <= current.ws_commit_id for all txns
+ * thus if committed.ws_commit_id < current.ws_txnid, transactions do NOT overlap
+ * For example, [17,20] is committed, [6,80] is being committed right now - these overlap
+ * [17,20] committed and [21,21] committing now - these do not overlap.
+ * [17,18] committed and [18,19] committing now - these overlap (here 18 started while 17 was still running)
+ */
+ try (ResultSet rs = checkForWriteConflict(stmt, txnid)) {
+ if (rs.next()) {
+ //found a conflict, so let's abort the txn
+ String committedTxn = "[" + JavaUtils.txnIdToString(rs.getLong(1)) + "," + rs.getLong(2) + "]";
+ StringBuilder resource = new StringBuilder(rs.getString(3)).append("/").append(rs.getString(4));
+ String partitionName = rs.getString(5);
+ if (partitionName != null) {
+ resource.append('/').append(partitionName);
+ }
+ String msg = "Aborting [" + JavaUtils.txnIdToString(txnid) + "," + commitId + "]" + " due to a write conflict on " + resource +
+ " committed by " + committedTxn + " " + rs.getString(7) + "/" + rs.getString(8);
+ //remove WRITE_SET info for current txn since it's about to abort
+ dbConn.rollback(undoWriteSetForCurrentTxn);
+ LOG.info(msg);
+ //todo: should make abortTxns() write something into TXNS.TXN_META_INFO about this
+ if (abortTxns(dbConn, Collections.singletonList(txnid), false, isReplayedReplTxn) != 1) {
+ throw new IllegalStateException(msg + " FAILED!");
+ }
+ dbConn.commit();
+ throw new TxnAbortedException(msg);
}
- dbConn.commit();
- throw new TxnAbortedException(msg);
}
}
+ } else if (isInsert(stmt, txnid)) {
Review comment:
You don't have to check for `insert` as there is no other alternative.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707182717
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
````With the new change we are only keeping the WRITE_SET based on the transactionId when the actual write was committed (not when the actual write was started), as the previous queries should not read the non-committed folder/data anyway````
that leads to the violation of the SNAPSHOT isolation, see `testUpdateSnapshotIsolation test
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707081936
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
This was the `findMinOpenTxnGLB`
```
/**
* See doc at {@link TxnStore#findMinOpenTxnId()}
* Note that {@link #openTxns(OpenTxnRequest)} makes update of NEXT_TXN and MIN_HISTORY_LEVEL
* a single atomic operation (and no one else should update these tables except the cleaner
* which deletes rows from MIN_HISTORY_LEVEL which can only allow minOpenTxn to move higher)
*/
private long findMinOpenTxnGLB(Statement stmt) throws MetaException, SQLException {
String s = "SELECT \"NTXN_NEXT\" FROM \"NEXT_TXN_ID\"";
LOG.debug("Going to execute query <" + s + ">");
ResultSet rs = stmt.executeQuery(s);
if (!rs.next()) {
throw new MetaException("Transaction tables not properly " +
"initialized, no record found in next_txn_id");
}
long hwm = rs.getLong(1);
s = "SELECT MIN(\"MHL_MIN_OPEN_TXNID\") FROM \"MIN_HISTORY_LEVEL\"";
LOG.debug("Going to execute query <" + s + ">");
rs = stmt.executeQuery(s);
rs.next();
long minOpenTxnId = rs.getLong(1);
if(rs.wasNull()) {
return hwm;
}
//since generating new txnid uses select for update on single row in NEXT_TXN_ID
assert hwm >= minOpenTxnId : "(hwm, minOpenTxnId)=(" + hwm + "," + minOpenTxnId + ")";
return minOpenTxnId;
}
```
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707012865
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
If my memory serves me well, `WS_COMMIT_ID` is the `NEXT_TXN_ID` at the time of the commit, and the `WS_TXNID` is the id of the given transaction
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679767488
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
@zchovan are you sure that is an equivalent change? @lcspinter, @pvary what do you think?
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707226585
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
+1
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707048954
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
that is correct. However, I don't understand how this is supposed to replace MIN_HISTORY table. Original code:
````
long minUncommittedTxnId = findMinOpenTxnGLB(stmt);
// If there are aborted txns, then the minimum aborted txnid could be the min_uncommitted_txnid
// if lesser than both NEXT_TXN_ID.ntxn_next and min(MIN_HISTORY_LEVEL .mhl_min_open_txnid).
String s = "SELECT MIN(\"TXN_ID\") FROM \"TXNS\" WHERE \"TXN_STATE\" = " + quoteChar(TXN_ABORTED);
minUncommittedTxnId = Math.min(minAbortedTxnId, minUncommittedTxnId);
```
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679770303
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java
##########
@@ -1442,7 +1443,9 @@ public void commitTxn(CommitTxnRequest rqst)
}
String conflictSQLSuffix = "FROM \"TXN_COMPONENTS\" WHERE \"TC_TXNID\"=" + txnid + " AND \"TC_OPERATION_TYPE\" IN (" +
- OperationType.UPDATE + "," + OperationType.DELETE + ")";
+ OperationType.UPDATE + "," + OperationType.DELETE + "," + OperationType.INSERT + ")";
Review comment:
Why is this needed? Looks like you are handling Inserts in a separate if block.
--
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] zchovan commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-889675384
retest
--
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] zchovan commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-915204071
@deniskuzZ I did some benchmarks on commitTxns, please find the results below:
`
Before HIVE-25346
Result "org.apache.hadoop.hive.metastore.tools.ACIDBenchmarks.TestOpenTxn.openTxn":
N = 100
mean = 58.136 ±(99.9%) 1.757 ms/op
Histogram, ms/op:
[40.000, 42.500) = 0
[42.500, 45.000) = 0
[45.000, 47.500) = 0
[47.500, 50.000) = 2
[50.000, 52.500) = 11
[52.500, 55.000) = 14
[55.000, 57.500) = 20
[57.500, 60.000) = 26
[60.000, 62.500) = 13
[62.500, 65.000) = 7
[65.000, 67.500) = 1
[67.500, 70.000) = 3
[70.000, 72.500) = 0
[72.500, 75.000) = 1
[75.000, 77.500) = 1
Percentiles, ms/op:
p(0.0000) = 49.111 ms/op
p(50.0000) = 57.856 ms/op
p(90.0000) = 63.579 ms/op
p(95.0000) = 67.901 ms/op
p(99.0000) = 79.537 ms/op
p(99.9000) = 79.567 ms/op
p(99.9900) = 79.567 ms/op
p(99.9990) = 79.567 ms/op
p(99.9999) = 79.567 ms/op
p(100.0000) = 79.567 ms/op
After HIVE-25346
Result "org.apache.hadoop.hive.metastore.tools.ACIDBenchmarks.TestOpenTxn.openTxn":
N = 100
mean = 51.015 ±(99.9%) 2.947 ms/op
Histogram, ms/op:
[30.000, 35.000) = 0
[35.000, 40.000) = 3
[40.000, 45.000) = 26
[45.000, 50.000) = 27
[50.000, 55.000) = 17
[55.000, 60.000) = 11
[60.000, 65.000) = 9
[65.000, 70.000) = 4
[70.000, 75.000) = 1
[75.000, 80.000) = 1
[80.000, 85.000) = 1
Percentiles, ms/op:
p(0.0000) = 38.385 ms/op
p(50.0000) = 48.248 ms/op
p(90.0000) = 64.176 ms/op
p(95.0000) = 68.280 ms/op
p(99.0000) = 81.816 ms/op
p(99.9000) = 81.838 ms/op
p(99.9900) = 81.838 ms/op
p(99.9990) = 81.838 ms/op
p(99.9999) = 81.838 ms/op
p(100.0000) = 81.838 ms/op
`
--
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] github-actions[bot] commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-974730512
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r679767488
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
@zchovan are you sure that is an equivalent change? @lcspinter what do you think?
--
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] zchovan edited a comment on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan edited a comment on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-895897980
> > > Would be great if you could run the HMS benchmark and see if that has affected the commit step performance. See if the index on opType improves the situation.
> >
> >
> > There is no benchmark currently for the commitTxn() call.
>
> Could we create one or at least perform similar to https://issues.apache.org/jira/browse/HIVE-23104?focusedCommentId=17083005&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17083005 test?
Based on the linked comment it is not clear to me what exactly was tested and how or what tools were used for it or what environment the tests were run in. I found no mention of the above details in the jira comments.
--
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] zchovan commented on pull request #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
zchovan commented on pull request #2547:
URL: https://github.com/apache/hive/pull/2547#issuecomment-895897980
> > > Would be great if you could run the HMS benchmark and see if that has affected the commit step performance. See if the index on opType improves the situation.
> >
> >
> > There is no benchmark currently for the commitTxn() call.
>
> Could we create one or at least perform similar to https://issues.apache.org/jira/browse/HIVE-23104?focusedCommentId=17083005&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17083005 test?
Based on the linked comment it is not clear to me what exactly was tested and how or what tools were used for it or what the environment the tests were run in. I found no mention of the above details in the jira comments.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707080469
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
This was the original code, so we might want to check `findMinOpenTxnGLB`:
```
// First need to find the min_uncommitted_txnid which is currently seen by any open transactions.
// If there are no txns which are currently open or aborted in the system, then current value of
// NEXT_TXN_ID.ntxn_next could be min_uncommitted_txnid.
long minUncommittedTxnId = findMinOpenTxnGLB(stmt);
// If there are aborted txns, then the minimum aborted txnid could be the min_uncommitted_txnid
// if lesser than both NEXT_TXN_ID.ntxn_next and min(MIN_HISTORY_LEVEL .mhl_min_open_txnid).
String s = "SELECT MIN(\"TXN_ID\") FROM \"TXNS\" WHERE \"TXN_STATE\" = " + TxnStatus.ABORTED;
LOG.debug("Going to execute query <" + s + ">");
rs = stmt.executeQuery(s);
if (rs.next()) {
long minAbortedTxnId = rs.getLong(1);
if (minAbortedTxnId > 0) {
minUncommittedTxnId = Math.min(minAbortedTxnId, minUncommittedTxnId);
}
}
```
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707012865
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
If my memory serves me well, `WS_COMMIT_ID` is the `NEXT_TXN_ID` at the time of the commit, and the `WS_TXNID` is the id of the given transaction and this was introduced when the `MIN_HISTORY` table was removed.
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707165193
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -531,7 +531,7 @@ public void cleanTxnToWriteIdTable() throws MetaException {
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\" " +
+ "SELECT MIN(\"WS_TXNID\") AS \"ID\" FROM \"WRITE_SET\" " +
Review comment:
So it think the theory is this:
- Original: Minimum open transactionId which was open when any of the currently executing transactions started.
- New: Minimum open transactionId which was open when the last write committed.
So originally we removed lines where the transaction older of any open/aborted transactions, and even the running transactions were not needed to see the older changes, but this was based on the starting time of the transaction.
With the new change we are only keeping the `WRITE_SET` based on the transactionId when the actual write was committed (not when the actual write was started), as the previous queries should not read the non-committed folder/data anyway
Question: What happens with the `WRITE_SET` when a compaction happened on this table (we are relying here on the fact that we are the only ones removing lines from this table)
--
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 #2547: HIVE-25346: cleanTxnToWriteIdTable breaks SNAPSHOT isolation
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2547:
URL: https://github.com/apache/hive/pull/2547#discussion_r707189777
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java
##########
@@ -3243,4 +3251,42 @@ public void testFullTableReadLock() throws Exception {
checkLock(LockType.SHARED_READ, LockState.ACQUIRED, "default", "tab_acid", null, locks);
checkLock(LockType.SHARED_READ, LockState.ACQUIRED, "default", "tab_not_acid", null, locks);
}
+
+ @Test
+ public void testInsertSnapshotIsolation() throws Exception {
+ driver.run("create table if not exists acid_insert_snapshot_isolation (a int, b int) " +
+ "stored as orc TBLPROPERTIES ('transactional'='true')");
+ driver.compileAndRespond("insert into acid_insert_snapshot_isolation values(1,2)");
+ DbTxnManager txnMgr2 = (DbTxnManager) TxnManagerFactory.getTxnManagerFactory().getTxnManager(conf);
+ swapTxnManager(txnMgr2);
+ driver2.compileAndRespond("select * from acid_insert_snapshot_isolation");
+ swapTxnManager(txnMgr);
+ driver.run();
+ txnHandler.cleanTxnToWriteIdTable();
+ swapTxnManager(txnMgr2);
+ driver2.run();
+ List res = new ArrayList();
+ driver2.getFetchTask().fetch(res);
+ Assert.assertEquals(0, res.size());
+ }
+
+ @Test
+ public void testUpdateSnapshotIsolation() throws Exception {
+ driver.run("create table if not exists acid_update_snapshot_isolation (a int, b int) " +
+ "stored as orc TBLPROPERTIES ('transactional'='true')");
+ driver.run("insert into acid_update_snapshot_isolation values(1,2)");
Review comment:
nit: formatting
--
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