Skip to content

/mysql-5.6 forked from facebook/mysql-5.6

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Clone in Desktop Download ZIP

Loading…

New issue

Don't update indexes if index values have not changed #1

Closed
jonahcohen opened this Issue · 3 comments

Labels

None yet

Milestone

  high-pri

Assignee

@spetrunia spetrunia

3 participants

@jonahcohen @spetrunia @yoshinorim
@jonahcohen
Collaborator
jonahcohen commented

Currently, ha_rocksdb::update_row() updates all indexes, even those who do not have a single changed column. We should update only those whose values were not changed.

A question: are there any rocksdb status counters that could be used to observe this?

@yoshinorim yoshinorim referenced this issue
Closed

slave crash on update with row based binary logging #18

@yoshinorim yoshinorim referenced this issue from a commit
@yoshinorim yoshinorim Supporting START TRANSACTION WITH CONSISTENT [ROCKSDB] SNAPSHOT … 
Summary:
This adds two features in RocksDB.
1. Supporting START TRANSACTION WITH CONSISTENT SNAPSHOT
2. Getting current binlog position in addition to #1.
With these features, mysqldump can take consistent logical backup.

The second feature is done by START TRANSACTION WITH
CONSISTENT ROCKSDB SNAPSHOT. This is Facebook's extension, and
it works like existing START TRANSACTION WITH CONSISTENT INNODB SNAPSHOT.

This diff changed some existing codebase/behaviors.

- Original Facebook-MySQL always started InnoDB transaction
regardless of engine clause. For example, START TRANSACTION WITH
CONSISTENT MYISAM SNAPSHOT was accepted but it actually started
InnoDB transaction, not MyISAM. This patch does not allow
setting engine that does not support consistent snapshot.

mysql> start transaction with consistent myisam snapshot;
ERROR 1105 (HY000): Consistent Snapshot is not supported for this engine

Currently only InnoDB and RocksDB support consistent snapshot.
To check engines, I modified sql/sql_yacc.yy, trans_begin()
and ha_start_consistent_snapshot() to pass handlerton.

- Changed constant name from
  MYSQL_START_TRANS_OPT_WITH_CONS_INNODB_SNAPSHOT to
MYSQL_START_TRANS_OPT_WITH_CONS_ENGINE_SNAPSHOT, because it's no longer
InnoDB dependent.

- When not setting engine, START TRANSACTION WITH CONSISTENT SNAPSHOT
takes both InnoDB and RocksDB snapshots, and both InnoDB and RocksDB
participate in transaction. When executing COMMIT, both InnoDB and
RocksDB modifications are committed. Remember that XA is not supported yet,
so mixing engines is not recommended anyway.

- When setting engine, START TRANSACTION WITH CONSISTENT.. takes
snapshot for the specified engine only. But it starts both
InnoDB and RocksDB transactions.

Test Plan: mtr --suite=rocksdb,rocksdb_rpl, --repeat=3

Reviewers: hermanlee4, jonahcohen, jtolmer, tian.xia, maykov, spetrunia

Reviewed By: spetrunia

Subscribers: steaphan

Differential Revision: https://reviews.facebook.net/D32355
 7f7da9f
@yoshinorim yoshinorim added this to the high-pri milestone
@spetrunia spetrunia was assigned by yoshinorim
@spetrunia
Collaborator
spetrunia commented

There are two ways to detect that the columns haven't changed:

  • Look at table->write_set and find which columns are written. If some index doesn't have any columns that are written, it doesn't need to be updated
    ** We don't need to make the check for every row, it is sufficient to do it per-query (todo is it the same as per-query or different?)
    ** Need to run a few examples to check that SQL layer indeed sets table->write_set to those columns that are updated

  • Right before issuing rocksdb->Put()/rocksdb->Delete() calls, check if we're going to write the same value as the one that's already there.

@yoshinorim
Owner
yoshinorim commented

If it works as expected, I like the first approach.

@spetrunia
Collaborator
spetrunia commented

Implemented the first approach and committed here: https://reviews.facebook.net/D33471

@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Issue #1: Don't update indexes if index values have not changed … 
Summary:
Don't update secondary indexes that do not have columns that
are included in table->write_map at query start.

Test Plan: mtr

Reviewers: maykov, yoshinorim, hermanlee4, jonahcohen

Reviewed By: jonahcohen

Differential Revision: https://reviews.facebook.net/D33471
 c513a0c
@spetrunia spetrunia closed this
This was referenced
Closed
Closed
@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Issue #67: Inefficient index condition pushdown … 
Summary:
Inside index_next_same() call, we should
1. first check whether the record matches the index
   lookup prefix,
2. then check pushed index condition.

If we try to check #2 without checking #1 first, we may walk
off the index lookup prefix and scan till the end of the index.

