- developers - lists.mariadb.org

Prepared statements analytics problem
by Dmitrii Kochetov 03 Jul '25

03 Jul '25

Most database monitoring systems, such as Newrelic, Datadog, Dolphie, Ottertune, Releem, Solarwinds, use data from the performance_schema database from the events_statements_summary_by_digest and events_statements_* tables to get query analytics for databases. This information allows to calculate system performance metrics, in particular Latency. Also, the data from these tables allows you to get a complete picture of the database operation - what queries are executed, their execution statistics, to get query templates based on digest_text, and based on SQL_TEXT field from events_statements_* or QUERY_SAMPLE_TEXT from events_statements_summary_by_digest (for MySQL >= 8) to get a sample query for digest and optimize it. The prepared statements analytics are contained in a single table prepared_statements_instances, which is managed by Performance_schema as follows: - Statement preparation If the statement is successfully instrumented, a new row is added to the prepared_statements_instances table. If the statement cannot be instrumented, Performance_schema_prepared_statements_lost status variable is incremented. - Prepared statement execution Execution of a EXECUTE command for an instrumented prepared statement instance updates the corresponding prepared_statements_instances table row. - Prepared statement deallocation Execution of a CLOSE command for an instrumented prepared statement instance removes the corresponding prepared_statements_instances table row. To avoid resource leaks, removal occurs even if the prepared statement instruments described previously are disabled. In this regard, there are the following problems with analytics for prepared statements compared to regular queries: 1) Most applications work with prepared statements using the following method when processing a single query - creating a prepared statement, executing the prepared statement, and then dropping the prepared statement. This leads to the fact that query statistics exist for a very short period of time, during which it may not be possible to fetch them. 2) The second problem of prepared statement analytics in comparison with regular queries is the absence of the query hash (digest) and information about the schema_name in which the prepared statement is executed, which makes it difficult to identify unique queries, since a separate record in the Prepared_Statements_instances table is created for each creation of a prepared statement. 3) Execution of prepared statements is possible in two protocols: Binary and text. Execution of a prepared statement in the binary protocol does not create an entry in the events_statements_* tables, but execution in the text protocol creates an entry in the specified tables, which leads to different behaviors of the same prepared statement but in different protocols. 4) For prepared statements, there is no option in getting an example request without a ? placeholder. The SQL_TEXT field in Prepared_Statements_instances contains The prepared statement text, with ? placeholder markers, and in some cases (when executing in the text protocol) it is possible to find only the execute command of this statement, which further requires mapping ? placeholder markers with parameters from the execute request. But there are limitations that the binary protocol does not store EXECUTE requests for prepared statements, and most of the interactions with the database occur through it, through drivers for different programming languages, and another limitation may be the use of variables in the EXECUTE command, the value of which still needs to be obtained. Proposed solution: 1) Records should not be deleted from the Prepared_Statements_instances table after dropping prepared statements. If the table is full, the counter Performance_schema_prepared_statements_lost and If no row has the statement digest value for the statement that just completed, and the table is full, the statistics for the statement that just completed are added to a special “catch-all” row with DIGEST = NULL, which is created if necessary. 2) Each statement must be unique by the fields schema_name, in which the given statement is executed and its hash, digest, which will be the same for the same prepared statement. 3) In the tables events_statements_* must be recorded information about the fact of PREPARE, EXECUTE and DEALLOCATE execution for prepared statement, where in the DIGEST_TEXT field is stored the call of the execute command, and in SQL_TEXT - the normalized prepared query, in which ? placeholder markers are replaced by concrete values.

2 2

Points of low-level design for MDEV-34705, binlog-in-engine
by Kristian Nielsen 30 Jun '25

30 Jun '25

