- developers - lists.mariadb.org

Re: 6d4479d0e32: MDEV-36565 Assertion `src != ((void *)0)' failed in my_casedn_8bit
by Sergei Golubchik 16 Apr '25

16 Apr '25

Hi, Alexander, I don't know, this looks like a strange place to fix this. I'd say that either Item_func_xml_extractvalue shouldn't return {0, 0} or, if we'll treat it as a valid String, caseup() and casedn() should be able to handle it. But in a similarly-looking bug MDEV-35945 Assertion `src != ((void *)0)' failed in my_caseup_8bit you fixed it to use { "", 0 } for an empty string. It's commit c69fb1a6273. Then I'd suggest to do the same here. Let's avoid {0, 0} ? On Apr 16, Alexander Barkov wrote: > revision-id: 6d4479d0e32 (mariadb-11.4.5-25-g6d4479d0e32) > parent(s): d3c9a2ee212 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2025-04-15 19:30:44 +0400 > message: > > MDEV-36565 Assertion `src != ((void *)0)' failed in my_casedn_8bit > > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 2e3c33059d3..5df25c7f237 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -1998,7 +1998,16 @@ String *Item_str_conv::val_str(String *str) > goto err; > > null_value= false; > - len= converter(collation.collation, (char*) res->ptr(), res->length(), > + /* > + Avoid passing nullptr as the second parameter to converter(): > + - val_str() of some Items (e.g. Item_func_xml_extractvalue) can return > + Strings with {Ptr=0,str_length=0} meaning an empty string. > + - But my_charset_handler_st virtual function caseup() and casedn() > + (which "converter" point to) do not expect {src=nullptr,srclen=0} input. > + */ > + DBUG_ASSERT(res->ptr() != nullptr || res->length() == 0); > + len= !res->ptr() ? 0/*Avoid passing nullptr*/ : > + converter(collation.collation, (char*) res->ptr(), res->length(), > (char*) str->ptr(), alloced_length); > DBUG_ASSERT(len <= alloced_length); > str->set_charset(collation.collation); > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: MDEV-36290: ALTER TABLE with multi-master can cause data loss (PR #3967)
by Kristian Nielsen 16 Apr '25

16 Apr '25

Brandon Nesterenko <notifications(a)github.com> writes: > MDEV-36290: ALTER TABLE with multi-master can cause data loss > https://github.com/MariaDB/server/pull/3967 Hi Brandon, Here is my initial review of the MDEV-36290 draft patch. Overall, it is clear that this is not something that's appropriate for 10.6. It implements a new way to apply row events, based on column names (if available) rather than column number. It is something for next dev version, 12.x. > commit de27bfbafc22601d53ddc0567401fcb26d28a91c > Author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > Date: Thu Apr 3 12:37:09 2025 -0600 > > MDEV-36290: ALTER TABLE with multi-master can cause data loss Please do not use the term "data loss" in the description (I know it is not you who introduced the term ;-). The issue here is conflicting queries run in parallel against multiple masters in a multi-master setup. In general, the support of multi-master in asynchronous replication requires the user to ensure that no conflicting queries are run, or to ensure that such conflicts will not cause undesirable behaviour. The behaviour addressed here is just one of many ways that conflicting ALTER TABLE could cause different data on slave and master. Using the term "data loss" can cause misunderstanding about what the problem is and what the change does. Of course, every case that is handled more correctly is in principle better. In this case, we introduce the facility to configure (--binlog_row_metadata=FULL) replication so that it is possible to replicate between tables on master and slave with different order of the columns. Not sure how useful such an option is in practice, seems to be very few users that will really need this, and such need has to be weighed against the increase in complexity in the code and potential for bugs. With that said, the approach in the patch generally looks reasonable. A few initial detail comments below, as you mentioned this is still a draft patch. As you also mentioned to me earlier, the logic in the patch is not trivial. I did not yet try to understand in detail all parts of the logic and the different corner cases. > + unsigned char *get_optional_metadata_str() > + { > + return m_optional_metadata; > + } > + > + unsigned int get_optional_metadata_len() > + { > + return m_optional_metadata_len; > + } > + Generally I do not like such do-nothing accessor functions. They just add abstraction and reduce grepability without doing anything useful. That's a style preference of course. > + /* > + Maps column index from master to slave. This is determined using the field > + names (provied by optional metadata when the master is configured with > + binlog_row_metadata=FULL). > + */ > + std::map<uint, uint> master_to_slave_index_map; > + > + /* > + When using field names to map from master->slave columns, this keeps track > + of column indices on the master which aren't present on the slave. > + > + It is used to skip columns when checking type-conversions and unpacking > + row data. > + */ > + std::set<uint> master_unmatched_cols; Is using std::map/std::set going to be expensive here, this is operations that will happen on every row-based event (when --binlog-row-metadata=FULL)? The std:: functions can imply a lot of memory allocations that are not very visible, and we usually try to avoid them in performance-relevant server code. master_to_slave_index_map is just a lookup table of numbers 0..#columns-1, right? So it doesn't need to be an associative array, a normal array is sufficient, and doesn't need to be re-allocated each event. And master_unmatched_cols is again just a set of numbers 0..#columns-1, so a bitmap could be used. - Kristian.

1 0

CI failures visible on GitHub (affecting contributors)
by Otto Kekäläinen 15 Apr '25

15 Apr '25

Hi, I see that on the 11.8 branch (https://github.com/MariaDB/server/commits/11.8/) which is supposed to be fully green and passing CI as it is soon to be released as a new LTS version, actually isn't consistently green. Out of the latest 10 CI runs the jobs passing rates are: 12/!2 11/12 12/!2 12/!2 8/8 10/12 8/12 7/8 8/8 11/12 Questions: - What is the current plan to get CI to be consistently green? - How is it possible that a commit is pushed on the 11.8 branch even though CI is not passing? Isn't branch protection preventing it? - If there is a way to get commits on 11.8 branch with failures bypassing branch protection, are all developers committed to follow-up on their commits and fix them before they do any other work? Or what is the plan for how to ensure post-push test failures are fixed? - The jobs buildbot/amd64-debian-11-msan-clang-16 and continuous-integration/appveyor/branch seem to be the most common failing ones. Can they be removed now to make the CI consistently green? And apply full branch protection so that they, or any other jobs, can't be added back to unless they are actually green? - Why is the CI occasionally tunning 8 jobs instead of 12? I also see plenty of red at https://buildbot.mariadb.org/#/grid?branch=11.8. Hopefully we get as much green as possible for the start of the 11.8 maintenance so that any potential regressions in future stable maintenance releases of 11.8 series are easier to catch. Most of the issues seem to be test issues and not real MariaDB bugs. I have also done a bunch of testing on Debian QA systems for MariaDB 11.8 and I have only found a couple small issues, so overall 11.8 is looking good! - Otto

2 1

MariaDB 11.8.1 fully compatible with Y2038
by Otto Kekäläinen 09 Apr '25

09 Apr '25

Hi, I just wanted to report that I tested building MariaDB in 2025 and running MTR in 2037 and 2038 using libfaketime[1]. I can confirm no test started failing in 2038, thus MariaDB had no Y2038 issues in my testing. [1] https://salsa.debian.org/mariadb-team/mariadb-server/-/merge_requests/74

1 0

MariaDB Memory Map
by o.halaiev＠gmail.com 08 Apr '25

08 Apr '25

Hi MariaDB team I'm Oleksii Halaev, studying Python and DataScience I'm currently working on a GSoC 2025 proposal and would like to contribute to MariaDB with an idea I'm developing - it's called ‘MariaDB Memory Map’ The idea is to create an interactive visualisation tool that shows how data changes in the MariaDB database over time - how it is added, modified, deleted and how it ‘travels’ between tables Think of it as a sort of ‘life map’ of your data I believe this can be a useful tool for developers and DBAs to gain a deeper understanding of their databases, make debugging easier, and even track down data anomalies Here is my suggestion: https://drive.google.com/file/d/1vOMF0MZRTueEKkavWdwvEIP6_t2UgJLA/view?usp=… I'd really appreciate any feedback, advice or thoughts - and of course, if there's anyone out there who might be interested in mentoring this project, I'd be happy to chat and learn more Thanks for your time! I look forward to hearing from you GitHub: https://github.com/Alekcentijules Email: o.halaiev(a)gmail.com

2 1

Re: 0ed7370d133: MDEV-24726 Assertion `0' failed in Field_varstring_compressed::key_cmp
by Sergei Golubchik 04 Apr '25