Test Plan: Run mtr

Reviewers: hermanlee4, maykov, jtolmer, yoshinorim

Reviewed By: yoshinorim

Differential Revision: https://reviews.facebook.net/D38769
 0fe186f
@maykov maykov referenced this issue from a commit
@maykov maykov Testing the ASAN failure … 
Summary:
I saw this intermittent ASAN failure
==662590== ERROR: AddressSanitizer: SEGV on unknown address 0x000000000098 (pc 0x00000112fc64 sp 0x7f0324c71710 bp 0x7f0324c71760 T21)
AddressSanitizer can not provide additional info.
    #0 0x112fc63 in my_pthread_fastmutex_lock /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/mysys/thr_mutex.c:479
    #1 0x18fb4b0 in _ZN23Dropped_indices_manager14remove_indicesERKSt13unordered_setIjSt4hashIjESt8equal_toIjESaIjEEP12Dict_manager /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/include/mysql/psi/mysql_thread.h:688
    #2 0x18b031a in _Z17drop_index_threadPv /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/storage/rocksdb/ha_rocksdb.cc:4927
    #3 0x7f0337d611e8 in _ZN6__asan10AsanThread11ThreadStartEv ??:0
    #4 0x7f03376dbfa7 in start_thread ??:0
    #5 0x7f0335a5f5bc in __clone ??:0
Thread T21 created by T0 here:
    #0 0x7f0337d5121b in pthread_create ??:0
    #1 0x18aec6a in _ZL17rocksdb_init_funcPv /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/storage/rocksdb/ha_rocksdb.cc:1765
    #2 0x60a039 in _Z24ha_initialize_handlertonP13st_plugin_int /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/sql/handler.cc:673
    #3 0xab04e0 in _ZL17plugin_initializeP13st_plugin_int /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/sql/sql_plugin.cc:1137
    #4 0xac8fe3 in _Z11plugin_initPiPPci /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/sql/sql_plugin.cc:1431
    #5 0x5ef74c in _ZL22init_server_componentsv .cc:5482
    #6 0x5f0cce in _Z11mysqld_mainiPPc .cc:6254
    #7 0x7f033597defe in __libc_start_main ??:0
    #8 0x5d86a8 in _start /home/engshare/third-party2/glibc/2.17/src/glibc-2.17/csu/../sysdeps/x86_64/start.S:123
==662590== ABORTING
It looks like the Dropped_indices_manager mutex is accessed by the thread after the mutex was cleaned up. I'm moving the cleanup of the manager to the thread in hopes that this will fix the failure.

Test Plan: ran rocksdb suite, nothing failed

Reviewers: hermanlee4

Reviewed By: hermanlee4

Differential Revision: https://reviews.facebook.net/D42909
 eeb5ef2
@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Fix Issue #1: AUTO_INCREMENT value doesn't survive server shutdown … 
Use ha_rocksdb::open() should check table->found_next_number_field,
not table->next_number_field (like InnoDB does)
 16670bd
@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Add remaining tests to mysql-test/suite/rocksdb. This fixes Issue #1 … 
Two testcases are empty because RocksDB-SE doesn't support the feature:
-autoincrement.test
-autoinc_secondary.test
 33131cc
@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Fix Issue #1: Enable Row-based binlogging and disable statement-based. 7c11cf4
@yoshinorim yoshinorim referenced this issue from a commit
@yoshinorim yoshinorim Supporting START TRANSACTION WITH CONSISTENT [ROCKSDB] SNAPSHOT … 
Summary:
This adds two features in RocksDB.
1. Supporting START TRANSACTION WITH CONSISTENT SNAPSHOT
2. Getting current binlog position in addition to #1.
With these features, mysqldump can take consistent logical backup.

The second feature is done by START TRANSACTION WITH
CONSISTENT ROCKSDB SNAPSHOT. This is Facebook's extension, and
it works like existing START TRANSACTION WITH CONSISTENT INNODB SNAPSHOT.

This diff changed some existing codebase/behaviors.

- Original Facebook-MySQL always started InnoDB transaction
regardless of engine clause. For example, START TRANSACTION WITH
CONSISTENT MYISAM SNAPSHOT was accepted but it actually started
InnoDB transaction, not MyISAM. This patch does not allow
setting engine that does not support consistent snapshot.

mysql> start transaction with consistent myisam snapshot;
ERROR 1105 (HY000): Consistent Snapshot is not supported for this engine

Currently only InnoDB and RocksDB support consistent snapshot.
To check engines, I modified sql/sql_yacc.yy, trans_begin()
and ha_start_consistent_snapshot() to pass handlerton.

- Changed constant name from
  MYSQL_START_TRANS_OPT_WITH_CONS_INNODB_SNAPSHOT to