As the new binlog format (MDEV-34705 Binlog-in-engine) is taking shape, I wanted to write up some more details on a number of the low-level design decisions made so far. Re-implementing the binlog format is a huge undertaking due to way it interacts with many different parts of the server code, as well as user-visible interfaces. I am not sure how many people actually read these detailed write-ups, but I think it is still important to at least try to present the design and give others a chance to comment. So many other large changes to or around the server code are being done without even an attempt to proper design review, often with serious defects as a result. All of the design elements mentioned in this mail can be seen in the code available in the branch "knielsen_binlog_in_engine" on Github: https://github.com/MariaDB/server/commits/knielsen_binlog_in_engine File format =========== It was clear from the start that a page-based file format should be used. Initially, the idea was to use a new type of InnoDB tablespace. But after discussions with Marko, this was revised to use a simplified custom format for the binlog files. This avoids much of the overhead of a full InnoDB tablespace and page format, but at the cost of having to implement new code to take over the functionality of the InnoDB buffer pool and recovery code. The InnoDB redo log is still used, with only minor modifications. The page size is currently 4kB, I'm considering 16kB to reduce per-page runtime overhead. The page size is independent of the InnoDB page size and can easily be made configurable. The first page of the file is reserved as a header page; rest of pages have only a four-byte overhead for a CRC32. The CRC32 replaces the iffy per-event checksum in the old legacy binlog. Using a page-based format means it is possible to random-access read the binlog file (this is not possible in the legacy binlog, as there is no easy way to determine the boundaries between events). The GTID indexes are thus no longer necessary; instead we simply binary-search the binlog file for the starting GTID position. To facilitate this binary search, the current GTID binlog state is written (compressed) every --innodb-binlog-state-interval bytes. Similarly, at restart restart, binary search is used to find the position at which to continue writing the last binlog file (as opposed to forcing a binlog rotation as in the legacy binlog). A similar mechanism is used to recover the current gtid_binlog_state without the need for a master-bin.state file. In the new binlog implementation, binlog files are identified solely by their number (starting from 0). The binlog file name is engine-specific, not user configurable. This considerably simplifies the binlog handling, avoiding the need for the binlog index file. A new --binlog-directory option allows to still place the binlog on a file system of choice. Generally 64-bit numbers are used to identify binlog file numbers, sizes, offsets, etc. to avoid limitations like the legacy binlog's 32-bit file offsets. A simple compression scheme is used to store small numbers in few bytes. See storage/innobase/include/ut0compr_int.h for the implementation, which is written to be efficient on modern superscalar CPUs, and helps avoid 64-bit penalty on data storage sizes. In the current design, the new binlog format is not very InnoDB specific, but it is still implemented inside the storage engine, in principle in an engine-dependent format. The advantage of this is that another engine has the flexibility to implement a different format, perhaps even interleaving the binlog data with other data in existing data files. On the other hand, fixing the binlog file format like it is with InnoDB would allow to move part of the code to the server layer and share it between engines. I believe this decision would be best revisited when/if another engine binlog implementation becomes relevant. The function ibb_write_header_page() shows how the header page is written. The function innodb_binlog_prealloc_thread() runs the background thread that pre-allocates new binlog files and closes fully written files. Binlog recovery =============== Just two InnoDB tablespace IDs (for redo records) are reserved for binlog files, used for odd/even files alternately. To be able to unambiguously match a redo record to a binlog file during recovery, we make sure that only the two most recent binlog files will need recovery. When the writer switches to binlog file N, we first pre-allocate (in a background thread) file N+1, and then we fsync() to disk the file (N-1). The page header contains an InnoDB LSN corresponding to the start of the binlog data in the file. Thus, when binlog recovery processes a redo record, it can select the last or second-to-last existing binlog file that has a matching tablespace id and compare the record's LSN against the LSN in the file header. If the record's LSN is smaller, then we know this record corresponds to a binlog file that was already durably fsync()ed to disk, so we can skip it; otherwise the record is applied to the file (possibly creating the file anew if required). This way, we avoid any need to stall during commits to fsync() any binlog data (except when binlog write load starts exceeding the disk I/O capacity). And InnoDB checkpoints can span arbitrary number of binlog files, so that we also avoid stalling binlog rotation while waiting to complete a checkpoint. As a special case, RESET MASTER (or the initial binlog creation at first server restart) fsync()s both the InnoDB redo log and the newly created file header to disk before starting to write data. This simplifies the binlog recovery as we then will always have at least one file header available with starting LSN to compare against the LSN in redo records. In the case of --innodb-flush-log-at-trx-commit=0|2, binlog recovery will truncate extra binlog data for transactions that were not durably written to the redo log. This allows the code to write binlog data out to disk immediately, without waiting for redo log sync to happen first, unlike for InnoDB table/index data. This works because binlog files are always written strictly append-only; data is never modified in place. In fact, each page is only ever written once to the file system, except for one special case. During an InnoDB checkpoint, binlog data prior to the checkpoint LSN need to be synced durably to disk. If there is not enough binlog traffic during this to fill up the last page, we might need to write a partial last page to disk. When this happens, the page is marked, and the next write to that page will redo log the complete page (as opposed to just the appended data). This avoids the need for any double-write buffer for the binlog writes. The class binlog_recovery implements the binlog recovery algorithm. Out-of-band binlogging ====================== The new binlog implementation removes the limitation in the legacy binlog that each event group (transaction) must be written consecutively to a single binlog file. When the binlog transaction cache is full, instead of spilling it to a temporary file, the cache data is written directly into the binlog as so-called out-of-band data. At commit time, large transactions only need to binlog a small commit record that back-references the previously written partial event data. This avoids a large stall at commit time of huge transactions. It also makes it possible to keep a fixed binlog file size and still allow transactions much larger than that. If a transaction rolls back, any already written out-of-band data is simply left in the binlog file as garbage, to disappear when that file is eventually purged. The individual out-of-band records are structured on disk as a forest of complete binary trees. This is used to allow efficient reading of the data starting from the commit record (used to re-assemble complete event groups to send to slaves from the binlog dump threads); and still allow to write the binlog files strictly append-only, without needing to go back to update already written records (eg. with forward-pointers). The header page of each binlog contains a reference to any earlier binlog file that may contain out-of-band data referenced by commit records in this file. This is used to prevent the purge of old binlog files that may still be needed by active dump threads now reading a newer file. This mechanism can also be used in a future version to implement proper XA replication support following the design described in MDEV-32020. The class innodb_binlog_oob_reader implements the logic that re-assembles the out-of-band pieces of data into a complete event group for the reader. The struct binlog_oob_context is the corresponding object that keeps track of the construction of the forest of complete binary trees in the writer. Page FIFO ========= The binlog implements a page FIFO as a replacement for the InnoDB buffer pool used for other tablespaces. As the binlog is written strictly append-only, the "buffer pool" functionality is much simplified. Pages are only added to the end, and can be written always in sequence from the start, hence "FIFO". The page FIFO is accessed by three main parts of the code: 1. By threads writing to the binlog, allocating new pages and writing data into them in-memory. Binlog writes are serialized by LOCK_commit_ordered. 2. By binlog dump thread readers (eg. connected slaves). Readers must read data from pages in the page FIFO until they have been written to disk (and removed from the FIFO); then they must be read from the file through the file system. 3. By the flush thread which writes out completed pages to the binlog files in the background, and a couple other similar non-performance-critical places, such as tablespace close and InnoDB checkpoint to flush binlog files to disk. A simple latching mechanism is used to allow writer and readers to access pages concurrently, to avoid readers stalling the writers. Atomic variable binlog_cur_end_offset[] is used to make readers never read ahead of the writer without requiring mutex locking during the read; see also the description of binlog_cur_durable_offset[] below under "Crash-safe replication". Writer and readers (1) and (2) are prioritised over background flushing (3); the latter will wait for a page to be no longer latched. A global mutex is used to protect the latching and data structure integrity of the page FIFO. This mutex is only held for a brief time (few instructions), to make the page FIFO scalable. The page FIFO is implemented in class fsp_binlog_page_fifo. Crash-safe replication ====================== One of the goals of the new binlog format is to make the binlog and replication crash-safe also when --innodb-flush-log-at-trx-commit=0|2. To achieve this, it is necessary to make sure that binlog events are not sent to slaves until they have become durable on the master; otherwise, if the master crashes, the slave might have a transaction that no longer exists on the master when it comes back up after crash recovery, and replication will diverge. (This is the default setup; the converse property that a slave is always ahead of the master and any master crash will demote the old master as a slave can be provided in a later version using semi-synchronous replication with an "AFTER-PREPARE" configuration). Binlog events become durable (together with the table-data part of the associated transaction) when the corresponding LSN in the InnoDB redo log has been durably flushed to disk. The binlog code keeps track of each binlog write and the associated redo log LSN in the singleton class pending_lsn_fifo. A binlog write of a commit record inserts an entry into this. Then when that LSN has been durably written, the atomic binlog_cur_durable_offset[] is updated to reflect the point up to which the binlog is now known to be durable. The dump thread readers (connected slaves) will use this to not read too far ahead. When a reader reaches the end of the durable part of the binlog, that reader can then initiate an InnoDB redo log flush to make more of the binlog durable and allow the read to continue. This way, we achieve that transactions can complete without waiting for fsync-per-commit (which can massively increase commits-per-second depending on disk latency); at the same time, waiting readers will trigger redo log sync immediately and provide low latency to slaves. Backup ====== With the new binlog implementation, binlog files are maintained transactionally using the same InnoDB crash recovery mechanism used for its other tablespaces. This allows mariabackup to include the binlog files in the backup, something that was not easily available before if not using something like LVM snapshots. The binlog files are copied at the end of the backup, after releasing backup locks. The backup LSN is used together with the InnoDB/binlog recovery algorithm (during mariabackup --prepare) to make the binlog in the backup consistent with the table data. Apart from the obvious desirability of having a backup facility for the binlogs, this also allows a better way to get a corresponding binlog position to use the backup to provision a new replication slave. After setting up a new slave from a backup of the master, the new slave can simply be started (using GTID) from the @@gtid_binlog_pos restored with the binlog (CHANGE MASTER TO ... MASTER_DEMOTE_TO_SLAVE can be used for this). When setting up from a backup of another slave, the starting GTID position is directly available from the restored mysql.gtid_slave_pos table (as is already the case in existing MariaDB versions). This avoids the need for the special code in InnoDB that transactionally stores the current binlog position in the undo segments, as well as the need to hold backup locks while querying @@gtid_binlog_pos or SHOW MASTER STATUS. Storage engine API ================== The initial implementation of the new binlog is inside InnoDB, but the intention is that another (transactional) storage engine could do its own implementation to achive similar benefits as InnoDB. Some effort has gone into making a clean and well-defined interface between the server/binlog layer and the storage engine to facilitate this. The API is seen as new handlerton methods in struct handlerton, as well as new classes handler_binlog_reader, handler_binlog_event_group_info, and handler_binlog_purge_info. Here is an overview of the new handlerton methods: bool (*binlog_init)(size_t binlog_size, const char *directory); This is used at server startup to initialize the engine's binlog data structures, when --binlog-storage-engine=ENGINE is configured by the user. bool (*binlog_write_direct_ordered)(IO_CACHE *cache, handler_binlog_event_group_info *binlog_info, const rpl_gtid *gtid); bool (*binlog_write_direct)(IO_CACHE *cache, handler_binlog_event_group_info *binlog_info, const rpl_gtid *gtid); These are called by the server to write binlog data into the engine-implemented binlog. They are dual methods similar to commit_ordered() and commit(); the _ordered variant runs serialized (in binlog and GTID order) under LOCK_commit_ordered, while the non-ordered runs without holding locks. In addition, the engine is expected to binlog the transaction itself as part of binlog group commit (during commit_ordered), obtaining the binlog data to write from the server-provided binlog_get_cache() function. The class handler_binlog_event_group_info object is used to hold context data related to the event group while it is being binlogged. void (*binlog_group_commit_ordered)(THD *thd, handler_binlog_event_group_info *binlog_info); This is called at the end of binlog group commit (after releasing LOCK_commit_ordered), with the binlog_info of the last commit in the group. It is used to optimise the handling of the crash-safe replication described above, so that the engine can perform operations that need only be done once per group commit (not once per commit), and which can be done without holding the performance-critical LOCK_commit_ordered mutex. bool (*binlog_oob_data_ordered)(THD *thd, const unsigned char *data, size_t data_len, void **engine_data); bool (*binlog_oob_data)(THD *thd, const unsigned char *data, size_t data_len, void **engine_data); These are called by the server layer to binlog large event groups (transactions) in multiple pieces that can be interleaved with other events in the binlog. This allows large transactions to span multiple binlog files, avoids stalls around commits of large transactions, and allows event groups larger than the maximum size of an InnoDB mini-transaction. void (*binlog_oob_reset)(THD *thd, void **engine_data); void (*binlog_oob_free)(THD *thd, void *engine_data); These are simple functions to coordinate (re-)initialisation and de-allocation of engine-specific data associated with the binlogging of event groups. handler_binlog_reader * (*get_binlog_reader)(bool wait_durable); This is the main function used to implement the reading of the binlog by binlog dump threads (eg. connected slaves). The class handler_binlog_reader implements an interface for the server to read binlog data and for the dump threads to wait for new data to arrive when they reach the current EOF. void (*binlog_status)(uint64_t * out_fileno, uint64_t *out_pos); This is used to implement SHOW MASTER STATUS. The file number and offset is mostly informational, the new binlog format is intended for GTID use. void (*get_filename)(char name[FN_REFLEN], uint64_t file_no); In the new binlog implementation, binlog files are identified only by number (the file name is engine-specific and not user configurable). This function allows the server to provide the engine-specific file name for informational purposes to the user. binlog_file_entry * (*get_binlog_file_list)(MEM_ROOT *mem_root); This is used to implement SHOW BINARY LOGS. bool (*binlog_flush)(); This is used to implement FLUSH BINARY LOGS. bool (*binlog_get_init_state)(rpl_binlog_state_base *out_state); This is used to implement FLUSH BIANRY LOGS DELETE_DOMAIN_ID, to get the binlog state at the start of the current (non-purged) binlog. bool (*reset_binlogs)(); This is used to implement RESET MASTER. int (*binlog_purge)(handler_binlog_purge_info *purge_info); This is used to implement PURGE BINARY LOGS as well as auto-purge. The class handler_binlog_purge_info is used to pass the various purge options to the engine, as well as to inform about active dump threads that would prevent purging past their current read position.

1 0

Re: 59aafa4ee46: MDEV-33633 Trigger tries to access already dropped view
by Sergei Golubchik 30 Jun '25

30 Jun '25

Hi, Aleksey, On Jun 27, Aleksey Midenkov wrote: > revision-id: 59aafa4ee46 (mariadb-10.6.22-50-g59aafa4ee46) > parent(s): 1221f49f312 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2025-06-04 23:42:36 +0300 > message: > > MDEV-33633 Trigger tries to access already dropped view > > diff --git a/mysql-test/main/ps_ddl.result b/mysql-test/main/ps_ddl.result > index 35998c05dc9..ceb85835623 100644 > --- a/mysql-test/main/ps_ddl.result > +++ b/mysql-test/main/ps_ddl.result > @@ -333,7 +333,7 @@ a > drop view v1; > create view v1 as select a from t3; > insert into t1 (a) values (6); > -ERROR 42S02: Table 'test.t2' doesn't exist > +ERROR HY000: Table definition has changed, please retry transaction Is it possible to the fallback and reopen here? This is a rather unexpected error, particularly when no transactions are involved. > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 1a4fa315c3c..c89e88d0f03 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1737,6 +1737,18 @@ bool is_locked_view(THD *thd, TABLE_LIST *t) > DBUG_RETURN(FALSE); > } > > + Reprepare_observer reprepare_observer; > + auto scope_exit= make_scope_exit( > + [thd]() mutable { thd->m_reprepare_observer= NULL; }, > + false); > + if (!thd->m_reprepare_observer) > + { > + reprepare_observer.reset_reprepare_observer(); > + reprepare_observer.error_code= ER_TABLE_DEF_CHANGED; > + thd->m_reprepare_observer= &reprepare_observer; > + scope_exit.engage(); > + } And you didn't use SCOPE_EXIT to use a manual engage? Would be clearer, I'd say, to use SCOPE_EXIT and have an if() inside, like if (m_reprepare_observer == &reprepare_observer). But ok, as you like. > if (!tdc_open_view(thd, t, CHECK_METADATA_VERSION)) > { > DBUG_ASSERT(t->view != 0); > @@ -3059,9 +3071,7 @@ bool tdc_open_view(THD *thd, TABLE_LIST *table_list, uint flags) > > DBUG_ASSERT(share->is_view); > > - err= mysql_make_view(thd, share, table_list, (flags & OPEN_VIEW_NO_PARSE)); > - > - if (!err && (flags & CHECK_METADATA_VERSION)) > + if (flags & CHECK_METADATA_VERSION) > { > /* > Check TABLE_SHARE-version of view only if we have been instructed to do > @@ -3076,6 +3086,8 @@ bool tdc_open_view(THD *thd, TABLE_LIST *table_list, uint flags) > goto ret; > } > > + err= mysql_make_view(thd, share, table_list, (flags & OPEN_VIEW_NO_PARSE)); But this one - I don't understand how it works. mysql_make_view() reads the view frm which contains versions that are checked by check_and_update_table_version(). How can check_and_update_table_version() work before mysql_make_view()? > ret: > tdc_release_share(share); > > diff --git a/sql/table.cc b/sql/table.cc > index 9bbc29d4ecf..6d2a80aeace 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -5799,6 +5799,12 @@ void TABLE::init(THD *thd, TABLE_LIST *tl) > DBUG_ASSERT(!auto_increment_field_not_null); > auto_increment_field_not_null= FALSE; > > + tl->view= NULL; > + tl->derived= NULL; > + tl->derived_type= VIEW_ALGORITHM_UNDEFINED; > + tl->updatable= true; > + tl->effective_with_check= VIEW_CHECK_NONE; > + I agree it's conceptually correct and for the new table these fields should be reset. But it's rather unexpected that TABLE::init() modifies its arguments. Could this reinitialization be done somewhere else? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 0ddb9744cea: MDEV-33957 UPDATE fails on replica replicating blob virtual column in NOBLOB mode when replica logging is off
by Sergei Golubchik 24 Jun '25

24 Jun '25

