forked from facebook/mysql-5.6
-
Notifications
You must be signed in to change notification settings - Fork 3
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
FB8-262: rpl_slave_tx_isolation fails in 8.0.23 #3
Closed
avbelov23
wants to merge
410
commits into
inikep:fb-8.0.23
from
avbelov23:avb-rpl_slave_tx_isolation
Closed
FB8-262: rpl_slave_tx_isolation fails in 8.0.23 #3
avbelov23
wants to merge
410
commits into
inikep:fb-8.0.23
from
avbelov23:avb-rpl_slave_tx_isolation
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: This diff adds logic that allows `libmysql` users to configure a callback with their own server cert validation logic even in case if they use SSL_CTX sreucture to specify TLS connection parameters. `mysql_options()` allows to clients to configure TLS parameters one by one using MYSQL_OPT_SSL_CERT, MYSQL_OPT_SSL_CAPATH, etc, or they can use MYSQL_OPT_SSL_CONTEXT option to provide a pre-initialized SSL_CTX structure. If MYSQL_OPT_SSL_CONTEXT option was set, all other SSL settings set by the client will be ignored. This diff makes libmysql respect the server cert validation callback set using MYSQL_OPT_TLS_CERT_CALLBACK in combination with MYSQL_OPT_SSL_CONTEXT. Differential Revision: D25325982 fbshipit-source-id: e41daab4487
Summary: [Mysql 8.0 Port] Avoid executing the identical statements in the same database Differential Revision: D25392410 fbshipit-source-id: d4250cf2343
Summary: Porting COLUMN_STATISTICS to MySQL 8.0 TABLE_JOINs show up as FILTER conditions with this version because prepare stage is one function with setup fields and query re-writing with optimizations for semi-joins etc. The approach required here would be slightly different. Reviewed By: mzait Differential Revision: D25551834 fbshipit-source-id: 85778fb2a57
Summary: This is a port of D22935862 (facebook@a3fcc94) (facebook@a3fcc94) and D24146869 (facebook@5011cd6) (facebook@5011cd6) to 8.0. Mostly the change is a boilerplate, with the only interesting bits of code in sql_class.* and pfs.cc. Reviewed By: mzait Differential Revision: D25507517 fbshipit-source-id: e4ffffc492a
…v support Summary: This is needed in case you are accidentally running a regular build without wsenv support in wsenv mode. We should fail fast in this case. Otherwise you may get wrong performance numbers or miss bugs. Differential Revision: D25631525 fbshipit-source-id: a255462d942
Summary: This is in preparation to porting the commit to 8.0. facebook@c1b8d83 The definition is needed because the struct os_event will be inlined in rwlock and mutex structures in subsequent commit. Differential Revision: D25364668 fbshipit-source-id: 58837fb40af
Summary: The change ports the commit to 8.0. facebook@c1b8d83 Due to extensive code changes between 5.6 and 8.0. This is not a cherry-picked commit. Instead I re-implemented this natively to 8.0. List of memory improvements by this change. Trimmed down some wasted space within the event structure. Consolidated m_set and signal_count. Before this, these 2 fields were taking combined 16 bytes (bool m_set changes the offset of signal_count by 8 bytes). Also removed event_iter field that is not accessed in code. event_iter was taking 16 bytes for every struct. Share the pthread mutex and condvar data in a pool for packed events. These two fields combined take 96 bytes and lead to a lot of wastage of memory in os_event. Changing os_event to use configurable pool enable sharing of these fields and saves a chunk of memory. Still allocate dedicated pthread data for each non-packed os_event_t. Added innodb_sync_pool_size sysvar to select pool size (def: 1024). Allow os_event structs to be packed into their parent structures. os_event. Packed event structures into mutex and rw_lock structures. The new size of os_event struct is just 16 bytes. So inlining this in parent structures living in buffer pool only increase the parent structure size by 8 bytes. Before this, parent structure was holding a pointer to os_event which was taking 8 bytes anyways. Memory usage test results (with 48 GB buffer pool) Space savings ~ 1.2 GB Differential Revision: D25364660 fbshipit-source-id: 55be76df3b8
Summary: This is a port of D24151364 (facebook@2d50f4c) (facebook@2d50f4c), D24319962 (facebook@a4406f7) (facebook@a4406f7) and D24556705 (facebook@ef3b4a3) (facebook@ef3b4a3). Reviewed By: lloyd Differential Revision: D25593439 fbshipit-source-id: 6a6f660b6a1
Summary: Today in wsenv there are built-in assumptions about SST file names - when files end with `.sst` wsenv will use more efficient encoding options, and LinkFile will also reject source files not ending with `.sst`. While these restrictions / hacks are temporary and in some cases unnecessary, it is just easier for us to follow this convention and change `bulk_load.tmp` to `bulk_load.tmp.sst` for bulk loading temp file names. It's probably not a bad change anyway since these are actually sst files created by `rocksdb::SstFileWriter`. Differential Revision: D25631859 fbshipit-source-id: 8b9d991b3c3
Summary: We noticed a divergent behavior with respect to enums in innodb vs rocksdb. This is because innodb treats alter table with compatible enum as inplace. But rocksdb always does alter by copy. With in-place alter, any insert does by RBR on the secondary instance is retained. This means that even if secondary sees enums that it does not yet recognize, it will still store that, until the alter runs on the standby. The fix makes the rocksdb behavior similar to innodb, by allowing inplace alter for ALTER_COLUMN_DEFAULT. The alter statement for compatible enums pass in ALTER_COLUMN_DEFAULT. Keeping this under system variable rocksdb_alter_column_default_inplace Reference patch: facebook@4911e09 Differential Revision: D25456572 fbshipit-source-id: 9ead7a17f05
Summary: * segment_reserve_factor determines the ratio of empty pages, the last segment in an ibd can have before innodb creates a new extent. If a new page is needed and the last segment has less than segment_reserve_factor * (current number of pages in this segment) empty pages, then a new extent is created and the new page is allocated from the new extent. The empty pages are useful when B-tree grows so that pages in the upper level of the B-tree can be allocated contiguously. An extent is 64 pages, and a segment is 256 extents. Before this diff, this variable was not configurable and it was determined by 1/x where x was the value defined by the compile time than 1/8th pages were empty, then InnoDB would allocate a new extent leaving many empty pages in the current extent. This caused lots of fragmentation. The ideal value for this variable needs investigation. The current default will have the previous behavior. A segment has 16384 pages, so for eg. If we are okay with leaving at most 16 empty pages per segment, then we can set innodb_segment_reserve_factor=0.001. innodb_segment_reserve_factor.test loads data into two identical tables but sets a lower segment_reserve_factor before loading data to the second table. This makes the second table about 10% smaller than the first table. The default value for this variable is set to 0.01 which should mean 1% of the pages will be allocated for growth. Reference Patch: facebook@0fd9494 Differential Revision: D25592735 fbshipit-source-id: 62df72553d7
Summary: Enabled rpl_raft_show_raft_logs mtr test case. Reviewed By: luqun Differential Revision: D25555318 fbshipit-source-id: bff142c2f82
Summary: check tittle Reviewed By: luqun Differential Revision: D25562747 fbshipit-source-id: 8219dc432e3
Summary: There's no reason why secondary key reads cannot support sorted MRR, since the `Mrr_sec_key_rowid_source::get_next_rowid` function is already iterating through the secondary key in order. The call to MultiGet still needs the sorted_input parameter set to false though, since the PK lookups are only ordered by SK, not PK. However, the MultiGet call preserves the key order that is passed in. I've also bumped the multiplier when calculating buffer size because for small tables, the estimate can be too small, leading to inefficient batching. It seems like this could also be an issue in production, so I changed the multiplier to 10x. We will regress memory a bit, but it is still bound by `mrr_batch_size`. Note that sorted MRR only supports forwards scan today. Reviewed By: luqun Differential Revision: D25600530 fbshipit-source-id: bb53cd249dc
…e debug build fbshipit-source-id: 6b1b5093f13
fbshipit-source-id: c7a68d13505
…ables=1 fbshipit-source-id: 1d11dd74a9f Squash with D21444116
Summary: This bug affects the async client case, in the event of a connection timeout. In some cases, we could be initiating a VIO shutdown and cleanup on a VIO struct that has a non-null ssl_arg using the fallback vio_delete() function instead of the vio_ssl_delete() function. This causes a memory leak on the SSL struct because nothing is freeing it. The ssl_arg is deleted in the call to vio_ssl_delete(), which gets called if the type == VIO_TYPE_SSL. However in some cases, the type we get is VIO_TYPE_TCPIP (probably some transient state, interrupted from becoming VIO_TYPE_SSL maybe by the timeout), and because of that, we call vio_delete() instead of vio_ssl_delete(), which doesn't free up the SSL data. Upstream bug: https://bugs.mysql.com/bug.php?id=102189 Reviewed By: lloyd Differential Revision: D25814364 fbshipit-source-id: 7f72d3100e2
Summary: Bypass updates rows_sent and rows_examined but didn't update rows_read. This fixes that and adds tests. We also get rocksdb_queries_point and rocksdb_queries_range for free as we are using MyRocks API to access data. Note I didn't include rows_examined in the test because querying unrelated information_schema tables will affect rows_examined and it is kinda flaky depending on many factors like perf schema. I did found a double counting issue with rocksdb_queries_range and fixed it as well. Reference Patch: facebook@6106eba ---- Porting Notes: Removed updates to table stats which is not ported to 8.0. Differential Revision: D25809584 fbshipit-source-id: 4cbb31e5b35
Summary: MYSQL_BIN_LOG class has a variable signal_cnt and its variable address should pass to raft side. in raft side, it will increase signal_cnt value by ++; Passing local signal_cnt to raft is dangerous and it will cause variable crash later(such one thread will accidentally change another thread local value). Differential Revision: D25864914 fbshipit-source-id: 86cd3917ef1
Summary: See title Differential Revision: D25859160 fbshipit-source-id: fc2e74f207c
Summary: Move the code using mutex into .cc file. Any plugin pulling in inlined code using mutex would have to be in sync with mysqld build in terms of the safe mutex usage. Removing this inlined code helps avoid this issue. Differential Revision: D25864393 fbshipit-source-id: 1dbb626ec0c
Summary: in addition to trace block cache access, add trace queries support for mysql update-submodule: rocksdb D24482306 landed to expose two API StartTrade and EndTrace on RocksDB, followed by add support on MySQL side Differential Revision: D24499637 fbshipit-source-id: b62e4cf1353
Summary: rpl_raft.rpl_raft_restart failed due to "Error writing master configuration." The issue is caused during we forgot to initialize master_log name and pos in raft_change_master(). later, try to flush/insert uninitialized master log name and pos into mysql.slave_master_info table, which may fail due to ER_RPL_INFO_DATA_TOO_LONG. ``` 2021-01-11T19:13:15.280079Z 6 [ERROR] [MY-010728] [Repl] Error writing master configuration. ^ Found warnings in /mnt/btrfs/trunk-git-mysql-131709377-1610391538/_build-8.0-Debug/mysql-test/var/1/log/mysqld.1.er ``` Differential Revision: D25877770 fbshipit-source-id: b35e1255dca
Summary: This diff fixes a few related issues: 1. We keep count of how many reads needing TTL had no snapshot timestamp, but this counter was never externally visible anywhere. I've added a new counter called `rocksdb_rows_unfiltered_no_snapshot` to track this. I've also added a debug assert that this should never happen. 2. During duplicate key checking, we may not have acquired any snapshots yet, meaning that there is no ttl timestamp. In the PK path, we use the current timestamp, but in the SK path, we just pass no timestamp, meaning that very old rows could be visible. Instead, we should just use the same consistent snapshot timestamp throughout the entire transaction, so the fix is acquire a snapshot when calling `check_uniqueness_and_lock`. This gives more correct repeatable read semantics. 3. The `m_is_delayed_snapshot` variable sometimes falls out of sync with the actual state in rocksdb. So we sometimes skip calling `SetSnapshotOnNextOperation` when we should, meaning that we would get inconsistent reads until a non-delayed snapshot was requested. Differential Revision: D25854248 fbshipit-source-id: 6af55ae30e7
Summary: During raft_reset_slave(), it specify 'mi->rli->inited = false;' but it forgot clean gtid_set in mi->rli, which cause later operation may accidentally write previous gtid_set info into new binary log even after `reset master;reset slave;` Differential Revision: D25933583 fbshipit-source-id: 22a908b7d02
Summary: This change adds a new build option `INSTALL_EXTRA_HEADERS` which would add additional headers to the install target. These headers would be consumed by fbcode plugins through tp2 without the need to maintain yet another copy of the headers. Additionally a couple of `#include`s are fixed to use full include path. This helps fbcode `TARGETS` file avoid specifying include directories reaching into `third-party-buck`. The only thing needed there would be this: ``` external_deps = [ ("mysql", None, "headers"), ], ``` Reviewed By: luqun Differential Revision: D25981543 fbshipit-source-id: 0d0d96a77ce
Summary: ASANDEBUG complains heap-use-after-free issue due to raft will store IO_CACHE from binlog, Later MYSQL_BIN_LOG will create a new binlog file and close its old binlog file(this also release old IO_CACHE). raft may use stale IO_CACHE during commit trx. The change is to register new binlog file to raft to make sure raft always keep latest IO_CACHE. TODO: We should update raft plugin to pass message to mysqld, mysqld should responsible to write message into IO_CACHE --- raft plugin shouldn't know IO_CACHE. Differential Revision: D25865086 fbshipit-source-id: 379339a8465
Summary: This adds write unprepared as part of the combinations tested in mtr. A few things were needed for write unprepared: 1. A new variable `rocksdb_write_batch_flush_threshold` was added that controls how large a write batch needs to be before the transaction starts writing to memtables. This is set to 1 in mtr, so that a flush is forced for every single write, just to stress this functionality. 2. XIDs need to be known at transaction start time, so that writes to the WAL from the same transaction can be correlated together on recovery. There is a debug check now at prepare time to verify that the XID hasn't somehow changed between transaction start and prepare. 3. Some minor test fixes that are documented inline. `rpl_gtid_crash_safe_wal_corrupt.inc` had to be updated because the WAL format is different. Porting notes: RocksDB had to be updated for 8.0 to pick up this patch (facebook/rocksdb@71239908). This is because 8.0 sometimes does not call index_end until after commit. Reference patch: facebook@f51e267 facebook@57e70d3 facebook@05af4e6 Differential Revision: D25468321 fbshipit-source-id: 94f81d53b2d fbshipit-source-id: a48ac155c3a
Summary: As raft works on LMP/DMP, change disallow_raft default value to false. Differential Revision: D25547337 fbshipit-source-id: 1f0a01c665f
inikep
pushed a commit
that referenced
this pull request
Jul 2, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Jul 2, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: This is an upstream bug: https://bugs.mysql.com/bug.php?id=92964 The issue is that if slave instance use sysvar_session_track_gtids == OWN_GTID and also enable MTS, the slave will lag behind master and its performance degrades over time. The reason for this lag/perf degrade is caused by each slave worker keep adding its own gtid into its own Session_consistency_gtids_ctx.m_gtid_set during each transaction commit. Overtime, some of Session_consistency_gtids_ctx.m_gtid_set may have millions gno_interval and it will take long time to insert 1 gtid into Session_consistency_gtids_ctx.m_gtid_set(time complexity O(n)), see [Gtid_set::add_gno_interval](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_gtid_set.cc#L323). There are couple related code to this issue: - During slave worker thread start, there is a [check](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1560) to not enable tracker for system threads but that check doesn't work---The THD system_thread field is initialized with default value(NON_SYSTEM_THREAD) during THD::THD. ``` m_enabled = thd->variables.session_track_gtids != OFF && /* No need to track GTIDs for system threads. */ thd->system_thread == NON_SYSTEM_THREAD; ``` ``` Call stack: ``` - in [Session_gtids_track::Update](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1581), the else code doesn't unregister listener due to mysql/mysql-server@1e0844c try to fix another issue. Solutions: 1. [Current]add system threads check during collecting data, since THD will be fully initialized at that time. -Pro: simple change(easy to port) and perf is ok 2. add `thd->session_track_gtids=OFF; thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` after [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: During init_slave_thread(), thd->system_thread will be set correctly value. -Con: it need to add couple places, such as handle_slave_io, handle_slave_sql, handle_slave_worker 3. add `thd->session_track_gtids=OFF;thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` inside [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: only add once for slave/slave worker/IO threads -Con: [should_collect](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_context.cc#L53) check still return true, thus it is less perf than #1 4. add `thd->session_track_gtids=OFF;thd->rpl_thd_ctx.session_gtids_ctx().unregister_ctx_change_listener(thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER));` inside init_slave_thread() -Pro: perf is better than #3, on par as #1 -Con: if there are other non-system threads(except IO/sql threads) do transactions, these threads will still collect its own gtids. 5. add another THD overload constructor which takes 'enum_thread_type' as a parameter whose default value is NON_SYSTEM_THREAD -Pro: it will initialize correct value with THD and won't enable those session_tracker -Con: similar to #2/#4, need to replace THD() with THD(enum_thread_type) couples places. Differential Revision: D18072672
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: Almost a direct port of the feature in 5.6 except for minor visual changes and some minor changes related to 8.0 thread handling Also brings in the basic raft_listener_queue without the implemented functions Differential Revision: D21670710 Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: port D21375044 (facebook@7fc9eaa) (facebook@7fc9eaa) and D17291883 (facebook@6a6a35f) (facebook@6a6a35f) **Summary** * As part of leadership changes, we will trim binlogs/relay logs on a running mysql instance. This needs the ability to remove gtids from executed_gtid set. This diff adds the ability to do the same. One can enqueue an event in the raft listener queue and the raft thread in the server will take care of removing and updating executed_gtid on a running instance. * When raft logs are trimmed as part of TruncateOpsAfter(), the gtid of the trimmed trxs are also removed from executed/logged gtids. However, when binlog files are rotated, the previous gtid written to the new file depends on what files are rotated. During binlog rotation the prev gtid are the logged/executed gtids of the instance. During relay log rotation prev gtids are the retrieved gtids of the instance. Hence, in addition to trimming logged gtids/executed gtids, we should also trim retrieved gtids. This prevents creation of holes in the instances executed_gtid set as the replicaset goes through multiple promotions over its lifetime. Reviewed By: luqun Differential Revision: D24690007 --------------------------------------------------------------------------------------- Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: This is an upstream bug: https://bugs.mysql.com/bug.php?id=92964 The issue is that if slave instance use sysvar_session_track_gtids == OWN_GTID and also enable MTS, the slave will lag behind master and its performance degrades over time. The reason for this lag/perf degrade is caused by each slave worker keep adding its own gtid into its own Session_consistency_gtids_ctx.m_gtid_set during each transaction commit. Overtime, some of Session_consistency_gtids_ctx.m_gtid_set may have millions gno_interval and it will take long time to insert 1 gtid into Session_consistency_gtids_ctx.m_gtid_set(time complexity O(n)), see [Gtid_set::add_gno_interval](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_gtid_set.cc#L323). There are couple related code to this issue: - During slave worker thread start, there is a [check](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1560) to not enable tracker for system threads but that check doesn't work---The THD system_thread field is initialized with default value(NON_SYSTEM_THREAD) during THD::THD. ``` m_enabled = thd->variables.session_track_gtids != OFF && /* No need to track GTIDs for system threads. */ thd->system_thread == NON_SYSTEM_THREAD; ``` ``` Call stack: ``` - in [Session_gtids_track::Update](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1581), the else code doesn't unregister listener due to mysql/mysql-server@1e0844c try to fix another issue. Solutions: 1. [Current]add system threads check during collecting data, since THD will be fully initialized at that time. -Pro: simple change(easy to port) and perf is ok 2. add `thd->session_track_gtids=OFF; thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` after [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: During init_slave_thread(), thd->system_thread will be set correctly value. -Con: it need to add couple places, such as handle_slave_io, handle_slave_sql, handle_slave_worker 3. add `thd->session_track_gtids=OFF;thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` inside [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: only add once for slave/slave worker/IO threads -Con: [should_collect](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_context.cc#L53) check still return true, thus it is less perf than #1 4. add `thd->session_track_gtids=OFF;thd->rpl_thd_ctx.session_gtids_ctx().unregister_ctx_change_listener(thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER));` inside init_slave_thread() -Pro: perf is better than #3, on par as #1 -Con: if there are other non-system threads(except IO/sql threads) do transactions, these threads will still collect its own gtids. 5. add another THD overload constructor which takes 'enum_thread_type' as a parameter whose default value is NON_SYSTEM_THREAD -Pro: it will initialize correct value with THD and won't enable those session_tracker -Con: similar to #2/#4, need to replace THD() with THD(enum_thread_type) couples places. Differential Revision: D18072672
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: Almost a direct port of the feature in 5.6 except for minor visual changes and some minor changes related to 8.0 thread handling Also brings in the basic raft_listener_queue without the implemented functions Differential Revision: D21670710 Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: port D21375044 (facebook@7fc9eaa) (facebook@7fc9eaa) and D17291883 (facebook@6a6a35f) (facebook@6a6a35f) **Summary** * As part of leadership changes, we will trim binlogs/relay logs on a running mysql instance. This needs the ability to remove gtids from executed_gtid set. This diff adds the ability to do the same. One can enqueue an event in the raft listener queue and the raft thread in the server will take care of removing and updating executed_gtid on a running instance. * When raft logs are trimmed as part of TruncateOpsAfter(), the gtid of the trimmed trxs are also removed from executed/logged gtids. However, when binlog files are rotated, the previous gtid written to the new file depends on what files are rotated. During binlog rotation the prev gtid are the logged/executed gtids of the instance. During relay log rotation prev gtids are the retrieved gtids of the instance. Hence, in addition to trimming logged gtids/executed gtids, we should also trim retrieved gtids. This prevents creation of holes in the instances executed_gtid set as the replicaset goes through multiple promotions over its lifetime. Reviewed By: luqun Differential Revision: D24690007 --------------------------------------------------------------------------------------- Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Jul 19, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
inikep
pushed a commit
that referenced
this pull request
Jul 30, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Jul 30, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
inikep
pushed a commit
that referenced
this pull request
Jul 31, 2024
Summary: This is an upstream bug: https://bugs.mysql.com/bug.php?id=92964 The issue is that if slave instance use sysvar_session_track_gtids == OWN_GTID and also enable MTS, the slave will lag behind master and its performance degrades over time. The reason for this lag/perf degrade is caused by each slave worker keep adding its own gtid into its own Session_consistency_gtids_ctx.m_gtid_set during each transaction commit. Overtime, some of Session_consistency_gtids_ctx.m_gtid_set may have millions gno_interval and it will take long time to insert 1 gtid into Session_consistency_gtids_ctx.m_gtid_set(time complexity O(n)), see [Gtid_set::add_gno_interval](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_gtid_set.cc#L323). There are couple related code to this issue: - During slave worker thread start, there is a [check](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1560) to not enable tracker for system threads but that check doesn't work---The THD system_thread field is initialized with default value(NON_SYSTEM_THREAD) during THD::THD. ``` m_enabled = thd->variables.session_track_gtids != OFF && /* No need to track GTIDs for system threads. */ thd->system_thread == NON_SYSTEM_THREAD; ``` ``` Call stack: ``` - in [Session_gtids_track::Update](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1581), the else code doesn't unregister listener due to mysql/mysql-server@1e0844c try to fix another issue. Solutions: 1. [Current]add system threads check during collecting data, since THD will be fully initialized at that time. -Pro: simple change(easy to port) and perf is ok 2. add `thd->session_track_gtids=OFF; thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` after [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: During init_slave_thread(), thd->system_thread will be set correctly value. -Con: it need to add couple places, such as handle_slave_io, handle_slave_sql, handle_slave_worker 3. add `thd->session_track_gtids=OFF;thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` inside [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: only add once for slave/slave worker/IO threads -Con: [should_collect](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_context.cc#L53) check still return true, thus it is less perf than #1 4. add `thd->session_track_gtids=OFF;thd->rpl_thd_ctx.session_gtids_ctx().unregister_ctx_change_listener(thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER));` inside init_slave_thread() -Pro: perf is better than #3, on par as #1 -Con: if there are other non-system threads(except IO/sql threads) do transactions, these threads will still collect its own gtids. 5. add another THD overload constructor which takes 'enum_thread_type' as a parameter whose default value is NON_SYSTEM_THREAD -Pro: it will initialize correct value with THD and won't enable those session_tracker -Con: similar to #2/#4, need to replace THD() with THD(enum_thread_type) couples places. Differential Revision: D18072672
inikep
pushed a commit
that referenced
this pull request
Jul 31, 2024
Summary: Almost a direct port of the feature in 5.6 except for minor visual changes and some minor changes related to 8.0 thread handling Also brings in the basic raft_listener_queue without the implemented functions Differential Revision: D21670710 Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Jul 31, 2024
Summary: port D21375044 (facebook@7fc9eaa) (facebook@7fc9eaa) and D17291883 (facebook@6a6a35f) (facebook@6a6a35f) **Summary** * As part of leadership changes, we will trim binlogs/relay logs on a running mysql instance. This needs the ability to remove gtids from executed_gtid set. This diff adds the ability to do the same. One can enqueue an event in the raft listener queue and the raft thread in the server will take care of removing and updating executed_gtid on a running instance. * When raft logs are trimmed as part of TruncateOpsAfter(), the gtid of the trimmed trxs are also removed from executed/logged gtids. However, when binlog files are rotated, the previous gtid written to the new file depends on what files are rotated. During binlog rotation the prev gtid are the logged/executed gtids of the instance. During relay log rotation prev gtids are the retrieved gtids of the instance. Hence, in addition to trimming logged gtids/executed gtids, we should also trim retrieved gtids. This prevents creation of holes in the instances executed_gtid set as the replicaset goes through multiple promotions over its lifetime. Reviewed By: luqun Differential Revision: D24690007 --------------------------------------------------------------------------------------- Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Jul 31, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Jul 31, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: MySQL bypass execution of select statements. All the query plan / execution are in `select_exec` class. When creating the query plan, the following two functions are the most important: 1. scan_value determines whether to unpack index, value, or both 2. scan_where determines which part of WHERE are indexes and builds the key with it, in index order. And the rest are filters used to include/skip values. When it comes to unpacking - we have two code path: 1. The unpack_fast* functions are the fast path that unpack directly into text buffer m_row_buf and a mapping data structure m_send_mapping that maps from SELECT item order to a (buf, len) tuple pointing to the text, later to be send out in the SELECT item order using protocol::store_string_data (I had to expose this internal function and rename it, but I'm open for suggestions. The string overload checks the type of argument so it asserts when you call store(string) for a int field, etc. But I'm wondering if this actually expose a bug that the client would see as incorrect type and our tools won't catch it. TODO: Validate this, but this is not necessarily blocking). Note in this case we don't support filtering which is to be added in the future for simple cases. 2. The slower path unpack* functions will unpack first into record[0], use eval_cond to do filtering using MySQL Item::val_int() to evaluate the conditional expressions. When sending rows, use protocol::send_result_set_row to send the results using select_lex->items pointing to record[0]. The rest should be self-explanatory. -------- Porting Notes: Removed unpack_fast_path as testing showed it didn't have much impact to the performance benchmarks. And also it didn't work in 8.0 because we've made some format changes to VARCHAR/BLOBs. This would greatly simplify bypass feature and make it work much better with future format changes. Reviewed By: luqun Differential Revision: D22818216 -------------------------------------------------------------------------------------------------- Fix bypass bug in prefix decoding optimization Summary: This is a classic stale pointer bug discovered in data-correctness testing and we are unpacking int fields to garbage as a result. In bypass we try to optimize prefix decoding for case like `SELECT d, c, b, a FROM t WHERE a=1 and b=2` with index being `(a, b, c, d)`. We unpack a, b and remember the location for the first time, and in future unpacking we can simply skip ahead to c, d. The problem is we remember the location of a/b in m_send_mapping with a pointer into m_row_buf (for unpacking) but that may reallocate and leading to stale pointers in m_send_mapping. The fix is to use offset rather than pointers. Also updated test case. But unfortunately it's hard to make sure the test actually did crash before the fix (usually it just prints out old value that are still there) - I did manually verify in debugging that it did trigger the condition. Reference Patch: facebook@e24b0bf454c --- Porting Notes: The fix no longer applies after removing fast unpacking code. Only keeping the tests. Reviewed By: luqun Differential Revision: D22854608 -------------------------------------------------------------------------------------------------- Need to consider all table fields when determine index/value packing Summary: For scenarios like `SELECT A FROM table WHERE B > 1`, we only consider the fields in the SELECT item list but not the WHERE condition, so we end up returning garbage. This uses readset and walk through all fields, just like what we do in setup_field_decoders. I have another pending change to make the cover check take columns keys into account. Fortunately for we don't have those in prod for bypass queries yet. Reference Patch: facebook@569f281 Reviewed By: Pushapgl Differential Revision: D26566928
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: some MTR failed in ubsan due to num of rows/records calculated in records_in_range() is a negative values which is caused by m_actual_disk_size is a negative value ``` storage/rocksdb/ha_rocksdb.cc:: runtime error: -56.8272 is outside the range of representable values of type 'unsigned long long' #0 myrocks::ha_rocksdb::records_in_range_internal(unsigned int, key_range*, key_range*, long, long, unsigned long long*, unsigned long long*) /data/sandcastle/boxes/trunk-git-mysql/storage/rocksdb/ha_rocksdb.cc:14855:16 #1 myrocks::ha_rocksdb::records_in_range(unsigned int, key_range*, key_range*) /data/sandcastle/boxes/trunk-git-mysql/storage/rocksdb/ha_rocksdb.cc:14760:3 #2 handler::multi_range_read_info_const(unsigned int, RANGE_SEQ_IF*, void*, unsigned int, unsigned int*, unsigned int*, Cost_estimate*) /data/sandcastle/boxes/trunk-git-mysql/sql/handler.cc:6608:26 #3 myrocks::ha_rocksdb::multi_range_read_info_const(unsigned int, RANGE_SEQ_IF*, void*, unsigned int, unsigned int*, unsigned int*, Cost_estimate*) /data/sandcastle/boxes/trunk-git-mysql/storage/rocksdb/ha_rocksdb.cc:19002:18 #4 check_quick_select(THD*, RANGE_OPT_P ``` Due to m_actual_disk_size is an estimated value, always reset to 0 if it i becomes negative. Differential Revision: D50531919
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: This is an upstream bug: https://bugs.mysql.com/bug.php?id=92964 The issue is that if slave instance use sysvar_session_track_gtids == OWN_GTID and also enable MTS, the slave will lag behind master and its performance degrades over time. The reason for this lag/perf degrade is caused by each slave worker keep adding its own gtid into its own Session_consistency_gtids_ctx.m_gtid_set during each transaction commit. Overtime, some of Session_consistency_gtids_ctx.m_gtid_set may have millions gno_interval and it will take long time to insert 1 gtid into Session_consistency_gtids_ctx.m_gtid_set(time complexity O(n)), see [Gtid_set::add_gno_interval](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_gtid_set.cc#L323). There are couple related code to this issue: - During slave worker thread start, there is a [check](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1560) to not enable tracker for system threads but that check doesn't work---The THD system_thread field is initialized with default value(NON_SYSTEM_THREAD) during THD::THD. ``` m_enabled = thd->variables.session_track_gtids != OFF && /* No need to track GTIDs for system threads. */ thd->system_thread == NON_SYSTEM_THREAD; ``` ``` Call stack: ``` - in [Session_gtids_track::Update](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/session_tracker.cc#L1581), the else code doesn't unregister listener due to mysql/mysql-server@1e0844c try to fix another issue. Solutions: 1. [Current]add system threads check during collecting data, since THD will be fully initialized at that time. -Pro: simple change(easy to port) and perf is ok 2. add `thd->session_track_gtids=OFF; thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` after [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: During init_slave_thread(), thd->system_thread will be set correctly value. -Con: it need to add couple places, such as handle_slave_io, handle_slave_sql, handle_slave_worker 3. add `thd->session_track_gtids=OFF;thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER)->update(thd);` inside [init_slave_thread()](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_slave.cc#L6788) -Pro: only add once for slave/slave worker/IO threads -Con: [should_collect](https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.13/sql/rpl_context.cc#L53) check still return true, thus it is less perf than #1 4. add `thd->session_track_gtids=OFF;thd->rpl_thd_ctx.session_gtids_ctx().unregister_ctx_change_listener(thd->session_tracker.get_tracker(SESSION_GTIDS_TRACKER));` inside init_slave_thread() -Pro: perf is better than #3, on par as #1 -Con: if there are other non-system threads(except IO/sql threads) do transactions, these threads will still collect its own gtids. 5. add another THD overload constructor which takes 'enum_thread_type' as a parameter whose default value is NON_SYSTEM_THREAD -Pro: it will initialize correct value with THD and won't enable those session_tracker -Con: similar to #2/#4, need to replace THD() with THD(enum_thread_type) couples places. Differential Revision: D18072672
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: Almost a direct port of the feature in 5.6 except for minor visual changes and some minor changes related to 8.0 thread handling Also brings in the basic raft_listener_queue without the implemented functions Differential Revision: D21670710 Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: port D21375044 (facebook@7fc9eaa) (facebook@7fc9eaa) and D17291883 (facebook@6a6a35f) (facebook@6a6a35f) **Summary** * As part of leadership changes, we will trim binlogs/relay logs on a running mysql instance. This needs the ability to remove gtids from executed_gtid set. This diff adds the ability to do the same. One can enqueue an event in the raft listener queue and the raft thread in the server will take care of removing and updating executed_gtid on a running instance. * When raft logs are trimmed as part of TruncateOpsAfter(), the gtid of the trimmed trxs are also removed from executed/logged gtids. However, when binlog files are rotated, the previous gtid written to the new file depends on what files are rotated. During binlog rotation the prev gtid are the logged/executed gtids of the instance. During relay log rotation prev gtids are the retrieved gtids of the instance. Hence, in addition to trimming logged gtids/executed gtids, we should also trim retrieved gtids. This prevents creation of holes in the instances executed_gtid set as the replicaset goes through multiple promotions over its lifetime. Reviewed By: luqun Differential Revision: D24690007 --------------------------------------------------------------------------------------- Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Aug 2, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
inikep
pushed a commit
that referenced
this pull request
Aug 6, 2024
Summary: Almost a direct port of the feature in 5.6 except for minor visual changes and some minor changes related to 8.0 thread handling Also brings in the basic raft_listener_queue without the implemented functions Differential Revision: D21670710 Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Aug 6, 2024
Summary: port D21375044 (facebook@7fc9eaa) (facebook@7fc9eaa) and D17291883 (facebook@6a6a35f) (facebook@6a6a35f) **Summary** * As part of leadership changes, we will trim binlogs/relay logs on a running mysql instance. This needs the ability to remove gtids from executed_gtid set. This diff adds the ability to do the same. One can enqueue an event in the raft listener queue and the raft thread in the server will take care of removing and updating executed_gtid on a running instance. * When raft logs are trimmed as part of TruncateOpsAfter(), the gtid of the trimmed trxs are also removed from executed/logged gtids. However, when binlog files are rotated, the previous gtid written to the new file depends on what files are rotated. During binlog rotation the prev gtid are the logged/executed gtids of the instance. During relay log rotation prev gtids are the retrieved gtids of the instance. Hence, in addition to trimming logged gtids/executed gtids, we should also trim retrieved gtids. This prevents creation of holes in the instances executed_gtid set as the replicaset goes through multiple promotions over its lifetime. Reviewed By: luqun Differential Revision: D24690007 --------------------------------------------------------------------------------------- Use DBUG_TRACE instead of DBUG_ENTER Summary: several testcase failed in asan debug, ==106552==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7f38f3590078 at pc 0x0000093aa495 bp 0x7f38f358ffc0 sp 0x7f38f358ffb8 READ of size 4 at 0x7f38f3590078 thread T203 #0 0x93aa494 in _db_enter_(char const*, int, char const*, unsigned int, _db_stack_frame_*) #1 0x8d0b9bb in Relay_log_info::remove_logged_gtids #2 0x8b1ec3f in trim_logged_gtid #3 0x8c767cf in process_raft_queue If Use DBUG_ENTER, then you need to use DBUG_RETURN to pop current frame in CODE_STATE. If use DBUG_TRACE, it will pop current frame during .dtor ps. small refactor changes for sql/rpl_handler.cc Reviewed By: bhatvinay Differential Revision: D28809418
inikep
pushed a commit
that referenced
this pull request
Aug 6, 2024
Summary: [Porting Notes] We want to dump raft logs to vanilla async replicas regardless of whether it's the relay log or binlog. Effectively after this change we'll dump relay logs on the followers and binlogs on the leader. When the raft role changes, the logs to the dumped are also changed. Dump_log class is introduced as a thin wrapper/continer around mysql_bin_log or rli->relay_log and is inited with mysql_bin_log to emulate vanilla mysql behavior. Dump threads use the global dump_log object instead of mysql_bin_log directly. We switch the log in dump log only when raft role changes (in binlog_change_to_binlog() and binlog_change_to_apply_log()). During raft role change we take all log releated locks (LOCK_log, LOCK_index, LOCK_binlog_end_pos, and dump log lock) to serialize it with other log operations like dumping logs. Related doc - https://fb.quip.com/oTVAAdgEi4zY This diff contains below 7 patches: D23013977 D24766787 D24716539 D24900223 D24955284 D25174166 D25775525 Reviewed By: luqun Differential Revision: D26141496 ------------------------------------------------------------------------------- Passing raw_log pointer to wait_with_heartbeat() and wait_without_heartbeat() Summary: When enable_raft_plugin is OFF Dump_log::lock() is a no-op. Which means that when enable_raft_plugin is OFF there can be a race between log switching and dump threads. This could lead to a scenario where the raw_log that wait_next_event() is working on might be different than what wait_with_heartbeat()/wait_without_heartbeat() is working on. This can cause deadlocks because wait_with_heartbeat()/wait_without_heartbeat()'s mysql_cond_wait would unlock and then lock a different log's LOCK_binlog_end_pos mutex which would then never be unlocked by wait_next_event(). Reviewed By: anirbanr-fb Differential Revision: D32152658 ----------------------------------------------------------------------------------------- Fix rpl_raft_dump_raft_logs Summary: This tests completes but fails because the following warning exists: ``` 2022-08-30T16:28:00.159525Z 11 [ERROR] [MY-013114] [Repl] Slave I/O for channel '': Got fatal error 1236 from master when reading data from binary log: 'Slave has more GTIDs than the master has, using the master's SERVER_UUID. This may indicate that the end of the binary log was truncated or that the last binary log file was lost, e.g., after a power or disk failure when sync_binlog != 1. The master may or may not have rolled back transactions that were already replicated to the slave. Suggest to replicate any transactions that master has rolled back from slave to master, and/or commit empty transactions on master to account for transactions that have been', Error_code: MY-013114 ``` Since the MTR result file is valid, we can suppress this error. Reviewed By: yichenshen Differential Revision: D39141846 ------------------------------------------------------------------------------- Fix heap overflow in group_relay_log_name handling Summary: We were accessing group_relay_log_name in Query_log_event::do_apply_event_worker() but it's assigned only after the coordinator thread encounters an end event (i.e. xid event or a query event with "COMMIT" or "ROLLBACK" query). This was causing a race between accessing group_relay_log_name in the worker thread and writing it on the coordinator thread. We don't need to set transaction position in events other than end event, so now we set transaction position in query event only if it's an end event. The race is eliminated because group_relay_log_name is set before enqueuing the event to the worker thread (in both dep repl and vanilla mts). Reviewed By: lth Differential Revision: D28767430 ------------------------------------------------------------------------------- fix memory during MYSQL_BIN_LOG::open_existing_binlog Summary: asandebug complain there are memory leaks during MYSQL_BIN_LOG open Direct leak of 50 byte(s) in 1 object(s) allocated from: #0 0x67460ef in malloc #1 0x93f0777 in my_raw_malloc(unsigned long, int) #2 0x93f064a in my_malloc(unsigned int, unsigned long, int) #3 0x93f0eb0 in my_strdup(unsigned int, char const*, int) #4 0x8af01a6 in MYSQL_BIN_LOG::open(unsigned int, char const*, char const*, unsigned int) #5 0x8af8064 in MYSQL_BIN_LOG::open_binlog(char const*, char const*, unsigned long, bool, bool, bool, Format_description_log_event*, unsigned int, RaftRotateInfo*, bool) #6 0x8b00c00 in MYSQL_BIN_LOG::new_file_impl(bool, Format_description_log_event*, RaftRotateInfo*) #7 0x8d65e47 in rotate_relay_log(Master_info*, bool, bool, bool, RaftRotateInfo*) #8 0x8d661c0 in rotate_relay_log_for_raft(RaftRotateInfo*) #9 0x8c7696a in process_raft_queue #10 0xa0fa1fd in pfs_spawn_thread(void*) #11 0x7f8c9a12b20b in start_thread release these memory before assign them Reviewed By: Pushapgl Differential Revision: D28819752
inikep
pushed a commit
that referenced
this pull request
Aug 6, 2024
Summary: Today in `SELECT count(*)` MyRocks would still decode every single column due to this check, despite the readset being empty: ``` // bitmap is cleared on index merge, but it still needs to decode columns bool field_requested = decode_all_fields || m_verify_row_debug_checksums || bitmap_is_set(field_map, m_table->field[i]->field_index); ``` As a result MyRocks is significantly slower than InnoDB in this particular scenario. Turns out in index merge, when it tries to reset, it calls ha_index_init with an empty column_bitmap, so our field decoders didn't know it needs to decode anything, so the entire query would return nothing. This is discussed in [this commit](facebook@70f2bcd), and [issue 624](facebook#624) and [PR 626](facebook#626). So the workaround we had at that time is to simply treat empty map as implicitly everything, and the side effect is massively slowed down count(*). We have a few options to address this: 1. Fix index merge optimizer - looking at the code in QUICK_RANGE_SELECT::init_ror_merged_scan, it actually fixes up the column_bitmap properly, but after init/reset, so the fix would simply be moving the bitmap set code up. For secondary keys, prepare_for_position will automatically call `mark_columns_used_by_index_no_reset(s->primary_key, read_set)` if HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (true for both InnoDB and MyRocks), so we would know correctly that we need to unpack PK when walking SK during index merge. 2. Overriding `column_bitmaps_signal` and setup decoders whenever the bitmap changes - however this doesn't work by itself. Because no storage engine today actually use handler::column_bitmaps_signal this path haven't been tested properly in index merge. In this case, QUICK_RANGE_SELECT::init_ror_merged_scan should call set_column_bitmaps_no_signal to avoid resetting the correct read/write set of head since head is used as first handler (reuses_handler=true) and subsequent place holders for read/write set updates (reuse_handler=false). 3. Follow InnoDB's solution - InnoDB delays it actually initialize its template again in index_read for the 2nd time (relying on `prebuilt->sql_stat_start`), and during index_read `QUICK_RANGE_SELECT::column_bitmap` is already fixed up and the table read/write set is switched to it, so the new template would be built correctly. In order to make it easier to maintain and port, after discussing with Manuel, I'm going with a simplified version of #3 that delays decoder creation until the first read operation (index_*, rnd_*, range_read_*, multi_range_read_*), and setting the delay flag in index_init / rnd_init / multi_range_read_init. Also, I ran into a bug with truncation_partition where Rdb_converter's tbl_def is stale (we only update ha_rocksdb::m_tbl_def), but it is fine because it is not being used after table open. But my change moves the lookup_bitmap initialization into Rdb_converter which takes a dependency on Rdb_converter::m_tbl_def so now we need to reset it properly. Reference Patch: facebook@44d6a8d --------- Porting Note: Due to 8.0's new counting infra (handler::record & handler::record_with_index), this only helps PK counting. Will send out a better fix that works better with 8.0 new counting infra. Reviewed By: Pushapgl Differential Revision: D26265470
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PSI thread requires pointer to thread and thread id