04 Apr '25

Hi, Aleksey, Despite the email subject, this is a combined diff of three commits, git diff 071c20fd23b2 2fecd944d6dc On Apr 04, Aleksey Midenkov wrote: > diff --git a/sql/field.cc b/sql/field.cc > index 8b96c473ab6..d6be66cf399 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2516,7 +2521,7 @@ Field *Field::new_key_field(MEM_ROOT *root, TABLE *new_table, > uchar *new_null_ptr, uint new_null_bit) > { > Field *tmp; > - if ((tmp= make_new_field(root, new_table, table == new_table))) > + if ((tmp= make_new_field({root, new_table, table == new_table}))) it's a creative trick, I admit, but let's not do it. looks very weird, we don't have it anywhere else. it's not needed either, because you don't need to pass down a new argument, see below > { > tmp->ptr= new_ptr; > tmp->null_ptr= new_null_ptr; > @@ -8278,12 +8281,28 @@ int Field_varstring::cmp_binary(const uchar *a_ptr, const uchar *b_ptr, > } > > > -Field *Field_varstring::make_new_field(MEM_ROOT *root, TABLE *new_table, > - bool keep_type) > +Field *Field_varstring::make_new_field(make_new_field_args args) > { > - Field_varstring *res= (Field_varstring*) Field::make_new_field(root, > - new_table, > - keep_type); > + Field_varstring *res= (Field_varstring*) Field::make_new_field(args); > + if (res) > + res->length_bytes= length_bytes; > + return res; > +} > + > + > +Field *Field_varstring_compressed::make_new_field(make_new_field_args args) > +{ > + Field_varstring *res; > + if (args.tmp_field) tmp field is a field of a tmp table. Unfortunately, INTERNAL_TMP_TABLE value of new_table->s->tmp_table is very overloaded and no longer means internal temporary table anymore. I think we'll need to fix it someday. But a smaller fix for today could be, like @@ -1113,6 +1113,11 @@ struct TABLE_SHARE return (tmp_table == SYSTEM_TMP_TABLE) ? 0 : table_map_id; } + bool is_optimizer_tmp_table() + { + return tmp_table == INTERNAL_TMP_TABLE && !db.length && table_name.length; + } + using the fact that optimizer tmp tables are all named "(temporary)" or something like that and have no db. Using this one can do a fix, like @@ -8281,11 +8279,23 @@ int Field_varstring::cmp_binary(const uchar *a_ptr, con> Field *Field_varstring::make_new_field(MEM_ROOT *root, TABLE *new_table, bool keep_type) { - Field_varstring *res= (Field_varstring*) Field::make_new_field(root, + Field_varstring *res; + if (new_table->s->optimizer_tmp_table()) + { + res= new (root) Field_varstring(ptr, field_length, length_bytes, null_ptr, + null_bit, Field::NONE, &field_name, + new_table->s, charset()); + if (res) + res->init_for_make_new_field(new_table, orig_table); + } + else + { + res= (Field_varstring*) Field::make_new_field(root, new_table, keep_type); if (res) res->length_bytes= length_bytes; + } return res; } > + { > + /* Compressed fields can't have keys (see Field_varstring_compressed::key_cmp()). */ > + res= new (args.root) Field_varstring(*this); > + res->init_new_field(args); > + /* See Column_definition::create_length_to_internal_length_string() */ > + res->field_length--; > + } > + else > + res= (Field_varstring*) Field::make_new_field(args); > if (res) > res->length_bytes= length_bytes; > return res; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

MDEV-36107: Proposal for Enhancements in MariaDB Operator Handling
by Harshit Vijay Kumar 02 Apr '25

02 Apr '25

Dear MariaDB Developer Community, I hope you are doing well. I am *Harshit Vijay Kumar*, a final-year undergraduate student in Computer Science. I have been researching *MDEV-36107 <https://jira.mariadb.org/browse/MDEV-36107>* for *GSoC 2025* and, during my exploration of operator operations in MariaDB, I came across some behaviors that caught my attention. To further investigate, I compared these behaviors with MySQL to understand their differences. *Observations: * *(1) Handling Large Number Multiplications Using Exponents* - Currently, multiplying two large integers results in an *overflow*, returning 0 instead of using an exponent-based representation. SELECT 18014398509481984 * 18014398509481984; *MariaDB Output:* 0 *SQL Output:* 3.2451855365842673e+32 - Interestingly, if one of the operands is a *floating-point number*, the result is computed without overflow: SELECT 18014398509481984 * 18014398509481984399405765456789087654390954.0; *Output:* 324518553658426733978210646499548233026655740664701555572736.0 - This inconsistency between integer-only and mixed-type operations is confusing. A standardized approach, possibly leveraging exponent notation, could improve clarity. *(2) Rounding Floating-Point Outputs* - MariaDB currently returns full precision in floating-point calculations, which can lead to unnecessary precision in practical use cases. SELECT 1.0376728938476536278903984765 + 1.039876453672890938476536278902345678908765434567890; *MariaDB Output:* 2.07754934752054456636693475540234567891 *SQL Output:* 2.077549347520544 - A rounding mechanism would allow for concise representation and reduce storage overhead, especially for users unaware of excessive precision in their datasets. *(3) Eliminating Unnecessary Zeros in Floating-Point Representation* SELECT 1.0000 * 1.0000000000; *MariaDB Output:* 1.00000000000000 - While useful in some cases, the inclusion of excessive trailing zeros unnecessarily increases storage usage. A more compact representation should be considered, where only *significant digits* are retained. *(4) Implementation of == Equality Operator* - Currently, MariaDB allows the *assignment operator (=)* for comparisons. However, since = is primarily used for assignments, this dual-purpose behavior can be confusing. - Proposal: Introduce a dedicated *== equality operator* for clear and intuitive comparisons across *INT, FLOAT, BOOL, and CHAR* types. - Expected behavior: - If values match → return 1 - If values don’t match → return 0 Testing & Next Steps To implement these improvements, I plan to add *test cases* in mysql-test covering all edge cases for these operators. I will follow the approach used in the following implementations: - func_equal.result <https://github.com/MariaDB/server/blob/main/mysql-test/main/func_equal.resu…> - func_equal.test <https://github.com/MariaDB/server/blob/main/mysql-test/main/func_equal.test> I would really appreciate your insights on these ideas. If any of these behaviors have intentional design choices, I would love to understand the rationale behind them. Looking forward to your feedback! Best Regards, Harshit Vijay Kumar

1 0

Re: 0a0c5f349c6: MDEV-36425 Extend read_only to also block share locks and super user
by Sergei Golubchik 02 Apr '25

02 Apr '25

Hi, Monty, On Apr 01, Michael Widenius wrote: > revision-id: 0a0c5f349c6 (mariadb-11.8.1-42-g0a0c5f349c6) > parent(s): 21321025e5e > author: Michael Widenius > committer: Michael Widenius > timestamp: 2025-03-31 20:09:03 +0300 > message: > > MDEV-36425 Extend read_only to also block share locks and super user > > The main purpose of this allow one to use the --read-only > option to ensure that no one can issue a query that can > block replication. > > The --read-only option can now take 4 different values: > 0 No read only (as before). > 1 Blocks changes for users without the 'READ ONLY ADMIN' > privilege (as before). > 2 Blocks in addition LOCK TABLES and SELECT IN SHARE MODE > for not 'READ ONLY ADMIN' users. > 3 Blocks in addition 'READ_ONLY_ADMIN' users for all the > previous statements. > diff --git a/mysql-test/main/read_only.result b/mysql-test/main/read_only.result > index 65cc12ffce9..89390ad65d5 100644 > --- a/mysql-test/main/read_only.result > +++ b/mysql-test/main/read_only.result > @@ -69,11 +69,14 @@ set global read_only=1; > connection con1; > select @@global.read_only; > @@global.read_only > -0 > +OFF > unlock tables ; > +Timeout in wait_condition.inc for SELECT @@global.read_only= 1 > +Id User Host db Command Time State Info Progress > +5 test localhost test Query 0 starting show full processlist 0.000 Forgot to fix? It should wait for @@global.read_only='READ_ONLY' now > select @@global.read_only; > @@global.read_only > -1 > +READ_ONLY > connection default; > reap; > connection default; > diff --git a/sql/lock.cc b/sql/lock.cc > index 1296e5455d5..8f9f97f88b7 100644 > --- a/sql/lock.cc > +++ b/sql/lock.cc > @@ -179,19 +198,48 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) > > /* > Prevent modifications to base tables if READ_ONLY is activated. > - In any case, read only does not apply to temporary tables. > + In any case, read only does not apply to temporary tables or slave > + threads. > */ > - if (!(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) && !t->s->tmp_table) > + if (unlikely(opt_readonly) && > + !(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) && !t->s->tmp_table && > + !thd->slave_thread) > { > - if (t->reginfo.lock_type >= TL_FIRST_WRITE && > - !ignore_read_only && opt_readonly && !thd->slave_thread) > + switch (opt_readonly) // Impossible you have "Impossible" comment on a wrong line > { > - my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only"); > - DBUG_RETURN(1); > + case READONLY_OFF: > + DBUG_ASSERT(0); > + break; > + case READONLY_ON: // Compatibility > + if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && > + t->reginfo.lock_type >= TL_FIRST_WRITE) > + { > + mariadb_error_read_only(); > + DBUG_RETURN(1); I'd make mariadb_error_read_only() to return 1. Then you can write DBUG_RETURN(mariadb_error_read_only()); here and also in all other places, check it out, it's always followed by return 1. > + } > + break; > + case READONLY_NO_LOCK: > + if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && > + (thd->lex->sql_command == SQLCOM_LOCK_TABLES || > + t->reginfo.lock_type >= TL_FIRST_WRITE || > + t->reginfo.lock_type == TL_READ_WITH_SHARED_LOCKS)) may be t->reginfo.lock_type >= TL_READ_WITH_SHARED_LOCKS ? > + { > + mariadb_error_read_only(); > + DBUG_RETURN(1); > + } > + break; > + case READONLY_NO_LOCK_NO_ADMIN: > + if (thd->lex->sql_command == SQLCOM_LOCK_TABLES || > + t->reginfo.lock_type >= TL_FIRST_WRITE || > + t->reginfo.lock_type == TL_READ_WITH_SHARED_LOCKS) and here > + { > + mariadb_error_read_only(); > + DBUG_RETURN(1); > + } > + break; > } > } > } > - > /* > Locking of system tables is restricted: > locking a mix of system and non-system tables in the same lock > diff --git a/sql/sql_class.h b/sql/sql_class.h > index d65cec37154..34ee8391009 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3448,9 +3452,12 @@ class THD: public THD_count, /* this must be first */ > */ > inline bool is_read_only_ctx() rename to, for example, check_read_only_with_errror() > { > - return opt_readonly && > - !(security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && > - !slave_thread; > + if (likely(!opt_readonly) || slave_thread || > + ((security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && > + opt_readonly != READONLY_NO_LOCK_NO_ADMIN)) > + return false; > + mariadb_error_read_only(); > + return true; > } > > private: > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 0247afd91a6..545b4674da1 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3253,15 +3253,24 @@ static bool fix_read_only(sys_var *self, THD *thd, enum_var_type type) > transition (especially when transitioning from false to true) and > synchronizes both booleans in the end. > */ > -static Sys_var_on_access_global<Sys_var_mybool, > + > +const char *read_only_mode_names[]= > +{"OFF", "READ_ONLY", "READ_ONLY_NO_LOCK", "READ_ONLY_NO_LOCK_NO_ADMIN"}; > + > +static Sys_var_on_access_global<Sys_var_enum, > PRIV_SET_SYSTEM_GLOBAL_VAR_READ_ONLY> > Sys_readonly( > "read_only", > - "Make all non-temporary tables read-only, with the exception for " > - "replication (slave) threads and users with the 'READ ONLY ADMIN' " > - "privilege", > - GLOBAL_VAR(read_only), CMD_LINE(OPT_ARG), DEFAULT(FALSE), > - NO_MUTEX_GUARD, NOT_IN_BINLOG, > + "Do not allow changes to non-temporary tables. Options are:" > + "OFF changes allowed. " > + "READ_ONLY blocks changes for users without the 'READ ONLY ADMIN' " "Disallow changes for users without the READ ONLY ADMIN privilege" > + "privilege. " > + "READ_ONLY_NO_LOCK blocks in addition LOCK TABLES and " > + "SELECT IN SHARE MODE for not ADMIN users. " "Additionally disallows LOCK TABLES and SELECT IN SHARE MODE" > + "READ_ONLY_NO_LOCK_NO_ADMIN blocks in addition 'ADMIN' users. " "Disallows also for users with READ_ONLY ADMIN privilege" > + "Replication (slave) threads are not affected by this option", > + GLOBAL_VAR(read_only), CMD_LINE(OPT_ARG), > + read_only_mode_names, DEFAULT(0), NO_MUTEX_GUARD, NOT_IN_BINLOG, > ON_CHECK(check_read_only), ON_UPDATE(fix_read_only)); > > // Small lower limit to be able to test MRR Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Inquiry About GSoC 2025 and Proposal Submission
by Harsh Gupta 31 Mar '25

31 Mar '25

*Dear MariaDB Developers,* I hope you are doing well. My name is *Harsh Gupta*, and I am a *second-year B.Tech Computer Science and Design student at Madhav Institute of Technology and Science, Gwalior*. I am passionate about databases and open-source development, and I am eager to contribute to MariaDB through *Google Summer of Code 2025*. I have gone through the project ideas listed for GSoC and found the MDEV-28395 <https://jira.mariadb.org/browse/MDEV-28395> LOAD DATA plugins particularly interesting. I have experience with *C++ and database systems*, and I have been exploring MariaDB’s codebase to get familiar with its architecture. I have prepared a draft proposal outlining my approach to this project, including my implementation plan and timeline. I would appreciate your feedback to refine it further before submitting it officially. Could you please review my proposal and provide any suggestions for improvement? Also, I would love to connect with potential mentors to discuss the project's scope and feasibility. Looking forward to your guidance and feedback! Best regards, *Harsh Gupta* 📧 harshgupta9329(a)gmail.com

1 0

Re: b7dbe187b1a: MDEV-34680 Asynchronous and Buffered Logging for Audit Plugin.
by Sergei Golubchik 31 Mar '25

31 Mar '25

Hi, Alexey, See few comments below. But the main question, could you have used FILE or IO_CACHE instead of a custom implementation? On Mar 08, Alexey Botchkov wrote: > revision-id: b7dbe187b1a (mariadb-11.8.1-8-gb7dbe187b1a) > parent(s): 6f1161aa34c > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2025-03-02 16:27:23 +0400 > message: > > MDEV-34680 Asynchronous and Buffered Logging for Audit Plugin. > > Buffering added to the file logger. > Two buffers are created so writing to the file doesn't > block other threads from logging. > Operations isolated with two mutexes. > diff --git a/include/mysql/service_logger.h b/include/mysql/service_logger.h > index 35c2eb1e3a9..adce7bc9038 100644 > --- a/include/mysql/service_logger.h > +++ b/include/mysql/service_logger.h > @@ -64,41 +64,44 @@ extern struct logger_service_st { > void (*logger_init_mutexes)(); > LOGGER_HANDLE* (*open)(const char *path, > unsigned long long size_limit, > - unsigned int rotations); > + unsigned int rotations, size_t buffer_size); incompatible API change. MUST change the major service version. > int (*close)(LOGGER_HANDLE *log); > int (*vprintf)(LOGGER_HANDLE *log, const char *fmt, va_list argptr) > ATTRIBUTE_FORMAT_FPTR(printf, 2, 0); > int (*printf)(LOGGER_HANDLE *log, const char *fmt, ...) > ATTRIBUTE_FORMAT_FPTR(printf, 2, 3); > - int (*write)(LOGGER_HANDLE *log, const char *buffer, size_t size); > + int (*write)(LOGGER_HANDLE *log, const void *data, size_t size); > int (*rotate)(LOGGER_HANDLE *log); > + int (*sync)(LOGGER_HANDLE *log); > } *logger_service; > > #ifdef MYSQL_DYNAMIC_PLUGIN > > #define logger_init_mutexes logger_service->logger_init_mutexes > -#define logger_open(path, size_limit, rotations) \ > - (logger_service->open(path, size_limit, rotations)) > +#define logger_open(path, size_limit, rotations, buffer_size) \ > + (logger_service->open(path, size_limit, rotations, buffer_size)) > #define logger_close(log) (logger_service->close(log)) > #define logger_rotate(log) (logger_service->rotate(log)) > +#define logger_rsync(log) (logger_service->sync(log)) > #define logger_vprintf(log, fmt, argptr) (logger_service->\ > vprintf(log, fmt, argptr)) > #define logger_printf (*logger_service->printf) > -#define logger_write(log, buffer, size) \ > - (logger_service->write(log, buffer, size)) > +#define logger_write(log, data, size) \ > + (logger_service->write(log, data, size)) > #else > > void logger_init_mutexes(); > LOGGER_HANDLE *logger_open(const char *path, > unsigned long long size_limit, > - unsigned int rotations); > + unsigned int rotations, size_t buffer_size); > int logger_close(LOGGER_HANDLE *log); > int logger_vprintf(LOGGER_HANDLE *log, const char *fmt, va_list argptr) > ATTRIBUTE_FORMAT(printf, 2, 0); > int logger_printf(LOGGER_HANDLE *log, const char *fmt, ...) > ATTRIBUTE_FORMAT(printf, 2, 3); > - int logger_write(LOGGER_HANDLE *log, const char *buffer, size_t size); > + int logger_write(LOGGER_HANDLE *log, const void *data, size_t size); > int logger_rotate(LOGGER_HANDLE *log); > + int logger_sync(LOGGER_HANDLE *log); > #endif > > > diff --git a/mysys/file_logger.c b/mysys/file_logger.c > index a753c049f68..5464e43eddf 100644 > --- a/mysys/file_logger.c > +++ b/mysys/file_logger.c > @@ -145,85 +230,108 @@ static int do_rotate(LOGGER_HANDLE *log) > namebuf[log->path_len]= 0; > result= my_rename(namebuf, logname(log, log->path, 1), MYF(0)); > log->file= my_open(namebuf, LOG_FLAGS, MYF(0)); > + log->in_file= 0; > exit: > errno= my_errno; > return log->file < 0 || result; > } > > > -/* > - Return 1 if we should rotate the log > -*/ > - > -my_bool logger_time_to_rotate(LOGGER_HANDLE *log) > -{ > - my_off_t filesize; > - if (log->rotations > 0 && > - (filesize= my_tell(log->file, MYF(0))) != (my_off_t) -1 && > - ((ulonglong) filesize >= log->size_limit)) > - return 1; > - return 0; > -} > - > - > int logger_vprintf(LOGGER_HANDLE *log, const char* fmt, va_list ap) > { > - int result; > char cvtbuf[1024]; > size_t n_bytes; > > - flogger_mutex_lock(&log->lock); > - if (logger_time_to_rotate(log) && do_rotate(log)) > - { > - result= -1; > - errno= my_errno; > - goto exit; /* Log rotation needed but failed */ > - } > - > n_bytes= my_vsnprintf(cvtbuf, sizeof(cvtbuf), fmt, ap); > if (n_bytes >= sizeof(cvtbuf)) > n_bytes= sizeof(cvtbuf) - 1; > > - result= (int)my_write(log->file, (uchar *) cvtbuf, n_bytes, MYF(0)); > - > -exit: > - flogger_mutex_unlock(&log->lock); > - return result; > + return logger_write(log, cvtbuf, n_bytes); > } > > > -static int logger_write_r(LOGGER_HANDLE *log, my_bool allow_rotations, > - const char *buffer, size_t size) > +int logger_write(LOGGER_HANDLE *log, const void *data, size_t size) > { > int result; > + size_t in_buffer; > + > + mysql_mutex_lock(&log->lock); > > - flogger_mutex_lock(&log->lock); > - if (allow_rotations && logger_time_to_rotate(log) && do_rotate(log)) > + if (log->buffer_left >= size) > + { > + memcpy(log->buffer_pos, data, size); > + log->buffer_pos+= size; > + log->buffer_left-= size; > + mysql_mutex_unlock(&log->lock); > + return size; > + } > + > + mysql_mutex_lock(&log->lock_buffer); > + > + in_buffer= log->buffer_pos - log->buffer; > + > + { /* swap buffers. */ > + uchar *tmp_buf= log->buffer; > + log->buffer= log->idle_buffer; > + log->idle_buffer= tmp_buf; > + } > + > + set_buffer_pos_and_left(log); > + > + /* Now other threads can write to the new buffer. */ > + mysql_mutex_unlock(&log->lock); > + > + if ((in_buffer && > + my_write(log->file, log->idle_buffer, in_buffer, MYF(0)) != in_buffer)|| > + my_write(log->file, (uchar *) data, size, MYF(0)) != size) > { > result= -1; > errno= my_errno; > - goto exit; /* Log rotation needed but failed */ > + goto exit_buf; > } > + else > + result= 0; > > - result= (int)my_write(log->file, (uchar *) buffer, size, MYF(0)); > + log->in_file+= in_buffer + size; > + if (log->rotations > 0 && log->in_file >= log->size_limit) > + { > + if (do_rotate(log)) > + { > + result= -1; > + errno= my_errno; > + goto exit_buf; /* Log rotation needed but failed */ > + } > + } > > -exit: > - flogger_mutex_unlock(&log->lock); > +exit_buf: > + mysql_mutex_unlock(&log->lock_buffer); > return result; > } > > > -int logger_write(LOGGER_HANDLE *log, const char *buffer, size_t size) > -{ > - return logger_write_r(log, TRUE, buffer, size); > -} > - > int logger_rotate(LOGGER_HANDLE *log) > { > int result; > - flogger_mutex_lock(&log->lock); > - result= do_rotate(log); > - flogger_mutex_unlock(&log->lock); > + mysql_mutex_lock(&log->lock); > + mysql_mutex_lock(&log->lock_buffer); > + result= flush_buffer(log) || do_rotate(log); I'd though you can switch buffers here too and flush/rotate only under the lock_buffer mutex > + mysql_mutex_unlock(&log->lock_buffer); > + mysql_mutex_unlock(&log->lock); > + return result; > +} > + > + > +int logger_sync(LOGGER_HANDLE *log) Why did you introduce an explicit sync API? > +{ > + int result; > + mysql_mutex_lock(&log->lock); > + mysql_mutex_lock(&log->lock_buffer); > + if ((result= flush_buffer(log))) > + { > + result= fsync(log->file); > + } > + mysql_mutex_unlock(&log->lock_buffer); > + mysql_mutex_unlock(&log->lock); > return result; > } > > diff --git a/plugin/server_audit/server_audit.c b/plugin/server_audit/server_audit.c > index 5fc0e3373f3..e51df7ab6b5 100644 > --- a/plugin/server_audit/server_audit.c > +++ b/plugin/server_audit/server_audit.c > @@ -1393,26 +1401,15 @@ static void change_connection(struct connection_info *cn, > static int write_log(const char *message, size_t len, int take_lock) > { > int result= 0; > + > if (take_lock) > - { > - /* Start by taking a read lock */ > mysql_prlock_rdlock(&lock_operations); > - } > > if (output_type == OUTPUT_FILE) > { > if (logfile) > { > - my_bool allow_rotate= !take_lock; /* Allow rotate if caller write lock */ > - if (take_lock && logger_time_to_rotate(logfile)) > - { > - /* We have to rotate the log, change above read lock to write lock */ > - mysql_prlock_unlock(&lock_operations); > - mysql_prlock_wrlock(&lock_operations); > - allow_rotate= 1; > - } > - if (!(is_active= (logger_write_r(logfile, allow_rotate, message, len) == > - (int) len))) > + if (!(is_active= (logger_write(logfile, message, len) == (int) len))) nice, rotation is indeed the logger's headache, not the caller's > { > ++log_write_failures; > result= 1; > @@ -2899,6 +2906,36 @@ static void update_file_rotations(MYSQL_THD thd __attribute__((unused)), > } > > > +static void update_file_buffer_size(MYSQL_THD thd __attribute__((unused)), > + struct st_mysql_sys_var *var __attribute__((unused)), > + void *var_ptr __attribute__((unused)), const void *save) > +{ > + file_buffer_size= *(unsigned int *) save; > + > + error_header(); > + fprintf(stderr, "Log file buffer size was changed to '%d'.\n", file_buffer_size); > + > + if (!logging || output_type != OUTPUT_FILE) > + return; > + > + ADD_ATOMIC(internal_stop_logging, 1); > + if (!maria_55_started || !debug_server_started) > + mysql_prlock_wrlock(&lock_operations); > + > + stop_logging(); > + if (start_logging()) this could be an API call in the logger. Like "reinit(new params)" and it'll close, change params, open - all under a log mutex, which doesn't need to be destroyed and reinitialized. > + { > + error_header(); > + fprintf(stderr, "Logging was disabled..\n"); > + CLIENT_ERROR(1, "Logging was disabled.", MYF(ME_WARNING)); > + } > + > + if (!maria_55_started || !debug_server_started) > + mysql_prlock_unlock(&lock_operations); > + ADD_ATOMIC(internal_stop_logging, -1); > +} > + > + > static void update_file_rotate_size(MYSQL_THD thd __attribute__((unused)), > struct st_mysql_sys_var *var __attribute__((unused)), > void *var_ptr __attribute__((unused)), const void *save) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1