Hi, Aleksey, On Jun 23, Aleksey Midenkov wrote: > commit 0ddb9744cea > Author: Aleksey Midenkov <midenok(a)mariadb.com> > Date: Sun May 25 23:23:29 2025 +0300 > > MDEV-33957 UPDATE fails on replica replicating blob virtual column in > NOBLOB mode when replica logging is off please, put the whole commit subject in one line. various tools assume the subject is one line only and don't handle line wrapping > > The sequence that causes the issue: > > 1. file->row_logging is false because slave-bin was not open; > 2. TABLE::mark_columns_per_binlog_row_image() didn't mark column for > read because file->row_logginbg is false. This was implemented in > e53ad95b733 (MDEV-6877); > 3. TABLE::update_virtual_fields() didn't update virtual field value > because column is not marked for read; > 4. calc_row_difference() sees o_len as UNIV_SQL_NULL, but new row > value is "1". The virtual column is added to update vector; > 5. row_upd() tries to update secondary index, but row_upd_sec_step() > doesn't see old value in the index. > > The patch does mark_virtual_column_with_deps() in case of rgi_slave in > mark_columns_per_binlog_row_image() so that non-stored virtual columns > are marked for update in slave thread. > > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index 01d15ca70ac..d926bd03137 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -576,6 +576,17 @@ class ha_partition final :public handler > { > m_file[part_id]->update_create_info(create_info); > } > + > + void column_bitmaps_signal() override > + { > + for (uint i= bitmap_get_first_set(&m_opened_partitions); > + i < m_tot_parts; > + i= bitmap_get_next_set(&m_opened_partitions, i)) > + { > + m_file[i]->column_bitmaps_signal(); > + } > + } That's a good one. Although it's not really related to the bug in question. Please at least mention this in the commit comment, like "also fixed..." > + > private: > int copy_partitions(ulonglong * const copied, ulonglong * const deleted); > void cleanup_new_partition(uint part_count); > diff --git a/sql/table.cc b/sql/table.cc > index 3a51228d1f3..b48aaadb6e1 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -8019,6 +8024,12 @@ void TABLE::mark_columns_per_binlog_row_image() > } > file->column_bitmaps_signal(); > } > +#if MYSQL_VERSION_ID<110201 > + else if (thd->rgi_slave) > + { > + file->column_bitmaps_signal(); > + } 1. This doesn't match the commit comment, please update the latter to describe the actual code change. 2. why MYSQL_VERSION_ID<110201 ? 3. you can also move `rpl_write_set= write_set` from the beginning of the function here, inside the else branch. This makes the code clearer: rpl_write_set was changed, so we have to signal it. > +#endif > > DBUG_VOID_RETURN; > } Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: [PATCH] MDEV-36866 Oracle outer join syntax (+): query with checking for null of non-null column uses wrong query plan and returns wrong result
by Yuchen Pei 24 Jun '25

24 Jun '25

Hi Oleksandr, > From 660e9eefd7c5c5a2b083573b8185be8b5748b0de Mon Sep 17 00:00:00 2001 > From: Oleksandr Byelkin <sanja(a)mariadb.com> > Date: Thu, 19 Jun 2025 14:40:03 +0200 > Subject: [PATCH] MDEV-36866 Oracle outer join syntax (+): query with checking > for null of non-null column uses wrong query plan and returns wrong result > Recalculate maybe_null for parts witch was fixed before converting s/witch/which/ > oracle joins. > --- > [... 97 lines elided] > diff --git a/sql/item.h b/sql/item.h > index 345d99d43cc..5b4ca045cc9 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -2292,6 +2292,7 @@ class Item :public Value_source, > return 0; > } > virtual bool ora_join_processor(void *arg) { return 0; } > + virtual bool recalc_maybe_null_processor(void *arg) { return 0; } Can you add a comment here? Something like "update maybe_nulls after converting oracle (+) outer joins" > virtual bool remove_ora_join_processor(void *arg) > { > with_flags&= ~item_with_t::ORA_JOIN; > @@ -2905,6 +2906,15 @@ class Item_args > } > return false; > } > + bool arg_check_maybe_null() > + { > + for (uint i= 0; i < arg_count; i++) > + { > + if (args[i]->maybe_null()) > + return true; > + } > + return false; > + } I suggest rename this function to some_maybe_null, to mean at least one of the args is maybe_null (some vs. all) > bool transform_args(THD *thd, Item_transformer transformer, uchar *arg); > void propagate_equal_fields(THD *, const Item::Context &, COND_EQUAL *); > bool excl_dep_on_table(table_map tab_map) > @@ -3945,6 +3955,11 @@ class Item_field :public Item_ident, > return 0; > } > bool ora_join_processor(void *arg) override; > + bool recalc_maybe_null_processor(void *arg) override > + { > + set_maybe_null(field->maybe_null()); > + return 0; > + } > bool check_ora_join(Item **reference, bool outer_ref_fixed); > void cleanup() override; > Item_equal *get_item_equal() override { return item_equal; } > @@ -5887,6 +5902,11 @@ class Item_func_or_sum: public Item_result_field, > return true; > return (this->*processor)(arg); > } > + bool recalc_maybe_null_processor(void *arg) override > + { > + set_maybe_null(arg_check_maybe_null()); > + return 0; > + } In the testcase, we come across this method for the item "t2.a is null" (aka "isnull(t2.a)"). And here it sets its maybe_null to true. But isnull/is null returns either 0 or 1, so can never have value NULL. Is it ok to set its maybe_null to true then? How about add an Item_func_isnull::recalc_maybe_null_processor() that simply does set_maybe_null(false)? (rr) p this $18 = (Item_func_isnull * const) 0x7f9e1801a000 (rr) p arg_check_maybe_null() $19 = true (rr) dbp this $20 = 0x5602a0000700 <dbug_item_print_buf> "t2.a is null" > /* > Built-in schema, e.g. mariadb_schema, oracle_schema, maxdb_schema > */ > @@ -6216,6 +6236,11 @@ class Item_ref :public Item_ident > return cleanup_processor(arg); > } > bool ora_join_processor(void *arg) override; > + bool recalc_maybe_null_processor(void *arg) override > + { > + set_maybe_null((*ref)->maybe_null()); > + return 0; > + } > Item *field_transformer_for_having_pushdown(THD *thd, uchar *arg) override > { return (*ref)->field_transformer_for_having_pushdown(thd, arg); } > }; Why only implement the processor for these four subclasses only (Item_field, Item_func_or_sum, Item_ref, Item_in_subselect)? Is it because oracle (+) only affects these types of items? > diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h > index 86c8807a75a..c65d5253c34 100644 > --- a/sql/item_cmpfunc.h > +++ b/sql/item_cmpfunc.h > @@ -3341,7 +3341,7 @@ class Item_cond :public Item_bool_func > void split_sum_func(THD *thd, Ref_ptr_array ref_pointer_array, > List<Item> &fields, uint flags) override; > friend int setup_conds(THD *thd, TABLE_LIST *tables, TABLE_LIST *leaves, > - COND **conds); > + COND **conds, List<Item> *all_fields); > void copy_andor_arguments(THD *thd, Item_cond *item); > bool walk(Item_processor processor, void *arg, > item_walk_flags flags) override; > diff --git a/sql/item_subselect.h b/sql/item_subselect.h > index cb58c62196c..e2c160b0ae3 100644 > --- a/sql/item_subselect.h > +++ b/sql/item_subselect.h > @@ -800,7 +800,12 @@ class Item_in_subselect :public Item_exists_subselect > } > return FALSE; > } > - > + bool recalc_maybe_null_processor(void *arg) override > + { > + if (left_expr->maybe_null()) > + set_maybe_null(true); > + return 0; > + } Can you add a test that covers the Item_in_subselect method here? > friend class Item_ref_null_helper; > friend class Item_is_not_null_test; > friend class Item_in_optimizer; > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index e4c914f8e7c..7f4cff4e253 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8913,10 +8913,11 @@ bool setup_on_expr(THD *thd, TABLE_LIST *table, bool is_update) > [... 53 lines elided] > #endif /* SQL_BASE_INCLUDED */ > diff --git a/sql/sql_oracle_outer_join.cc b/sql/sql_oracle_outer_join.cc > index 524dd3391db..84988d0147a 100644 > --- a/sql/sql_oracle_outer_join.cc > +++ b/sql/sql_oracle_outer_join.cc > @@ -744,7 +744,8 @@ static bool init_tables_array(TABLE_LIST *tables, > bool setup_oracle_join(THD *thd, Item **conds, > TABLE_LIST *tables, > SQL_I_List<TABLE_LIST> &select_table_list, > - List<TABLE_LIST> *select_join_list) > + List<TABLE_LIST> *select_join_list, > + List<Item> *all_fields) > { > DBUG_ENTER("setup_oracle_join"); > uint n_tables= select_table_list.elements; > @@ -964,6 +965,9 @@ bool setup_oracle_join(THD *thd, Item **conds, > { > DBUG_ASSERT(curr->on_conds.elements > 0); > curr->table->outer_join|=JOIN_TYPE_LEFT; > + // it is done after setting table map so maybe_null also set I assume "setting table map" here refers to a call to setup_table_map(). If so, I suggest that the comment be made more readable, such as: // update maybe_null which was previously set in setup_table_map() > + if (curr->table->table) > + curr->table->table->maybe_null= JOIN_TYPE_LEFT; > if (curr->on_conds.elements == 1) > { > curr->table->on_expr= curr->on_conds.head(); > @@ -1028,6 +1032,34 @@ bool setup_oracle_join(THD *thd, Item **conds, > if (add_conditions_to_where(thd, conds, std::move(return_to_where))) > DBUG_RETURN(TRUE); > + // refresh nulability of already fixed parts (WHERE, SELECT list, moved ON) Thoughts: - s/nulability/nullability/ - all_fields is not just the SELECT list. - conds[0] = WHERE + moved ON So I suggest update the comment to be something more accurate like: // refresh nullability of already fixed parts: WHERE and moved ON, // all_fields including the SELECT list, and original ON > + if (conds[0]) > + { > + conds[0]->update_used_tables(); > + conds[0]->walk(&Item::recalc_maybe_null_processor, 0, 0); > + } > + if (all_fields) > + { > + List_iterator<Item> it(*all_fields); > + Item *item; > + while((item= it++)) > + { > + item->update_used_tables(); > + item->walk(&Item::recalc_maybe_null_processor, 0, 0); > + } > + } > + for (i= 0; i < n_tables; i++) > + { > + // we have to count becaust this lists are included in other lists > + List_iterator<Item> it(*all_fields); > + Item *item; > + for (uint j= 0; j < tab[i].on_conds.elements && (item= it++); j++) > + { > + item->update_used_tables(); > + item->walk(&Item::recalc_maybe_null_processor, 0, 0); > + } > + } > + > DBUG_RETURN(FALSE); > } Are the three calls to Item::update_used_tables() needed? I commented them out and nothing bad happened to the test. > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d3bfcb8475c..9473746c150 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -954,7 +954,7 @@ setup_without_group(THD *thd, Ref_ptr_array ref_pointer_array, > DBUG_ENTER("setup_without_group"); > thd->lex->allow_sum_func.clear_bit(select->nest_level); > - res= setup_conds(thd, tables, leaves, conds); > + res= setup_conds(thd, tables, leaves, conds, &all_fields); > /* it's not wrong to have non-aggregated columns in a WHERE */ > select->set_non_agg_field_used(saved_non_agg_field_used); > diff --git a/sql/table.cc b/sql/table.cc > index 8717c740c4f..1ee7184bdb8 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -10569,7 +10569,7 @@ bool TR_table::query(ulonglong trx_id) > Item *field= newx Item_field(thd, &slex.context, (*this)[FLD_TRX_ID]); > Item *value= newx Item_int(thd, trx_id); > COND *conds= newx Item_func_eq(thd, field, value); > - if (unlikely((error= setup_conds(thd, this, dummy, &conds)))) > + if (unlikely((error= setup_conds(thd, this, dummy, &conds, NULL)))) > return false; > select= make_select(table, 0, 0, conds, NULL, 0, &error); > if (unlikely(error || !select)) > @@ -10608,7 +10608,7 @@ bool TR_table::query(MYSQL_TIME &commit_time, bool backwards) > conds= newx Item_func_ge(thd, field, value); > else > conds= newx Item_func_le(thd, field, value); > - if (unlikely((error= setup_conds(thd, this, dummy, &conds)))) > + if (unlikely((error= setup_conds(thd, this, dummy, &conds, NULL)))) > return false; > // FIXME: (performance) force index 'commit_timestamp' > select= make_select(table, 0, 0, conds, NULL, 0, &error); > -- I'm not sure what the TR_table is about - from git blame it looks like it has something to do with versioning. Is the oracle (+) join relevant here? If not (e.g. these tables are not accessible by users) then I suppose it's ok to pass NULL here. > 2.39.2 Best, Yuchen

1 1

How to verify a clean MariaDB shutdown in logs before a major version upgrade?
by Otto Kekäläinen 22 Jun '25

22 Jun '25

Hi! Congrats on the MariaDB 11.8 release! So far in Debian and Ubuntu no regressions have been reported and looks everything is working well. The most frequent thing I see people complain about is not new to 11.8, but I think introduced in 10.4 with InnoDB refusing to start if crash recovery is needed for a database of an earlier major version. This is explained in e.g. https://web.archive.org/web/20240912062010/https://mariadb.com/kb/en/innodb… To help DBAs ensure they have a clean shutdown before doing a major version upgrade, I was wondering what are the best ways to confirm a clean shutdown by checking the MariaDB logs or files in /var/log/mysql? Specifically, what log entries or patterns in the MariaDB logs indicate a clean shutdown? Additionally, are there particular files or indicators in /var/log/mysql that reliably show the clean shutdown status? For example, if all buffers were flushed, should I expect that some files have size zero?

3 2

Re: [PATCH] MDEV-36883 Oracle outer join syntax (+): operator (+) is not processed in condition like "(t2.b(+) , t1.b) in (select ...)"
by Yuchen Pei 19 Jun '25

19 Jun '25

> From 6311e48c8b60b091c9030612e73ae2d42ec3da9b Mon Sep 17 00:00:00 2001 > From: Oleksandr Byelkin <sanja(a)mariadb.com> > Date: Mon, 16 Jun 2025 19:05:04 +0200 > Subject: [PATCH] MDEV-36883 Oracle outer join syntax (+): operator (+) is not > processed in condition like "(t2.b(+) , t1.b) in (select ...)" > Fixed transfer of ora join flag from left expression to whole query > (fix for all flags and versions?) > Prohibit (+) in row operations. What is a row operation? I can infer some rules from this patch and some related code, where the following is not allowed: 1. An Item_row has a (+) somewhere 2. The LHS of an IN function or subquery is a tuple of at least two items and has a (+) somewhere 3. The RHS of an IN function has a (+) somewhere Note that Items 1 and 2 above are introduced in this patch, and Item 3 was already there. It is hard to tell from these rules what a row operation is. For example, "t1.c1 IN (t2.c2(+), t2.c3(+))" is banned because of Item 3 above, but Item 1 causes "(t2.a, t2.b) = (t1.c(+), t1.b(+))" to be banned. Does that mean that the latter is a row operation but not the former? As another example, "t2.a(+) IN (t1.c, t1.b)" is allowed so it can't be a row operation, but Item 1 bans "(t2.a(+), t2.b(+)) = (t1.c, t1.b)". If there is no simple definition of row operations, can you list the actual Rules 1 and 2 in the commit message? Items 1-3 above is also not the complete set of rules of all disallowed cases. And the division between what is allowed and what is not allowed is getting quite complex (another example: "t1.c1(+) IN (t2.c2, t2.c2)" is allowed but "t1.c1 IN (t2.c2(+), t2.c2(+))" is not). I suppose the complete set of rules will be in the docs when the feature is released. But it would be nice to document them in the code as well, especially that the beginning of sql_oracle_outer_join.cc already mentions some forbidden cases such as cycles, so it would be natural for the rest of the rules to go there. > --- > .../compat/oracle/r/ora_outer_join.result | 4 +-- > .../compat/oracle/r/ora_outer_join_err.result | 22 ++++++++++++++ > .../compat/oracle/t/ora_outer_join_err.test | 29 +++++++++++++++++++ > sql/item_cmpfunc.cc | 6 ++++ > sql/item_row.h | 11 +++++++ > sql/item_subselect.cc | 4 ++- > sql/item_subselect.h | 11 +++++++ > sql/share/errmsg-utf8.txt | 2 +- > 8 files changed, 85 insertions(+), 4 deletions(-) > diff --git a/mysql-test/suite/compat/oracle/r/ora_outer_join.result b/mysql-test/suite/compat/oracle/r/ora_outer_join.result > index ac0a09d5c43..1b99e1b3014 100644 > --- a/mysql-test/suite/compat/oracle/r/ora_outer_join.result > +++ b/mysql-test/suite/compat/oracle/r/ora_outer_join.result > @@ -294,9 +294,9 @@ Warning 4232 Oracle outer join operator (+) ignored in '"test"."tj1"."a" = 1' > # ORA-01719 > # > SELECT * FROM tj1, tj2 WHERE tj1.a IN (tj2.c(+), tj2.d(+)); > -ERROR HY000: Invalid usage of (+) operator: used in OR or IN > +ERROR HY000: Invalid usage of (+) operator: used in OR, IN or ROW operation > SELECT * FROM tj1, tj2 WHERE tj1.a NOT IN (tj2.c(+), tj2.d(+)); > -ERROR HY000: Invalid usage of (+) operator: used in OR or IN > +ERROR HY000: Invalid usage of (+) operator: used in OR, IN or ROW operation > # > # Outer join in 'IN' condition with a single expression > # This is also allowed in oracle since the expression is > diff --git a/mysql-test/suite/compat/oracle/r/ora_outer_join_err.result b/mysql-test/suite/compat/oracle/r/ora_outer_join_err.result > index 59ea247ccb3..f54ee14bbf4 100644 > --- a/mysql-test/suite/compat/oracle/r/ora_outer_join_err.result > +++ b/mysql-test/suite/compat/oracle/r/ora_outer_join_err.result > @@ -83,3 +83,25 @@ select u2.a, (select t1.a, t2.a from t1,t2 where u1.a(+) = t2.a) > from t1 as u1, t2 as u2 where u1.a(+) = u2.a; > ERROR HY000: Invalid usage of (+) operator with outer reference a > DROP TABLE t1,t2; > +# > +# MDEV-36883: Oracle outer join syntax (+): operator (+) is not > +# processed in condition like "(t2.b(+) , t1.b) in (select ...)" > +# > +create table t1 ( c int, b char(1)); > +insert into t1 values (1,'b'); > +create table t2 ( a int , b char(1)); > +insert into t2 values (1,'a'); > +create table t3 (c1 char(1), c2 char(2)); > +insert into t3 values ('c','d'); > +insert into t3 values ('c','d'); > +SELECT t2.b > +FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t1.b) IN (SELECT * from t3); > +ERROR HY000: Invalid usage of (+) operator: used in OR, IN or ROW operation Question: I made a little modification to this query: SELECT t2.b FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t2.b) IN (SELECT c1, c1 from t3); Instead of ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC, I got ER_INVALID_USE_OF_ORA_JOIN_CYCLE. Does it make sense to count this as a cycle? > +SELECT t2.b > +FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t1.b) IN (('a','a'),('b','b')); > +ERROR HY000: Invalid usage of (+) operator: used in OR, IN or ROW operation > +SELECT t2.b > +FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t1.b) = (SELECT * from t3 > +ORDER BY a LIMIT 1); > +ERROR HY000: Invalid usage of (+) operator: used in OR, IN or ROW operation > +drop tables t1,t2,t3; > diff --git a/mysql-test/suite/compat/oracle/t/ora_outer_join_err.test b/mysql-test/suite/compat/oracle/t/ora_outer_join_err.test > index 22bb8514b79..94faa293323 100644 > --- a/mysql-test/suite/compat/oracle/t/ora_outer_join_err.test > +++ b/mysql-test/suite/compat/oracle/t/ora_outer_join_err.test > @@ -107,3 +107,32 @@ select u2.a, (select t1.a, t2.a from t1,t2 where u1.a(+) = t2.a) > from t1 as u1, t2 as u2 where u1.a(+) = u2.a; > DROP TABLE t1,t2; > + > +--echo # > +--echo # MDEV-36883: Oracle outer join syntax (+): operator (+) is not > +--echo # processed in condition like "(t2.b(+) , t1.b) in (select ...)" > +--echo # > +create table t1 ( c int, b char(1)); > +insert into t1 values (1,'b'); > + > +create table t2 ( a int , b char(1)); > +insert into t2 values (1,'a'); > + > +create table t3 (c1 char(1), c2 char(2)); > +insert into t3 values ('c','d'); > +insert into t3 values ('c','d'); > + > +--error ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC > +SELECT t2.b > +FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t1.b) IN (SELECT * from t3); > + > +--error ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC > +SELECT t2.b > +FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t1.b) IN (('a','a'),('b','b')); > + > +--error ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC > +SELECT t2.b > +FROM t1, t2 WHERE t1.c = t2.a(+) AND (t2.b(+), t1.b) = (SELECT * from t3 > +ORDER BY a LIMIT 1); > + > +drop tables t1,t2,t3; > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index ca03b13f724..c94e185aa33 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -5037,6 +5037,12 @@ bool Item_func_in::ora_join_processor(void *arg) > { > if (with_ora_join()) > { > + if (cols() > 1 && args[0]->with_ora_join()) rr is telling me that cols() always returns 1 here (see below), so this if condition will always be false (a run of compat/oracle.ora_outer_join_err also does go into this if branch). Do you mean args[0]->cols()? If so, then testcases such as "(t2.b(+), t1.b) IN (('a','a'),('b','b'))" already works because the Item_row method is already returning error. But I suppose there's no harm doing another check here. (rr) list Item_func_in::cols 2523 current value and pointer passed via parameter otherwise. 2524 */ 2525 virtual Item **this_item_addr(THD *thd, Item **addr_arg) { return addr_arg; } 2526 2527 // Row emulation 2528 virtual uint cols() const { return 1; } 2529 virtual Item* element_index(uint i) { return this; } 2530 virtual Item** addr(uint i) { return 0; } 2531 virtual bool check_cols(uint c); 2532 bool check_type_traditional_scalar(const LEX_CSTRING &opname) const; > + { > + // used in ROW operaton > + my_error(ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC, MYF(0)); > + return TRUE; > + } > uint n= argument_count(); > DBUG_ASSERT(n >= 2); > // first argument (0) is right part of IN where oracle joins are allowed > diff --git a/sql/item_row.h b/sql/item_row.h > index 2c884c0f858..b234a0499c5 100644 > --- a/sql/item_row.h > +++ b/sql/item_row.h > @@ -152,6 +152,17 @@ class Item_row: public Item_fixed_hybrid, > Item *do_get_copy(THD *thd) const override > { return get_item_copy<Item_row>(thd, this); } > Item *do_build_clone(THD *thd) const override; > + > + bool ora_join_processor(void *arg) override > + { > + if (with_ora_join()) > + { > + // Oracle join operator is used inside rows. > + my_error(ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC, MYF(0)); > + return(TRUE); > + } > + return (FALSE); > + } > }; Just a note: this part does not apply to "t1.c1 IN (t2.c2(+), t2.c3(+))" because no Item_row is involved here even though the rhs looks exactly the same as an Item_row. The Item_func_in has three args. This is banned is because anything on the RHS of Item_func_in with a (+) is banned. > #endif /* ITEM_ROW_INCLUDED */ > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 57a1f01294c..e14f1a01763 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -3614,11 +3614,13 @@ bool Item_in_subselect::fix_fields(THD *thd_arg, Item **ref) > } > } > - if (left_expr && left_expr->fix_fields_if_needed(thd_arg, &left_expr)) > + if (!left_expr || left_expr->fix_fields_if_needed(thd_arg, &left_expr)) > goto err; What is this change for? The test in the patch compat/oracle.ora_outer_join_err do not reach inside the if branch. Neither does the other test compat/oracle.ora_outer_join > else > if (Item_subselect::fix_fields(thd_arg, ref)) > goto err; > + if (left_expr->with_ora_join()) > + copy_flags(left_expr, item_with_t::ORA_JOIN); What are the circumstances when this is needed? All statements in compat/oracle.ora_outer_join_err that reach the copy_flags call here have left_expr already having the ORA_JOIN flag. The other test compat/oracle.ora_outer_join does not reach here. > base_flags|= item_base_t::FIXED; > thd->where= save_where; > DBUG_RETURN(FALSE); > diff --git a/sql/item_subselect.h b/sql/item_subselect.h > index f75fafc4812..cb58c62196c 100644 > --- a/sql/item_subselect.h > +++ b/sql/item_subselect.h > @@ -790,6 +790,17 @@ class Item_in_subselect :public Item_exists_subselect > Subq_materialization_tracker *get_materialization_tracker() const > { return materialization_tracker; } > + bool ora_join_processor(void *arg) override > + { > + if (left_expr->with_ora_join() && left_expr->cols() > 1) > + { > + // used in ROW operaton > + my_error(ER_INVALID_USE_OF_ORA_JOIN_WRONG_FUNC, MYF(0)); > + return TRUE; > + } > + return FALSE; > + } > + This if branch is not entered in the test, likely because the Item_row method already returns error before the walk calls the method on the Item_in_subselect. Is it possible to add a test that covers this? > [... 19 lines elided] Best, Yuchen

1 0

Re: b69bd351c1f: MDEV-35617: DROP USER should leave no active session for that user
by Sergei Golubchik 12 Jun '25

12 Jun '25

Hi, Dmitri, On Jun 06, Dmitri Shulga wrote: > commit b69bd351c1f > Author: Dmitri Shulga <dmitry.shulga(a)mariadb.com> > Date: Wed May 28 00:05:05 2025 +0700 > > MDEV-35617: DROP USER should leave no active session for that user > > On handling the DROP USER statement it is counted a number of sessions > established on behalf every user being dropped. In case the DROP USER > statement is executed in sql_mode = oracle the error > ER_CANNOT_USER > if there are active connections for any of the users listed at > the DROP USER statement. For sql_mode != oracle the warning > ER_ACTIVE_CONNECTIONS_FOR_USER_TO_DROP > if there are active connections. This is difficult to read. Better: DROP USER looks for sessions by the do-be-dropped user and if found: * fails with ER_CANNOT_USER in Oracle mode * continues with ER_ACTIVE_CONNECTIONS_FOR_USER_TO_DROP warning otherwise New connections for that user are rejected with ER_CONNECT_WHILE_DROP_USER_IN_PROGRESS > > Every user being dropped is marked with flag that disallow establishing > a new connections on behalf this user. In case the user tries to establish > a new session while the DROP USER is executed for this account, > connection establishing is failed with the error > ER_CONNECT_WHILE_DROP_USER_IN_PROGRESS > > diff --git a/mysql-test/main/create_drop_user.result b/mysql-test/main/create_drop_user.result > index 67717f3e4e0..4f46481dfc7 100644 > --- a/mysql-test/main/create_drop_user.result > +++ b/mysql-test/main/create_drop_user.result > @@ -41,3 +41,57 @@ Warnings: > Note 1974 Can't drop user 'u1'@'%'; it doesn't exist > DROP USER u2; > ERROR HY000: Operation DROP USER failed for 'u2'@'%' > +# > +# MDEV-35617: DROP USER should leave no active session for that user > +# > +CREATE USER u1; > +CREATE USER u2; > +CREATE USER u3; > +GRANT ALL on test.* to u1; > +GRANT ALL on test.* to u2; > +GRANT ALL on test.* to u3; > +# Establish two connections on behalf the users u1, u3 > +# A connection on behalf the user u2 isn't etablished intentionally "established" > +connect con1, localhost, u1, , test; > +connect con3, localhost, u3, , test; > +# Drop the users u1, u2, u3. Since the users u1 and u3 have active > +# connections to the server, the warning about it be output "will be" > +connection default; > +DROP USER u1, u2, u3; > +Warnings: > +Note 4226 Dropped users ['u1'@'%','u3'@'%'] have active connections. Use KILL CONNECTION if they should not be used anymore. I don't think we have any other error message that puts a list in square brackets. So let's not do it here either. See e.g. ER_CANNOT_USER > +# None of the users u1, u2, u3 should be presence in the system "be present" > +SELECT user, host FROM mysql.user WHERE user IN ('u1', 'u2', 'u3'); > +User Host > +disconnect con1; > +disconnect con3; > +# Check behaviour of the DROP USER statement in > +# oracle compatibility mode > +SET @save_sql_mode = @@sql_mode; > +SET sql_mode="oracle"; > +CREATE USER u1; > +CREATE USER u2; > +CREATE USER u3; > +GRANT ALL on test.* to u1; > +GRANT ALL on test.* to u2; > +GRANT ALL on test.* to u3; > +# Established two connections on behalf the users u1, u3; > +# A connection on behalf the user u2 isn't etablished intentionally "established" > +connect con1, localhost, u1, , test; > +connect con3, localhost, u3, , test; > +connection default; > +# In oracle compatibility mode, DROP USER fails in case > +# there are connections on behalf the users being dropped. > +DROP USER u1, u2, u3; > +ERROR HY000: Operation DROP USER failed for 'u1'@'%','u3'@'%' See? No square brackets :) > +# It is expected to see all three users in output of the query I don't think it's a good idea. DROP USER drops everything it can and reports an error for everything it couldn't drop. It's not all-or-nothing behavior. Same for all other statements that can fail with ER_CANNOT_USER. Same for DROP TABLES. All-or-nothing here is rather unexpected and inconsisent with pretty much every other multi-object command in the server. > +SELECT user, host FROM mysql.user WHERE user IN ('u1', 'u2', 'u3'); > +User Host > +u1 % > +u2 % > +u3 % > +SET sql_mode= @save_sql_mode; > +disconnect con1; > +disconnect con3; > +# Clean up > +DROP USER u1, u2, u3; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 26905f0d415..b519eec15c8 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -210,6 +210,12 @@ class ACL_USER_PARAM > > class ACL_USER :public ACL_USER_BASE, public ACL_USER_PARAM > { > + /* > + Ephemeral state (meaning it is not stored anywhere in the Data Dictionary) > + to disable establishing sessions in case the user is being dropped. > + */ > + bool dont_accept_conn= false; should be in ACL_USER_PARAM, I suppose, for consistency. password_errors is there, and it's ephemeral too. > + > public: > ACL_USER() = default; > ACL_USER(THD *, const LEX_USER &, const Account_options &, const privilege_t); > @@ -243,6 +249,7 @@ class ACL_USER :public ACL_USER_BASE, public ACL_USER_PARAM > dst->host.hostname= safe_strdup_root(root, host.hostname); > dst->default_rolename= safe_lexcstrdup_root(root, default_rolename); > bzero(&dst->role_grants, sizeof(role_grants)); > + dst->dont_accept_conn= dont_accept_conn; not needed, there's `*dst= *this` ~20 lines above. > return dst; > } > > @@ -11337,6 +11354,85 @@ bool mysql_create_user(THD *thd, List <LEX_USER> &list, bool handle_as_role) > DBUG_RETURN(result); > } > > + > +struct count_user_threads_callback_arg > +{ > + explicit count_user_threads_callback_arg(LEX_USER *user_arg) > + : user(user_arg), counter(0) > + {} > + > + LEX_USER *user; > + uint counter; > +}; > + > + > +/** > + Callback function invoked for every active THD to count a number of > + sessions established by specified user > + > + @param[in] thd Thread context > + @param arg Account info for that a number of active connections > + should be countered > +*/ > + > +static my_bool count_threads_callback(THD *thd, > + count_user_threads_callback_arg *arg) > +{ > + if (thd->security_ctx->user) > + { > + /* > + Check that hostname (if given) and user name matches. > + > + host.str[0] == '%' means that host name was not given. See sql_yacc.yy > + */ > + if (((arg->user->host.str[0] == '%' && !arg->user->host.str[1]) || > + !strcmp(thd->security_ctx->host_or_ip, arg->user->host.str)) && > + !strcmp(thd->security_ctx->user, arg->user->user.str)) eh, no. Don't invent new username matching rules. In particular, incorrect ones :) This session was already matched, so do if (!strcmp(arg->user->host.str, thd->main_security_ctx.priv_host) && !strcmp(arg->user->user.str, thd->main_security_ctx.priv_user)) And let's add a test, like create user u@localhost; create user u@'%'; connect u,localhost,u; connection default; drop user u@'%'; and there should be neither warning nor an error because of the existing connection by u@localhost. > + arg->counter++; > + } > + return false; > +} > + > + > +/** > + Get a number of connections currently established on behalf the user > + > + @param[in] thd Thread context > + @param[in] user User credential to count up a number of connections > + > + @return a number of connections established by the user at the moment of > + the function call > +*/ > + > +static int count_sessions_for_user(LEX_USER *user) > +{ > + count_user_threads_callback_arg arg(user); > + bool ret __attribute__((unused)); > + > + ret= server_threads.iterate(count_threads_callback, &arg); iterate() takes a lock. You want to release it as soon as possible. And you don't really use the value of a counter, so why bother? Abort the search and return on the first match. > + DBUG_ASSERT(ret == false); > + > + return arg.counter; > +} > + > + > +/** > + Find the specified user and mark it as not accepting incoming sessions > + > + @param user_name the user for that accept of incoming connections > + should be disabled > +*/ > + > +static void disable_connections_for_user(LEX_USER *user) > +{ > + ACL_USER *found_user= find_user_or_anon(user->host.str, user->user.str, > + nullptr); No, find_user_exact(). And let's add this test case: create user u1@'%'; --error ER_CANNOT_USER drop user u1@localhost; connect u1,localhost,u1; select user(), current_user(); > + > + if (found_user != nullptr) > + found_user->disable_new_connections(); > +} > + > + > /* > Drop a list of users and all their privileges. > > @@ -11395,6 +11496,39 @@ bool mysql_drop_user(THD *thd, List <LEX_USER> &list, bool handle_as_role) > continue; > } > > + if (count_sessions_for_user(user_name) != 0) > + append_user(thd, &connected_users, user_name); > + > + correct_users_list.push_back(user_name); > + /* > + Prevent new connections to be established on behalf the user > + being dropped. > + */ > + disable_connections_for_user(user_name); > + } > + > + if (!connected_users.is_empty() && > + (thd->variables.sql_mode & MODE_ORACLE)) > + { > + /* > + Throw error in case there are connections for the user being dropped AND > + sql_mode = oracle > + */ > + mysql_mutex_unlock(&acl_cache->lock); > + mysql_rwlock_unlock(&LOCK_grant); > + > + my_error(ER_CANNOT_USER, MYF(0), > + (handle_as_role) ? "DROP ROLE" : "DROP USER", There can be no DROP ROLE here, everything you're doing above shouldn't even be done for DROP ROLE, check handle_as_role to avoid unnecessary locks. > + connected_users.c_ptr_safe()); > + > + DBUG_RETURN(true); ok, another test case to add: create user u@localhost; set sql_mode=oracle; connect u,localhost,u; connection default; --error ER_CANNOT_USER drop user u@'%'; connect u,localhost,u; select user(), current_user(); > + } > + > + user_list.init(correct_users_list); you don't need correct_users_list, because there shouldn't be inconsistent all-or-nothing behavior > + while ((user_name= user_list++)) > + { > + int rc; > + > if ((rc= handle_grant_data(thd, tables, 1, user_name, NULL)) > 0) > { > // The user or role was successfully deleted > @@ -15127,6 +15267,18 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len) > } > #endif > > + /* > + Attempt to establish a new connection on behalf the user that is > + currently being dropped from a concurrent session. > + Terminate the connection. > + */ > + if (acl_user->dont_accept_new_connections()) > + { > + my_error(ER_CONNECT_WHILE_DROP_USER_IN_PROGRESS, MYF(0), > + acl_user->get_username()); I'd just say user not found. That is, no need to a special error, but find_mpvio_user() can simply skip entries that are being dropped. Also, how can a connection see a user being dropped, if mysql_drop_user() takes acl_cache->lock and doesn't release it until everything is dropped? > + DBUG_RETURN(1); > + } > + > if (set_privs_on_login(thd, acl_user)) > DBUG_RETURN(1); > } Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: MDEV-36851 COALESCE() returns nullable column while IFNULL() does not — unexpected behavior in VIEW definition
by Alexander Barkov 05 Jun '25

