- developers - lists.mariadb.org

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

Re: 39f1b4c93e7: MDEV-36053 CURSOR declarations in PACKAGE BODY
by Sergei Golubchik 30 May '25

30 May '25

Hi, Alexander, Phew, it was a big patch. Took me 20 days. Please, see comments below. On May 09, Alexander Barkov wrote: > commit 39f1b4c93e7 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Mon Apr 14 12:00:57 2025 +0400 > > MDEV-36053 CURSOR declarations in PACKAGE BODY > > This change adds CURSOR declarations inside PACKAGE BODY. > > PL/SQL mode: > > SET sql_mode=ORACLE; > CREATE PACKAGE BODY pkg AS > CURSOR mc0 IS SELECT c0, c1 FROM t1; > PROCEDURE p1 AS > rec mc0%ROWTYPE; > BEGIN > OPEN mc0; > FETCH mc0 INTO rec; > CLOSE mc0 > END; > END; > / > > SQL/PSM mode: > > SET sql_mode=DEFAULT; > CREATE PACKAGE BODY pkg > mc0 CURSOR FOR SELECT c0, c1 FROM t1; > PROCEDURE p1() > BEGIN > DECLARE rec ROW TYPE OF mc0; > OPEN mc0; > FETCH mc0 INTO rec; > CLOSE mc0 > END; > END; > / > PACKAGE BODY cursors like local cursors > (declared inside a FUNCTION or a PROCEDURE) support: > - OPEN/FETCH/CLOSE > - FOR rec IN cur - an explicit cursor loop > - Using a cursor row as an anchored variable data type: > * DECLARE var cur%ROWTYPE; -- sql_mode=ORACLE > * DECLARE var ROW TYPE OF cur; -- sql_mode=DEFAULT > > The patch details: > > - Changing various class members and function/method parameters which > store a CURSOR run-time address from "uint" to sp_rcontext_addr. > A few classes now derive from sp_rcontext_addr instead of having > a "uint m_cursor;" member. > > This change uses the same idea with what we did for SP variables, > when we implemented PACKAGE BODY variables a few years ago. > > - Fixing the grammar in sql_yacc.yy to allow CURSOR declarations > inside PACKAGE BODY. > > - Moving the C++ code handing the "CLOSE_SYM ident" grammar > and creating an sp_instr_cclose instance from sql_yacc.yy > into a new method LEX::sp_close(). > This is to have the grammar file smaller. > Also, this code has significantly changed anyway. > > - Adding a new class sp_pcontext_top. It's used for the top level parse > context (of an sp_head). Note, its children contexts still use the old > class sp_pcontext. > > sp_pcontext_top context additionally to sp_pcontext has: > > const sp_head *m_sp; -- The pointer to the sp_head owning this context > Dynamic_array<sp_pcursor> m_member_cursors; -- PACKAGE BODY wide cursors > > m_sp->m_parent->get_parse_context() is used to find the sp_pcontext > belonging to the parent PACKAGE BODY from a sp_pcontext_top instance > belonging to a PROCEDURE/FUNCTION sp_pcontext_top. > > - Adding a new member in sp_rcontext: > Dynamic_array<sp_cursor*> m_member_cursors; > It's used to store run-time data of PACKAGE BODY wide cursors. > > - Adding a new class sp_instr_copen2. It's used to open PACKAGE BODY cursors. > Unlike the usual cursors, PACKAGE BODY cursors: > * do not use the cursor stack (sp_rcontext::m_cstack) > * do not need a preceeding sp_instr_cpush > * do not need a following sp_instr_cpop > > All cursor information such as "sp_lex_cursor" resides inside > sp_instr_copen2 itself (rather than inside sp_instr_cpush which is > used to store "sp_lex_cursor" in case of sp_instr_copen). > > Note, the other cursor related instructions: > sp_instr_cfetch > sp_instr_cclose > sp_instr_cursor_copy_struct > do not need sp_instr_xxx2 counter-parts. > Thy just use sp_rcontext_addr to address cursors. > > - Adding Sp_rcontext_handler_member > It's used to handle PACKAGE BODY members: > cursors and variables declared in the PACKAGE BODY, > when they are accessed from its executable initialization section: > > CREATE PACKAGE BODY pkg AS > CURSOR mc0 IS SELECT c0, c1 FROM t1; -- A member (PACKAGE BODY cursor) > mv0 mc0%ROWTYPE; -- A member (PACKAGE BODY variable) > PROCEDURE p1 AS > BEGIN > -- Accessing members from here use sp_rcontext_handler_package_body > -- (members of the parent PACKAGE BODY) > OPEN mc0; > FETCH mc0 INTO mv0; > CLOSE mc0; > END; > BEGIN > -- NEW: > -- Accessing members from here use sp_rcontext_handler_member > -- (PACKAGE BODY own members) > OPEN mc0; > FETCH mc0 INTO mv0; > CLOSE mc0; > END; > / > > Member variables and cursor are now marked with the "MEMBER." prefix > in the "SHOW PACKAGE BODY code" output. > Some old MTR tests have been re-recorded accordingly. > > - Adding new virtual methods into Sp_rcontext_handler: > > virtual const sp_variable *get_pvariable(const sp_pcontext *pctx, > uint offset) const; > > virtual const sp_pcursor *get_pcursor(const sp_pcontext *pctx, > uint offset) const; > > They're used from sp_instr::print() virtual implementations. > They internally calculate a proper sp_pcontext using as a parameter > the sp_pcontext pointed by sp_instr::m_ctx. > > For example, Sp_handler_package_body::get_pvariable()/get_pcursor() > accesses to this sp_pcontext: > m_ctx->top_context()->m_sp->m_parent->get_parse_context(), > i.e. the parse context of the PACKAGE BODY which is the parent for > the current package PROCEDURE of FUNCTION an sp_instr belongs to. > > - Adding a new method LEX::find_cursor(). It searches for a cursor in > this order: > * Local cursors in the nearst upper BEGIN/END block. > * A member cursor of the current PACKAGE BODY > (used from the PACKAGE BODY initialization section) > * A member cursor of the parrent PACKAGE BODY > (used from a package PROCEDURE or a package FUNCTION) better write here (and in the function comment below), like 1. Local cursors... 2. A member cursor of the current PACKAGE BODY OR 2. A member cursor of the parrent PACKAGE BODY to emphasize that second and third are alternatives and it never search both in "current PACKAGE BODY" and then in the "parrent PACKAGE BODY" in this order. > > Adding a new method LEX::find_cursor_with_error(). > In case when a cursor is not found, it automatically > raises the ER_SP_CURSOR_MISMATCH SQL condition into > the diagnostics area. > > - Adding a new method sp_head::add_instr_copenX(). > It creates sp_instr_copen for local cursors, > or sp_instr_copen2 for non-local cursors. > > - Adding a new abstract class sp_lex_cursor_instr. > It's used a common parent class for a few sp_instr_xxx classes, > including the new sp_instr_copen2. > This change is needed to avoid code duplication. > > - Adding a new protected method sp_instr::print_cmd_and_var(), to print > an instruction using this format: "command name@offset". > It's used from a few implementations of sp_instr_xxx::print(), > including sp_instr_copen2::print(). > This change is also needed to avoid code duplication. > > - Moving the definition of "class Sp_rcontext_handler" from item.h > into a new file sp_rcontext_handler.h > This is to maitain header dependencies easier, as well as to move > declarations not directly related to "class Item" outside of item.h > > - Adding a new method sp_pcontext::frame_for_members(), to distinguish > easier between local cursors/variables and PACKAGE BODY cursors/variables. > > - Fixing "struct Lex_for_loop_st" to addionally store > a const pointer to Sp_rcontext_handler, to distinguish between: > * FOR rec IN local_cursor > * FOR rec IN package_body_cursor > > diff --git a/mysql-test/include/sp/sp-cursor-package-body-init.inc b/mysql-test/include/sp/sp-cursor-package-body-init.inc > --- /dev/null > +++ b/mysql-test/include/sp/sp-cursor-package-body-init.inc > @@ -0,0 +1,4 @@ > +--disable_query_log > +--let $oracle=`SELECT @@sql_mode LIKE '%ORACLE%'` > +--let $code=`SELECT COALESCE(@code, 0)` well, sure, but wouldn't it be simpler to do let $code=1; in sp-cursor-package-body-code.test ? > +--enable_query_log > diff --git a/mysql-test/include/sp/sp-cursor-package-body-06.inc b/mysql-test/include/sp/sp-cursor-package-body-06.inc > --- /dev/null > +++ b/mysql-test/include/sp/sp-cursor-package-body-06.inc > @@ -0,0 +1,87 @@ > +--echo # > +--echo # PACKAGE BODY initialization: > +--echo # OPEN package_body_cursor > +--echo # FUNCTION: > +--echo # FETCH package_body_cursor INTO package_body_variable > +--echo # PROCEDURE: > +--echo # CLOSE package_body_cursor > +--echo # > + > +--source sp-cursor-package-body-init.inc > + > +if ($oracle == 0) > +{ > + > +DELIMITER /; > +CREATE PACKAGE pkg > + FUNCTION f1() RETURNS INT; > + PROCEDURE p1close(); > +END; > +/ > +CREATE PACKAGE BODY pkg > + DECLARE cur CURSOR FOR SELECT 1 AS c FROM DUAL; > + DECLARE vc0 INT; > + FUNCTION f1() RETURNS INT > + BEGIN > + FETCH cur INTO vc0; > + RETURN vc0; > + END; > + PROCEDURE p1close() > + BEGIN > + CLOSE cur; > + END; > + -- initialization > + OPEN cur; > +END; cool. that's what I was asking for :) > +/ > +DELIMITER ;/ > + > +} > + > + > +if ($oracle > 0) > +{ > + > +DELIMITER /; > +CREATE PACKAGE pkg AS > + FUNCTION f1 RETURN INT; > + PROCEDURE p1close; > +END; > +/ > +CREATE PACKAGE BODY pkg AS > + CURSOR cur IS SELECT 1 AS c FROM DUAL; > + vc0 INT; > + FUNCTION f1 RETURN INT AS > + BEGIN > + FETCH cur INTO vc0; > + RETURN vc0; > + END; > + PROCEDURE p1close AS > + BEGIN > + CLOSE cur; > + END; > +BEGIN > + OPEN cur; > +END; > +/ > +DELIMITER ;/ > + > +} > + > + > +--disable_view_protocol > +--disable_ps2_protocol > +SELECT pkg.f1() FROM DUAL; > +--enable_ps2_protocol > +CALL pkg.p1close(); > +--error ER_SP_CURSOR_NOT_OPEN > +SELECT pkg.f1() FROM DUAL; > +--enable_view_protocol > + > +if ($code > 0) > +{ > +SHOW PACKAGE BODY CODE pkg; > +SHOW FUNCTION CODE pkg.f1; > +SHOW PROCEDURE CODE pkg.p1close; > +} > +DROP PACKAGE pkg; > diff --git a/sql/sp_rcontext_handler.h b/sql/sp_rcontext_handler.h > --- /dev/null > +++ b/sql/sp_rcontext_handler.h > @@ -0,0 +1,141 @@ > +#ifndef SP_RCONTEXT_HANDLER_INCLUDED > +#define SP_RCONTEXT_HANDLER_INCLUDED > + > +/* Copyright (c) 2009, 2025, MariaDB Corporation. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */ > + > + > +class sp_rcontext; > +class sp_pcontext; > +class sp_pcursor; > +class sp_cursor; > + > +/** > + A helper class to handle the run time context of various components of SP: > + Variables: > + - local SP variables and SP parameters > + - PACKAGE BODY routine variables > + - (there will be more kinds in the future) > + Cursors: > + - static local cursors > + - static PACKAGE BODY cursors of the parent PACKAGE BODY > + - static PACKAGE BODY own cursors (when used in the executable secion) > +*/ > + > +class Sp_rcontext_handler > +{ > +public: > + virtual ~Sp_rcontext_handler() = default; > + > + > + /** > + A prefix used for SP variable names in queries: > + - EXPLAIN EXTENDED > + - SHOW PROCEDURE CODE > + Local variables and SP parameters have empty prefixes. > + Package body variables are marked with a special prefix. > + This improves readability of the output of these queries, > + especially when a local variable or a parameter has the same > + name with a package body variable. > + */ > + virtual const LEX_CSTRING *get_name_prefix() const= 0; > + > + // Find a parse time SP variable > + virtual const sp_variable *get_pvariable(const sp_pcontext *pctx, > + uint offset) const= 0; > + > + // Find a parse time SP cursor > + virtual const sp_pcursor *get_pcursor(const sp_pcontext *pctx, > + uint offset) const= 0; > + > + /** > + At execution time THD->spcont points to the run-time context (sp_rcontext) > + of the currently executed routine. > + Local variables store their data in the sp_rcontext pointed by thd->spcont. > + Package body variables store data in separate sp_rcontext that belongs > + to the package. > + This method provides access to the proper sp_rcontext structure, > + depending on the SP variable kind. > + */ > + virtual sp_rcontext *get_rcontext(sp_rcontext *ctx) const= 0; > + > + // Find a run time SP cursor > + virtual sp_cursor *get_cursor(THD *thd, uint offset) const= 0; > +}; > + > + > +/* > + A handler to access local variables and cursors. > +*/ > +class Sp_rcontext_handler_local final: public Sp_rcontext_handler > +{ > +public: > + const LEX_CSTRING *get_name_prefix() const override; > + const sp_variable *get_pvariable(const sp_pcontext *pctx, > + uint offset) const override; > + const sp_pcursor *get_pcursor(const sp_pcontext *pctx, > + uint offset) const override; > + sp_rcontext *get_rcontext(sp_rcontext *ctx) const override; > + sp_cursor *get_cursor(THD *thd, uint offset) const override; > +}; > + > + > +/* > + A handler to access parent members, e.g.: > + PACKAGE BODY variables and cursors when used in package routines. > +*/ > +class Sp_rcontext_handler_package_body final :public Sp_rcontext_handler > +{ > +public: > + const LEX_CSTRING *get_name_prefix() const override; > + const sp_variable *get_pvariable(const sp_pcontext *pctx, > + uint offset) const override; > + const sp_pcursor *get_pcursor(const sp_pcontext *pctx, > + uint offset) const override; > + sp_rcontext *get_rcontext(sp_rcontext *ctx) const override; > + sp_cursor *get_cursor(THD *thd, uint offset) const override; > +}; > + > + > +/* > + A handler to access its own members, e.g.: > + PACKAGE BODY variables and cursors when used in > + the initialization section of PACKAGE BODY. > +*/ > +class Sp_rcontext_handler_member final :public Sp_rcontext_handler > +{ > +public: > + const LEX_CSTRING *get_name_prefix() const override; > + const sp_variable *get_pvariable(const sp_pcontext *pctx, > + uint offset) const override; > + const sp_pcursor *get_pcursor(const sp_pcontext *pctx, > + uint offset) const override; > + sp_rcontext *get_rcontext(sp_rcontext *ctx) const override; > + sp_cursor *get_cursor(THD *thd, uint offset) const override; > +}; > + > + > +extern MYSQL_PLUGIN_IMPORT > + Sp_rcontext_handler_local sp_rcontext_handler_local; why is it MYSQL_PLUGIN_IMPORT? I know that it was and you've just copied it, but still? Plugins aren't supposed to use it, are they? Same below. > + > + > +extern MYSQL_PLUGIN_IMPORT > + Sp_rcontext_handler_package_body sp_rcontext_handler_package_body; > + > + > +extern MYSQL_PLUGIN_IMPORT > + Sp_rcontext_handler_member sp_rcontext_handler_member; > + > +#endif // SP_RCONTEXT_HANDLER_INCLUDED > diff --git a/sql/sp_head.h b/sql/sp_head.h > --- a/sql/sp_head.h > +++ b/sql/sp_head.h > @@ -222,6 +223,7 @@ class sp_head :private Query_arena, > LEX_CSTRING m_defstr; > AUTHID m_definer; > > + Query_arena *query_arena() { return this; } this is practically equivalent to inheriting from Query_arena publicly. Why not to do it instead? > const st_sp_chistics &chistics() const { return m_chistics; } > const LEX_CSTRING &comment() const { return m_chistics.comment; } > void set_suid(enum_sp_suid_behaviour suid) { m_chistics.suid= suid; } > diff --git a/sql/sp_pcontext.h b/sql/sp_pcontext.h > --- a/sql/sp_pcontext.h > +++ b/sql/sp_pcontext.h > @@ -319,6 +320,10 @@ class sp_pcursor: public LEX_CSTRING > class sp_pcontext *param_context() const { return m_param_context; } > class sp_lex_cursor *lex() const { return m_lex; } > bool check_param_count_with_error(uint param_count) const; > + bool eq_name(const LEX_CSTRING &name) const Why name isn't Lex_ident_column? > + { > + return !system_charset_info->strnncoll(name.str, name.length, str, length); > + } > }; > > > @@ -826,4 +861,105 @@ class sp_pcontext : public Sql_alloc > }; // class sp_pcontext : public Sql_alloc > > would be good to have a comment here. and why was it not needed for package-wide variables? > +class sp_pcontext_top: public sp_pcontext > +{ > +public: > + sp_pcontext_top(const sp_head *sp); > + > + const sp_head *sp() const > + { > + return m_sp; > + } > + > + bool add_member_cursor(const LEX_CSTRING *name, sp_pcontext *param_ctx, > + class sp_lex_cursor *lex) > + { > + uint dummy_offset; > + if (find_member_cursor(name, &dummy_offset)) > + { > + my_error(ER_SP_DUP_CURS, MYF(0), name->str); > + return true; > + } > + return m_member_cursors.append(sp_pcursor(name, param_ctx, lex)); > + } > + > + const sp_pcursor *find_member_cursor(const LEX_CSTRING *name, > + uint *poff) const > + { > + for (uint i= 0; i < m_member_cursors.elements(); i++) > + { > + if (m_member_cursors.at(i).eq_name(*name)) > + { > + *poff= i; > + return &m_member_cursors.at(i); > + } > + } > + return nullptr; > + } > + > + const sp_pcursor *find_member_cursor(uint offset) const > + { > + return &m_member_cursors.at(offset); > + } > + > + uint member_cursor_count() const > + { > + return (uint) m_member_cursors.elements(); > + } > + > + /* > + Return a possible parse context frame for members. > + > + Members are: > + - CREATE PACKAGE wide variables and cursors (coming soon - MDEV-13139) > + - CREATE PACKAGE BODY wide variables and cursors > + > + Example: "Example (using Oracle syntax):" > + > + CREATE PACKAGE BODY pkg AS -- sp_package > + a0 INT; -- A member > + CURSOR c0; -- A member > + > + PROCEDURE p1() AS -- A method sp_head (package routine) > + a1 INT; -- A local variable - not an sp_package member > + CURSOR c1; -- A local cursor - not an sp_package member > + BEGIN > + OPEN c0; -- Open a cursor member of the parent sp_package > + CLOSE c0; -- Close a cursor member of the parent sp_package > + END; > + > + BEGIN -- Executable initialization section (constructor) > + DECLARE > + a2 INT; -- a local variable - not a member > + CURSOR c2; -- a local cursor - not a member > + BEGIN > + OPEN c0; -- Open its own cursor member (of sp_package) > + CLOSE c0; -- Close its own cursor member (of sp_package) > + END; > + END; > + > + Members are visible in all subroutines and are treated in a different way > + comparing to local variables/cursors. For example, member cursors > + do not need sp_instr_cpush and sp_instr_cpop, unlike local cursors. > + > + The top level parse context frame of an sp_head stores formal parameters: > + - PROCEDURE and FUNCTION can have formal parameters. > + - PACKAGE and PACKAGE BODY cannot have formal parameters, > + but still the top level context frame exists (it's just empty). > + Members reside in child(0) of the top level parse context. > + > + The owning sp_head will check if this frame is really for members, > + depending on the sp_head type (a package vs a routine). > + */ > + sp_pcontext *frame_for_members_candidate() const > + { > + return child_context(0); > + } > + > +private: > + const sp_head *m_sp; > + Dynamic_array<sp_pcursor> m_member_cursors; > +}; > + > + > #endif /* _SP_PCONTEXT_H_ */ > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -3818,7 +3821,26 @@ sp_head::set_local_variable_row_field_by_name(THD *thd, sp_pcontext *spcont, > } > > > +bool sp_head::add_instr_copenX(THD *thd, sp_pcontext *spcont, > + const sp_rcontext_addr &addr) > +{ > + sp_instr *i; > + if (addr.rcontext_handler() == &sp_rcontext_handler_local) > + i= new (thd->mem_root) sp_instr_copen(instructions(), spcont, > + addr.offset()); > + else > + { > + const sp_pcursor *pcursor= addr.rcontext_handler()-> > + get_pcursor(spcont, addr.offset()); or spcont->get_pcursor(addr) that's why you've added it, after all :) > + i= new (thd->mem_root) sp_instr_copen2(instructions(), spcont, > + addr, pcursor->lex()); > + } > + return i == NULL || add_instr(i); > +} > + > + > -bool sp_head::add_open_cursor(THD *thd, sp_pcontext *spcont, uint offset, > +bool sp_head::add_open_cursor(THD *thd, sp_pcontext *spcont, > + const sp_rcontext_addr &addr, > sp_pcontext *param_spcont, > List<sp_assignment_lex> *parameters) > { > diff --git a/sql/sp_instr.cc b/sql/sp_instr.cc > --- a/sql/sp_instr.cc > +++ b/sql/sp_instr.cc > @@ -1912,22 +1925,10 @@ sp_instr_cclose::execute(THD *thd, uint *nextp) > void > sp_instr_cclose::print(String *str) > { > - const LEX_CSTRING *cursor_name= m_ctx->find_cursor(m_cursor); > - > + const LEX_CSTRING cmd= {STRING_WITH_LEN("cclose ")}; > + const sp_pcursor *cursor= m_ctx->get_pcursor(*this); > /* cclose name@offset */ > - size_t rsrv= SP_INSTR_UINT_MAXLEN+8; > - > - if (cursor_name) > - rsrv+= cursor_name->length; > - if (str->reserve(rsrv)) > - return; > - str->qs_append(STRING_WITH_LEN("cclose ")); > - if (cursor_name) > - { > - str->qs_append(cursor_name->str, cursor_name->length); > - str->qs_append('@'); > - } > - str->qs_append(m_cursor); > + print_cmd_and_var(cmd, cursor ? *cursor : empty_clex_str, *this, str); may be print_cmd_and_var() should take LEX_CSTRING* as a second argument, not LEX_CSTRING&, then you wouldn't need to repeat cursor ? *cursor : empty_clex_str every time > } > > > @@ -2068,10 +2060,64 @@ sp_instr_cursor_copy_struct::exec_core(THD *thd, uint *nextp) > { > sp_cursor tmp(thd, true); > // Open the cursor without copying data > + // TODO: Check with DmitryS if hiding ROOT_FLAG_READ_ONLY is OK. and? > + DBUG_ASSERT(thd->lex == m_lex_keeper.lex()); > + auto backup_flags= thd->lex->query_arena()->mem_root->flags; > + thd->lex->query_arena()->mem_root->flags&= ~ROOT_FLAG_READ_ONLY; > + > + /* > + Open the cursor in its sp_rcontext, because > + Items inside the cursor statement have rcontext handler > + relative to the cursor rcontext. In this example package: > + > + CREATE PACKAGE BODY pkg AS > + CURSOR c0(p0 INT, p1 VARCHAR(10)) IS SELECT p0, p1 FROM DUAL; > + PROCEDURE p1 AS > + BEGIN > + FOR r0 IN c0(1,'c1') > + LOOP > + SELECT r0.c0, r0.c1; > + END LOOP; > + END; > + END; > + > + Item_splocal instances for the cursor parameters p0 and p1 > + (in the cursor SELECT statement) use sp_rcontext_handler_local. > + But the cursor is opened from the PROCEDURE p1, which > + has its own sp_rcontext. thd->spcont is pointing to the > + PROCEDURE p1 sp_rcontext at the time of the current > + sp_instr::exec_core(). > + > + Using the above package as an example again, let's do the following: > + - backup thd->spcont (pointing to the PROCEDURE p1 sp_rcontext) > + - reset thd->spcont to the sp_rcontext of the PACKAGE BODY pkg > + - open the cursor in its sp_rcontext > + - restore thd->spcont to the sp_rcontext of the PROCEDURE p1 > + */ > + sp_rcontext *rcontext_backup= thd->spcont; > + thd->spcont= m_rcontext_handler->get_rcontext(thd->spcont); > + ret= tmp.open(thd); > + thd->spcont= rcontext_backup; wouldn't it be logical, if the cursor would know its context instead of relying on thd->spcont? then you wouldn't need this hackish swapping. > + thd->lex->query_arena()->mem_root->flags= backup_flags; > + > - if (!(ret= tmp.open(thd))) > + if (!ret) > { > Row_definition_list defs; > /* > + If m_rcontext_handler is sp_rcontext_handler_member then the cursor > + structure is being exported from a package wide cursor to a > + variable, e.g.: > + CREATE PACKAGE BODY pkg AS > + CURSOR c0 IS SELECT 1 AS c0, 'c1' AS c1 FROM DUAL; > + mr0 c0%ROWTYPE; > + ... > + END; may be you should use primarily non-ORACLE-mode syntax in code comments? > + In this case the cursor structure must have the same life cycle with > + the sp_package (pointed by thd->spcont->m_sp), thus let's use > + thd->spcont->m_sp->query_arena() as the arena. > + > + Otherwise, the structure is being exported for a local variable > + whose life cycle is the current sp_head::execute(). > Create row elements on the caller arena. > It's the same arena that was used during sp_rcontext::create(). > This puts cursor%ROWTYPE elements on the same mem_root > @@ -2106,14 +2155,95 @@ sp_instr_cursor_copy_struct::execute(THD *thd, uint *nextp) > void > sp_instr_cursor_copy_struct::print(String *str) > { > - sp_variable *var= m_ctx->find_variable(m_var); > - const LEX_CSTRING *name= m_ctx->find_cursor(m_cursor); > - str->append(STRING_WITH_LEN("cursor_copy_struct ")); > - str->append(name); > + const LEX_CSTRING cmd= {STRING_WITH_LEN("cursor_copy_struct ")}; > + const sp_pcursor *cursor= m_ctx->get_pcursor(*this); > + const sp_variable *var= m_ctx->get_pvariable(m_var); > + // cursor_copy_struct cur@0 var@0 > + print_cmd_and_var(cmd, cursor ? *cursor : empty_clex_str, *this, str); > str->append(' '); > + str->append(*m_var.rcontext_handler()->get_name_prefix()); > str->append(&var->name); > str->append('@'); > - str->append_ulonglong(m_var); > + str->append_ulonglong(m_var.offset()); > +} > + > + > +/* > + sp_instr_copen2 class functions. > +*/ > + > +PSI_statement_info sp_instr_copen2::psi_info= > +{ 0, "copen2", 0}; > + > + > +int > +sp_instr_copen2::execute(THD *thd, uint *nextp) > +{ > + DBUG_ENTER("sp_instr_copen2::execute"); > + m_lex_keeper.disable_query_cache(); > + int res= m_lex_keeper.cursor_reset_lex_and_exec_core(thd, nextp, false, this); > + *nextp= m_ip + 1; > + DBUG_RETURN(res); > +} > + > + > +int sp_instr_copen2::exec_core(THD *thd, uint *nextp) > +{ > + DBUG_ENTER("sp_instr_copen2::exec_core"); > + sp_cursor *cursor= m_rcontext_handler->get_cursor(thd, m_offset); > + /* > + CREATE PACKAGE BODY pkg > + CURSOR package_body_wide_cursor; > + PROCEDURE p1() > + BEGIN > + OPEN package_body_wide_cursor; -- #2 > + CLOSE package_body_wide_cursor; > + END; > + BEGIN > + OPEN package_body_wide_cursor; -- #1 > + CLOSE package_body_wide_cursor; > + END; > + > + "OPEN package_body_wide_cursor" can be called from two different places: > + #1. From the package initialization secion. In this case > + sp_head::mem_main_root::flags does not have the ROOT_FLAG_READ_ONLY bit > + yet - it will be set in the end of the current sp_head::execute(). > + #2. From a package routine. In this case sp_head::execute() representing > + the initialization section of the package was called earlier and > + its mem_main_root::flags already has the ROOT_FLAG_READ_ONLY flag. Why is it set? > + Unset it temporarily. > + */ > + /* > + TODO: > + this assert crashes due to a problem in the metadata modification code what problem? > + */ > + //DBUG_ASSERT(thd->spcont->m_sp->get_package() || > + // (thd->lex->query_arena()->mem_root->flags & ROOT_FLAG_READ_ONLY)); > + DBUG_ASSERT(thd->lex == m_lex_keeper.lex()); > + auto backup_flags= thd->lex->query_arena()->mem_root->flags; > + thd->lex->query_arena()->mem_root->flags&= ~ROOT_FLAG_READ_ONLY; > + > + /* > + Open the cursor in its sp_rcontext. > + See sp_instr_cursor_copy_struct::exec_core() for details. > + */ > + sp_rcontext *ctx_backup= thd->spcont; > + thd->spcont= m_rcontext_handler->get_rcontext(thd->spcont); > + int rc= cursor->open(thd); > + thd->spcont= ctx_backup; > + > + thd->lex->query_arena()->mem_root->flags= backup_flags; > + DBUG_RETURN(rc); > +} > + > + > +void > +sp_instr_copen2::print(String *str) > +{ > + const LEX_CSTRING cmd= {STRING_WITH_LEN("copen2 ")}; > + const sp_pcursor *cursor= m_ctx->get_pcursor(*this); > + /* copen2 name@offset */ > + print_cmd_and_var(cmd, cursor ? *cursor : empty_clex_str, *this, str); > } > > > diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc > --- a/sql/sp_rcontext.cc > +++ b/sql/sp_rcontext.cc > @@ -34,17 +34,48 @@ > > Sp_rcontext_handler_local sp_rcontext_handler_local; > Sp_rcontext_handler_package_body sp_rcontext_handler_package_body; > +Sp_rcontext_handler_member sp_rcontext_handler_member; > > sp_rcontext *Sp_rcontext_handler_local::get_rcontext(sp_rcontext *ctx) const > { > return ctx; > } > > +const sp_variable * > +Sp_rcontext_handler_local::get_pvariable(const sp_pcontext *pctx, uint i) const > +{ > + return pctx->find_variable(i); > +} > + > +const sp_pcursor * > +Sp_rcontext_handler_local::get_pcursor(const sp_pcontext *pctx, uint i) const > +{ > + return pctx->find_cursor(i); > +} > + > + > sp_rcontext *Sp_rcontext_handler_package_body::get_rcontext(sp_rcontext *ctx) const > { > return ctx->m_sp->m_parent->m_rcontext; > } > > +const sp_variable * > +Sp_rcontext_handler_package_body::get_pvariable(const sp_pcontext *pctx, uint i) > + const > +{ > + DBUG_ASSERT(0); > + return nullptr; What do you mean? One cannot define a variable in a package body? > +} > + > +const sp_pcursor * > +Sp_rcontext_handler_package_body::get_pcursor(const sp_pcontext *pctx, uint i) > + const > +{ > + return pctx->top_context()->sp()->m_parent-> > + get_parse_context()->find_member_cursor(i); > +} > + > + > const LEX_CSTRING *Sp_rcontext_handler_local::get_name_prefix() const > { > return &empty_clex_str; > @@ -57,6 +88,48 @@ const LEX_CSTRING *Sp_rcontext_handler_package_body::get_name_prefix() const > return &sp_package_body_variable_prefix_clex_str; > } > > +sp_cursor *Sp_rcontext_handler_local::get_cursor(THD *thd, uint offset) const > +{ > + return thd->spcont->get_cursor(offset); this is starting to look fragile, because you're now swapping thd->spcont back and forth. One more argument, why it wasn't a good idea and better to let cursor know its context instead. > +} > + > +sp_cursor *Sp_rcontext_handler_package_body::get_cursor(THD *thd, > + uint offset) const > +{ > + return get_rcontext(thd->spcont)->get_member_cursor(offset); > +} > + > + > +sp_rcontext *Sp_rcontext_handler_member::get_rcontext(sp_rcontext *ctx) const > +{ > + return ctx; > +} > + > +const LEX_CSTRING *Sp_rcontext_handler_member::get_name_prefix() const > +{ > + static const LEX_CSTRING sp_package_body_variable_prefix_clex_str= > + {STRING_WITH_LEN("MEMBER.")}; > + return &sp_package_body_variable_prefix_clex_str; > +} > + > +sp_cursor *Sp_rcontext_handler_member::get_cursor(THD *thd, > + uint offset) const > +{ > + return thd->spcont->get_member_cursor(offset); > +} > + > +const sp_variable * > +Sp_rcontext_handler_member::get_pvariable(const sp_pcontext *pctx, uint i) const > +{ > + return pctx->find_variable(i); > +} > + > +const sp_pcursor * > +Sp_rcontext_handler_member::get_pcursor(const sp_pcontext *pctx, uint i) const > +{ > + return pctx->top_context()->find_member_cursor(i); > +} > + > > /////////////////////////////////////////////////////////////////////////// > // sp_rcontext implementation. > diff --git a/sql/structs.h b/sql/structs.h > --- a/sql/structs.h > +++ b/sql/structs.h > @@ -862,26 +862,47 @@ class Lex_for_loop_bounds_intrange: public Lex_for_loop_bounds_st > }; > > > +/* > + This structure is used as a Bison token type. > + It must stay as small as possible, hence it uses a "union" declaration: > + - m_target_bound is used only in "FOR idx IN 0..9" style loops > + - m_cursor_rcontext_handler is used only in "FOR rec IN cursor" style loops > + They are never needed at the same time. > + See "%union size check" in sql_yacc.yy for details on the token size limit. > +*/ > struct Lex_for_loop_st > { > public: > + enum class Type : uchar > + { > + INT_RANGE= 0, // FOR i IN 1..10 > + CURSOR_IMPLICIT= 1, // FOR r IN (SELECT 1 FROM DUAL) > + CURSOR_EXPLICIT= 2 // FOR r IN cursor1 > + }; > class sp_variable *m_index; // The first iteration value (or cursor) > - class sp_variable *m_target_bound; // The last iteration value > + union > + { > + class sp_variable *m_target_bound; // The last iteration value > + const class Sp_rcontext_handler *m_cursor_rcontext_handler; > + }; > int m_cursor_offset; > int8 m_direction; > - bool m_implicit_cursor; > + Type m_type; you can also use bit fields to reduce sizeof(Lex_for_loop_st) even further. Like m_type:2, m_direction:1, m_cursor_offset:29 > void init() > { > m_index= 0; > m_target_bound= 0; > m_cursor_offset= 0; > m_direction= 0; > - m_implicit_cursor= false; > + m_type= Type::INT_RANGE; > + } > + bool is_for_loop_cursor() const > + { > + return m_type == Type::CURSOR_IMPLICIT || m_type == Type::CURSOR_EXPLICIT; > } > - bool is_for_loop_cursor() const { return m_target_bound == NULL; } > bool is_for_loop_explicit_cursor() const > { > - return is_for_loop_cursor() && !m_implicit_cursor; > + return m_type == Type::CURSOR_EXPLICIT; > } > }; > > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -6537,16 +6537,20 @@ LEX::find_variable(const LEX_CSTRING *name, > const Sp_rcontext_handler **rh) const > { > sp_variable *spv; > - if (spcont && (spv= spcont->find_variable(name, false))) > + const sp_pcontext *frame_pctx; > + if (spcont && (spv= spcont->find_variable(name, &frame_pctx, false))) > { > *ctx= spcont; > - *rh= &sp_rcontext_handler_local; > + if (frame_pctx == sphead->frame_for_members()) > + *rh= &sp_rcontext_handler_member; > + else > + *rh= &sp_rcontext_handler_local; > return spv; > } > sp_package *pkg= sphead ? sphead->m_parent : NULL; > - if (pkg && (spv= pkg->find_package_variable(name))) > + if (pkg && (spv= pkg->find_member_variable(name))) > { > - *ctx= pkg->get_parse_context()->child_context(0); > + *ctx= pkg->frame_for_members(); > *rh= &sp_rcontext_handler_package_body; I don't quite understand. Before your change the first if() was handling local variables, and the second - package-wide, as far as I understand. afte your change the first if() apparently also can find package-wide variables. What does the second if() do now? > return spv; > } > @@ -7182,20 +7248,26 @@ bool LEX::sp_for_loop_cursor_declarations(THD *thd, > thd->parse_error(); > DBUG_RETURN(true); > } > - if (unlikely(!(pcursor= spcont->find_cursor_with_error(&name, &coffs, > - false)) || > + > + sp_rcontext_addr cursor_addr(nullptr, 0); > + > + if (unlikely(!(pcursor= find_cursor_with_error(&name, &cursor_addr)) || > pcursor->check_param_count_with_error(param_count))) > DBUG_RETURN(true); > > if (!(loop->m_index= sp_add_for_loop_cursor_variable(thd, index, > - pcursor, coffs, > + pcursor, > + cursor_addr, > bounds.m_index, > item_func_sp))) > DBUG_RETURN(true); > loop->m_target_bound= NULL; > loop->m_direction= bounds.m_direction; > - loop->m_cursor_offset= coffs; > - loop->m_implicit_cursor= bounds.m_implicit_cursor; > + loop->m_cursor_rcontext_handler= cursor_addr.rcontext_handler(); > + loop->m_cursor_offset= cursor_addr.offset(); why do you store m_cursor_rcontext_handler and m_cursor_offset separately, wouldn't it be more natural to store, like loop->m_cursor_addr= cursor_addr; ? > + loop->m_type= bounds.m_implicit_cursor ? > + Lex_for_loop_st::Type::CURSOR_IMPLICIT : > + Lex_for_loop_st::Type::CURSOR_EXPLICIT; > DBUG_RETURN(false); > } > > @@ -8128,21 +8221,21 @@ Item *LEX::make_item_colon_ident_ident(THD *thd, > Item *LEX::make_item_plsql_cursor_attr(THD *thd, const LEX_CSTRING *name, > plsql_cursor_attr_t attr) > { > - uint offset; > - if (unlikely(!spcont || !spcont->find_cursor(name, &offset, false))) > + Cursor_ref ref(name, sp_rcontext_addr(nullptr, 0)); > + if (unlikely(!spcont || !find_cursor(name, &ref))) wait, why find_cursor() gets the name as an argument if name is already set in Cursor_ref? > { > my_error(ER_SP_CURSOR_MISMATCH, MYF(0), name->str); > return NULL; > } > switch (attr) { > case PLSQL_CURSOR_ATTR_ISOPEN: > - return new (thd->mem_root) Item_func_cursor_isopen(thd, name, offset); > + return new (thd->mem_root) Item_func_cursor_isopen(thd, ref); > case PLSQL_CURSOR_ATTR_FOUND: > - return new (thd->mem_root) Item_func_cursor_found(thd, name, offset); > + return new (thd->mem_root) Item_func_cursor_found(thd, ref); > case PLSQL_CURSOR_ATTR_NOTFOUND: > - return new (thd->mem_root) Item_func_cursor_notfound(thd, name, offset); > + return new (thd->mem_root) Item_func_cursor_notfound(thd, ref); > case PLSQL_CURSOR_ATTR_ROWCOUNT: > - return new (thd->mem_root) Item_func_cursor_rowcount(thd, name, offset); > + return new (thd->mem_root) Item_func_cursor_rowcount(thd, ref); > } > DBUG_ASSERT(0); > return NULL; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0