MYSQL_START_TRANS_OPT_WITH_CONS_ENGINE_SNAPSHOT, because it's no longer
InnoDB dependent.

- When not setting engine, START TRANSACTION WITH CONSISTENT SNAPSHOT
takes both InnoDB and RocksDB snapshots, and both InnoDB and RocksDB
participate in transaction. When executing COMMIT, both InnoDB and
RocksDB modifications are committed. Remember that XA is not supported yet,
so mixing engines is not recommended anyway.

- When setting engine, START TRANSACTION WITH CONSISTENT.. takes
snapshot for the specified engine only. But it starts both
InnoDB and RocksDB transactions.

Test Plan: mtr --suite=rocksdb,rocksdb_rpl, --repeat=3

Reviewers: hermanlee4, jonahcohen, jtolmer, tian.xia, maykov, spetrunia

Reviewed By: spetrunia

Subscribers: steaphan

Differential Revision: https://reviews.facebook.net/D32355
 8cf2f3a
@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Issue #1: Don't update indexes if index values have not changed … 
Summary:
Don't update secondary indexes that do not have columns that
are included in table->write_map at query start.

Test Plan: mtr

Reviewers: maykov, yoshinorim, hermanlee4, jonahcohen

Reviewed By: jonahcohen

Differential Revision: https://reviews.facebook.net/D33471
 e90f795
@spetrunia spetrunia referenced this issue from a commit
@spetrunia spetrunia Issue #67: Inefficient index condition pushdown … 
Summary:
Inside index_next_same() call, we should
1. first check whether the record matches the index
   lookup prefix,
2. then check pushed index condition.

If we try to check #2 without checking #1 first, we may walk
off the index lookup prefix and scan till the end of the index.

Test Plan: Run mtr

Reviewers: hermanlee4, maykov, jtolmer, yoshinorim

Reviewed By: yoshinorim

Differential Revision: https://reviews.facebook.net/D38769
 32c3ac8
@maykov maykov referenced this issue from a commit
@maykov maykov Testing the ASAN failure … 
Summary:
I saw this intermittent ASAN failure
==662590== ERROR: AddressSanitizer: SEGV on unknown address 0x000000000098 (pc 0x00000112fc64 sp 0x7f0324c71710 bp 0x7f0324c71760 T21)
AddressSanitizer can not provide additional info.
    #0 0x112fc63 in my_pthread_fastmutex_lock /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/mysys/thr_mutex.c:479
    #1 0x18fb4b0 in _ZN23Dropped_indices_manager14remove_indicesERKSt13unordered_setIjSt4hashIjESt8equal_toIjESaIjEEP12Dict_manager /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/include/mysql/psi/mysql_thread.h:688
    #2 0x18b031a in _Z17drop_index_threadPv /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/storage/rocksdb/ha_rocksdb.cc:4927
    #3 0x7f0337d611e8 in _ZN6__asan10AsanThread11ThreadStartEv ??:0
    #4 0x7f03376dbfa7 in start_thread ??:0
    #5 0x7f0335a5f5bc in __clone ??:0
Thread T21 created by T0 here:
    #0 0x7f0337d5121b in pthread_create ??:0
    #1 0x18aec6a in _ZL17rocksdb_init_funcPv /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/storage/rocksdb/ha_rocksdb.cc:1765
    #2 0x60a039 in _Z24ha_initialize_handlertonP13st_plugin_int /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/sql/handler.cc:673
    #3 0xab04e0 in _ZL17plugin_initializeP13st_plugin_int /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/sql/sql_plugin.cc:1137
    #4 0xac8fe3 in _Z11plugin_initPiPPci /data/users/jenkins/workspace/myrocks-prod/BUILD_TYPE/ASan/CLIENT_MODE/Sync/PAGE_SIZE/32/TEST_SET/MixedOther/label/mysql/mysql/sql/sql_plugin.cc:1431
    #5 0x5ef74c in _ZL22init_server_componentsv .cc:5482
    #6 0x5f0cce in _Z11mysqld_mainiPPc .cc:6254
    #7 0x7f033597defe in __libc_start_main ??:0
    #8 0x5d86a8 in _start /home/engshare/third-party2/glibc/2.17/src/glibc-2.17/csu/../sysdeps/x86_64/start.S:123
==662590== ABORTING
It looks like the Dropped_indices_manager mutex is accessed by the thread after the mutex was cleaned up. I'm moving the cleanup of the manager to the thread in hopes that this will fix the failure.

Test Plan: ran rocksdb suite, nothing failed

Reviewers: hermanlee4

Reviewed By: hermanlee4

Differential Revision: https://reviews.facebook.net/D42909
 28c20ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.