05 Jun '25

Hello Raghu, This is a review for your patch for MDEV-36851 > · > MDEV-36581: Fix nullability logic of COALESCE The MDEV number in the comment is incorrect. Should be MDEV-36851 Please next time make sure to copy the MDEV number together with its title from Jira, to avoid typos. > COALESCE currently uses a generic logic- "Result of a function > is nullable if any of the arguments is nullable", which is wrong. > > This patch fixes the logic behind the nullability of COALESCE > to make the result NULL, only if all of the arguments are NULL. > > Tests included in `mysql-test/main/func_hybrid_type.test` The general idea of the patch is correct. I have small suggestions for the implementation. <cut> > --- a/sql/item_cmpfunc.h > +++ b/sql/item_cmpfunc.h > @@ -1148,6 +1148,14 @@ class Item_func_interval :public Item_long_func > > class Item_func_coalesce :public Item_func_case_expression > { > +protected: > + bool all_args_maybe_null= true; > + inline void set_nullability_with(const Item *item) override > + { > + if (all_args_maybe_null && !(bool) (item->base_flags & item_base_t::MAYBE_NULL)) > + all_args_maybe_null= false; > + base_flags|= item->base_flags & item_base_t::MAYBE_NULL > + } I don't think we need a new member and a new virtual method. This is too complex to fix this problem. Please (instead of trying to build-in the new code inside fix_fields()) add a loop into fix_length_and_dec() searching for a non-NULL argument and removing the MAYBE_NULL flag if such argument is found. I also suggest additional tests: without view and without derived tables. Just using regular tables. Thanks. > public: > Item_func_coalesce(THD *thd, Item *a, Item *b): > Item_func_case_expression(thd, a, b) {} > @@ -1162,6 +1170,12 @@ class Item_func_coalesce :public Item_func_case_expression > bool native_op(THD *thd, Native *to) override; > bool fix_length_and_dec(THD *thd) override > { > + /* > + Result of COALESCE is nullable iff all arguments > + are NULL, otherwise NOT NULL. > + */ > + if (!all_args_maybe_null) > + base_flags &= ~item_base_t::MAYBE_NULL; > if (aggregate_for_result(func_name_cstring(), args, arg_count, true)) > return TRUE; > fix_attributes(args, arg_count); <cut>

1 0

11.62 to 11.72 Database Upgrade Failure
by jashley＠deadacated.com 03 Jun '25

03 Jun '25

I updated and upgraded the database from 1.32 to 1.62 successfully. 11.62 to 11.72 database upgrade is failing. Is this a known issue? Any suggestions for fixing? Thank you, Jay

2 1