Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only rebase transaction start for read replica #3722

Merged
merged 12 commits into from
Jul 2, 2022

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Jun 29, 2022

Changes:

  • Remove rebase on TransactionStart, unless the engine is a read replica, in which case we continue to poll for updates.
  • Starting a server writes a lock file .dolt/sql-server.lock that is deleted when the server is terminated.
  • A SqlEngine created after a server lock is written is limited to ReadOnly mode. Bulk table import does not appear to respect read only mode, so it has an additional check.

@max-hoffman
Copy link
Contributor Author

#benchmark

@max-hoffman
Copy link
Contributor Author

#newbenchmark

@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@max-hoffman

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 8.28 7.84 0
groupby_scan 33.12 32.53 0
index_join_scan 34.95 34.95 0
index_scan 81.48 81.48 0
oltp_point_select 1.39 1.23 1
oltp_read_only 13.46 13.22 0
select_random_points 2.22 2.07 0
select_random_ranges 2.39 2.22 0
table_scan 78.6 78.6 0
write_tests from_latency_median to_latency_median is_faster
bulk_insert 0.001 0.001 0
oltp_delete 2.11 1.96 0
oltp_insert 10.09 9.91 0
oltp_read_write 44.17 43.39 0
oltp_update_index 11.45 11.45 0
oltp_update_non_index 8.58 8.43 0
oltp_write_only 30.81 30.81 0

2 similar comments
@github-actions
Copy link

@max-hoffman

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 8.28 7.84 0
groupby_scan 33.12 32.53 0
index_join_scan 34.95 34.95 0
index_scan 81.48 81.48 0
oltp_point_select 1.39 1.23 1
oltp_read_only 13.46 13.22 0
select_random_points 2.22 2.07 0
select_random_ranges 2.39 2.22 0
table_scan 78.6 78.6 0
write_tests from_latency_median to_latency_median is_faster
bulk_insert 0.001 0.001 0
oltp_delete 2.11 1.96 0
oltp_insert 10.09 9.91 0
oltp_read_write 44.17 43.39 0
oltp_update_index 11.45 11.45 0
oltp_update_non_index 8.58 8.43 0
oltp_write_only 30.81 30.81 0

@github-actions
Copy link

@max-hoffman

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 8.28 7.84 0
groupby_scan 33.12 32.53 0
index_join_scan 34.95 34.95 0
index_scan 81.48 81.48 0
oltp_point_select 1.39 1.23 1
oltp_read_only 13.46 13.22 0
select_random_points 2.22 2.07 0
select_random_ranges 2.39 2.22 0
table_scan 78.6 78.6 0
write_tests from_latency_median to_latency_median is_faster
bulk_insert 0.001 0.001 0
oltp_delete 2.11 1.96 0
oltp_insert 10.09 9.91 0
oltp_read_write 44.17 43.39 0
oltp_update_index 11.45 11.45 0
oltp_update_non_index 8.58 8.43 0
oltp_write_only 30.81 30.81 0

@github-actions
Copy link

@max-hoffman

test_name from_latency_median to_latency_median is_faster
tpcc-scale-factor-1 3982.86 3639.94 0
test_name server_name server_version tps test_name server_name server_version tps is_faster
tpcc-scale-factor-1 dolt 972c3ca 1.68 tpcc-scale-factor-1 dolt d54b52d 1.38 0

@github-actions
Copy link

@max-hoffman

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 6.91 6.67 0
groupby_scan 65.65 65.65 0
index_join_scan 34.33 34.33 0
index_scan 75.82 74.46 0
oltp_point_select 0.83 0.7 1
oltp_read_only 13.46 13.22 0
select_random_points 2.18 2.0 0
select_random_ranges 1.73 1.61 0
table_scan 70.55 69.29 0
write_tests from_latency_median to_latency_median is_faster
bulk_insert 0.001 0.001 0
oltp_delete 1.1 0.99 0
oltp_insert 3.43 3.36 0
oltp_read_write 21.89 21.5 0
oltp_update_index 5.0 5.18 0
oltp_update_non_index 5.18 5.47 0
oltp_write_only 9.73 9.73 0

@timsehn
Copy link
Sponsor Contributor

timsehn commented Jun 29, 2022

These still aren't bad perf wine ~5-10% on most metrics.

}

// IsLocked returns true if this database's lockfile exists
func (dEnv *DoltEnv) IsLocked() bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should these be mutex protected?

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM

As a followup, you need to extend this to all dolt commands which can update db state.

@@ -35,6 +36,8 @@ import (
"github.com/dolthub/dolt/go/libraries/doltcore/sqlserver"
)

var ErrActiveServerLock = errors.New("database locked by another sql-server; either clone the database to run a second server, or delete the '.dolt/sql-server.lock' if no other sql-servers are active")
Copy link
Member

Choose a reason for hiding this comment

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

Error message should have the absolute path to the lock file

@@ -32,6 +33,8 @@ import (
"github.com/dolthub/dolt/go/libraries/utils/filesys"
)

var ErrActiveServerLock = errors.New("database locked by another sql-server; either clone the database to run a second server, or delete the '.dolt/sql-server.lock' if no other sql-servers are active")
Copy link
Member

Choose a reason for hiding this comment

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

What's with the duplicate error def?

@@ -2,7 +2,7 @@
set -e
set -o pipefail

SYSBENCH_TEST="oltp_point_select"
Copy link
Member

Choose a reason for hiding this comment

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

Revert?

@max-hoffman max-hoffman merged commit d5ec438 into main Jul 2, 2022
@max-hoffman max-hoffman deleted the max/only-rebase-for-read-replica branch July 2, 2022 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants