- developers - lists.mariadb.org

Re: 07364d701b3: Added status variable "Max_memory_used" to SHOW STATUS
by Sergei Golubchik 22 Dec '24

22 Dec '24

Hi, Michael, On Dec 22, Michael Widenius wrote: > revision-id: 07364d701b3 (mariadb-10.6.20-64-g07364d701b3) > parent(s): 13fed908291 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:26 +0200 > message: > > Added status variable "Max_memory_used" to SHOW STATUS > > This shows the maximum memory allocations used by the current connection. > The value for @@global.max_memory_used is 0 as we are not collecting this > value as it would cause a notable performance issue registering this for > all threads for every memory allocation Showing 0 is very confusing. I suggest not to print this variable in SHOW GLOBAL STATUS at all (probably possible, by returning a zero-length SHOW_ARRAY). Or show it as NULL. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 13fed908291: Added more information to errors reported by report_reply_packet()
by Sergei Golubchik 22 Dec '24

22 Dec '24

Hi, Monty, On Dec 22, Michael Widenius wrote: > revision-id: 13fed908291 (mariadb-10.6.20-63-g13fed908291) > parent(s): ca5ac5ac490 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:26 +0200 > message: > > Added more information to errors reported by report_reply_packet() > > In particular ""Read semi-sync reply magic number error" now prints out > what was wrong with the packet. > > @@ -622,7 +626,8 @@ int Repl_semi_sync_master::report_reply_packet(uint32 server_id, > log_file_len = packet_len - REPLY_BINLOG_NAME_OFFSET; > if (unlikely(log_file_len >= FN_REFLEN)) > { > - sql_print_error("Read semi-sync reply binlog file length too large"); > + sql_print_error("Read semi-sync reply binlog file length too large: %llu", > + (longlong) log_file_pos); You used unsigned "%llu" but cast to a signed longlong. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Can find sponsor: Would anyone be interested in patching mha to be MariaDB gtid compatible?
by highclass99 22 Dec '24

22 Dec '24

Hello, Would anyone be interested in patching mha to be MariaDB gtid compatible? I think I would be able to find a sponsor to pay a bit for it. reference links) https://github.com/yoshinorim/mha4mysql-manager https://github.com/yoshinorim/mha4mysql-node https://github.com/charlesdirk/MariaDB-MHA -> could not get this to work: if anyone got it to work please let me know how to get it to work. Thank you. Have a nice day.

3 3

Re: 3297680579f: Remove end newlines from CHECK TABLE output.
by Sergei Golubchik 21 Dec '24

21 Dec '24

Hi, Michael, On Dec 21, Michael Widenius wrote: > revision-id: 3297680579f (mariadb-10.6.20-59-g3297680579f) > parent(s): 4038ec2cc2c > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-12-17 10:45:25 +0200 > message: > > Remove end newlines from CHECK TABLE output. > > InnoDB has in some cases (not always) an newline at the end of error > messages. This messes up the CHECK TABLE output. > Fixed CHECK TABLE to remove the end newlines in its result set. I'd rather fix InnoDB to not do that, instead of working around InnoDB bug that garbles the CHECK TABLE output Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: e0afb5f08b5: cleanup: further replace thd->alloc() with operator new
by Sergei Golubchik 10 Dec '24

10 Dec '24

Hi, Nikita, Despite the commit has in the title, comments below apply to four commits: $ git log --oneline 7d5165894282^..0f6fd23e1604 0f6fd23e160 cleanup: add new(thd) to Sql_alloc and Item e0afb5f08b5 cleanup: further replace thd->alloc() with operator new 8daea2e14b2 cleanup: add operator new(size_t, const THD*) for single-object allocations 7d516589428 cleanup: add operator new[](size_t, const THD*) On Dec 06, Nikita Malyavin wrote: > diff --git a/sql/sql_class.h b/sql/sql_class.h > index aff02df69ec..299c8298606 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1279,11 +1279,5 @@ class Query_arena > { return state == STMT_CONVENTIONAL_EXECUTION; } > > template <typename T=char> > - inline T* alloc(size_t size) const > - { > - return (T*)alloc_root(mem_root, sizeof(T)*size); > - } > - > - template <typename T=char> > inline T* calloc(size_t size) const Hmm, you didn't remove thd->calloc<>() I understand why, but that kind of makes the API inconsistent, perhaps it would've been cleaner to keep thd->alloc<>() and only implement new (thd) and new (arena) for Sql_alloc? May be not only for Sql_alloc. There are 1. pod types. they can be alloc'ed an calloc'ed, I think thd->alloc/thd->calloc is fine for them 2. non-pod types, can be allocated on the heap and on a mem_root. your new (thd)/delete (thd) operators are best for them. 3. non-pod types that can only be allocated on a mem_root. Sql_alloc can be used for them, if it'll disallow non-placement new. I feel like we don't need to make a distinction between 2 and 3, in that case Sql_alloc can simply go away. > { > diff --git a/plugin/feedback/feedback.cc b/plugin/feedback/feedback.cc > index bc8a98c11ae..dcc2e4417e5 100644 > --- a/plugin/feedback/feedback.cc > +++ b/plugin/feedback/feedback.cc > @@ -103,22 +103,22 @@ static COND* make_cond(THD *thd, TABLE_LIST *tables, LEX_STRING *filter) > nrc->resolve_in_table_list_only(tables); > nrc->select_lex= tables->select_lex; > > - res= new (thd) Item_cond_or(thd); > + res= new (thd) Item_cond_or(thd); > if (!res) > return OOM; > > for (; filter->str; filter++) > { > - Item_field *fld= new (thd) Item_field(thd, nrc, db, table, > + Item_field *fld= new (thd) Item_field(thd, nrc, db, table, > field); > - Item_string *pattern= new (thd) Item_string(thd, filter->str, > + Item_string *pattern= new (thd) Item_string(thd, filter->str, > (uint) filter->length, cs); you might want to go through the diff and fix the indentation > - Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs); > + Item_string *escape= new (thd) Item_string(thd, "\\", 1, cs); > > if (!fld || !pattern || !escape) > return OOM; > > - Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern, > + Item_func_like *like= new (thd) Item_func_like(thd, fld, pattern, > escape, 0); > > if (!like) > diff --git a/sql/item.h b/sql/item.h > index 631fd1b19f8..b0cf03c1435 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -857,8 +857,9 @@ class Item :public Value_source, > static void *operator new(size_t size); > > public: > - static void *operator new(size_t size, MEM_ROOT *mem_root) throw () > + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept > { return alloc_root(mem_root, size); } > + static void *operator new(size_t size, const THD *thd) noexcept; > static void operator delete(void *ptr,size_t size) { TRASH_FREE(ptr, size); } > static void operator delete(void *ptr, MEM_ROOT *mem_root) {} Better inherit it from Sql_alloc > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index d86fdf5c5d6..e0adc5c3bb4 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -2007,7 +2007,7 @@ bool Item_func_interval::fix_length_and_dec(THD *thd) > > if (not_null_consts) > { > - intervals= thd->alloc<interval_range>(rows - 1); > + intervals= new(current_thd) interval_range[rows - 1]; current_thd ? > if (!intervals) > return true; > > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 008384a6590..21364286260 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -6086,8 +6086,8 @@ bool Ordered_key::init(int col_idx) > > // TIMOUR: check for mem allocation err, revert to scan > > - key_columns= thd->alloc<Item_field*>(1); > - compare_pred= thd->alloc<Item_func_lt*>(1); > + key_columns= new(thd) Item_field*();; > + compare_pred= new(thd) Item_func_lt*();; ;; > > key_columns[0]= new (thd) Item_field(thd, tbl->field[col_idx]); > /* Create the predicate (tmp_column[i] < outer_ref[i]). */ > diff --git a/sql/opt_subselect.h b/sql/opt_subselect.h > index 8b4dd1f0092..37902c6eebe 100644 > --- a/sql/opt_subselect.h > +++ b/sql/opt_subselect.h > @@ -357,7 +357,7 @@ uint get_number_of_tables_at_top_level(JOIN *join); > the tuple. > */ > > -class SJ_TMP_TABLE : public Sql_alloc > +class SJ_TMP_TABLE why? > { > public: > /* > diff --git a/sql/sql_alloc.h b/sql/sql_alloc.h > index f5d2d4e8b1a..e643a9f817d 100644 > --- a/sql/sql_alloc.h > +++ b/sql/sql_alloc.h > @@ -23,17 +23,21 @@ > class Sql_alloc > { > public: > - static void *operator new(size_t size) throw () > + static void *operator new(size_t size) noexcept > { > return thd_alloc(_current_thd(), size); > } can this be disallowed completely? > - static void *operator new[](size_t size) throw () > + static void *operator new[](size_t size) noexcept > { > return thd_alloc(_current_thd(), size); > } > - static void *operator new[](size_t size, MEM_ROOT *mem_root) throw () > + > + static void *operator new[](size_t size, const THD *thd) noexcept; > + static void *operator new(size_t size, const THD *thd) noexcept; > + > + static void *operator new[](size_t size, MEM_ROOT *mem_root) noexcept > { return alloc_root(mem_root, size); } > - static void *operator new(size_t size, MEM_ROOT *mem_root) throw() > + static void *operator new(size_t size, MEM_ROOT *mem_root) noexcept > { return alloc_root(mem_root, size); } > static void operator delete(void *ptr, size_t size) { TRASH_FREE(ptr, size); } > static void operator delete(void *, MEM_ROOT *){} > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 7847389dabc..37869da63ae 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -8831,3 +8831,46 @@ LEX_CSTRING make_string(THD *thd, const char *start_ptr, > size_t length= end_ptr - start_ptr; > return {strmake_root(thd->mem_root, start_ptr, length), length}; > } > +void* operator new(size_t size, const Query_arena *thd) noexcept > +{ > + return alloc_root(thd->mem_root, size); > +} > + > +void operator delete[](void *ptr, const Query_arena *thd) noexcept > +{} > + > +void operator delete(void *ptr, const Query_arena *thd) noexcept > +{} if you put them into sql_class.h - will they be inlined? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

MDEV-34705: Storing binlog in InnoDB
by Kristian Nielsen 06 Dec '24

06 Dec '24

I am in the early stages of a project to hopefully implement a new binlog format that is managed transactionally by InnoDB. This is a complex change, and I want to solicit comments early, so I encourage feedback. The TLDR; of this: - The current binlog format stores events separately from InnoDB. This requires a complex and _very_ constly 2-phase commit protocol between the binlog and InnoDB for every commit. - The binlog format is rather naive, and not well suited as a high-performance transactional log. It exposes internal implementation detail to the slaves, and its flat-file nature makes it inflexible to read (for example finding the last event sequential start from the start). Replacing it with something new has the potential to reap many benefits. - This project proposes to extend the storage engine API to allow an engine to implement binlog storage. It will be implemented in InnoDB so that binlog events are stored in InnoDB tablespaces, and the standard InnoDB transactional and recovery mechanisms are used to provide crash safety and optionally durability. - This mechanism will allow full crash-recovery and consistency without any fsync per commit (--innodb-flush-log-at-trx-commit=0), providing huge improvement in throughput for applications that do not require strict durability requirements. And it will reduce fsync requirement to one per commit for durable config, with greatly improved opportunities for group commit. Below a long and somewhat raw write-up of my current state of design. There was some interest expressed in learning more about this work, an interest that I'm very pleased with ;-). The current state of my prototype implementation is available in the branch knielsen_binlog_in_engine: https://github.com/MariaDB/server/commits/knielsen_binlog_in_engine Architectural considerations. The fundamental problem with the binlog is that it functions as a transactional log, but it is separate from the InnoDB transactional log. This is very unoptimal. To ensure consistency in case of crash, a two-phase commit protocol is needed that requires two fsync per commit, which is very costly in terms of performance. And it causes a lot of complexity in the code as well. We should make it so that there is only a single transactional log ("single source of truth") used, at least for the case where only a single storage engine is used (eg. InnoDB) for all tables. Some different options for doing this can be considered (explained in detail below): 1. Implement a new binlog format that is stored in a new InnoDB tablespace type (or more generally in any storage engine that implements the appropriate new API for binlog). 2. Keep the current binlog format, extend the InnoDB redo log format with write-ahead records for binlog writes. Integrate binlog writes with InnoDB checkpointing and buffer pool flush. 3. Store the binlog events directly inside the InnoDB redo log. Implement log archiving of the InnoDB log to preserve old binlog data as long as desired. 4. (Mentioned only for completeness): Implement a general redo log facility on the server level, and change both InnoDB and binlog to use that facility. I am currently working on option (1), inspired by talks I had with Marko Mäkelä at the MariaDB fest last Autumn. He suggested that a new type of tablespace be added to InnoDB which would hold the binlog, and explained that this would require relative small changes in the InnoDB code. I think this is the best option. The InnoDB log and tablespace implementation is very mature, it is very good to be able to re-use it for the binlog. The integration into InnoDB looks to be very clean, and all the existing infrastructure for log write, recovery, checkpointing, buffer pool can be re-used directly. All the low-level detail of how the binlog is stored on disk can be handled efficiently inside the InnoDB code. This option will require a lot of changes in the binlog and replication code, which is the main cost of choosing this option. I have a _lot_ of experience with the MariaDB replication code, and I think this is feasible. And I think there are another set of huge benefits possible from implementing a new binlog format. The existing binlog format is very naive and neither very flexible, nor well suited as a high-performance transactional log. So changing the binlog format is something that should be done eventually anyway. Option (2) would also be feasible I think, and has the advantage of needing less changes to the replication code. Monty mentioned seeing a patch that sounded something like this. The main disadvantage is that I think this will require complex interaction between low-level InnoDB checkpointing code and low-level binlog writing code, which I fear will eventually lead to complex code that will be hard to maintain and will stiffle further innovation on either side. It also doesn't address the other problems with the current binlog format. To me, letting the storage engine control how it stores data transactionally - including the binlog events - is the "right" solution, and worth persuing over a quicker hack that tries to avoid changing code. Option (3) is also interesting. The idea would be that the binlog becomes identical to the InnoDB redo log, storing the binlog data interleaved with the InnoDB log records. This is conceptually simpler than options (1) and (2), in the sense that we don't have to first write-ahead-log a record for modifying binlog data, and then later write the actual data modification to a separate tablespace/file. But the InnoDB log is cyclic, and old data is overwritten as the log LSN wraps over the end. Thus, this option (3) will require implementing log archiving in InnoDB, which I am not sure how much work will be required. Either the binlog data could be archived by copying out asynchroneously to persistent binlog files, which becomes somewhat similar to option (1) or (2) then perhaps. Or InnoDB could be changed to create a new log file at wrap-over, renaming the old one to archive it, though this will leave the binlog data somewhat "dilluted", mixed up with InnoDB internal redo records. One pragmatic reason I've chosen option (1) over this option (3) for now is that I am more comfortable with doing large changes to replication code than to InnoDB code. Also, option (3) would more drastically change the way users can work with the binlog files, as a large amount of the most recent data would be sitting inside the InnoDB redo log and not in dedicated binlog files, unlike options (1) and (2). Still, option (3) is an interesting idea and has the potential to reduce the write amplification that will occur from (1) and (2). So I'm eager to hear any suggestions around this. Option (4) I mention only for completeness. Somehow, the "clean" way to handle transactional logging would be if the server layer provides a general logging service, and all the components would then use this for logging. InnoDB and the binlog, but also all the other logs or log-like things we have, eg. Aria, DDL log, partitioning, etc. However, I don't think it makes sense to try to discard the whole InnoDB write-ahead logging code and replace it with some new implementation done on the server level. And I'm not even sure it makes sense from a theoretical point of view - somehow, the logging method used is intrinsically a property of the storage engine; it seems dubious if a general server-level log facility could be designed that would be optimal for both InnoDB and RocksDB, for example. High-level design. Introduce a new option --binlog-storage-engine=innodb. Extend the storage engine API with the option for an engine to provide a binlog implementation, supported by InnoDB initially. Initially the option cannot be changed dynamically. When --binlog-storage-engine=innodb, the binlog files will use a different format, stored as a new type of InnoDB tablespace. It should support that the old part of the binlog is the legacy format and the new is the InnoDB format, to facilitate migrations. Maybe also support going back again, though this is less critical initially. A goal is to clean up some of the unnecessary complexity of the legacy binlog format. The old format will be supported for the foreseeable future, so the new format can break backwards compatibility. For example, the name of binlog files will be fixed, binlog-<NNNNNN>.ibb, so each file can be identified solely by its number NNNNNN. The option to decide the directory in which to store the binlog files should be supported though. I think we can require the slaves to use GTID when --binlog-storage-engine=innodb is enabled on the master. This way we can avoid exposing slaves to internal implementation details of the binlog, and the slaves no longer need to update the file relay-log.info at every commit. We should also be able to get rid of the Format_description_log_event and the Rotate_log_event at the start of every binlog file, so that the slave can view the events from the master as one linear stream and not care about how the data is split into separate binlog files on the master. The binlog format will be page based. This will allow pre-allocating the binlog files and efficiently writing them page by page. And it will be possible to access the files without scanning them sequentially from the start; eg. we can find the last event by reading the last page of the file, binary-searching for the last used page if the binlog file is partially written. The GTID indexes can be stored inside the same tablespace as the binlog data (eg. at the end of the tablespace), avoiding the need for a separate index file. GTID index design is still mostly TBD, but I think it can be implemented so that indexes are updated transactionally as part of the InnoDB commit and no separate crash recovery of GTID indexes is needed. With GTID indexes being guaranteed available, we can use them to obtain the GTID state at the start of each binlog file, and avoid the need for the Gtid_list_log_event at the start of the binlog. With a page-based log with extensible format, metadata can be added to the binlog that is only used on the master without having to introduce new replication events that are not relevant to the replication on the slave. This can be used eg. to eliminate the Binlog_checkpoint_log_event, for example. Possibly the binlog checkpoint mechanism can be completely avoided for the --binlog-storage-engine=innodb case, since there is no more 2-phase commit needed in the common case of an InnoDB-only transaction. The existing binlog checksum and encryption will no longer be used, instead the standard InnoDB checksums and encryption will be reused. Implementation plan. The storage engine API must be extended to provide a facilities for writing to the binlog and for reading from the binlog. The design of the API is TBD, should be discussed to try to make it generic and suitable for other engines than InnoDB. When writing to binlog in InnoDB, the central idea is to use the same mini-transaction (mtr) that marks the transaction as committed in InnoDB. This is what makes the binlog data guaranteed consistent with the InnoDB table data without the need for two-phase commit. The initial version I think can use the existing binlog group commit framework; this will simplify the implementation. This will thus keep the LOCK_commit_ordered and the queue_for_group_commit() mechanisms. Later work can then be to see if this can be even further improved in terms of scalability. I want to implement that large event groups can be split into multiple chunks in the binlog that no longer need to be consecutive. The final chunk will be the one that contains the GTID and marks the event group binlogged; this final chunk will then refer back to the other parts. This way, a large transactions can be binlogged without stalling the writing of other (smaller) transactions in parallel, which is a bottleneck in the legacy binlog. And it avoids problems with exceeding the maximum size of an mtr. In the first version, I think the binlog reader in the dump thread will collect and concatenate the different chunks before sending them to the slave, so simplify the initial implementation. But a later change could allow the slave to receive the different chunks interleaved between different event groups. This can even eventually allow the slave to speculatively execute events even before the transaction has committed on the master, to potentially reduce slave lag to less than the time of even a single transaction; this would be a generalisation of the --binlog-alter-two-phase feature. The binlog will be stored in separate tablespace files, each of size --max-binlog-size. The binlog files will be pre-allocated by a background thread. Since event groups can spill over to the next file, each file can be a fixed size, and hopefully also the rotate events will no longer be necessary. A busy server will quickly cycle through tablespace files, and we want to avoid "using up" a new tablespace ID for each file (InnoDB tablespace IDs are limited to 2**32 and are not reused). Two system tablespace IDs will be reserved for the binlog, and new binlog files will alternate between them. This way, the currently written binlog can be active while the previous one is being flushed to disk and the remaining writes checkpointed. Once the previous log has been flushed completely, its tablespace ID can be re-used for the next, pre-allocated binlog file and be ready for becoming active. This way, the switching to the next binlog file should be able to occur seamlessly, without any stalls, as long as page flushing can keep up. The flushing of binlog pages will be prioritised, to avoid stalling binlog writes, to free up buffer pool pages that can be used more efficiently than holding binlog data, and to quickly make the binlog files readable from outside the server (eg. with mysqlbinlog). The binlog dump thread that reads from the binlog and sends the data to slaves will use a binlog reading API implemented in InnoDB. I will prefer to read directly from the binlog files, in order to reduce pressure on the InnoDB buffer pool etc. A slave that connects from an old position back in time may need to read a lot of old data from the binlogs; there is little value in loading this data into the buffer pool, evicting other more useful pages. The reader can lookup in the InnoDB buffer pool with the BUF_GET_IF_IN_POOL flag. This way, the data can be accessed from the buffer pool if it is present. If not present, we can be sure that the data will be in the data file, and can read it from the file directly. If the data is known to be already flushed to disk before the specific binlog position, then the buffer pool lookup can be skipped altogether. The mysqlbinlog program will need to be extended somehow to be able to read from an InnoDB tablespace (or in general other storage engine). I think this means mysqlbinlog needs some kind of engine plugin facility for reading binlog. Note that the -read-from-remote-server option will also be available to read data from a mysqlbinlog that doesn't the new format, or to read the very newest data before it gets written to disk. Final words. This is still an early draft of the feature, as it is being worked on and refined; things are subject to change. And this also means that things are *open* to change, according to any suggestions with a good rationale.

2 7

Review of MDEV-6017 Add support for Indexes on Expressions
by Michael Widenius 03 Dec '24

03 Dec '24

Hi! Here is the review for MDEV-6017 Add support for Indexes on Expressions commit c1e7574c799163f5d8a26ca8e0f91af6c91f7077 Instead of adding bool function virtual bool vcol_subst_analyzer(uchar **) { return false; } Why not add Item_base_t flag: IS_SIMPLE_BOOL_FUNC This takes one bit instead of increasing ALL created item base structures with 8 bytes. As we have 100+ items, adding a virtual function increases code space with 1K or more + the functions. You can also replace vcol_subst_analyzer() with a top level function: bool Item::vcol_subst_analyzer/( { return (type() == Item::FUNC_ITEM && (((Item_func*) this)->bitmap_bit() & (BITMAP_EQ | BITMAP_EQUAL | BITMAP_LT | BITMAP_GT | BITMAP_LE | BITMAP_GE | BITMAP_BETWEEN | BITMAP_IN | BITMAP_ISNULL | BITMAP_ISNOTNULL ) } For this to work, you would have to add the last two bitmaps to enum Bitmap in item_func.h @@ -3272,6 +3292,7 @@ class Item_cond :public Item_bool_func bool excl_dep_on_table(table_map tab_map) override; bool excl_dep_on_grouping_fields(st_select_lex *sel) override; + bool vcol_subst_analyzer(uchar **) override { return true; } I assume you need the above to be able to traverse AND and OR expressions for WHERE. Please add a comment that this is the intent. If you decide to use the bitmaps, add an explanation why AND and OR is in the vcol_subst_analyzer list. ---- +void collect_indexed_vcols_for_table(TABLE *tbl, List<Field> *vcol_fields) +{ Please use table instead of tbl. We use table as a varible much more than tbl: ((/my/maria-10.5/sql)) grep tbl *.* | wc 1143 5371 73297 ((/my/maria-10.5/sql)) grep table *.* | wc 40886 236754 2832202 + for (uint i=0; i < tbl->s->keys; i++) + { + // note: we could also support histograms here + if (!tbl->keys_in_use_for_query.is_set(i)) + continue; I wonder if it would be a good idea to extend bitmap to have an iterator like we have for tables where we only loop over the set bits. From sql_bitmap.h: class Table_map_iterator { ulonglong bmp; public: Table_map_iterator(ulonglong t): bmp(t){} uint next_bit() { if (!bmp) return BITMAP_END; uint bit= my_find_first_bit(bmp); bmp &= ~(1ULL << bit); return bit; } int operator++(int) { return next_bit(); } enum { BITMAP_END= 64 }; }; Would be quite trival to do also for keys. This would help tables with many keys and also would allows us to remove the 'call' to if (!tbl->keys_in_use_for_query.is_set(i)) We could also do an optimized version where we implement bitmap_get_next_set() where we do not have to reset the previous bits for each call. --------- + if (field->vcol_info) + vcol_fields->push_back(field); You have to take care of error handling here. Please change the functions collect_indexed_vcols_for_table(), collect_indexed_vcols_for_join() and other functions that calls push_back() to bool and return 1 in case of error. + item->top_level_compile(ctx->thd, + &Item::vcol_subst_analyzer, &yes, + &Item::vcol_subst_transformer, (uchar*)ctx); You also have to check the return of the above for errors. Another option is to just check thd->fatal_error() as this will be set in case of OOM. .... + { + if (auto nested_join= table->nested_join) + subst_vcols_in_join_list(ctx, &nested_join->join_list); PLEASE do not use auto. I want to know the type while reading and debugging the code! auto can also introduce bugs if a type changes. + @todo: this cannot handle VIEW columns currently. View columns are wrapped + in Item_direct_view_ref objects which do not comsider themselves equal to + Item_field objects they're wrapping. Please add this also to the commit comment. + /* + Do not do the substitution if the datatypes do not match. + Virtual column's datatype can be different from the expression, for + example: + + col3 INT AS (CONCAT(col1, col2)) + + Do not allow substitutions that change the semantics of comparison. + At the moment, we require that datatypes are the same. + This probably could be relaxed. + + For strings: we allow two cases: + - vcol_expr and vcol_field have the same collation. + - vcol_field has the same collation as the comparison collation. + + (Note: MySQL calls resolve_type() after it has done the substitution. + This can potentially update the comparator. The idea is that this + shouldn't be necessary as we do not want to change the comparator. + Changing the comparator will change the semantics of the condition, + our point is that this must not happen) + */ Move to a function comment. It is hard to read read the function code with this long comment in the middle. + if (vcol_expr->type_handler_for_comparison() != + vcol_field->type_handler_for_comparison() || + vcol_expr->maybe_null() != vcol_field->maybe_null()) + fail_cause="type mismatch"; Why cannot we handle this case if vcol_expr->maybe_null is 0 and vcol_field->maybe_null() is set? In this case we know that the vcol_field's we are going to encounter can never be null, which should satisfy our requirements. + Item_field *itf= new (thd->mem_root) Item_field(thd, vcol_field); + if (!itf) + return false; Better to return true in case of errors, like we do in most of the other code. Remove and a blank line instead + //Item *const_expr; ... remove + //const_expr= args[0]; ----- +Item* Item_func_in::vcol_subst_transformer(THD *thd, uchar *arg) +{ + Vcol_subst_context *ctx= (Vcol_subst_context*)arg; + if (!compatible_types_scalar_bisection_possible()) Add comment /* Check that all arguments inside IN() are constants */ Regards, Monty

1 0

Re: 5b6b23563bf: MDEV-34979 generate SBOM from server builds
by Sergei Golubchik 02 Dec '24

02 Dec '24

Hi, Vladislav, Thanks! See comments below On Dec 02, Vladislav Vaintroub wrote: > revision-id: 5b6b23563bf (mariadb-11.7.1-5-g5b6b23563bf) > parent(s): eff9c198e32 > author: Vladislav Vaintroub > committer: Vladislav Vaintroub > timestamp: 2024-11-25 19:04:18 +0100 > message: > > MDEV-34979 generate SBOM from server builds > > This commit adds the capability to generate a Software Bill of Materials > (SBOM) from server builds. > > It introduces a new WITH_SBOM variable, which defaults to ON for package > builds (i.e if BUILD_CONFIG is used) and to OFF otherwise. > > When enabled, the build process will produce an sbom.json document in > CycloneDX format, capturing information about various dependencies, > which is gathered from various sources. > > We use git submodule information and CMake external projects properties > to gather version information for 3rd party code, but also handle > dependencies if external code is part of our repository > (zlib, or Connect storage engine's minizip) > > The SBOM document is stored in the root build directory in sbom.json file, > but is not currently installed. > > diff --git a/CMakeLists.txt b/CMakeLists.txt > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -581,6 +581,13 @@ ENDIF() > > INCLUDE(build_depends) > > +INCLUDE(CMakeDependentOption) > +CMAKE_DEPENDENT_OPTION(WITH_SBOM "Generate Software Bill of Materials (SBOM)" OFF "BUILD_CONFIG" ON) > +IF(WITH_SBOM) > + INCLUDE(generate_sbom) > + GENERATE_SBOM() > +ENDIF() This doesn't work for me, I mean, cmake -DBUILD_CONFIG=mysql_release . does not generate the file, but cmake -DBUILD_CONFIG=mysql_release . -DWITH_SBOM=ON does. May be it'd be more robust to use OPTION() and set it from cmake/build_configuration/mysql_release.cmake? Like it's done for other options. > + > INCLUDE(CPack) > > IF(WIN32 AND SIGNCODE) > diff --git a/cmake/generate_submodule_info.cmake b/cmake/generate_submodule_info.cmake > --- /dev/null > +++ b/cmake/generate_submodule_info.cmake > @@ -0,0 +1,73 @@ ... > + FOREACH(s ${out_list}) > + IF(${counter} EQUAL 0) > + IF (NOT("${s}" MATCHES "Entering '")) does it depend on the locale? > + MESSAGE(FATAL "Unexpected output ${outvar}") > + ENDIF() > + STRING(LENGTH "${s}" slen) > + MATH(EXPR substr_len "${slen} - 11") ... > diff --git a/cmake/generate_sbom.cmake b/cmake/generate_sbom.cmake > --- /dev/null > +++ b/cmake/generate_sbom.cmake > @@ -0,0 +1,240 @@ > +INCLUDE(generate_submodule_info) > +INCLUDE(ExternalProject) > + > +# Remove all matches from a cmake list > +FUNCTION (REMOVE_ALL_MATCHES list_var_name match_str) > + SET(new_list) > + FOREACH(el ${${list_var_name}}) > + IF(NOT("${el}" MATCHES "${match_str}")) > + LIST(APPEND new_list ${el}) > + ENDIF() > + ENDFOREACH() > + SET(${list_var_name} ${new_list} PARENT_SCOPE) > +ENDFUNCTION() you could've used LIST(FILTER ...) instead. (but not LIST(REMOVE_ITEM ...) which is 3.16+) > + > +# Extract user name and repository name from a github URL. > +FUNCTION (EXTRACT_REPO_NAME_AND_USER repo_url repo_name_var repo_user_var) > + IF(repo_url MATCHES "^git@") > + # normalize to https-style URLs > + STRING(REGEX REPLACE "^git@([^:]+):(.*)$" "https://\\1/\\2" repo_url "${repo_url}") I don't think we have that, is it even tested? :) note, I'm not saying it isn't needed, better to have it than not. > + ENDIF() > + # Extract the repository user > + STRING(REGEX REPLACE "https://([^/]+)/([^/]+)/.*" "\\2" repo_user "${repo_url}") > + > + STRING(REGEX REPLACE ".*/([^/]*)$" "\\1" repo_name "${repo_url}") > + STRING(REGEX REPLACE "\\.git$" "" repo_name "${repo_name}") > + > + SET(${repo_name_var} ${repo_name} PARENT_SCOPE) > + SET(${repo_user_var} ${repo_user} PARENT_SCOPE) > +ENDFUNCTION() > + > +# Add a known 3rd party dependency for SBOM generation > +# Currently used for "vendored" (part of our repository) source code we know about > +# such as zlib, as well ExternalProject_Add() projects > +MACRO(ADD_THIRD_PARTY_DEPENDENCY name url tag rev version description) > + LIST(FIND ALL_THIRD_PARTY ${name} idx) > + IF (idx GREATER -1) > + MESSAGE(FATAL_ERROR "${name} is already in ALL_THIRD_PARTY") > + ENDIF() > + SET(${name}_URL ${url}) > + SET(${name}_TAG ${tag}) > + SET(${name}_REVISION ${rev}) > + SET(${name}_DESCRIPTION "${description}") > + SET(${name}_VERSION "${version}") > + LIST(APPEND ALL_THIRD_PARTY ${name}) > +ENDMACRO() > + > +# Match third party component with supplier > +# CyclonDX documentation says it is > +# "The organization that supplied the component. > +# The supplier may often be the manufacturer, but may also be a distributor or repackager." > +# > +# Perhaps it can always be "MariaDB", but security team recommendation is different > +# more towards "author" > +FUNCTION (sbom_get_supplier repo_name repo_user varname) why a function? just curious > + IF("${repo_name_SUPPLIER}") > + SET(${varname} "${repo_name_SUPPLIER}" PARENT_SCOPE) ... > + # It is possible to provide EXTRA_SBOM_DEPENDENCIES > + # and accompanying per-dependency data, to extend generared sbom > + # document. > + # Example below injects an extra "ncurses" dependency using several > + # command line parameters for CMake. > + # -DEXTRA_SBOM_DEPENDENCIES=ncurses > + # -Dncurses_URL=https://github.com/mirror/ncurses > + # -Dncurses_TAG=v6.4 > + # -Dncurses_VERSION=6.4 > + # -Dncurses_DESCRIPTION="A fake extra dependency" > + SET(ALL_THIRD_PARTY ${ALL_SUBMODULES} ${EXTRA_SBOM_DEPENDENCIES}) okay. Though I personally wouldn't use ALL_SUBMODULES and EXTRA_SBOM_DEPENDENCIES and instead would scan GET_CMAKE_PROPERTY(VARS VARIABLES) for *_TAG or something > + > + IF(TARGET ha_connect OR TARGET connect) > + ADD_THIRD_PARTY_DEPENDENCY(minizip > + "https://github.com/zlib-ng/minizip-ng" > + "" 252588f "" > + "Vendored minizip, inside connect storage engine, storage/connect/zip.c et al") did you verify that our file matches the content of the file from that repo at that commit? also, a though - how can we prevent our minigzip.c from being modified without updaing the dependency? Suggestion, check the commit hash, like git log -n1 --format=%h storage/connect/zip.c > + ENDIF() > + > + IF(TARGET libfmt) > + ExternalProject_GET_PROPERTY(libfmt URL) > + STRING(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" libmt_TAG "${URL}") > + ADD_THIRD_PARTY_DEPENDENCY(libfmt > + "https://github.com/fmtlib/fmt" > + "${libmt_TAG}" "${libmt_TAG}" "${libmt_TAG}" > + "header only library, used in server") > + ENDIF() I wonder, wouldn't it be simpler and more robust to overwrite or wrap ExternalProject() to do the above automatically? (using the same name or a different one like MyExternalProject) ... Also, I noticed that the server entry gets "purl": "pkg:github//@5b6b23563bf" perhaps it should have a repo name here? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 534dda1234b: MDEV-35522: MariaDB Audit does not detect all DCLs forms when masking password
by Sergei Golubchik 02 Dec '24

02 Dec '24

Hi, Oleksandr, Ok to push. But do you no longer delete server_audit.log after the test? On Dec 02, Oleksandr Byelkin wrote: > revision-id: 534dda1234b (mariadb-10.5.27-64-g534dda1234b) > parent(s): fdb6db6b47f > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2024-11-29 11:45:01 +0100 > message: > > MDEV-35522: MariaDB Audit does not detect all DCLs forms when masking password > > 1. skip OR REPLACE > (to make it filter_query_type made recursive) > > 2. skip SET STATEMENT ... FOR before checking > statements with passwords > > diff --git a/mysql-test/suite/plugins/t/server_audit_pwd_mask.test b/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > --- a/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > +++ b/mysql-test/suite/plugins/t/server_audit_pwd_mask.test > --echo # cleaunup > +DROP SERVER s1; > DROP USER u1; > DROP USER u2; > set global server_audit_logging=off; > ---remove_file $SEARCH_FILE > +##--remove_file $SEARCH_FILE why? > --disable_warnings > UNINSTALL PLUGIN ed25519; > UNINSTALL PLUGIN server_audit; > --enable_warnings Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: ff3d06f6601: MDEV-22217 Make OS character sets "utf8" and "utf-8" map to MariaDB character set "utf8mb4"
by Sergei Golubchik 16 Nov '24

16 Nov '24

Hi, Alexander, Of course, there can be no comments about the code change here. But the question is, is it something we want to do? 1. Is utf8/utf-8 locale indeed 4-byte utf8? 2. Why is hard-coding to utf8mb4 better than having it dependent on UTF8_IS_UTF8MB3 ? On Nov 16, Alexander Barkov wrote: > revision-id: ff3d06f6601 (mariadb-11.6.1-44-gff3d06f6601) > parent(s): 4b6922a315f > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-10-28 14:11:40 +0400 > message: > > MDEV-22217 Make OS character sets "utf8" and "utf-8" map to MariaDB character set "utf8mb4" > > diff --git a/mysql-test/main/mysql_locale_posix.test b/mysql-test/main/mysql_locale_posix.test > index c047b7b1ae9..b37d04baca2 100644 > --- a/mysql-test/main/mysql_locale_posix.test > +++ b/mysql-test/main/mysql_locale_posix.test > @@ -203,3 +203,20 @@ DROP DATABASE `Ã³`; > + > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_client" > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_connection" > +--exec LC_ALL=en_US.utf8 $MYSQL --default-character-set=auto test -e "SELECT @@character_set_results" may be do it in one SELECT? or in SHOW VARIABLES LIKE 'character_set%' ? to have it a tiny bit faster? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0