- developers - lists.mariadb.org

Re: [Maria-developers] 6a72d15: MDEV-10767 /tmp/wsrep_recovery.${RANDOM} file created in unallowed SELinux context
by Sergei Golubchik 13 Sep '17

13 Sep '17

Hi, Sachin! On Aug 31, sachin wrote: > revision-id: 6a72d154d6ec231eeff861496e4d07101ac41553 (mariadb-10.1.26-16-g6a72d15) > parent(s): dda40b930498b70bb5546f857b27744039a5649d > author: sachin > committer: sachin > timestamp: 2017-08-31 12:17:10 +0530 > message: > > MDEV-10767 /tmp/wsrep_recovery.${RANDOM} file created in unallowed SELinux context > > Solution:- Allowed mysqld_t to open file with context initrc_tmp_t. Why does the server need it? I've only found scripts/galera_recovery.sh, that does: log_file=$(mktemp /tmp/wsrep_recovery.XXXXXX) /usr/sbin/mysqld $cmdline_args --user=$user --wsrep_recover --log-error="$log_file" For that use case, I wouldn't bother with a new selinux policy, it'd be safer to do log_file=$(mktemp /tmp/wsrep_recovery.XXXXXX) /usr/sbin/mysqld $cmdline_args --user=$user --wsrep_recover --disable-log-error 2>"$log_file" Note, that the similar piece of code is used in mysqld_safe.sh, only there it uses DATADIR/wsrep_recovery.XXXXXX, apparently somebody's attempt to work around selinux. Better to make it identical to galera_recovery.sh, I'd say. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] [External] Obsolete GTID domain delete on master (MDEV-12012, MDEV-11969)
by andrei.elkin＠pp.inet.fi 13 Sep '17

13 Sep '17

Hello, Simon, Kristian. (The mail was meant to be sent out yesterday, but it got stuck in my outgoing box). > Simon Mudd <simon.mudd(a)booking.com> writes: > >> ids. Obviously once all appropriate bin logs have been purged >> (naturally by other means) then no special processing will be needed. > > Right. Hence my original idea (which was unfortunately never implemented so > far). If at some point a domain has been unused for so long that all GTIDs > in that domain are gone, it is relatively safe to pretend that the domain > never existed. > > I would like to understand if you can think of significant use cases where > the DBA needs to have active binlog files in the master containing some > domain, while simultaneously pretending that this domain never existed. > > Or if it is more of a general concern, and the inconvenience for users to > have to save old binlogs somewhere else than the master's data directory and > binlog index (SHOW BINARY LOGS). > >> removing old binary logs should _not_ IMO be done as a way of >> forgetting the past obsolete domains. >> BINLOGS are important so throwing them away is an issue. I think >> that somehow the code needs >> to be aware of the cut-off point and when the “stale domain ids” are removed.) Simon, initially I thought of masking out the problematic domain so that the most recent binlog file would not have it in its Gtid_list header. Yet I've given up that idea to have agreed the strict setup on master weighs much more. > > I understand the desire to not delete binlog files. And the mdev-12012 use case might even not require to conduct this purging/flush-delete-domain procedure if IGNORE_DOMAIN_IDS (for the zero id domain in question) would do. There seems to be MDEV-9108 in the way though, but conceptually the DBA may have a way to stay with the binlog files even having a problematic domain. > > The problem is: If you want to have GTIDs with some domain in your active > binlog files, _and_ you also want to pretend that this domain never existed, > what does it mean? What is the semantics? It creates a lot of complexities > for defining the semantics, for documenting it, for the users to understand > it, and for the code to implement it correctly. > > So basically, I do not understand what is the intended meaning of FLUSH > BINARY LOGS DELETE DOMAIN d _and_ at the same time keeping GTIDs with domain > d around in active binlog files? In what respects is the domain deleted, and > in what respects not? > > For the master, the binlog files are mainly used to stream to connecting > slaves. Deleting a domain means replacing the conceptual binlog history with > one in which that domain never existed. So that domain will be ignored in a > connecting slaves position, assuming it is served by another multi-source > master. If a new GTID in that domain appears later, it will be considered > the very first GTID ever in that domain. > > So consider what happens if there is anyway GTIDs in that domain deeper in > the binlog: > > 1. An already connected slave may be happily replicating those GTIDs. If > that slave reconnects (temporary network error for example), it will instead > fail with unknown GTID, or perhaps just start silently ignoring all further > GTIDs in that domain. This kind of unpredictable behaviour seems bad. > > 2. Suppose a slave connects with a position without the deleted domain. The > master starts reading the binlog from some point. What happens if a GTID is > encountered that contains the deleted domain? The slave will start > replicating that domain from some arbitrary point that depends on where it > happened to be in other domains at the last disconnect. This also seems > undesirable. > > There may be other scenarios that I did not think about. > >> DBAs do not like to remove bin logs “early" as unless you keep a copy >> somewhere you may lose valuable information, >> for recovery, for backups etc. Not everyone will be making automatic >> copies (as MySQL does not provide an automatic way to do this) > > Understood. Maybe what is needed is a PURGE BINARY LOGS that removes the > entries from the binlog index (SHOW BINARY LOGS), but leaves the files in > the file system for the convenience of the sysadmin? (Well, you can just > hand-edit binlog.index, but that requires master restart I think). Like I said above, a filtering solution could be helpful. > >> The other comment I see mentioned here was “make sure all slaves are >> up to date”. That’s going to be hard. The master can only be >> aware of “connected slaves” and if you have intermediate masters, or a > > Indeed, the master cannot ensure this. The idea is that the DBA, who decides > to delete a domain, must understand that this should not be done if any > slave still needs GTIDs from that domain. This is similar to configuring > normal binlog purge, where the DBA needs to ensure that binlogs are kept > long enough for the needs of the slowest slave. > >> FWIW expiring old domains is good to do. There’s a similar FR for > >> completely different the problem space is the same. Coming up with a >> solution which is simple to use and understand and also >> avoids where that’s possible making mistakes which may break >> replication is good. So thanks for looking at this. > > Indeed. And the input from people like you with strong operational > experience is very valuable to end up with a good solution, hence my request > for additional input. > > - Kristian. I hope clarifications given by Kristian back up the new feature idea. On the other hand it would be good to have a way for DBA to cope with the likes of mdev-12012 (there are few reports on this matter) without purging. I rely on a solution to MDEV-9108 in that regard. Cheers, Andrei

1 0

[Maria-developers] A problem with implementing Group Commit with Binlog with MyRocks
by Sergey Petrunia 12 Sep '17

12 Sep '17

Hello, This is about https://jira.mariadb.org/browse/MDEV-11934. I've encountered an insteresting issue here, so I thought I would consult on both MyRocks and MariaDB lists. == Some background == "group commit with binlog" feature needs to accomplish two goals: 1. Keep the binlog and the storage engine in sync. This is done by employing XA between the binlog and the storage engine. It works by making these calls: /* Make the transaction's changes to be ready to be committed (no conflicts with other transactions, etc) but do not commit them yet. The effects of the prepare operation must be synced to disk, as the storage engine needs to be able to recover (i.e. commit) the prepared transaction after a crash */ storage_engine->prepare(sync=true); /* After this call, the transaction is considered committed. In case of a crash, the recovery process will use the contents of the binlog to determine which of the prepared transactions are to be committed and which are to be rolled back. */ binlog->write(sync=true); /* Commit the transaction in the storage engine. This makes its changes visible to other transactions (and also releases its locks and so forth) Note that most of the time(*) we don't need to sync there. In case of a crash we will be able to recover using the binlog. */ storage_engine->commit(); 2. The second goal is to make operation performant. We need two coordinated disk flushes per transaction, the idea is to do "Group Commit" where multiple transactions share disk flushes. So, we need to do group commit and keep the storage engine and the binlog in sync while doing that. == Group Commit with Binlog in MySQL == MySQL (and fb/mysql-5.6 in particular) does in the following phases: Phase #1: Call storage_engine->prepare() for all transactions in the group. The call itself is not persistent. Phase #2: Call storage->engine->flush_logs(). This makes the effect of all Prepare operations from Phase#1 persistent. Phase #3: Write and sync the binary log. Phase #4: Call storage_engine->commit(). This does not need to be persistent. MyRocks implements them. == Group Commit with Binlog in MariaDB == MariaDB does not have these phases described above: > Phase #1: > Call storage_engine->prepare() for all transactions in the group. > The call itself is not persistent. > > Phase #2: Call storage->engine->flush_logs(). > This makes the effect of all Prepare operations from Phase#1 persistent. A quote from Kristian's description at https://lists.launchpad.net/maria-developers/msg10832.html >> So the idea is to do group prepare with the same group of transactions that >> will later group commit to the binlog. In MariaDB, this concept does not >> exist. Storage engine prepares are allowed to run in parallel and in any >> order compared to binlog commit. Initially this looked like it could work for MyRocks. MyRocks has a group commit implementation, both Prepare() and Commit() operations participate in groups. However when I implemented a group commit implementation I found its performance to be close to what one would expect if there was no commit grouping, and commit() call flushed to disk https://jira.mariadb.org/browse/MDEV-11934 has the details. == The issue == (I'm 95% certain about this. It's not 100% yet but it is very likely) RocksDB's Group Write (see rocksdb/rocksdb/db/db_impl_write.cc, DBImpl::WriteImpl function) handles both Prepare() and Commit() commands and does the following: 1. Controls writing the commited data into the MemTable 2. Writes transactions to WAL 3. Syncs the WAL. All three steps are done for the whole group. This has a consequence: a Commit() operation that does not need to sync the WAL will still be delayed if another operation in the group needs the WAL to be synced. This delay has a disastrous effect, because SQL layer tries to have the same order of transactions in the storage engine and in the binlog. In order to do that, it calls rocksdb_commit_ordered() for each transaction sequentially. Delaying one transaction causes a delay of the entire SQL-level commit group. == Possible solutions == I am not sure what to do. - Make the SQL layer's Group Commit implementation invoke hton->flush_logs() explicitly, like MySQL does? - Modify RocksDB so that Transaction::Commit(sync=false) do not use Group Write? I am not sure if this is possible: Group Write is not about only performance, it's about preventing concurrent MemTable writes. AFAIU one cannot just tell a certain DBImpl::WriteImpl() call to not participate in write groups and work as if there was no other activity. - Modify RocksDB so that Transaction::Commit(sync=false) does not wait until its write group finishes WAL sync? This could be doable but is potentially complex. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

3 5

[Maria-developers] ha_innobase::info_low() n_rows hack
by Aleksey Midenkov 11 Sep '17

11 Sep '17

In ha_innobase::info_low() there is following dirty hack: /* The MySQL optimizer seems to assume in a left join that n_rows is an accurate estimate if it is zero. Of course, it is not, since we do not have any locks on the rows yet at this phase. Since SHOW TABLE STATUS seems to call this function with the HA_STATUS_TIME flag set, while the left join optimizer does not set that flag, we add one to a zero value if the flag is not set. That way SHOW TABLE STATUS will show the best estimate, while the optimizer never sees the table empty. */ if (n_rows == 0 && !(flag & HA_STATUS_TIME)) { n_rows++; } It is very old (from 5.0 or earlier) and bug-prone. Because in ha_innobase::open(): info(HA_STATUS_NO_LOCK | HA_STATUS_VARIABLE | HA_STATUS_CONST); every opened empty table will be non-empty! I don't know what is the problem with join optimizer, but having storage engine to handle it seems not the right thing to do. Moreover, relying on HA_STATUS_TIME in this is definitely wrong. We can make join optimizer to ignore "0 rows case" for all storage engines. Is it big win from "1 row case" anyway? Or we can make new flag HA_JOIN_STAT and use it in make_join_statistics().

2 3

Re: [Maria-developers] A problem with implementing Group Commit with Binlog with MyRocks
by Sergey Petrunia 11 Sep '17

11 Sep '17

Hi Kristian, On Mon, Sep 11, 2017 at 01:02:31PM +0200, Kristian Nielsen wrote: > Sergey Petrunia <sergey(a)mariadb.com> writes: > > > This is about https://jira.mariadb.org/browse/MDEV-11934. I've encountered > > an insteresting issue here, so I thought I would consult on both MyRocks and > > MariaDB lists. > > Is your current code available somewhere? > No, it wasn't. Just pushed the current code into bb-10.2-mdev11934 branch: https://github.com/MariaDB/server/tree/bb-10.2-mdev11934 All changes are in the last commit. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] 2nd 2017 Developers Unconference
by Ian Gilfillan 11 Sep '17

11 Sep '17

The dates for the Developers Unconference in Shenzhen, China, are coming up fast. This is our first official event in Asia, and will be held from 13 November - 17 November. 13 November - new contributor day 14-15 November - traditional developers unconference 16-17 November - patch days As always, it's free to attend, and open to anyone interested in MariaDB development. Details are at https://mariadb.org/2017-2-developers-unconference-and-related-events-shenz… and signup on the Meetup page: https://www.meetup.com/MariaDB-Developers/events/241873269/

1 0

[Maria-developers] GSoC NUMA Evaluation Student failed Mail 2
by Sumit Lakra 06 Sep '17

06 Sep '17

Hello, I am not aware of what information regarding this project has been exchanged between my mentor Daniel Black and others including serg, so I am sharing this here for everyone to see. My last mail contains the conversations I had with my mentors. Some quotes from the final evaluation - "Your questions in the last week have been exceptionally keen even though these seem to be the same questions from 1-2 months ago" Remind me when you (Daniel Black) answered them. "Sumit, overall we are happy with the quality of work. Unfortunately the quantity of work showed a lack of putting in hours and limited test cases fell short of the project plan and organisational expectations" What limited test cases ? I wrote all the test cases you (Daniel Black) and JAN Lindstrom asked me to write. Which test case did I miss ? I even ran a bench-marking test with all possible values and sent you the results. However, testing the actual performance difference is something that can only be done in a real NUMA Machine. Sorry, I don't have that. And have you ever even tested my code in a real NUMA Machine ? I too deserve to know if my work has made any real difference on the machine it is supposed to work on. And I have asked you to run it on a real machine more than once over the past months. You are happy with the quality of the work. Good. You are unhappy with the quantity. How do you expect me to increase the quantity when I already completed the tasks I promised in my GSoC proposal except for those that you removed from the list. What project plan are you referring to ? My GSoC proposal - https://docs.google.com/document/d/1UbOQysgOOzCM7z5FPC7gmuCpyxPmB7RGg7y9Ogq… Tasks spreadsheet - https://docs.google.com/spreadsheets/d/1nE-qFXhwwhF0hpOX3wUlczPjlPZTK_UzBu_… The tasks can be categorised as follows. [1] The tasks mentioned in my GSoC proposal which were not removed / changed later. - I completed all these. [2] The tasks mentioned in my GSoC proposal which were crossed out / (under consideration to be dropped / unnecessary) / or were implemented differently. - I either did not complete these for obvious reasons or implemented some of them differently. [3] The tasks you added (in the last month) to the spreadsheet originally under the title 'Out of Time' in the last month. - I completed some of these too. Then when these were done you moved a task (the queue per node task) from the 'Out of Time' to the third month group. We both agreed it was an important part for the completion of the project and I tried my best to implement it. I spent days trying to figure it out, asked you for help, didn't get any, tried again and yet failed because I am no expert of either InnoDB or SQl side of MariaDB. Which organisational expectations did you ask me to fulfil ? When ? I don't remember ever being told about any organisational expectations. Perhaps these were your personal expectations of having a complete NUMA Support in MariaDB by the end of GSoC. If so, then I am really sorry I wasn't able to fulfill those expectations because I got stuck at an important part of the project which also was a seemingly impossible task to do and *YOU FAILED TO HELP ME* with it even though I repeatedly asked (begged actually) you for help. You just kept reminding me that to do the task was my job. That's some real good mentoring right there. Another quote from the final evaluation "Passing to get your continued work isn't honoring the rules of the project or being fair to Google" Yeah, How about showing some good mentor ship by helping me to complete the project while we had the time to do it within Google's timelines in stead. Oh! wait a sec... you never went through the code.. until 3-4 days ago. I was determined to complete it if you had helped me. I have been mentioning it in all mails since the mail I wrote to you and Jan on : 22nd Aug - "Last but not least, if you think it can still be done and have a idea, I will be more than willing to attempt it. After all, GSoC has only got me started to contribute to open-source and this evaluation won't be the end of it." 25th Aug - "I am sorry I am unable to create an implementation. It's not like I don't want to. I just can't completely understand how it has been implemented currently. I have tried to follow the code before, and I did again the whole day today, but I can't come up with an idea at all." 25th Aug - "Like I said, I am not unwilling to complete this task. But I can't even think of where to start. Since, the two of you have greater experience with InnoDB, maybe you should give it a shot. You will definitely understand the present structure better than I do. And if you can come up with even a verbal solution that sounds like it could work, I will implement it asap." 25th Aug - "I spent all day trying to think of any way to implement a queue-per-node thing, and I confess I failed to come up with one. So, in stead of spending more time on that, I will work on migrating these commits to a new branch today. And I also urge you to give this task a try yourselves." 25th Aug - " I haven't been able to implement the queue per node structure for background threads, and I am afraid I won't be able to do it without your help. It is beyond me. I gave it a few attempts and I have failed. It's not the implementation part but the 'coming up with a way to implement it' part that I haven't been able to figure out yet. I need you to know that I am trying my best here." 25th Aug - "But anyway I kindly request you two to try it out yourselves now. You don't have to do the work. I just urge you to take some time out this weekend and go through the code. Let me know how you think it may be possible to implement it. I will do it. I have 4 days 2 hrs before the deadline to submit the evaluation, ends. If you can come up with an implementation plan within the next 2 or 2.5 days, I assure you I will code the implementation within a day of that and probably commit it before submitting the final evaluation as well. I am willing to take this risk." On Aug 28th you wrote a mail and asked me "Knowing what plans you had and how they failed would have been good to know." So, on the same day I wrote you a mail with a detailed explanation of the attempts I had made in trying to complete the task, asking you again to try it yourself and help me if you could come up with an idea. You never did. 31st Aug http://marialog.archivist.info/2017-08-31.txt Me - "did you have an attempt at the task ? come up with any ideas yet ?" dragonheart - been a bit busy with my own work so far. I know I'm going to have to look at a lot of stuff to finish the final GSoC evaluation 4th Sept http://marialog.archivist.info/2017-09-04.txt Me - "You must have seen my mails. Did you have an attempt at the queue per node thing ? Any ideas ?" dragonheart - well, implementing it was your task. but no, haven't found the hours to look though the code again. As per our conversation on IRC clearly Daniel Black hadn't even gone through the code by 4th Sept or attempted to solve the problem. How did he hope to complete the final evaluation. Even if he went through the code a day before submitting his evaluation, isn't this too late for a mentor to go through his/her student's code. Here is a list of some questions I would like MariaDB staff to ponder upon especially serg. [1] Was the task doable at all ? [A] Not unless you are an expert of both the SQL side and the InnoDB side, expert enough to re-write a major part of the interfaace between them. Jan himself acknowledged he wouldn't be able to help me with this, because he has no experience with the interface. I really appreciate that. [2] Even if the task was doable by a team of SQL and InnoDB experts, was it doable by an individual student working on MariaDB for the past four months ? [A] Nope. [3] Why didn't Daniel Black help me with the task even when I clearly mentioned I would need help with the task weeks ago ? [A] ??? [4] Did my mentor Daniel Black even try to help me with this particular task ? Ever ? [A] Nope. [5] Did Daniel Black evaluate me on the basis of the number of commits or the tasks accomplished by those commits ? [A] Based on the evaluation, he clearly has been unhappy with the decreased number of commits, not the content, as all the tasks mentioned in the original proposal have been achieved along with a few more. [6] Can Daniel Black himself complete this task, and thus the project ? [A] I bet not. No doubt the project is incomplete because it involves doing a task that is nearly impossible to do, at least for me, especially without any help. But does GSoC means that the student has to necessarily complete the entire project. I have gone through the code submitting guidelines plenty of times. It clearly states that we must state which tasks were done, and which tasks remain. Obviously not all projects are complete, some only complete the project partially, but mention everything and continue working on them. I did the same, and mentioned in my evaluation that almost all tasks were complete, in stead of all, and I have always been willing to complete this project but man, I can't do something impossible, especially when the people I am supposed to ask for help themselves fail to help their students. Thanks, Sumit

1 0

[Maria-developers] GSoC NUMA Evaluation Student failed Mail 1
by Sumit Lakra 06 Sep '17

06 Sep '17

Hello Serg, I am sorry to bug you with this mail but I am not sure what discussions you and my mentors have had before I was failed in GSoC. I completed all the tasks I had mentioned in my proposal but the project is incomplete. No doubts about that, but it is so because I got stuck with a major task (added in the last month) of the project for which I received no help from my mentors. These are the conversations I had with my mentors recently regarding the same task. Thanks, Sumit ---------- Forwarded message ---------- From: Sumit Lakra <sumitlakradev(a)gmail.com> Date: Tue, Aug 22, 2017 at 1:04 AM Subject: GSoC Final Evaluation To: Daniel Black <daniel.black(a)au1.ibm.com>, Jan Lindström < jan.lindstrom(a)mariadb.com> Hello, I went through the code that the InnoDB background threads use to pick tasks and execute them, in detail. This is how they seem to work. The reader/writer threads are passed an integer which acts as a segment. They then call fil_aio_wait(segment) which calls os_aio_handler(segment, &node, &message, &type). The control then goes to the os_aio_simulated_handler(segment, m1, m2, request) where the code gets more complicated with AIO arrays and slots. It gets harder to understand how they choose their tasks. It is definitely not a simple queue structure from which they pick their tasks. Also, which buffer pool the task is related to can only be figured out quite later.. based on the value of m2, which stores the address of a bpage. A simple queue could have been easily replaced with multiple queues, i.e. a queue per numa-node like we had once discussed on IRC. Lastly, all these procedures are common for log threads as well. Another thing, you mentioned more than once you wanted the reader threads to look for a bpage in their local nodes before looking them up in other nodes, but they use the bpage structure itself like I mentioned. Obviously neither of us had a proper understanding of how InnoDB worked in these aspects when we started the project. These threads seem to operate on bpages mostly rather than buf_pools, which makes numa mapping even harder (buf_pools to numa nodes would comparatively be easier), but is definitely more efficient than buf_pools and hence shouldn't be changed. Then again there were cases when the tasks assigned to the background threads were done by other server threads as well, especially in case of flushing, but I successfully restricted them to their nodes. If we were to create queues per node, which queues would these other threads work on. In other words the way in which InnoDB was initially written and expanded has made it significantly difficult to be adapted for explicit NUMA support. It wouldn't just require restructuring and changing the way these threads use parameters, pick tasks etc, but may also require re-ordering the order in which they are executed. For example, the background threads are created before the user threads, and trying to use the NUMA node from user THD later on would mean more system calls and probably thread migrations. When you added the task in the spreadsheet, you were right to anticipate that it could require large cleanup of the InnoDB structure, but I am beginning to think it will be way more complicated. Also, as you once mentioned that most architectures have 2 to 4 NUMA nodes. No doubt, if we could implement a support for NUMA architectuure such that InnoDB would make the best use out of it, there would be a performance difference but it would still be quite negligible in a very fast computer, and I really don't think making such big changes to InnoDB would be worth the effort and risk. Trying to bring a major change to a working and verified code (written by someone else) is more error-prone than adding some modular code and functions to support a new feature. I hope you agree. Last but not least, if you think it can still be done and have a idea, I will be more than willing to attempt it. After all, GSoC has only got me started to contribute to open-source and this evaluation won't be the end of it. And since there's no other database software out there which supports NUMA (let's ignore numa-interleave) and since I was the first to start working on this, I will be really proud to see it through to the end. Some Evaluation stuff : I see only one text field in the form for a link to the work done, and it takes a single url. Reserving this for the github link, where do I add a link to the spreadsheet or documentation ? Assuming you have already verified the work done so far, I will be creating a pull request shortly, so I can add whether it was merged or not, in the evaluation. Thanking You, Sumit ---------- Forwarded message ---------- From: Daniel Black <daniel.black(a)au1.ibm.com> Date: Thu, Aug 24, 2017 at 12:11 PM Subject: Re: GSoC Final Evaluation To: Sumit Lakra <sumitlakradev(a)gmail.com>, Jan Lindström < jan.lindstrom(a)mariadb.com> On 22/08/17 05:34, Sumit Lakra wrote: > Hello, > > I went through the code that the InnoDB background threads use to pick > tasks and execute them, in detail. This is how they seem to work. > > The reader/writer threads are passed an integer which acts as a segment. > They then call fil_aio_wait(segment) which calls os_aio_handler(segment, > &node, &message, &type). The control then goes to the > os_aio_simulated_handler(segment, m1, m2, request) where the code gets > more complicated with AIO arrays and slots. It gets harder to understand > how they choose their tasks. It is definitely not a simple queue > structure from which they pick their tasks. Also, which buffer pool the > task is related to can only be figured out quite later.. based on the > value of m2, which stores the address of a bpage. > A simple queue could > have been easily replaced with multiple queues, i.e. a queue per > numa-node like we had once discussed on IRC. Yes, you know where requests are created and where the job message ends up so you should be able to create an implementation. Even if it doesn't preserver the existing behaviour it should be enough to test it. > Lastly, all these > procedures are common for log threads as well. relevance? > Another thing, you mentioned more than once you wanted the reader SQL threads? Or is this the same thing? > threads to look for a bpage in their local nodes before looking them up > in other nodes, but they use the bpage structure itself like I > mentioned > Obviously neither of us had a proper understanding of how > InnoDB worked in these aspects when we started the project. No project knows all the details. > These > threads seem to operate on bpages mostly rather than buf_pools, which > makes numa mapping even harder You could add a node_id to the class buf_page_t? > (buf_pools to numa nodes would > comparatively be easier), but is definitely more efficient than > buf_pools and hence shouldn't be changed. > Then again there were cases when the tasks assigned to the background > threads were done by other server threads as well, especially in case of > flushing, but I successfully restricted them to their nodes. If we were > to create queues per node, which queues would these other threads work on. Yes, which is why this issue was raised with you months ago. > In other words the way in which InnoDB was initially written and > expanded has made it significantly difficult to be adapted for explicit > NUMA support. The split of buffer pool instances was in appreciation that there there are hot locks causes by a large number of threads. It just needed more thought than implementing a few thread bindings. I'm sorry you though it was that easy. It wouldn't just require restructuring and changing the > way these threads use parameters, pick tasks etc, You've changed very few parameters so far. https://github.com/MariaDB/server/compare/10.2...theGodlessLakra:numa_one but may also require > re-ordering the order in which they are executed. I can't think of one example of this. > For example, the > background threads are created before the user threads, and trying to > use the NUMA node from user THD later on would mean more system calls > and probably thread migrations. Huh, what? "may..require..(bad design)". Which is why a mapping of THD to known background threads per node was desired, to avoid syscall thread migrations. > When you added the task in the spreadsheet, you were right to anticipate > that it could require large cleanup of the InnoDB structure, but I am > beginning to think it will be way more complicated. I mentioned in on 2017-07-10 too (http://marialog.archivist.info/2017-07-10.txt) too and you said you'd give it a try. > Also, as you once > mentioned that most architectures have 2 to 4 NUMA nodes. No doubt, if > we could implement a support for NUMA architectuure such that InnoDB > would make the best use out of it, there would be a performance > difference but it would still be quite negligible in a very fast > computer, Cross node cache-coherency takes significant clock cycles compared to local access. What you'd end up with is a system that has idle time that can't be used. By finishing this task this could be measured. local/remote perf access can be measure (perhaps even with fake_numa). https://joemario.github.io/blog/2016/09/01/c2c-blog/ > and I really don't think making such big changes to InnoDB > would be worth the effort and risk. There is no gain without risk. Small changes with careful review, running full tests between them is they way large changes are made low risk. > Trying to bring a major change to a > working and verified code (written by someone else) is more error-prone > than adding some modular code and functions to support a new feature. I > hope you agree. When developing you are always developing for current and future use. As such architectural changes need to be commiserate to the functional changes. Jan and I have placed emphasis on writing test cases. These ensure that changes. > Last but not least, if you think it can still be done and have a idea, I > will be more than willing to attempt it. After all, GSoC has only got me > started to contribute to open-source and this evaluation won't be the > end of it. > And since there's no other database software out there which > supports NUMA (let's ignore numa-interleave) and since I was the first > to start working on this There is, found https://github.com/aerospike/aerospike-server/blob/master/cf/src/hardware.c the other day. Wouldn't have helped much as it has a different architecture and getting on top of one was hard enough. >, I will be really proud to see it through to > the end. This week looks like you've done one commit that is largely a revert that Jan asked for. You have slowed down considerably. > Some Evaluation stuff : > I see only one text field in the form for a link to the work done, and > it takes a single url. Reserving this for the github link, where do I > add a link to the spreadsheet or documentation ? > > Assuming you have already verified the work done so far, I will be > creating a pull request shortly, so I can add whether it was merged or > not, in the evaluation. Looks like the fake_numa stuff is incomplete. Pinning of background threads looks odd as they aren't big allocators of memory and aren't associated with SQL threads/buffer pool instance nodes. I have looked though it. As a pull request 60 commits with 26(?) of them fixing prior commits will look messy for anyone trying to look at the changes. If you are up for trying to clean it up please give it a go. Doing it in a copied repository is a good idea as you've seen before it can get a bit hard to keep track of. > Thanking You, > Sumit > ---------- Forwarded message ---------- From: Sumit Lakra <sumitlakradev(a)gmail.com> Date: Fri, Aug 25, 2017 at 3:03 AM Subject: Re: GSoC Final Evaluation To: Daniel Black <daniel.black(a)au1.ibm.com> Cc: Jan Lindström <jan.lindstrom(a)mariadb.com> On Thu, Aug 24, 2017 at 12:11 PM, Daniel Black <daniel.black(a)au1.ibm.com> wrote: > > > On 22/08/17 05:34, Sumit Lakra wrote: > > Hello, > > > > I went through the code that the InnoDB background threads use to pick > > tasks and execute them, in detail. This is how they seem to work. > > > > The reader/writer threads are passed an integer which acts as a segment. > > They then call fil_aio_wait(segment) which calls os_aio_handler(segment, > > &node, &message, &type). The control then goes to the > > os_aio_simulated_handler(segment, m1, m2, request) where the code gets > > more complicated with AIO arrays and slots. It gets harder to understand > > how they choose their tasks. It is definitely not a simple queue > > structure from which they pick their tasks. Also, which buffer pool the > > task is related to can only be figured out quite later.. based on the > > value of m2, which stores the address of a bpage. > > > A simple queue could > > have been easily replaced with multiple queues, i.e. a queue per > > numa-node like we had once discussed on IRC. > > Yes, you know where requests are created and where the job message ends > up so you should be able to create an implementation. Even if it doesn't > preserver the existing behaviour it should be enough to test it. > I am sorry I am unable to create an implementation. It's not like I don't want to. I just can't completely understand how it has been implemented currently. I have tried to follow the code before, and I did again the whole day today, but I can't come up with an idea at all. > > > Lastly, all these > > procedures are common for log threads as well. > > relevance? The reader/writer/log threads all start by executing fil_aio_wait(segment), in srv0start.cc > > > Another thing, you mentioned more than once you wanted the reader > > SQL threads? Or is this the same thing? > > > threads to look for a bpage in their local nodes before looking them up > > in other nodes, but they use the bpage structure itself like I > > mentioned > > Obviously neither of us had a proper understanding of how > > InnoDB worked in these aspects when we started the project. > > No project knows all the details. > > > These > > threads seem to operate on bpages mostly rather than buf_pools, which > > makes numa mapping even harder > > You could add a node_id to the class buf_page_t? > Okay, but how to use it ? where ? In which functions ? > > > (buf_pools to numa nodes would > > comparatively be easier), but is definitely more efficient than > > buf_pools and hence shouldn't be changed. > > > > Then again there were cases when the tasks assigned to the background > > threads were done by other server threads as well, especially in case of > > flushing, but I successfully restricted them to their nodes. If we were > > to create queues per node, which queues would these other threads work > on. > > Yes, which is why this issue was raised with you months ago. > > > In other words the way in which InnoDB was initially written and > > expanded has made it significantly difficult to be adapted for explicit > > NUMA support. > > The split of buffer pool instances was in appreciation that there there > are hot locks causes by a large number of threads. > > It just needed more thought than implementing a few thread bindings. I'm > sorry you though it was that easy. > The split of buffer pool is not problematic to NUMA implementation. Its the task queues for the background threads that's wrecking my brain. > > It wouldn't just require restructuring and changing the > > way these threads use parameters, pick tasks etc, > > > You've changed very few parameters so far. > > https://github.com/MariaDB/server/compare/10.2...theGodlessLakra:numa_one > > but may also require > > re-ordering the order in which they are executed. > > I can't think of one example of this. > > For example, the > > background threads are created before the user threads, and trying to > > use the NUMA node from user THD later on would mean more system calls > > and probably thread migrations. > > Huh, what? "may..require..(bad design)". > One way (a really bad design), would be to create background threads when a request is made by a SQL thread, and then bind it to the node depending on the request i.e. the buffer pool instance in which the associated bpage belongs, which would mean more system calls and migrations. Obviously a very bad design. > > Which is why a mapping of THD to known background threads per node was > desired, to avoid syscall thread migrations. > The SQL threads were expected to be bound to numa nodes, which I did, but I can't think of how to map them to background threads. Can you think of a way to do this ? Let me know how you think this can be done. I will implement it asap. > When you added the task in the spreadsheet, you were right to anticipate > > that it could require large cleanup of the InnoDB structure, but I am > > beginning to think it will be way more complicated. > > I mentioned in on 2017-07-10 too > (http://marialog.archivist.info/2017-07-10.txt) too and you said you'd > give it a try. > > > Also, as you once > > mentioned that most architectures have 2 to 4 NUMA nodes. No doubt, if > > we could implement a support for NUMA architectuure such that InnoDB > > would make the best use out of it, there would be a performance > > difference but it would still be quite negligible in a very fast > > computer, > > Cross node cache-coherency takes significant clock cycles compared to > local access. What you'd end up with is a system that has idle time that > can't be used. > > By finishing this task this could be measured. local/remote perf access > can be measure (perhaps even with fake_numa). > https://joemario.github.io/blog/2016/09/01/c2c-blog/ > > > and I really don't think making such big changes to InnoDB > > would be worth the effort and risk. > > There is no gain without risk. Small changes with careful review, > running full tests between them is they way large changes are made low > risk. > Agreed, but I honestly can't make out the heads and tails of the changes that will be required to replace the present task queue with a queue-per-node structure :( > > > Trying to bring a major change to a > > working and verified code (written by someone else) is more error-prone > > than adding some modular code and functions to support a new feature. I > > hope you agree. > > When developing you are always developing for current and future use. As > such architectural changes need to be commiserate to the functional > changes. Jan and I have placed emphasis on writing test cases. These > ensure that changes. > Like I said, I am not unwilling to complete this task. But I can't even think of where to start. Since, the two of you have greater experience with InnoDB, maybe you should give it a shot. You will definitely understand the present structure better than I do. And if you can come up with even a verbal solution that sounds like it could work, I will implement it asap. > > > Last but not least, if you think it can still be done and have a idea, I > > will be more than willing to attempt it. After all, GSoC has only got me > > started to contribute to open-source and this evaluation won't be the > > end of it. > > > > And since there's no other database software out there which > > supports NUMA (let's ignore numa-interleave) and since I was the first > > to start working on this > > There is, found > https://github.com/aerospike/aerospike-server/blob/master/cf > /src/hardware.c > the other day. Wouldn't have helped much as it has a different > architecture and getting on top of one was hard enough. > > >, I will be really proud to see it through to > > the end. > > This week looks like you've done one commit that is largely a revert > that Jan asked for. You have slowed down considerably. > You have a good reason to be upset at me and I am not complaining. But although it doesn't look from Github like much work due to no changes, I spent a good deal of time brainstorming on the implementation of the first task from the spreadsheet under the "should be done" title, unfortunately to no success. > > > Some Evaluation stuff : > > I see only one text field in the form for a link to the work done, and > > it takes a single url. Reserving this for the github link, where do I > > add a link to the spreadsheet or documentation ? > > > > Assuming you have already verified the work done so far, I will be > > creating a pull request shortly, so I can add whether it was merged or > > not, in the evaluation. > > Looks like the fake_numa stuff is incomplete. Pinning of background > threads looks odd as they aren't big allocators of memory and aren't > associated with SQL threads/buffer pool instance nodes. > Fixed the fake numa test which was failing in non-debug builds. Most of the memory allocation is done during the buffer pool creation, and these have been managed properly while creating buffer pools per numa node. What other allocators of memory do you wish to be managed ? and how ? (Non-pinned threads should probably be allowed to allocate memory from any node) > > I have looked though it. As a pull request 60 commits with 26(?) of them > fixing prior commits will look messy for anyone trying to look at the > changes. If you are up for trying to clean it up please give it a go. > Doing it in a copied repository is a good idea as you've seen before it > can get a bit hard to keep track of. > I spent all day trying to think of any way to implement a queue-per-node thing, and I confess I failed to come up with one. So, in stead of spending more time on that, I will work on migrating these commits to a new branch today. And I also urge you to give this task a try yourselves. Thanks, Sumit ---------- Forwarded message ---------- From: Sumit Lakra <sumitlakradev(a)gmail.com> Date: Fri, Aug 25, 2017 at 6:52 PM Subject: Re: GSoC Final Evaluation To: Daniel Black <daniel.black(a)au1.ibm.com> Cc: Jan Lindström <jan.lindstrom(a)mariadb.com> Hello, I am currently working on copying the changes from numa_one branch to a different branch in a more orderly fashion. I haven't been able to implement the queue per node structure for background threads, and I am afraid I won't be able to do it without your help. It is beyond me. I gave it a few attempts and I have failed. It's not the implementation part but the 'coming up with a way to implement it' part that I haven't been able to figure out yet. I need you to know that I am trying my best here. Back in http://marialog.archivist.info/2017-07-10.txt, you also said "warning the whole io read/write thread segements and scheduling is really messy and could do with a significant rework", and you also mentioned that you and Jan would have an attempt at it yourselves, if I am unable to do it and I move on. Well, I regret moving on to SQL threads back then without informing you that I wasn't successfull. A 'queue-per-numa-node' is easier said than done. But anyway I kindly request you two to try it out yourselves now. You don't have to do the work. I just urge you to take some time out this weekend and go through the code. Let me know how you think it may be possible to implement it. I will do it. I have 4 days 2 hrs before the deadline to submit the evaluation, ends. If you can come up with an implementation plan within the next 2 or 2.5 days, I assure you I will code the implementation within a day of that and probably commit it before submitting the final evaluation as well. I am willing to take this risk. However, if you are unable to come up with an implementation plan, we don't really have to hurry and stress ourselves. We can work together (I can't do this on my own) on this particular task later on and come up with something, assuming I am not failed in the final evaluation. Happy Weekend, Sumit ---------- Forwarded message ---------- From: Daniel Black <daniel.black(a)au1.ibm.com> Date: Mon, Aug 28, 2017 at 7:56 AM Subject: Re: GSoC Final Evaluation To: Sumit Lakra <sumitlakradev(a)gmail.com> Cc: Jan Lindström <jan.lindstrom(a)mariadb.com> On 25/08/17 23:22, Sumit Lakra wrote: > Hello, > > I am currently working on copying the changes from numa_one branch to a > different branch in a more orderly fashion. This is looking good: https://github.com/theGodlessLakra/server/tree/NUMA_Support Don't be afraid to be very descriptive in the commit message especially about the problem being solved and what functionality it implements/changes. Don't re-say what the code is doing, just cover its meaning at a high level. > I haven't been able to > implement the queue per node structure for background threads, and I am > afraid I won't be able to do it without your help. It is beyond me. > I > gave it a few attempts and I have failed. It's not the implementation > part but the 'coming up with a way to implement it' part that I haven't > been able to figure out yet. I need you to know that I am trying my best > here. Knowing what plans you had and how they failed would have been good to know. > Back in http://marialog.archivist.info/2017-07-10.txt, > you also said "warning the whole io read/write thread segements and > scheduling is really messy and could do with a significant rework", To do it properly yes. > and > you also mentioned that you and Jan would have an attempt at it > yourselves, if I am unable to do it and I move on. Well, I regret moving > on to SQL threads back then without informing you that I wasn't > successfull. I probably should of harassed you a bit too. Acknowledging failures is an important part to learning, even if you just acknowledge to yourself. How did SQL threads end up? > A 'queue-per-numa-node' is easier said than done. But > anyway I kindly request you two to try it out yourselves now. You don't > have to do the work. I just urge you to take some time out this weekend > and go through the code. Let me know how you think it may be possible to > implement it. I will do it. Having only just read this, this is a bit late notice. The aspect of understanding the interoperation between parts is also a fundamental aspect of getting a plan to implement it. > I have 4 days 2 hrs before the deadline to submit the evaluation, ends. > If you can come up with an implementation plan within the next 2 or 2.5 > days, I assure you I will code the implementation within a day of that > and probably commit it before submitting the final evaluation as well. I > am willing to take this risk. I thought your submission of a URL left it open to still committing on it. Please don't risk missing it. Making sure the existing code/comments/documentation is as clean as possible is better than tackling new work now. > However, if you are unable to come up with an implementation plan, we > don't really have to hurry and stress ourselves. We can work together (I > can't do this on my own) on this particular task later on and come up > with something, assuming I am not failed in the final evaluation. > > Happy Weekend, > Sumit ---------- Forwarded message ---------- From: Sumit Lakra <sumitlakradev(a)gmail.com> Date: Mon, Aug 28, 2017 at 9:50 PM Subject: Re: GSoC Final Evaluation To: Daniel Black <daniel.black(a)au1.ibm.com> Cc: Jan Lindström <jan.lindstrom(a)mariadb.com> Hello, The following is a detailed explanation of what I did, how and why, while trying to come up with a solution to the task queue problem. However, before you go through this I would still request you to have an attempt at it yourselves, so your perspectives won't be affected by my ideas and you may come up with something different. I had once tried this before in https://github.com/MariaDB/ server/commit/4b8436d3f7c573668a0d37b54dab2d05e8e102be. I even pointed this commit out to once on IRC, (can't access logs of this time). Anyway I didn't put much focus on this as I was onto some other task. Before trying to replace the present queue structure with a queue-per-node, I first tried to let all the threads access this queue as before but only pick tasks dealing with their local nodes. Neither this queue nor these tasks seem to be easily distinguishable in code. The threads start by running fil_aio_wait(segment). This function calls os_aio_handler(segment, &node, &message, &type). This calls os_aio_simulated_handler(segment, m1, m2, request). This calls AIO::get_array_and_local_segment(&array, global_segment), and also creates SimulatedAIOHandler handler(array, segment), followed by calling handler.check_pending(global_segment, event), handler.init(n_slots), and slot = handler.check_completed(&n_reserved). Now, in order to skip a task to be done by other thread, what a thread must do is pick the task, check whether the task is related to the same node or not. If the task is concerned with the same node execute it or else leave it in the queue. This is where the problem occurs. In order to figure out whether a task is concerned with the same node or not, it must know which buffer pool instance the task is associated to. This can only be figured out after the task picks up the bpage it has to deal with. The bpage already has this information by now. Anyway, the bpage a thread has to deal with can only be found out by buf_page_t* bpage = static_cast<buf_page_t*>(*m2). Also, the task is not picked up at any particular place, but is picked up in parts in different functions. All these function calls seem to make some or other kind of changes to the segment/slot/array they are picking up the task from. So, when a thread finally gets a task and finds it concerned with a different node, then in order to safely return the task back to the queue, it must also undo the changes made by all these procedures so far. This is the difficult part. Compare this with pc_flush_slot() where I successfully implemented this method of skipping tasks if not related to local pool. In the above commit you will find I have a few lines like 'ib::info() << thread_name << " accessing buf_page on node : " << node_of_pool << " in fil_aio_wait()";' If you compile and run this code you can see in real time which threads are accessing which pools. I have given proper names to them. Also, when I tried to skip tasks which were not of the same queue by returning from the function os_aio_simulated_handler(), which partly worked but on trying to return from the funtion fil_aio_wait(), there seems to be either an infinite loop or a deadlock somewhere, as the server stops midway (doesn't abort) while starting up. Hence I have commented out those lines on lines 5463 and 5464 in fil0fil.cc in the above commit. Another thing is the role of segments here. Are these segments related to buffer pool instances somehow ? Because all these threads including the log threads start there work by executing fil_aio_wait(segment), but segment here seems to be just an incrementing integer. At one point assuming these segments could contain tasks related with different pools/nodes, I added a loop such that both the read and write threads loop around fil_aio_wait(segment) calling it with all valid segments. Didn't work out. These were attempted by me when I pushed this commit weeks ago in the temp branch. And even though I pointed it out to you, I think I should have given much more importance to this task. Over the past few days, when I tried to give this task another try, I couldn't come up with any idea either to replace this queue with a queue-per-node thing or even to complete this skip-if-diff-node thing.

1 0

Re: [Maria-developers] [Commits] dccebdf862d: Rdb_io_watchdog doesn't support osx so disabling it
by Vicențiu Ciorbaru 30 Aug '17

30 Aug '17

Hi Varun! Please change the CMakeLists.txt to be like so: diff --git a/storage/rocksdb/CMakeLists.txt b/storage/rocksdb/CMakeLists.txt index 5db6d88..6db8211 100644 --- a/storage/rocksdb/CMakeLists.txt +++ b/storage/rocksdb/CMakeLists.txt @@ -146,7 +146,10 @@ ADD_CONVENIENCE_LIBRARY(rocksdb_aux_lib ADD_DEPENDENCIES(rocksdb_aux_lib GenError) # MARIAROCKS-TODO: how to properly depend on -lrt ? -TARGET_LINK_LIBRARIES(rocksdb_aux_lib rocksdblib ${ZLIB_LIBRARY} -lrt) +TARGET_LINK_LIBRARIES(rocksdb_aux_lib rocksdblib ${ZLIB_LIBRARY}) +if (UNIX AND NOT APPLE) + TARGET_LINK_LIBRARIES(rocksdb_aux_lib -lrt) +endif() TARGET_LINK_LIBRARIES(rocksdb rocksdb_aux_lib) @@ -182,7 +185,9 @@ ENDIF() # ADD_SUBDIRECTORY(unittest) #ENDIF() -SET(rocksdb_static_libs ${rocksdb_static_libs} "-lrt") +if (UNIX AND NOT APPLE) + SET(rocksdb_static_libs ${rocksdb_static_libs} "-lrt") +endif() ADD_LIBRARY(rocksdb_tools STATIC rocksdb/tools/ldb_tool.cc Also, change how you define the __APPLE__ #ifdef like so: - #ifndef _WIN32 + #if not defined(_WIN32) && not defined(__APPLE__) This way, you don't have to add an extra #endif. The point of #ifndef is to avoid typing, but it doesn't work well with multiple conditions. We can make use #if then. Vicențiu On Wed, 30 Aug 2017 at 15:37 Varun <varunraiko1803(a)gmail.com> wrote: > revision-id: dccebdf862d66e65528e18562c4503f40b695eca > (mariadb-10.3.0-107-gdccebdf862d) > parent(s): e1051590b61b45532284071ac78c50e7a2f24c63 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-08-30 18:05:52 +0530 > message: > > Rdb_io_watchdog doesn't support osx so disabling it > > --- > storage/rocksdb/CMakeLists.txt | 4 ++++ > storage/rocksdb/ha_rocksdb.cc | 14 +++++++++++++- > storage/rocksdb/rdb_io_watchdog.cc | 2 ++ > storage/rocksdb/rdb_io_watchdog.h | 6 ++++-- > 4 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/storage/rocksdb/CMakeLists.txt > b/storage/rocksdb/CMakeLists.txt > index 5db6d888bb6..435fe784d6c 100644 > --- a/storage/rocksdb/CMakeLists.txt > +++ b/storage/rocksdb/CMakeLists.txt > @@ -146,7 +146,11 @@ ADD_CONVENIENCE_LIBRARY(rocksdb_aux_lib > ADD_DEPENDENCIES(rocksdb_aux_lib GenError) > > # MARIAROCKS-TODO: how to properly depend on -lrt ? > +if (APPLE) > +TARGET_LINK_LIBRARIES(rocksdb_aux_lib rocksdblib ${ZLIB_LIBRARY}) > +else() > TARGET_LINK_LIBRARIES(rocksdb_aux_lib rocksdblib ${ZLIB_LIBRARY} -lrt) > +endif() > > TARGET_LINK_LIBRARIES(rocksdb rocksdb_aux_lib) > > diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc > index e4d9ce02f10..4ffe6caf70b 100644 > --- a/storage/rocksdb/ha_rocksdb.cc > +++ b/storage/rocksdb/ha_rocksdb.cc > @@ -173,9 +173,11 @@ Rdb_ddl_manager ddl_manager; > const char *m_mysql_gtid; > Rdb_binlog_manager binlog_manager; > > -#ifndef _WIN32 > +#ifndef _WIN32 > +#ifndef __APPLE__ > Rdb_io_watchdog *io_watchdog = nullptr; > #endif > +#endif > /** > MyRocks background thread control > N.B. This is besides RocksDB's own background threads > @@ -555,7 +557,9 @@ static void rocksdb_set_io_write_timeout( > DBUG_ASSERT(save != nullptr); > DBUG_ASSERT(rdb != nullptr); > #ifndef _WIN32 > +#ifndef __APPLE__ > DBUG_ASSERT(io_watchdog != nullptr); > +#endif > #endif > > RDB_MUTEX_LOCK_CHECK(rdb_sysvars_mutex); > @@ -564,7 +568,9 @@ static void rocksdb_set_io_write_timeout( > > rocksdb_io_write_timeout_secs = new_val; > #ifndef _WIN32 > +#ifndef __APPLE__ > io_watchdog->reset_timeout(rocksdb_io_write_timeout_secs); > +#endif > #endif > RDB_MUTEX_UNLOCK_CHECK(rdb_sysvars_mutex); > } > @@ -3746,7 +3752,9 @@ static int rocksdb_init_func(void *const p) { > */ > if (status.IsIOError() > #ifndef _WIN32 > +#ifndef __APPLE__ > && errno == ENOENT > +#endif > #endif > ) { > sql_print_information("RocksDB: Got ENOENT when listing column > families"); > @@ -3985,8 +3993,10 @@ static int rocksdb_init_func(void *const p) { > } > > #ifndef _WIN32 > +#ifndef __APPLE__ > io_watchdog = new Rdb_io_watchdog(directories); > io_watchdog->reset_timeout(rocksdb_io_write_timeout_secs); > +#endif > #endif > > // NO_LINT_DEBUG > @@ -4077,9 +4087,11 @@ static int rocksdb_done_func(void *const p) { > commit_latency_stats = nullptr; > > #ifndef _WIN32 > +#ifndef __APPLE__ > delete io_watchdog; > io_watchdog = nullptr; > #endif > +#endif > > // Disown the cache data since we're shutting down. > // This results in memory leaks but it improved the shutdown time. > diff --git a/storage/rocksdb/rdb_io_watchdog.cc > b/storage/rocksdb/rdb_io_watchdog.cc > index a599ba58aec..a9e79283382 100644 > --- a/storage/rocksdb/rdb_io_watchdog.cc > +++ b/storage/rocksdb/rdb_io_watchdog.cc > @@ -23,6 +23,7 @@ > > /* Rdb_io_watchdog doesn't work on Windows [yet] */ > #ifndef _WIN32 > +#ifndef __APPLE__ > > namespace myrocks { > > @@ -236,4 +237,5 @@ int Rdb_io_watchdog::reset_timeout(const uint32_t > &write_timeout) { > } // namespace myrocks > > #endif > +#endif > > diff --git a/storage/rocksdb/rdb_io_watchdog.h > b/storage/rocksdb/rdb_io_watchdog.h > index de8c1b9500e..12aec516f26 100644 > --- a/storage/rocksdb/rdb_io_watchdog.h > +++ b/storage/rocksdb/rdb_io_watchdog.h > @@ -35,7 +35,8 @@ > namespace myrocks { > > // Rdb_io_watchdog does not support Windows ATM. > -#ifndef _WIN32 > +#ifndef _WIN32 > +#ifndef __APPLE__ > > class Rdb_io_watchdog { > const int RDB_IO_WRITE_BUFFER_SIZE = 4096; > @@ -113,5 +114,6 @@ class Rdb_io_watchdog { > Rdb_io_watchdog &operator=(const Rdb_io_watchdog &) = delete; > }; > > -#endif > +#endif > +#endif > } // namespace myrocks > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] Review: MDEV-12985: Support percentile and media window functions
by Vicențiu Ciorbaru 30 Aug '17

30 Aug '17

Hi Varun! Here is the in-detail review for the percentile functions. Please add a grammar definition for median too and test it. All in all, not many changes, but please address my comments. If you disagree with some of them, let's discuss them. > diff --git a/mysql-test/r/win_percentile_cont.result b/mysql-test/r/win_percentile_cont.result > new file mode 100644 > index 00000000000..61f70892887 > --- /dev/null > +++ b/mysql-test/r/win_percentile_cont.result > diff --git a/mysql-test/t/win_percentile_cont.test b/mysql-test/t/win_percentile_cont.test > new file mode 100644 > index 00000000000..75fde963b2a > --- /dev/null > +++ b/mysql-test/t/win_percentile_cont.test > @@ -0,0 +1,55 @@ Please put more tests here, as we discussed. Also, to make test cases easier to follow, please add cume_dist as a function in the select to all these use cases. > +CREATE TABLE student (name CHAR(10), test double, score DECIMAL(19,4)); > +INSERT INTO student VALUES > +('Chun', 0, 3), ('Chun', 0, 7), > +('Kaolin', 0.5, 3), ('Kaolin', 0.6, 7), > +('Kaolin', 0.5, 4), > +('Tatiana', 0.8, 4), ('Tata', 0.8, 4); > + Make this --echo # No partition clause > +#no partition clause > +select name, percentile_disc(0.5) within group(order by score) over () from student; > +select name, percentile_cont(0.5) within group(order by score) over () from student; > + Same as before, use --echo # so we can see it in the result set too. > +# argument set to null > +select name, percentile_cont(null) within group(order by score) over (partition by name) from student; > +select name, percentile_disc(null) within group(order by score) over (partition by name) from student; > + Use --echo # > +# complete query with partition column > +select name, percentile_cont(0.5) within group(order by score) over (partition by name) as c from student; > +select name, percentile_disc(0.5) within group(order by score) over (partition by name) as c from student; > + Use --echo # > +#subqueries having percentile functions > + > +select * from ( select name , percentile_cont(0.5) within group ( order by score) over (partition by name ) from student ) as t; > +select * from ( select name , percentile_disc(0.5) within group ( order by score) over (partition by name ) from student ) as t; > +select name from student a where (select percentile_disc(0.5) within group (order by score) over (partition by name) from student b limit 1) >= 0.5; > + USe --echo > +# WITH STORED PROCEDURES > + > + USe --echo > +#DISALLOWED FIELDS IN ORDER BY CLAUSE > +--error ER_WRONG_TYPE_FOR_PERCENTILE_CONT > +select score, percentile_cont(0.5) within group(order by name) over (partition by score) from student; > +select score, percentile_disc(0.5) within group(order by name) over (partition by score) from student; > USe --echo > + > +# error with 2 order by elements > + > +--error ER_NOT_SINGLE_ELEMENT_ORDER_LIST > +select percentile_disc(0.5) within group(order by score,test) over (partition by name) from student; > +--error ER_NOT_SINGLE_ELEMENT_ORDER_LIST > +select percentile_cont(0.5) within group(order by score,test) over (partition by name) from student; > + Use --echo and fix that comment. We thought of a way for this right? > +#parameter value should be in the range of 0 to 1( NEED TO THINK A WAY FOR THIS) > +--error ER_ARGUMENT_OUT_OF_RANGE > +select percentile_disc(1.5) within group(order by score) over (partition by name) from student; > +--error ER_ARGUMENT_OUT_OF_RANGE > +select percentile_cont(1.5) within group(order by score) over (partition by name) from student; > + > +--error ER_ARGUMENT_NOT_CONSTANT > +select name,percentile_cont(test) within group(order by score) over (partition by name) from student; > +--error ER_ARGUMENT_NOT_CONSTANT > +select name, percentile_disc(test) within group(order by score) over (partition by name) from student; > + This is commented out. Not good, fix it please. And use --echo # > +#CHECK TYPE OF THE ARGUMENT, SHOULD BE ONLY NUMERICAL > +#select name, percentile_cont(name) within group(order by score) over (partition by name) from student; > + > > +drop table student; > diff --git a/sql/item.h b/sql/item.h > index 76ce4aa935f..108b0838dfc 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5245,6 +5246,7 @@ class Cached_item_int :public Cached_item_item > Cached_item_int(Item *item_par) :Cached_item_item(item_par),value(0) {} > bool cmp(void); > int cmp_read_only(); Delete this, it's not used. > + longlong get_value(){ return value;} > }; > > > @@ -5255,6 +5257,7 @@ class Cached_item_decimal :public Cached_item_item > Cached_item_decimal(Item *item_par); > bool cmp(void); > int cmp_read_only(); Delete this, it's not used. > + my_decimal *get_value(){ return &value;}; > }; > > class Cached_item_field :public Cached_item > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc > index 6ab903a81bb..916f4113682 100644 > --- a/sql/item_windowfunc.cc > +++ b/sql/item_windowfunc.cc > diff --git a/sql/item_windowfunc.h b/sql/item_windowfunc.h > index 77cbd556e60..ca2ed7136ec 100644 > --- a/sql/item_windowfunc.h > +++ b/sql/item_windowfunc.h > @@ -678,6 +687,275 @@ class Item_sum_ntile : public Item_sum_window_with_row_count > ulong current_row_count_; > }; > > +class Item_sum_percentile_disc : public Item_sum_cume_dist, > + public Type_handler_hybrid_field_type > +{ > +public: > + Item_sum_percentile_disc(THD *thd, Item* arg) : Item_sum_cume_dist(thd, arg), > + Type_handler_hybrid_field_type(&type_handler_longlong), > + value(NULL), val_calculated(FALSE), first_call(TRUE), > + prev_value(0), order_item(NULL){} > + > + double val_real() > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; > + return value->val_real(); > + } > + > + longlong val_int() > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; > + return value->val_int(); > + } > + > + my_decimal* val_decimal(my_decimal* dec) > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; > + return value->val_decimal(dec); > + } > + > + String* val_str(String *str) > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; > + return value->val_str(str); > + } > + > + bool add() > + { An extra space here before = > + Item *arg = get_arg(0); > + if (arg->is_null()) > + return false; > + > + if (first_call) > + { > + prev_value= arg->val_real(); We are missing a space after >. > + if (prev_value >1 || prev_value < 0) > + { > + my_error(ER_ARGUMENT_OUT_OF_RANGE, MYF(0)); > + return true; > + } > + first_call= false; > + } > + > + double arg_val= arg->val_real(); > + There is one extra space here. > + if (prev_value != arg_val) > + { > + my_error(ER_ARGUMENT_NOT_CONSTANT, MYF(0)); > + return true; > + } > + > + if (val_calculated) > + return false; > + > + value->store(order_item); > + value->cache_value(); > + if (value->null_value) > + return false; > + > + Item_sum_cume_dist::add(); > + double val= Item_sum_cume_dist::val_real(); > + > + if (val >= prev_value && !val_calculated) > + val_calculated= true; > + return false; > + } > + > + enum Sumfunctype sum_func() const > + { > + return PERCENTILE_DISC_FUNC; > + } > + > + void clear() > + { > + val_calculated= false; > + first_call= true; > + value->clear(); > + Item_sum_cume_dist::clear(); > + } > + > + const char*func_name() const > + { > + return "percentile_disc"; > + } > + > + void update_field() {} > + void set_type_handler(Window_spec *window_spec); > + const Type_handler *type_handler() const > + {return Type_handler_hybrid_field_type::type_handler();} > + > + void fix_length_and_dec() > + { > + decimals = 10; // TODO-cvicentiu find out how many decimals the standard > + // requires. > + } > + > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_sum_percentile_disc>(thd, mem_root, this); } > + void setup_window_func(THD *thd, Window_spec *window_spec); > + void setup_hybrid(THD *thd, Item *item); > + > +private: > + Item_cache *value; > + bool val_calculated; > + bool first_call; > + double prev_value; > + Item *order_item; > +}; > + > +class Item_sum_percentile_cont : public Item_sum_cume_dist, > + public Type_handler_hybrid_field_type > +{ > +public: > + Item_sum_percentile_cont(THD *thd, Item* arg) : Item_sum_cume_dist(thd, arg), > + Type_handler_hybrid_field_type(&type_handler_double), > + floor_value(NULL), ceil_value(NULL), first_call(TRUE),prev_value(0), > + ceil_val_calculated(FALSE), floor_val_calculated(FALSE), order_item(NULL){} > + > + double val_real() > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; > + double val= 1 + prev_value * (get_row_count()-1); > + > + /* > + Applying the formula to get the value > + If (CRN = FRN = RN) then the result is (value of expression from row at RN) > + Otherwise the result is > + (CRN - RN) * (value of expression for row at FRN) + > + (RN - FRN) * (value of expression for row at CRN) > + */ > + > + if(ceil(val) == floor(val)) > + return floor_value->val_real(); > + > + double ret_val= ((val - floor(val)) * ceil_value->val_real()) + > + ((ceil(val) - val) * floor_value->val_real()); > + > + return ret_val; > + } > + > + bool add() > + { > + Item *arg = get_arg(0); > + if (arg->is_null()) > + return false; > + > + if (first_call) > + { > + first_call= false; > + prev_value= arg->val_real(); > + if (prev_value >1 || prev_value < 0) > + { > + my_error(ER_ARGUMENT_OUT_OF_RANGE, MYF(0)); > + return true; > + } > + } > + > + double arg_val= arg->val_real(); > + if (prev_value != arg_val) > + { > + my_error(ER_ARGUMENT_NOT_CONSTANT, MYF(0)); > + return true; > + } > + > + if (!floor_val_calculated) > + { > + floor_value->store(order_item); > + floor_value->cache_value(); > + if (floor_value->null_value) > + return false; > + } > + if (floor_val_calculated && !ceil_val_calculated) > + { > + ceil_value->store(order_item); > + ceil_value->cache_value(); > + if (ceil_value->null_value) > + return false; > + } > + > + Item_sum_cume_dist::add(); > + double val= 1 + prev_value * (get_row_count()-1); > + > + if (!floor_val_calculated && get_row_number() == floor(val)) > + floor_val_calculated= true; > + > + if (!ceil_val_calculated && get_row_number() == ceil(val)) > + ceil_val_calculated= true; > + return false; > + } > + > + enum Sumfunctype sum_func() const > + { > + return PERCENTILE_CONT_FUNC; > + } > + > + void clear() > + { > + first_call= true; > + floor_value->clear(); > + ceil_value->clear(); > + floor_val_calculated= false; > + ceil_val_calculated= false; > + Item_sum_cume_dist::clear(); > + } > + > + const char*func_name() const > + { > + return "percentile_cont"; > + } > + void update_field() {} > + void set_type_handler(Window_spec *window_spec); > + const Type_handler *type_handler() const > + {return Type_handler_hybrid_field_type::type_handler();} > + > + void fix_length_and_dec() > + { > + decimals = 10; // TODO-cvicentiu find out how many decimals the standard > + // requires. > + } > + > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_sum_percentile_cont>(thd, mem_root, this); } > + void setup_window_func(THD *thd, Window_spec *window_spec); > + void setup_hybrid(THD *thd, Item *item); > + > +private: > + Item_cache *floor_value; > + Item_cache *ceil_value; > + bool first_call; > + double prev_value; > + bool ceil_val_calculated; > + bool floor_val_calculated; > + Item *order_item; > +}; > + > + > + > > class Item_window_func : public Item_func_or_sum > { > @@ -781,12 +1063,40 @@ class Item_window_func : public Item_func_or_sum > case Item_sum::DENSE_RANK_FUNC: > case Item_sum::PERCENT_RANK_FUNC: > case Item_sum::CUME_DIST_FUNC: > + case Item_sum::PERCENTILE_CONT_FUNC: > + case Item_sum::PERCENTILE_DISC_FUNC: > return true; > default: > return false; > } > } Let's make the grammar enforce this. This is silly. Is there a big limitation why we can't define a special rule for these functions? There already is order_by_single_element_list in sql_yacc.yy, it just isn't defined properly. We can get rid of these checks altogether then. > + bool only_single_element_order_list() const > + { > + switch (window_func()->sum_func()){ > + case Item_sum::PERCENTILE_CONT_FUNC: > + case Item_sum::PERCENTILE_DISC_FUNC: > + return true; > + default: > + return false; > + } > + } > + This is not needed. Put this in Item_sum_percentile_disc's fix_fields. > + void setting_handler_for_percentile_functions(Item_result rtype) const > + { > + switch (window_func()->sum_func()){ > + case Item_sum::PERCENTILE_DISC_FUNC: > + ((Item_sum_percentile_disc* ) window_func())->set_handler_by_cmp_type(rtype); > + break; > + default: > + return; > + } > + } > + Get rid of this function here. Put the logic of checking this result type in fix_fields specific to Item_sum_percentile_cont / disc. > + bool check_result_type_of_order_item(); > + > + > + > /* > Computation functions. > TODO: consoder merging these with class Group_bound_tracker. > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index 1f282e6aee5..d8d6975d92c 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7490,3 +7490,11 @@ ER_WRONG_INSERT_INTO_SEQUENCE > eng "Wrong INSERT into a SEQUENCE. One can only do single table INSERT into a squence object (like with mysqldump). If you want to change the SEQUENCE, use ALTER SEQUENCE instead." > ER_SP_STACK_TRACE > eng "At line %u in %s" This one will go away when we enforce the limitation in the grammar, make sure to remove it. > +ER_NOT_SINGLE_ELEMENT_ORDER_LIST > + eng "Incorrect number of elements in the order list for '%s'" > +ER_WRONG_TYPE_FOR_PERCENTILE_CONT > + eng "Numeric datatype is required for Percentile_CONT function" As discussed have these two errors print the function name to which the error is for, just like ER_NOT_SINGLE_ELEMENT_ORDER_LIST uses now. > +ER_ARGUMENT_NOT_CONSTANT > + eng "Argument to the percentile functions is not a constant" > +ER_ARGUMENT_OUT_OF_RANGE > + eng "Argument to the percentile functions does not belong to the range [0,1]" > diff --git a/sql/sql_window.cc b/sql/sql_window.cc > index 74e1ad25183..127d6d73ee6 100644 > --- a/sql/sql_window.cc > +++ b/sql/sql_window.cc > @@ -304,6 +304,12 @@ setup_windows(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables, > win_func_item->update_used_tables(); > } > 2 problems with this code. 1. We can probably move this logic in Item_sum_percentile_[cont|disc]::fix_fields. See if that works. 2. There already is an identical while loop before this. USe that one, don't have an extra loop here for no reason. > + li.rewind(); > + while ((win_func_item= li++)) > + { > + if (win_func_item->check_result_type_of_order_item()) > + DBUG_RETURN(1); > + } > DBUG_RETURN(0); > } > > @@ -1658,6 +1666,49 @@ class Frame_unbounded_following_set_count : public Frame_unbounded_following > } > }; > This cursor counts all rows in the partition that don't have the order_item set to null. Let's rewrite this like so: Extract this logic: """ List_iterator_fast<Item_sum> it(sum_functions); Item_sum* item; while ((item= it++)) { Item_sum_window_with_row_count* item_with_row_count = static_cast<Item_sum_window_with_row_count *>(item); item_with_row_count->set_row_count(num_rows_in_partition); } """ 1. Nake it a protected method for Frame_unbounded_following_set_count. Lets call it set_win_funcs_row_count. 2. Rewrite Frame_unbounded_following_set_count to use that function. 3. Make a subclass Frame_unbounded_following_set_count_no_nulls 4. Make use of the protected method in the Frame_unbounded_following_set_count_no_nulls > > +class Frame_unbounded_following_set_count_special: public Frame_unbounded_following_set_count > +{ > + > +public: > + Frame_unbounded_following_set_count_special(THD *thd, > + SQL_I_List<ORDER> *partition_list, > + SQL_I_List<ORDER> *order_list, Item* arg) : > + Frame_unbounded_following_set_count(thd,partition_list, order_list) > + { > + order_item= order_list->first->item[0]; > + } > + void next_partition(ha_rows rownum) > + { > + ha_rows num_rows_in_partition= 0; > + if (cursor.fetch()) > + return; > + > + /* Walk to the end of the partition, find how many rows there are. */ > + do > + { > + if (!order_item->is_null()) > + num_rows_in_partition++; You are missing a space after } > + }while (!cursor.next()); > + > + List_iterator_fast<Item_sum> it(sum_functions); > + Item_sum* item; > + while ((item= it++)) > + { > + Item_sum_window_with_row_count* item_with_row_count = > + static_cast<Item_sum_window_with_row_count *>(item); > + item_with_row_count->set_row_count(num_rows_in_partition); > + } > + } > + > + ha_rows get_curr_rownum() const > + { > + return cursor.get_rownum(); > + } > + > +private: > + Item* order_item; > +}; > + > ///////////////////////////////////////////////////////////////////////////// > // ROWS-type frame bounds > ///////////////////////////////////////////////////////////////////////////// > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index a0bbf39b138..b3b34fce3a4 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -10210,6 +10215,11 @@ geometry_function: > Geometry::wkb_polygon, > Geometry::wkb_linestring)); > } I think this is something that should not be here right? What is this? > + | WITHIN '(' expr ',' expr ')' > + { > + $$= GEOM_NEW(thd, Item_func_spatial_precise_rel(thd, $3, $5, > + Item_func::SP_WITHIN_FUNC)); > + } > ; > > /* > @@ -10671,6 +10681,47 @@ simple_window_func: > } > ; > > +inverse_distribution_function: > + inverse_distribution_function_def WITHIN GROUP_SYM > + '(' > + { Select->prepare_add_window_spec(thd); } > + order_by_single_element_list ')' OVER_SYM > + '(' opt_window_ref opt_window_partition_clause ')' > + { > + LEX *lex= Lex; > + if (Select->add_window_spec(thd, lex->win_ref, > + Select->group_list, > + Select->order_list, > + NULL)) > + MYSQL_YYABORT; > + $$= new (thd->mem_root) Item_window_func(thd, (Item_sum *) $1, > + thd->lex->win_spec); > + if ($$ == NULL) > + MYSQL_YYABORT; > + if (Select->add_window_func((Item_window_func *) $$)) > + MYSQL_YYABORT; > + } > + ; > + > +inverse_distribution_function_def: > + PERCENTILE_CONT_SYM '(' expr ')' > + { > + $$= new (thd->mem_root) Item_sum_percentile_cont(thd, $3); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | PERCENTILE_DISC_SYM '(' expr ')' > + { > + $$= new (thd->mem_root) Item_sum_percentile_disc(thd, $3); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + ; > + > See if you can make this accept only one order element. > +order_by_single_element_list: > + ORDER_SYM BY order_list > + ; > + > window_name: > ident > {

1 0

[Maria-developers] Patch for MDEV-12874 - UPDATE statements with the same source and target
by jerome brauge 29 Aug '17

29 Aug '17

Hello, Here is a patch for MDEV-12874 (on branch 10.2-ext). Can anyone review it ? Alexander, during my work on this MDEV, I've found a new issue with update statement and Oracle compatibility. create table t1 (c1 integer, c2 integer, c3 integer); insert into t1(c1,c2,c3) values (1,1,1); update t1 set c1 = c1+1, c2 = c1+1, c3 = c2+1; select * from t1; Mariadb : +------+------+------+ | c1 | c2 | c3 | +------+------+------+ | 2 | 3 | 4 | +------+------+------+ 1 row in set (0.00 sec) Oracle : DVTORA> C1 C2 C3 ---------- ---------- ---------- 2 2 2 Can you open a new MDEV for this ? Best regards, Jérôme.

3 8

[Maria-developers] Oracle strings functions compatibility : substr
by jerome brauge 17 Aug '17

17 Aug '17

Hello Alexander, Here is a patch for function SUBSTR in sql_mode=oracle (If position is 0, then it is treated as 1). Regards, Jérôme.

1 0

[Maria-developers] sys_vars.sysvars_server_embedded test failure
by Alexander Barkov 15 Aug '17

15 Aug '17

Hello Igor, sys_vars.sysvars_server_embedded is failing. It looks like it's related to your recent optimizer change. -SESSION_VALUE index_merge=on,index_merge_union=on,index_merge_sort_union=on,index_merge_intersection=on,index_merge_sort_intersection=off,engine_condition_pushdown=off,index_condition_pushdown=on,derived_merge=on,derived_with_keys=on,firstmatch=on,loosescan=on,materialization=on,in_to_exists=on,semijoin=on,partial_match_rowid_merge=on,partial_match_table_scan=on,subquery_cache=on,mrr=off,mrr_cost_based=off,mrr_sort_keys=off,outer_join_with_cache=on,semijoin_with_cache=on,join_cache_incremental=on,join_cache_hashed=on,join_cache_bka=on,optimize_join_buffer_size=off,table_elimination=on,extended_keys=on,exists_to_in=on,orderby_uses_equalities=on,condition_pushdown_for_derived=on -GLOBAL_VALUE index_merge=on,index_merge_union=on,index_merge_sort_union=on,index_merge_intersection=on,index_merge_sort_intersection=off,engine_condition_pushdown=off,index_condition_pushdown=on,derived_merge=on,derived_with_keys=on,firstmatch=on,loosescan=on,materialization=on,in_to_exists=on,semijoin=on,partial_match_rowid_merge=on,partial_match_table_scan=on,subquery_cache=on,mrr=off,mrr_cost_based=off,mrr_sort_keys=off,outer_join_with_cache=on,semijoin_with_cache=on,join_cache_incremental=on,join_cache_hashed=on,join_cache_bka=on,optimize_join_buffer_size=off,table_elimination=on,extended_keys=on,exists_to_in=on,orderby_uses_equalities=on,condition_pushdown_for_derived=on +SESSION_VALUE index_merge=on,index_merge_union=on,index_merge_sort_union=on,index_merge_intersection=on,index_merge_sort_intersection=off,engine_condition_pushdown=off,index_condition_pushdown=on,derived_merge=on,derived_with_keys=on,firstmatch=on,loosescan=on,materialization=on,in_to_exists=on,semijoin=on,partial_match_rowid_merge=on,partial_match_table_scan=on,subquery_cache=on,mrr=off,mrr_cost_based=off,mrr_sort_keys=off,outer_join_with_cache=on,semijoin_with_cache=on,join_cache_incremental=on,join_cache_hashed=on,join_cache_bka=on,optimize_join_buffer_size=off,table_elimination=on,extended_keys=on,exists_to_in=on,orderby_uses_equalities=on,condition_pushdown_for_derived=on,split_grouping_derived=on +GLOBAL_VALUE index_merge=on,index_merge_union=on,index_merge_sort_union=on,index_merge_intersection=on,index_merge_sort_intersection=off,engine_condition_pushdown= Can you please fix this? Thanks!

1 0

[Maria-developers] Valgrind reports uninitialized value on "if (!with_two_phase_optimization..."
by Alexander Barkov 15 Aug '17

15 Aug '17

Hello Igor, Please check the output of "mtr --valgrind main.sp" I'm getting these warnings: line ==6490== Thread 6: ==6490== Conditional jump or move depends on uninitialised value(s) ==6490== at 0x7762A1: JOIN::optimize() (sql_select.cc:1123) ==6490== by 0x77F2A7: mysql_select(THD*, TABLE_LIST*, unsigned int, List<Item>&, Item*, unsigned int, st_order*, st_order*, Item*, st_order*, unsigned long long, select_result*, st_select_lex_unit*, st_select_lex*) (sql_select.cc:3721) ==6490== by 0x773994: handle_select(THD*, LEX*, select_result*, unsigned long) (sql_select.cc:376) ==6490== by 0x73F055: execute_sqlcom_select(THD*, TABLE_LIST*) (sql_parse.cc:6431) ==6490== by 0x7354C9: mysql_execute_command(THD*) (sql_parse.cc:3631) ==6490== by 0x742A48: mysql_parse(THD*, char*, unsigned int, Parser_state*, bool, bool) (sql_parse.cc:7886) ==6490== by 0x730684: dispatch_command(enum_server_command, THD*, char*, unsigned int, bool, bool) (sql_parse.cc:1814) ==6490== by 0x72EFF7: do_command(THD*) (sql_parse.cc:1372) ==6490== by 0x883380: do_handle_one_connection(CONNECT*) (sql_connect.cc:1354) ==6490== by 0x883100: handle_one_connection (sql_connect.cc:1260) ==6490== by 0x10D6329: pfs_spawn_thread (pfs.cc:1862) ==6490== by 0x4E3C619: start_thread (in /usr/lib64/libpthread-2.22.so) ==6490== by 0x6DB75FC: clone (in /usr/lib64/libc-2.22.so) ^ Found warnings in /home/bar/maria-git/server.10.2-ext/mysql-test/var/6/log/mysqld.1.err Thanks!

1 0

[Maria-developers] history of lf_hash
by MARK CALLAGHAN 14 Aug '17

14 Aug '17

Comments for lf_hash are sparse. Does it implement a well-known algorithm? https://github.com/mysql/mysql-server/blob/8.0/mysys/lf_hash.cc https://github.com/mysql/mysql-server/blob/8.0/include/lf.h -- Mark Callaghan mdcallag(a)gmail.com

2 1

[Maria-developers] On Recursive CTE and CONNECT BY
by Sergey Petrunia 12 Aug '17

12 Aug '17

Hi Vicentiu, This is a followup to the Wednesday discussion. Background: see attached file for two queries. One uses a Recursive-CTE, another uses CONNECT-BY syntax. They produce different results for some reason. Initial guess about the cause was depth-first vs breadth-first traversal. Short answer: No it's not. The reason is that Recursive-CTE query is not equivalent to the CONNECT-BY query. Results from the recursive-CTE query: ID N P CYCLE PATH 1 A 0 /1A 2 B A 0 /1A/2B 3 C A 0 /1A/3C 4 D B 0 /1A/2B/4D 5 D C 0 /1A/3C/5D 6 E D 0 /1A/3C/5D/6E 6 E D 0 /1A/2B/4D/6E 7 F E 0 /1A/2B/4D/6E/7F 7 F E 0 /1A/3C/5D/6E/7F 8 C F 0 /1A/3C/5D/6E/7F/8C 8 C F 0 /1A/2B/4D/6E/7F/8C 5 D C 0 /1A/2B/4D/6E/7F/8C/5D 12 rows selected. Results from CONNECT-BY query: ID N P CYCLE Path 1 A 0 /1A 2 B A 0 /1A/2B 4 D B 0 /1A/2B/4D 6 E D 0 /1A/2B/4D/6E 7 F E 0 /1A/2B/4D/6E/7F 8 C F 1 /1A/2B/4D/6E/7F/8C 3 C A 0 /1A/3C 5 D C 0 /1A/3C/5D 6 E D 0 /1A/3C/5D/6E 7 F E 1 /1A/3C/5D/6E/7F 10 rows selected. The rows that are missing are: ID N P CYCLE PATH 8 C F 0 /1A/3C/5D/6E/7F/8C 5 D C 0 /1A/2B/4D/6E/7F/8C/5D (denote this as MISSING-ROWS) Another thing to note is that the CTE query has returned CYCLE=0 for all rows, while the CONNECT BY one has returned two rows with CYCLE=1: 8 C F 1 /1A/2B/4D/6E/7F/8C 7 F E 1 /1A/3C/5D/6E/7F Note that these rows are parents of rows in MISSING-ROWS. I don't think the issue is caused by breadth-first vs depth-first traversal differences. The issue is caused by us not understanding how Oracle's NOCYCLE clause works. Take a look at the original-dataset.jpg, and PATH columns of MISSING-ROWS We see that the path forms the entire loop (or "cycle"). Oracle's NOCYCLE seems to be about *not* constructing the entire loop. To rule out breadth-first vs depth-first differences, let's make an example where there's just one loop: create table g3(a int, b int); insert into g3 values (1, 2); insert into g3 values (2, 3); insert into g3 values (3, 4); insert into g3 values (4, 5); insert into g3 values (5, 6); insert into g3 values (6, 7); insert into g3 values (7, 4); select a, CONNECT_BY_ISCYCLE as cycle, SYS_CONNECT_BY_PATH(a, '/') "Path" from g3 start with a=1 connect by NOCYCLE prior b= a; A CYCLE Path 1 0 /1 2 0 /1/2 3 0 /1/2/3 4 0 /1/2/3/4 5 0 /1/2/3/4/5 6 1 /1/2/3/4/5/6 The row with a=7 is not listed. If it did, the path would have had a complete cycle. It is a bit counter intuitive that /1/2/3/4/5/6 has CYCLE=1, while actually it does not form a cycle. It's one step short of doing so. The documentation is vague about this, but here's somebody thinking the same on the internet: http://www.sql.ru/forum/255418/zadachka-anagrammy-na-sql?hl=%e0%ed%e0%e3%f0… : While posting messages on MetaLink, I think I finally got the logic of NOCYCLE. Clue was right there in the name itself. NOCYCLE means do not cycle. So if you have a hierarchical query without NOCYCLE and a cycle is detected Oracle does not know how to handle it and throws ORA-01436. If NOCYCLE is used Oracle simply cuts of part of a branch right at loop point without raising an error (another word it simply does not follow the loop). And that explains why CONNECT_BY_ISCYCLE is set to 1 for the last row in first iteration of the loop. Otherwise, we would not get 1 at all. Now next logical step would be to implement CYCLE. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Win32 - Sysvar 'optimizer_switch' failed 'def_val < my_set_bits(typelib.count)'
by jerome brauge 11 Aug '17

11 Aug '17

Hi All, Build of last 10.2-ext fail on Win32 with message : Sysvar 'optimizer_switch' failed 'def_val < my_set_bits(typelib.count)' The problem seems to be in sys_vars.ic (Sys_var_set) : > > { > option.var_type|= GET_SET; > global_var(ulonglong)= def_val; > SYSVAR_ASSERT(typelib.count > 0); > SYSVAR_ASSERT(typelib.count <= 64); > SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); Patch b14e2b04 introduce a new optimizer_switch and now 'optimizer_switch' has 32 values. my_set_bits works with unit32. A new function like this static inline uint64 my_set_bits64(int n) { return (((1ULL << (n - 1)) - 1) << 1) | 1; } might solve this issue. Regards.

1 0

[Maria-developers] Virtual columns in sql_mode=ORACLE
by jerome brauge 11 Aug '17

11 Aug '17

Hello Alexander, Something seems to be wrong in sql_yacc_ora.yy. (10.2-ext branch) We can't create a table with a virtual column because the grammar try to reduce the rule sp_cursor_name_and_offset. CREATE TABLE t1 (c1 INTEGER, c2 INTEGER AS (c1 MOD 10) VIRTUAL) ENGINE=InnoDb; ERROR 1324 (42000): Undefined CURSOR: c1 Regards, Jérôme.

2 1

[Maria-developers] Default value on column in sql_mode=ORACLE
by jerome brauge 11 Aug '17

11 Aug '17

Hello Alexander, Do you know why STRICT_TRANS_TABLES is not set in sql_mode=ORACLE ? The following script should not work: set sql_mode=oracle; create table t1 (c1 integer not null,c2 integer not null); insert into t1 (c1) values(1); Query OK, 1 row affected, 1 warning Regards, Jérôme.

2 1

Re: [Maria-developers] MDEV-13439: Database permissions are not enough to run a subquery with GROUP BY within a view
by Sergei Golubchik 09 Aug '17

09 Aug '17

Hi, Oleksandr! On Aug 07, Oleksandr Byelkin wrote: > revision-id: cf66c30ce592ad88d447568272bdced2046f2906 (mariadb-10.2.7-20-gcf66c30ce59) > parent(s): bcc10a5a447805ce64aa13ee6a037c1618219616 > committer: Oleksandr Byelkin > timestamp: 2017-08-07 13:42:35 +0200 > message: > > MDEV-13439: Database permissions are not enough to run a subquery with GROUP BY within a view > > The bug is result adding ability to have derived tables inside views. > Fixed checks should be a switch between view/derived or select derived and information schema. > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 77f502d3a7b..9bffcbe6a9e 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -7594,8 +7594,11 @@ bool check_grant(THD *thd, ulong want_access, TABLE_LIST *tables, > /* > It is subquery in the FROM clause. VIEW set t_ref->derived after > table opening, but this function always called before table opening. > + > + NOTE: is_derived() can't be used here bacause subquery in this case typo "bacause" > + the FROM clase (derived tables) can be not be marked yet. > */ > - if (!t_ref->referencing_view) > + if (t_ref->is_anonymous_derived_table() || t_ref->schema_table) > { > /* > If it's a temporary table created for a subquery in the FROM > diff --git a/sql/sql_derived.cc b/sql/sql_derived.cc > index 649f745fdc4..8f8d051a976 100644 > --- a/sql/sql_derived.cc > +++ b/sql/sql_derived.cc > @@ -822,10 +822,12 @@ bool mysql_derived_prepare(THD *thd, LEX *lex, TABLE_LIST *derived) > table->derived_select_number= first_select->select_number; > table->s->tmp_table= INTERNAL_TMP_TABLE; > #ifndef NO_EMBEDDED_ACCESS_CHECKS > - if (derived->referencing_view) > + if (derived->is_view()) > table->grant= derived->grant; > else > { > + DBUG_ASSERT(derived->is_derived()); > + DBUG_ASSERT(derived->is_anonymous_derived_table()); Hmm, if derived->is_derived() is always true here, why there's an if() below? If it's redundant - please, remove it. > table->grant.privilege= SELECT_ACL; > if (derived->is_derived()) > derived->grant.privilege= SELECT_ACL; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] commit_checkpoint_request() vs. thd_get_durability_property() (in relation to MDEV-11937)
by Kristian Nielsen 08 Aug '17

08 Aug '17

Monty asked me to fix MDEV-11937. This particular one is a performance regression in InnoDB commit. But there is a wider problem that I thought I should explain, so it can be perhaps avoided in the future. MariaDB and MySQL use different mechanisms for storage engines to avoid having to fsync during commit when binlog is enabled. In MariaDB, storage engines implement the commit_checkpoint_request() handlerton method. In MySQL, storage engines call thd_get_durability_property() to check if they can avoid fsync. The problem is that somehow the thd_get_durability_property() function was introduced into MariaDB code, but it is completely non-functional. So now there is code in InnoDB, TokuDB and RocksDB that calls this function and does not work correctly. This lead to performance regression due to extra fsync() calls. This seems to me a serious problem. Now new code can be merged and compile fine, where in reality it is wrong. There really should not be two separate and different mechanisms for the same thing, and certainly not with one of them non-functional. The "expected" approach would be to remove thd_get_durability_property() and update storage engines to use the corresponding MariaDB APIs (commit_ordered() and commit_checkpoint_request()). This should not be hard. A simple commit_checkpoint_request() implementation can just fsync all transactions immediately (similar to what MySQL does). A more detailed implementation can avoid any extra fsyncs, and just asynchroneously notify the upper layer with commit_checkpoint_notify_ha() later when such fsync happens normally (this is what InnoDB does). See comments in handler.h for details. Or was the intention to eventually replace the whole MariaDB binlog group commit implementation with the MySQL one, to make MariaDB less divergent? This would require a number of changes to MariaDB binlog and replication. The binlog recovery code should be replaced (MySQL does not have the ability to recover from more than one binlog). The binlog group commit code must be replaced as well, and the commit_ordered() mechanism removed. I think this would also require a re-design of the MariaDB in-order parallel replication. MySQL has some optional mechanism for in-order, but my understanding is that it is not sufficient to do optimistic parallel replication. I am not intimately familiar with the MySQL code though. I hope this helps. The wider problem behind MDEV-11937 is one of policy more than one of code bugs, so not too much else I can do to address it. ----------------------------------------------------------------------- Incidentally, I noticed some code in InnoDB trx0trx.cc: /* We set the HA_IGNORE_DURABILITY during prepare phase of binlog group commit to not flush redo log for every transaction here. So that we can flush prepared records of transactions to redo log in a group right before writing them to binary log during flush stage of binlog group commit. */ So the idea is to do group prepare with the same group of transactions that will later group commit to the binlog. In MariaDB, this concept does not exist. Storage engine prepares are allowed to run in parallel and in any order compared to binlog commit. So the InnoDB group prepare can include more transactions than participate in the binlog commit (but also less, of course). IIRC, the important thing is to ensure that all transactions are durably prepared in storage engines before being written to the binlog. In MariaDB, there is MYSQL_LOG_BIN::group_commit_queue that holds the list of transactions to be group committed to the binlog. A similar mechanism in MariaDB might use the group_commit_queue as the set of transactions to send to storage engines for group prepare. But if we wait for a prepare fsync() after building this list, some transactions that could prepare during this fsync may be unnecesarily delayed to the next binlog commit. The MySQL 5.7 code grabs the list of transactions to binlog group commit, and then flushes the log in _all_ storage engines, unconditionally. That just seems horrybly wrong, but I can't see any other way to read the code (from MYSQL_BIN_LOG::process_flush_stage_queue()): ha_flush_logs(NULL, true); It even does so while holding LOCK_log :-( I guess the MySQL idea is that there is only one storage engine anyway, InnoDB. - Kristian.

3 3

Re: [Maria-developers] 4c26b71c77d: MDEV-8960: Can't refer the same column twice in one ALTER TABLE
by Sergei Golubchik 04 Aug '17

04 Aug '17

Hi, Jan! A question and a suggestion, see below On Jul 27, jan wrote: > revision-id: 4c26b71c77d58fbeb3192e2142ef66efe5f122dd (mariadb-10.0.31-43-g4c26b71c77d) > parent(s): a8c1817846eb9f3fd8738e80db87cff642b9a78b > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-07-27 13:17:13 +0300 > message: > > MDEV-8960: Can't refer the same column twice in one ALTER TABLE > > Problem was that if column was created in alter table when > it was refered again it was not tried to find from list > of current columns. > > mysql_prepare_alter_table: > There is two cases > (1) If alter table adds a new column and then later alter > changes the field definition, there was no check from > list of new columns, instead an incorrect error was given. > (2) If alter table adds a new column and then later alter > changes the default, there was no check from list of > new columns, instead an incorrect error was given. > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2eff8fd5e2f..3ba3ec6847e 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -7503,9 +7503,25 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, > { > if (def->change && ! def->field) > { > - my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change, > - table->s->table_name.str); > - goto err; > + /* > + Check if there is modify for newly added field. > + */ > + Create_field *find; > + find_it.rewind(); > + while((find=find_it++)) > + { > + if (!my_strcasecmp(system_charset_info,find->field_name, def->field_name)) > + break; > + } > + > + if (find && !find->field) Can it be here that find->field != NULL? I don't see how. But if yes, the ER_BAD_FIELD_ERROR looks rather weird in that case. If not - simple if (find) would be less confusing. > + find_it.remove(); > + else > + { > + my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change, > + table->s->table_name.str); > + goto err; > + } > } > /* > Check that the DATE/DATETIME not null field we are going to add is > @@ -7571,6 +7587,29 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, > find_it.after(def); // Put column after this > } > } > + /* > + Check if there is alter for newly added field. > + */ > + alter_it.rewind(); > + Alter_column *alter; > + while ((alter=alter_it++)) > + { > + if (!my_strcasecmp(system_charset_info,def->field_name, alter->name)) > + break; > + } > + if (alter) > + { > + if (def->sql_type == MYSQL_TYPE_BLOB) > + { > + my_error(ER_BLOB_CANT_HAVE_DEFAULT, MYF(0), def->change); > + goto err; > + } > + if ((def->def=alter->def)) // Use new default > + def->flags&= ~NO_DEFAULT_VALUE_FLAG; > + else > + def->flags|= NO_DEFAULT_VALUE_FLAG; > + alter_it.remove(); > + } Please, move this into a function. Don't just copy 20 lines around. For example if (apply_alter_column(alter_it, def, field->field_name)) goto err; > } > if (alter_info->alter_list.elements) > { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Review 2 of compressed columns
by Michael Widenius 04 Aug '17

04 Aug '17

Hi! Here is a review for the patches marked with fixup! MDEV-11371 - column compression > commit f1a3cd40ecc4c144ac32d43ab5c1e88c8e4fae7e > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Mon Jul 31 16:07:39 2017 +0400 > > fixup! MDEV-11371 - column compression > > In worst case we have to store data length + 1 for compressed data, > we should increase field length by 1 internally so that data is not > truncated. > > diff --git a/mysql-test/t/column_compression.test b/mysql-test/t/column_compression.test > index 256e96c..d3f8481 100644 > --- a/mysql-test/t/column_compression.test > +++ b/mysql-test/t/column_compression.test > @@ -54,8 +54,8 @@ SHOW CREATE TABLE t1; > --cat_file $MYSQLD_DATADIR/test/t1.CSV > DROP TABLE t1; > > ---echo # Test fields with length equal to data length > -CREATE TABLE t1(a VARCHAR(10) COMPRESSED); > +--echo # Test fields that don't fit data > +CREATE TABLE t1(a VARCHAR(9) COMPRESSED); > --error ER_DATA_TOO_LONG > INSERT INTO t1 VALUES(REPEAT('a', 10)); > INSERT INTO t1 VALUES(REPEAT(' ', 10)); Please also insert a correct row, and not only edge cases. (You may do it already, but I can't see that in the diff) > @@ -70,3 +70,12 @@ INSERT INTO t1 VALUES(REPEAT(' ', 255)); > SET column_compression_threshold=DEFAULT; > SELECT a, LENGTH(a) FROM t1; > DROP TABLE t1; > + > +--echo # Corner case: VARCHAR(255) COMPRESSED must have 2 bytes pack length > +CREATE TABLE t1(a VARCHAR(255) COMPRESSED); > +SHOW CREATE TABLE t1; > +SET column_compression_threshold=300; > +INSERT INTO t1 VALUES(REPEAT('a', 255)); > +SET column_compression_threshold=DEFAULT; > +SELECT a, LENGTH(a) FROM t1; > +DROP TABLE t1; Please insert also a row without column_compression_threshold, just to cover all cases in the test case. > diff --git a/sql/field.cc b/sql/field.cc > index 216fb95..e1dc193 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7555,7 +7555,8 @@ void Field_varstring::sql_type(String &res) const > length= cs->cset->snprintf(cs,(char*) res.ptr(), > res.alloced_length(), "%s(%d)", > (has_charset() ? "varchar" : "varbinary"), > - (int) field_length / charset()->mbmaxlen); > + (int) field_length / charset()->mbmaxlen - > + MY_TEST(compression_method())); > res.length(length); > if ((thd->variables.sql_mode & (MODE_MYSQL323 | MODE_MYSQL40)) && > has_charset() && (charset()->state & MY_CS_BINSORT)) > @@ -10015,6 +10016,8 @@ void Column_definition::create_length_to_internal_length(void) > case MYSQL_TYPE_STRING: > case MYSQL_TYPE_VARCHAR: > length*= charset->mbmaxlen; > + if (sql_type == MYSQL_TYPE_VARCHAR && compression_method()) > + length++; Add a comment: /* Compressed columns needs one extra byte to store the compression method. This byte is invisible to the end user, but not for the storage engine. */ > key_length= length; > pack_length= calc_pack_length(sql_type, length); > break; <cut> > commit 274ab73a89573554d07a6de1deb846ad47eb1004 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 27 13:06:33 2017 +0400 > > fixup! MDEV-11371 - column compression > > Fixed BLOB COMPRESSED -> BLOB COMPRESSED replication. > > The problem was that Field_blob::unpack gets compressed data and calls > Field_blob_compressed::store(), which compressed it again. Fixed by calling > Field_blob::set_ptr() instead of ::store(). > > Note that now no data copy is performed and blob is pointing to data allocated > by replication routines. It used to work this way before 06fb8c2d10, but still > should be reviewed carefully to make sure no copy is actually needed. > patch ok. > commit 93e00970100503c3f365063d9261119f096bee48 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 27 11:44:20 2017 +0400 > > Get rid of Field::do_save_field_metadata() > > It doesn't serve any purpose, but generates extra virtual function call. > ok > commit e1247d4991bb4ebd3f98788c7bfe31a34faf7b69 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 27 10:58:47 2017 +0400 > > fixup! MDEV-11371 - column compression > > This is an addition to "Fixed compressed columns to work with partitioning". > > Mroonga calls key_copy() on write/delete, when column is not in read_set. > Adjust read_set for the duration of val_str(). > > diff --git a/sql/field.cc b/sql/field.cc > index ec82984..ab96ee9 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -7659,8 +7659,12 @@ uint Field_varstring::get_key_image(uchar *buff, uint length, > { > String val; > uint local_char_length; > + my_bitmap_map *old_map; > > + old_map= dbug_tmp_use_all_columns(table, table->read_set); > val_str(&val, &val); > + dbug_tmp_restore_column_map(table->read_set, old_map); > + Shouldn't this be in mronga code and not in MariaDB? Looking at the original code, we didn't check for read_set before either for get_key_image, so I assume this is ok. > local_char_length= val.charpos(length / field_charset->mbmaxlen); > if (local_char_length < val.length()) > val.length(local_char_length); > commit 94040156d8007663beb6785ca653d41b26758301 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Wed Jul 26 17:28:57 2017 +0400 > > fixup! MDEV-11371 - column compression > > Get rid of hardcoded "8" (zlib compression method id) by moving towards > support for multiple compression methods. > commit 6c8b1e563c7456eb0b8ca11a621333237de37ec4 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Tue Jul 25 13:27:07 2017 +0400 > > fixup! MDEV-11371 - column compression > > Fixed compressed columns to work with partitioning. > > Reviewer note: this patch reuses get_key_image()/set_key_image() of > Field_varstring to avoid code duplication. Cons: this adds virtual function > calls val_str() and store(). > ok > commit 9c85ad8d105e74baae0b6aba0db01b4d6aac7ba6 > Author: Sergey Vojtovich <svoj(a)mariadb.org> > Date: Thu Jul 20 16:57:46 2017 +0400 > > fixup! MDEV-11371 - column compression ok. The only thing left is now to fix replication to handle compressed columns. We also have agreed to handle the case where the master and slave have different COMPRESSED column attributes for VARCHAR and BLOB. Regards, Monty

1 0

[Maria-developers] Debian 9 support for MaxScale
by Martin Toth 03 Aug '17

03 Aug '17

Hi all, I would like to ask if there is some release date for fixing https://jira.mariadb.org/browse/MXS-1292 . Debian 9 should be included as a build target ASAP. I am installing only D9 systems on new servers and I am unable to use builds for MaxScale, so its unusable in D9. Thanks. Martin

3 7

Re: [Maria-developers] 1 Transaction and several threads
by Jan Lindström 03 Aug '17

03 Aug '17

Hi, If we talk about bulk load I would write directly .ibd file format pages, now as InnoDB has always some order between pages doing this in parallel has challenges, we can read and parse input, sort it, reformat to InnoDB pages, write these pages (maybe you could do this also parallel). To speed this up, I would remove secondary keys that are not needed from table and rebuild them after bulk load. R: Jan On Thu, Aug 3, 2017 at 12:09 PM, Oleksandr Byelkin <sanja(a)mariadb.com> wrote: > Hi! > > I am thinking about efficient inserting in a table using several threads > (LOAD), is it possible (and how difficult if it does) to make inserts with > help of several threads abut inside one InnoDB transaction (maybe one XA > transaction)? >

3 2

[Maria-developers] Git jira integration plugin
by Daniel Black 03 Aug '17

03 Aug '17

For those that haven't seen it. https://www.npmjs.com/package/jira-git

1 0

[Maria-developers] Patch for MDEV-13417 - UPDATE produces wrong values if an updated column is later used as an update source
by jerome brauge 02 Aug '17

02 Aug '17

Hello, Here is a patch for MDEV-13417 (on branch 10.2-ext). Can anyone review it ? Best regards, Jérôme.

1 0

[Maria-developers] HAVE_LONG_LONG check
by Aleksey Midenkov 20 Jul '17

20 Jul '17

Do we need HAVE_LONG_LONG in item.cc: #ifdef HAVE_LONG_LONG case MYSQL_TYPE_LONGLONG: field= new (mem_root) Field_longlong((uchar*) 0, max_length, null_ptr, 0, Field::NONE, name, 0, unsigned_flag); break; #endif ? There is no such check in field.cc: case MYSQL_TYPE_LONGLONG: return new (mem_root) Field_longlong(ptr,field_length,null_pos,null_bit, unireg_check, field_name, f_is_zerofill(pack_flag) != 0, f_is_dec(pack_flag) == 0);

2 1

Re: [Maria-developers] 0aa6217: MDEV-13065 rpl.rpl_mdev-11092 fails sporadically in buildbot
by Sergei Golubchik 19 Jul '17

19 Jul '17

Hi, Sachin! On Jul 19, sachin wrote: > revision-id: 0aa621722d74d317f39b4c463800959ee84a6097 (mariadb-10.0.31-33-g0aa6217) As discussed, it should probably go into 5.5 > parent(s): 2f07709aed863ea1852a78a0d6ab718e49df8cca > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-07-19 11:59:43 +0530 > message: > > MDEV-13065 rpl.rpl_mdev-11092 fails sporadically in buildbot > > Problem > rpl.rpl_mdev-11092 fails in buildbot because after starting slave in > wait_for_slave_sql_error_and_skip.inc slave is started but there may be > chances that we have not skipped the last error and Last_SQL_Errno is > still not zero untill the end of rpl_end.inc , which will compare > Last_SQL_Errno to 0. So in this this case rpl_mdev-11092 fails. > > Solution > After starting slave in wait_for_slave_sql_error_and_skip.inc we will wait for > Last_SQL_Errno to become 0. > > --- > mysql-test/include/wait_for_slave_sql_error_and_skip.inc | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/mysql-test/include/wait_for_slave_sql_error_and_skip.inc b/mysql-test/include/wait_for_slave_sql_error_and_skip.inc > index 9246c18..60c8319 100644 > --- a/mysql-test/include/wait_for_slave_sql_error_and_skip.inc > +++ b/mysql-test/include/wait_for_slave_sql_error_and_skip.inc > @@ -66,6 +66,21 @@ if (!$slave_skip_counter) { > } > source include/start_slave.inc; > > +# Wait for 5 seconds Last_SQL_Errno to become zero > +--let $wait_counter=50 > +while ($wait_counter) > +{ > + let $wait_condition= query_get_value("SHOW SLAVE STATUS", Last_SQL_Errno, 1); > + if ($wait_condition == $slave_sql_errno) > + { > + real_sleep 0.1; > + dec $wait_counter; > + } > + if (!$wait_condition) > + { > + --let $wait_counter = 0; > + } > +} I'd suggest to use include/wait_for_slave_param.inc here, like let $slave_param= Last_SQL_Errno; let $slave_param_value= 0; source include/wait_for_slave_param.inc; and put a comment before it, like # start_slave.inc returns when Slave_SQL_Running=Yes. But the slave # thread sets it before clearing Last_SQL_Errno. So we have to wait # for Last_SQL_Errno=0 separately. > --let $include_filename= wait_for_slave_sql_error_and_skip.inc [errno=$slave_sql_errno] > --source include/end_include_file.inc Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 05b678bc8c1: MDEV-12489 The select stmt may fail due to "having clause is ambiguous" unexpected
by Oleksandr Byelkin 12 Jul '17

12 Jul '17

Hi, Sergei! Am 12.07.2017 um 18:13 schrieb serg(a)mariadb.org: > revision-id: 05b678bc8c166c9015ca3dc3ccd9e9c89094f3ca (mariadb-5.5.56-42-g05b678bc8c1) > parent(s): c83d6ff881dcfd1cc68124a9663c2a31ca7e2dce > author: Sergei Golubchik > committer: Sergei Golubchik > timestamp: 2017-07-12 17:46:19 +0200 > message: > > MDEV-12489 The select stmt may fail due to "having clause is ambiguous" unexpected > > only compare field references with an alias (from the SELECT clause) > when this reference doesn't specify an explicit table name part. > OK to push! [skip]

1 0

Re: [Maria-developers] [Commits] be55bbc2b20: MDEV-7826 Server crashes in Item_subselect::enumerate_field_refs_processor
by Oleksandr Byelkin 12 Jul '17

12 Jul '17

Hi, Sergei! Am 12.07.2017 um 12:49 schrieb serg(a)mariadb.org: > revision-id: be55bbc2b206f2fe295ad4179b23791c3119fe02 (mariadb-5.5.56-39-gbe55bbc2b20) > parent(s): c5975eaea174460e774e77717e972a8d32e6c8de > author: Sergei Golubchik > committer: Sergei Golubchik > timestamp: 2017-07-12 12:49:29 +0200 > message: > > MDEV-7826 Server crashes in Item_subselect::enumerate_field_refs_processor > > upper->item can be NULL if we're referring to an aggregate function OK to push! [skip]

1 0

Re: [Maria-developers] [Commits] c5975eaea17: MDEV-7339 Server crashes in Item_func_trig_cond::val_int
by Oleksandr Byelkin 12 Jul '17

12 Jul '17

Hi! Am 12.07.2017 um 10:39 schrieb serg(a)mariadb.org: > revision-id: c5975eaea174460e774e77717e972a8d32e6c8de (mariadb-5.5.56-38-gc5975eaea17) > parent(s): f305a7ce4bccbd56520d874e1d81a4f29bc17a96 > author: Sergei Golubchik > committer: Sergei Golubchik > timestamp: 2017-07-12 10:39:02 +0200 > message: > > MDEV-7339 Server crashes in Item_func_trig_cond::val_int > > Item_in_subselect::pushed_cond_guards[] array is allocated only when > left_expr->maybe_null. And it is used (for row expressions) when > left_expr->element_index(i)->maybe_null. > > For left_expr being a multi-column subquery, its maybe_null is > always false when the subquery doesn't use tables (see > Item_singlerow_subselect::fix_length_and_dec() > and subselect_single_select_engine::fix_length_and_dec()), > otherwise it's always true. > > But row elements can be NULL regardless, so let's always allocate > pushed_cond_guards for multi-column subqueries, no matter whether > its maybe_null was forced to true or false. OK to push! [skip]

1 0

Re: [Maria-developers] [Commits] 7801756: MDEV-13826: Floating point exception in Filesort_tracker::print_json_members(Json_writer*)
by Sergey Petrunia 12 Jul '17

12 Jul '17

Hi Varun, On Wed, Jul 12, 2017 at 12:43:02AM +0530, Varun wrote: > revision-id: 780175646bb9312184d8a676e86b125f663b5156 (mariadb-10.2.2-591-g7801756) > parent(s): 3904014ed37cc2b58844e8d24a5d6aaa69d80f59 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-07-12 00:41:52 +0530 > message: > > MDEV-13826: Floating point exception in Filesort_tracker::print_json_members(Json_writer*) > > Whenever Filesort_tracker has r_loops=0, r_ouptut_rows would be 0, so we should add the value zero to the member "r_output_rows" expliciytly > > --- > mysql-test/r/analyze_format_json.result | 60 +++++++++++++++++++++++++++++++++ > mysql-test/t/analyze_format_json.test | 11 ++++++ > sql/sql_analyze_stmt.cc | 11 +++--- > 3 files changed, 78 insertions(+), 4 deletions(-) > > diff --git a/mysql-test/r/analyze_format_json.result b/mysql-test/r/analyze_format_json.result > index 686da92..436b13b 100644 > --- a/mysql-test/r/analyze_format_json.result > +++ b/mysql-test/r/analyze_format_json.result > @@ -757,3 +757,63 @@ ANALYZE > } > } > drop table t1,t2,t3; > +# > +# MDEV-13286: Floating point exception in Filesort_tracker::print_json_members(Json_writer*) > +# > +create table t0(a int); > +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > +create table t1 (a int, b int, c int); > +insert into t1 select a,a,a from t0; > +create table t2 as select * from t1; > +analyze format=json select a, (select t2.b from t2 where t2.a<t1.a order by t2.c limit 1) from t1 where t1.a<0; > +ANALYZE > +{ > + "query_block": { > + "select_id": 1, > + "r_loops": 1, > + "r_total_time_ms": 0.0324, > + "table": { > + "table_name": "t1", > + "access_type": "ALL", > + "r_loops": 1, > + "rows": 10, > + "r_rows": 10, > + "r_total_time_ms": 0.0098, > + "filtered": 100, > + "r_filtered": 0, > + "attached_condition": "t1.a < 0" > + }, > + "subqueries": [ > + { > + "expression_cache": { > + "state": "uninitialized", > + "r_loops": 0, > + "query_block": { > + "select_id": 2, > + "read_sorted_file": { > + "r_rows": null, > + "filesort": { > + "sort_key": "t2.c", > + "r_loops": 0, > + "r_limit": "(varied across executions)", > + "r_used_priority_queue": false, Look at the "table" field below: the parameters whose values are not defined because of r_loop=0 had value of null (e.g. r_filtered, r_rows). Let's follow that pattern here: r_loops:null, r_limit:null, r_used_priority_queue: null. Please change this. Ok to push after this is addressed. > + "r_output_rows": 0, > + "table": { > + "table_name": "t2", > + "access_type": "ALL", > + "r_loops": 0, > + "rows": 10, > + "r_rows": null, > + "filtered": 100, > + "r_filtered": null, > + "attached_condition": "t2.a < t1.a" > + } > + } > + } > + } > + } > + } > + ] > + } > +} > +drop table t0,t1,t2; > diff --git a/mysql-test/t/analyze_format_json.test b/mysql-test/t/analyze_format_json.test > index 88a9077..02603bc 100644 > --- a/mysql-test/t/analyze_format_json.test > +++ b/mysql-test/t/analyze_format_json.test > @@ -212,3 +212,14 @@ GROUP BY sq ORDER BY gc; > > drop table t1,t2,t3; > > + > +--echo # > +--echo # MDEV-13286: Floating point exception in Filesort_tracker::print_json_members(Json_writer*) > +--echo # > +create table t0(a int); > +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > +create table t1 (a int, b int, c int); > +insert into t1 select a,a,a from t0; > +create table t2 as select * from t1; > +analyze format=json select a, (select t2.b from t2 where t2.a<t1.a order by t2.c limit 1) from t1 where t1.a<0; > +drop table t0,t1,t2; > diff --git a/sql/sql_analyze_stmt.cc b/sql/sql_analyze_stmt.cc > index cc6e0f8..3e49af1 100644 > --- a/sql/sql_analyze_stmt.cc > +++ b/sql/sql_analyze_stmt.cc > @@ -43,15 +43,18 @@ void Filesort_tracker::print_json_members(Json_writer *writer) > } > > writer->add_member("r_used_priority_queue"); > - if (r_used_pq == get_r_loops()) > - writer->add_bool(true); > - else if (r_used_pq == 0) > + if (r_used_pq == 0) > writer->add_bool(false); > + else if (r_used_pq == get_r_loops()) > + writer->add_bool(true); > else > writer->add_str(varied_str); > > - writer->add_member("r_output_rows").add_ll((longlong) rint(r_output_rows / > + if (r_output_rows) > + writer->add_member("r_output_rows").add_ll((longlong) rint(r_output_rows / > get_r_loops())); > + else > + writer->add_member("r_output_rows").add_ll((longlong)0); > > if (sort_passes) > { > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] RFC: new replication feature "per-engine mysql.gtid_slave pos"
by Kristian Nielsen 11 Jul '17

11 Jul '17

I plan to implement MDEV-12179, per-engine mysql.gtid_slave_pos. Here is a description of the high-level design, as a request for comments and/or suggestion for changes. The purpose of this is to fix a serious performance issue in replication when multiple storage engines are used. Every replicated transaction updates the replication GTID position in table mysql.gtid_slave_pos. There is a large overhead if that table is using another storage engine than the rest of the transaction, as then a cross-engine XA transaction needs to be done. But if more than one storage engine is used on a server, some replicated transactions will necessarily end up being cross-engine. This problem becomes more interesting to fix with the new interest in MyRocks. Though it already exists also for eg. people using MyISAM or TokuDB simultaneously with InnoDB, for example. The basic idea is to create multiple copies of the table, eg. mysql.gtid_slave_pos_innodb and mysql.gtid_slave_pos_myrocks. This way, an InnoDB transaction can update the InnoDB version of the table, and similar for other engines. To enable the feature, the DBA would create extra copies of the table for the engines she wants: CREATE TABLE mysql.gtid_slave_pos_myrocks LIKE mysql.gtid_slave_pos; ALTER TABLE mysql.gtid_slave_pos_myrocks ENGINE=myrocks; The server will look for all tables called mysql.gtid_slave_pos* at startup, as well as on each START SLAVE. This method makes migration from earlier versions simple. MariaDB already has the ability to work with multiple redundant rows in the mysql.gtid_slave_pos table. The column "sub_id" provides a version to identify the most recent entry. So the server just needs to read multiple tables (if present), and pick the newest version amongst all rows. Thus, the new feature is compatible with older versions without any schema changes. I think it is better than automatically creating copies of the table for all engines; it is often the case that an engine (eg. TokuDB) is available in the server binary, but the user does not want any tables created in that engine. This means that no speedup is obtained if the user does not create additional mysql.gtid_slave_pos* tables. (However, things should still work fine, thanks to XA synchronising cross-engine transactions). - Kristian.

6 12

[Maria-developers] mariadb_config broken in 10.2.6
by Jeroen Ooms 07 Jul '17

07 Jul '17

Running 'mariadb_config --libs' returns invalid output. See https://github.com/Homebrew/homebrew-core/issues/15333

3 3

[Maria-developers] Confusion between variable / alias and row type.
by jerome brauge 07 Jul '17

07 Jul '17

Hello Alexander, I think there is a regression introduced with row type. Attached stored procedure creation fail with error : ERROR 1105 (HY000) at line 9: 'a' is not a row variable. Regards, Jérôme.

2 2

[Maria-developers] Review of PERCENTILE_CONT && PERCENTILE_DISC
by Vicențiu Ciorbaru 04 Jul '17

04 Jul '17

Hi Varun, Here's the review for your patch. > diff --git a/mysql-test/t/percentile.test b/mysql-test/t/percentile.test > new file mode 100644 > index 00000000000..0958fc05e7d > --- /dev/null > +++ b/mysql-test/t/percentile.test > @@ -0,0 +1,41 @@ Whenever you are adding a test case, please add the result file too. It makes things easier to review. > +CREATE TABLE student (name CHAR(10), test double, score TINYINT); > + > +INSERT INTO student VALUES > +('Chun', 0, null), ('Chun', 0, 4), > +('Esben', 1, null), ('Esben', 1, null), > +('Kaolin', 0.5, 56), ('Kaolin', 0.5, 88), > +('Tatiana', 0.8, 2), ('Tatiana', 0.8, 1); > + > + > + > +select name, percentile_disc(0.6) within group(order by score) over (partition by name) from student; > +select name, percentile_disc(test) within group(order by score) over (partition by name) from student; > +select name, percentile_disc(0.4) within group(order by score) over (partition by name) from student; > + > + > +#select name, percentile_cont(null) within group(order by score) over (partition by name) from student; > +#select name, cume_dist() over (partition by name order by score) from student; > + > + > +#normal parsing > +#select percentile_cont(0.5) within group(order by score) over w1 from student > +#window w1 AS (partition by name); > + > +# no partition clause > +#select percentile_cont(0.5) within group(order by score) over () from student; > + > + > +# only one sort allowed > +#select percentile_cont(0.5) within group(order by score) over (partition by name); > + > +#parameter value should be in the range of 0 to 1 > +#select percentile_cont(1.5) within group(order by score) over (partition by name); > + > + > +# > +#select rank() over (partition by name order by score ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) from student; > + > + > + > +drop table student; > + > diff --git a/sql/item.h b/sql/item.h > index 76ce4aa935f..3df4ec4f358 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5215,6 +5217,14 @@ class Cached_item_item : public Cached_item > cmp(); > item= save; > } This is not used in the patch. Is there a need for it any more? > + Item* get_item() > + { > + return item; > + } Add an empty line here here. > + void clear() > + { > + null_value= false; > + } > }; > > class Cached_item_str :public Cached_item_item > @@ -5236,6 +5250,12 @@ class Cached_item_real :public Cached_item_item > Cached_item_real(Item *item_par) :Cached_item_item(item_par),value(0.0) {} > bool cmp(void); > int cmp_read_only(); Space after ). Mark this function as const > + double get_value(){ return value;} I don't quite get what this clear is supposed to do. I'd expect clear to set the null_value to true, not to false. Same for other functions in the diff. Add a comment explaining this. > + void clear() > + { Space after = > + value=0.0; > + null_value= false; > + } > }; > > class Cached_item_int :public Cached_item_item > @@ -5245,6 +5265,12 @@ class Cached_item_int :public Cached_item_item > Cached_item_int(Item *item_par) :Cached_item_item(item_par),value(0) {} > bool cmp(void); > int cmp_read_only(); Space after ). Mark this function as const. > + longlong get_value(){ return value;} > + void clear() > + { Space after = > + value=0.0; > + null_value= false; > + } > }; > > > @@ -5255,6 +5281,12 @@ class Cached_item_decimal :public Cached_item_item > Cached_item_decimal(Item *item_par); > bool cmp(void); > int cmp_read_only(); Space after ) Mark this function as const. > + my_decimal *get_value(){ return &value;}; > + void clear() > + { > + null_value= false; > + my_decimal_set_zero(&value); > + } > }; > > class Cached_item_field :public Cached_item > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc > index 6ab903a81bb..b711642c52c 100644 > --- a/sql/item_windowfunc.cc > +++ b/sql/item_windowfunc.cc > @@ -93,6 +93,17 @@ Item_window_func::fix_fields(THD *thd, Item **ref) > my_error(ER_NO_ORDER_LIST_IN_WINDOW_SPEC, MYF(0), window_func()->func_name()); > return true; > } > + > + if (only_single_element_order_list()) > + { Mark such comments as TODO, like so: TODO(varunt): <message> > + // need to change the error, the error should say that we have more than one element in the order list > + if (window_spec->order_list->elements != 1) > + { > + my_error(ER_NO_ORDER_LIST_IN_WINDOW_SPEC, MYF(0), window_func()->func_name()); > + return true; > + } > + } > + > /* > TODO: why the last parameter is 'ref' in this call? What if window_func > decides to substitute itself for something else and does *ref=.... ? > @@ -179,6 +190,16 @@ void Item_sum_dense_rank::setup_window_func(THD *thd, Window_spec *window_spec) > clear(); > } > > I don't see any added value for this extra setup_percentile_func, when it's only I guess you can just put the code in directly. > +void Item_sum_percentile_disc::setup_window_func(THD *thd, Window_spec *window_spec) > +{ > + setup_percentile_func(thd, window_spec->order_list); > +} > + This function does nothing. get_handler_by_cmp_type returns an item and then you don't do anything with it. > +void Item_sum_percentile_disc::set_type_handler(Window_spec *window_spec) > +{ > + type_handler()->get_handler_by_cmp_type(window_spec->order_list->first->item[0]->result_type()); > +} > + > bool Item_sum_dense_rank::add() > { > if (peer_tracker->check_if_next_group() || first_add) > diff --git a/sql/item_windowfunc.h b/sql/item_windowfunc.h > index 77cbd556e60..5179d1832e8 100644 > --- a/sql/item_windowfunc.h > +++ b/sql/item_windowfunc.h > @@ -16,10 +17,13 @@ class Group_bound_tracker > > Group_bound_tracker(THD *thd, SQL_I_List<ORDER> *list) > { As discussed on slack, lets keep this Group_bound_tracker as is. Better yet, add a DBUG_ASSERT(list). > - for (ORDER *curr = list->first; curr; curr=curr->next) > + if (list) > { > - Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); > - group_fields.push_back(tmp); > + for (ORDER *curr = list->first; curr; curr=curr->next) > + { > + Cached_item *tmp= new_Cached_item(thd, curr->item[0], TRUE); > + group_fields.push_back(tmp); > + } > } > } > > @@ -71,6 +75,19 @@ class Group_bound_tracker > return 0; > } > Add a comment as to what this function does. I don't quite like how this function works. cmp has a side effect of overwriting the cached item with the compared value. In addition this function stops if we encounter a null value and the group fields are now partially updated. I assume this is only for a single element order list. Why not make something specialised for that and not use this cached_item construct. > + bool compare_with_cache_for_null_values() > + { > + List_iterator<Cached_item> li(group_fields); > + Cached_item *ptr; > + while ((ptr= li++)) > + { > + ptr->cmp(); > + if (ptr->null_value) > + return true; > + } > + return false; > + } > + > private: > List<Cached_item> group_fields; > /* > @@ -678,6 +704,138 @@ class Item_sum_ntile : public Item_sum_window_with_row_count > ulong current_row_count_; > }; > > +class Item_sum_percentile_disc : public Item_sum_cume_dist, > + public Type_handler_hybrid_field_type > +{ > +public: > + Item_sum_percentile_disc(THD *thd, Item* arg) : Item_sum_cume_dist(thd, arg), > + Type_handler_hybrid_field_type(&type_handler_longlong), > + value(NULL), result_value(NULL), val_calculated(FALSE) {} > + > + double val_real() > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; Look at Item_sum_hybrid_simple. See how Item_cache * works there. Make use of that instead of Cached_item_xxx. > + return ((Cached_item_real*) result_value)->get_value(); > + } > + > + longlong val_int() > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; Same comment as for val_real() > + return ((Cached_item_int*) result_value)->get_value(); > + } > + > + my_decimal* val_decimal(my_decimal* dec) > + { > + if (get_row_count() == 0 || get_arg(0)->is_null()) > + { > + null_value= true; > + return 0; > + } > + null_value= false; Same comment as for val_decimal() > + return ((Cached_item_decimal*) result_value)->get_value(); > + } > + Lets try implementing this with Item_cache, instead of cached_item. Cached_item is designed for comparison, while Item_cache is used for storing something just to cache it. > + bool add() > + { > + Item *arg = get_arg(0); > + if (arg->is_null()) > + return true; > + /* > + need to ensure that the Item arg is constant across the entire partition > + and its value ranges between [0,1] > + */ > + value->cmp(); > + > + /* for the null values of the row, we dont count take those rows in account for calculating > + the CUME_DIST */ > + > + if(value->null_value) > + return false; > + > + Item_sum_cume_dist::add(); > + double val1= Item_sum_cume_dist::val_real(); > + /* need to check type and return value accordingly*/ > + double val2 =arg->val_real_from_decimal(); > + > + /* use Cached_item to do the comparision using cmp_read_only() */ > + > + if( val1 >= val2 && !val_calculated) > + { > + val_calculated= true; > + result_value->cmp(); > + return false; > + } > + return false; > + } > + > + enum Sumfunctype sum_func() const > + { > + return PERCENTILE_DISC_FUNC; > + } > + > + void clear() > + { > + val_calculated= false; > + value->clear(); > + result_value->clear(); > + Item_sum_cume_dist::clear(); > + } > + > + const char*func_name() const > + { > + return "percentile_disc"; > + } > + > + void update_field() {} > + void set_type_handler(Window_spec *window_spec); > + const Type_handler *type_handler() const > + {return Type_handler_hybrid_field_type::type_handler();} > + > + void fix_length_and_dec() > + { > + decimals = 10; // TODO-cvicentiu find out how many decimals the standard > + // requires. > + } > + > + Item *get_copy(THD *thd, MEM_ROOT *mem_root) > + { return get_item_copy<Item_sum_percentile_disc>(thd, mem_root, this); } > + void setup_window_func(THD *thd, Window_spec *window_spec); > + void setup_percentile_func(THD *thd, SQL_I_List<ORDER> *list) > + { > + value= new_Cached_item(thd, list->first->item[0], FALSE); > + result_value= new_Cached_item(thd, list->first->item[0], FALSE); > + } > + void cleanup() > + { > + if (value) > + { > + delete value; > + value= NULL; > + } > + if(result_value) > + { > + delete result_value; > + result_value= NULL; > + } > + Item_sum_num::cleanup(); > + } > + > +private: > + Cached_item *value; > + Cached_item *result_value; > + bool val_calculated; > +}; > + > > class Item_window_func : public Item_func_or_sum > { > diff --git a/sql/sql_window.cc b/sql/sql_window.cc > index 74e1ad25183..9fda4520a05 100644 > --- a/sql/sql_window.cc > +++ b/sql/sql_window.cc > @@ -868,9 +873,39 @@ class Partition_read_cursor : public Table_read_cursor > } > return 0; > } This is not something we want in Partition_read_cursor. Move the contents of it to the most derived class that needs it. > + bool next_func(ha_rows *counter) > + { > + if (next()) > + return true; > + if (!check_for_null_row()) > + { > + (*counter)++; > + } > + return false; > + } This is also not something we want in Partition_read_cursor. > + bool fetch_func(ha_rows *counter) > + { > + if (fetch()) > + return true; > + if (!check_for_null_row()) > + { > + (*counter)++; > + } > + return false; > + } Null row here has no meaning. Partition_read_cursor looks at the bound_tracker for partitions. What "row" are we talking about here. Then again, this logic can probably go into our "special" frame cursor below. > + bool check_for_null_row() > + { > + if (!end_of_partition) > + { Check compare_with_cache_for_null_values comment. > + if (order_tracker.compare_with_cache_for_null_values()) > + return true; > + } > + return false; > + } > > private: > Group_bound_tracker bound_tracker; Not something we want in Partition_read_cursor. If you need an order_tracker define a different class. > + Group_bound_tracker order_tracker; > bool end_of_partition; > }; > > @@ -1102,7 +1137,7 @@ class Frame_range_n_top : public Frame_cursor > SQL_I_List<ORDER> *partition_list, > SQL_I_List<ORDER> *order_list, > bool is_preceding_arg, Item *n_val_arg) : So this gets reverted as discussed on slack. > - cursor(thd, partition_list), n_val(n_val_arg), item_add(NULL), > + cursor(thd, partition_list, NULL), n_val(n_val_arg), item_add(NULL), > is_preceding(is_preceding_arg) > { > DBUG_ASSERT(order_list->elements == 1); > @@ -1241,7 +1276,7 @@ class Frame_range_n_bottom: public Frame_cursor > SQL_I_List<ORDER> *partition_list, > SQL_I_List<ORDER> *order_list, > bool is_preceding_arg, Item *n_val_arg) : So this gets reverted as discussed on slack. > - cursor(thd, partition_list), n_val(n_val_arg), item_add(NULL), > + cursor(thd, partition_list, NULL), n_val(n_val_arg), item_add(NULL), > is_preceding(is_preceding_arg), added_values(false) > { > DBUG_ASSERT(order_list->elements == 1); > @@ -1371,7 +1406,7 @@ class Frame_range_current_row_bottom: public Frame_cursor > Frame_range_current_row_bottom(THD *thd, > SQL_I_List<ORDER> *partition_list, > SQL_I_List<ORDER> *order_list) : So this gets reverted as discussed on slack. > - cursor(thd, partition_list), peer_tracker(thd, order_list) > + cursor(thd, partition_list, NULL), peer_tracker(thd, order_list) > { > } > > @@ -1658,6 +1693,35 @@ class Frame_unbounded_following_set_count : public Frame_unbounded_following > } > }; > As discussed on slack, this needs to be renamed to something that makes sense. > +class Frame_unbounded_following_set_count_special : public Frame_unbounded_following_set_count > +{ > +public: > + Frame_unbounded_following_set_count_special( > + THD *thd, > + SQL_I_List<ORDER> *partition_list, SQL_I_List<ORDER> *order_list) : > + Frame_unbounded_following_set_count(thd, partition_list, order_list) > + {} > + next_partition can have the logic of fetch_func directly in it. > + void next_partition(ha_rows rownum) > + { > + ha_rows num_rows_in_partition= 0; > + if (cursor.fetch_func(&num_rows_in_partition)) > + return; > + Dangerous to have a while loop like this. Lets put next_func code here directly. > + /* Walk to the end of the partition, find how many rows there are. */ > + while (!cursor.next_func(&num_rows_in_partition)); > + > + List_iterator_fast<Item_sum> it(sum_functions); > + Item_sum* item; > + while ((item= it++)) > + { > + Item_sum_window_with_row_count* item_with_row_count = > + static_cast<Item_sum_window_with_row_count *>(item); > + item_with_row_count->set_row_count(num_rows_in_partition); > + } > + } > +}; > + > ///////////////////////////////////////////////////////////////////////////// > // ROWS-type frame bounds > ///////////////////////////////////////////////////////////////////////////// > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index a0bbf39b138..6c64477f08d 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -10671,6 +10676,45 @@ simple_window_func: > } > ; > > +inverse_distribution_function: > + inverse_distribution_function_def WITHIN GROUP_SYM > + '(' > + { Select->prepare_add_window_spec(thd); } > + order_by_single_element_list ')' OVER_SYM > + '(' opt_window_ref opt_window_partition_clause ')' > + { > + LEX *lex= Lex; > + if (Select->add_window_spec(thd, lex->win_ref, > + Select->group_list, > + Select->order_list, > + NULL)) > + MYSQL_YYABORT; > + $$= new (thd->mem_root) Item_window_func(thd, (Item_sum *) $1, > + thd->lex->win_spec); > + if ($$ == NULL) > + MYSQL_YYABORT; > + if (Select->add_window_func((Item_window_func *) $$)) > + MYSQL_YYABORT; > + } > + ; > + > +inverse_distribution_function_def: > + PERCENTILE_CONT_SYM '(' expr ')' Indentation for { and } is not correct here. Please check this file and do the same. > + { mark it as TODO(varun): > + //Not yet started implementing > + } > + | PERCENTILE_DISC_SYM '(' expr ')' Same, check indentation for { and } > + { > + $$= new (thd->mem_root) Item_sum_percentile_disc(thd, $3); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + ; > + Indentation check! > +order_by_single_element_list: > + ORDER_SYM BY order_list > + ; > + > window_name: > ident > {

1 0

[Maria-developers] MDEV-13240 Wrong warning with MAX(datetime_field) OVER (...)
by Alexander Barkov 04 Jul '17

04 Jul '17

Hello Vicentiu, can you please review a patch for MDEV-13240? The patch is for bb-10.2-ext / 10.3. Thanks.

1 0

Re: [Maria-developers] [Commits] 2d6649aa24c: MDEV-13186: main.win failure post MDEV-12336
by Oleksandr Byelkin 04 Jul '17

04 Jul '17

Hi, Vicentiu! Am 27.06.2017 um 11:31 schrieb vicentiu: > revision-id: 2d6649aa24c77321df349de5cee576dab1e344f4 (mariadb-10.2.6-55-g2d6649aa24c) > parent(s): c036d5ada7a5816d25915a2c6e0b6836b0cd6743 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-06-27 12:26:38 +0300 > message: > > MDEV-13186: main.win failure post MDEV-12336 > > During statement preparation st_order::item gets set to a value in > ref_ptr_array. During statement execution we were overriding that value, > causing subsequent checks for window functions to return true. > > Whenever we do any setting from ref_ptr_array, make sure to always > store the value in all_fields as well. > > For function items containing window functions, as MDEV-12336 has > discovered, we don't need to create a separate Item_direct_ref or > Item_aggregate_ref as they will be computed directly from the top-level > item once the window function argument columns are computed. OK to push but add comment before IF with return explaining it. > > --- > mysql-test/r/win.result | 12 ++++++++++++ > mysql-test/t/win.test | 10 ++++++++++ > sql/item.cc | 6 +++--- > 3 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result > index ec83998aa98..a67c336e5b4 100644 > --- a/mysql-test/r/win.result > +++ b/mysql-test/r/win.result > @@ -3085,3 +3085,15 @@ select max(id), rank() over (order by max(id)) from t1 where id < 3; > max(id) rank() over (order by max(id)) > 2 1 > drop table t1; > +# > +# main.win failure post MDEV-12336 > +# > +create table t(a decimal(35,10), b int); > +insert into t values (1, 10), (2, 20), (3, 30); > +prepare stmt from "SELECT (CASE WHEN sum(t.a) over (partition by t.b)=1 THEN 1000 ELSE 300 END) AS a FROM t"; > +execute stmt; > +a > +1000 > +300 > +300 > +drop table t; > diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test > index 95d32c5bd14..0ecb2b35074 100644 > --- a/mysql-test/t/win.test > +++ b/mysql-test/t/win.test > @@ -1877,3 +1877,13 @@ select count(max(id)) over (order by max(id)) from t1 where id < 3; > select max(id), rank() over (order by max(id)) from t1 where id < 3; > > drop table t1; > + > +--echo # > +--echo # main.win failure post MDEV-12336 > +--echo # > +create table t(a decimal(35,10), b int); > +insert into t values (1, 10), (2, 20), (3, 30); > + > +prepare stmt from "SELECT (CASE WHEN sum(t.a) over (partition by t.b)=1 THEN 1000 ELSE 300 END) AS a FROM t"; > +execute stmt; > +drop table t; > diff --git a/sql/item.cc b/sql/item.cc > index df615b5ace9..05e5fd1ce45 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -1920,6 +1920,9 @@ void Item::split_sum_func2(THD *thd, Ref_ptr_array ref_pointer_array, > point to the temporary table. > */ > split_sum_func(thd, ref_pointer_array, fields, split_flags); > + if (type() == FUNC_ITEM) { > + return; > + } > } > else > { > @@ -1979,9 +1982,6 @@ void Item::split_sum_func2(THD *thd, Ref_ptr_array ref_pointer_array, > &ref_pointer_array[el], 0, name)))) > return; // fatal_error is set > } > - else if (type() == FUNC_ITEM && > - ((Item_func *) this)->with_window_func) > - return; > else > { > if (!(item_ref= (new (thd->mem_root) > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] Project Progress Report[PHASE-1]
by ANKIT KUMAR 30 Jun '17

30 Jun '17

Hi, I have newly started contributing to opensource projects, so these 3.5 weeks have been really awesome from learning point of view. Everyday I learnt something new, kept experimenting various approaches to a particular problem. I have come a long way as I am about to finish 2 subtasks among 4(4th being optional as it already does what it's expected to). Whole MariaDB community has been really helpful, and special thanks to my Mentor without whom support it wouldn't have been possible. Just recollecting everything I did in this phase- 1. Learnt how bison works, how grammar is written, how actions can be written(w.r.t to extended syntax), how parsing is done, 2. Implemented Extended CREATE USER and GRANT USER syntax. 3. Created tests for the acceptance of newly implemented grammar. 4. Designed in-memory structures that will store value of plugins and perform AND/OR operations as per precedence and authentication will be provided accordingly. 5. Wrote actions for this complex syntax(fixed some of the errors, some are yet to be fixed). In the remaining 3-4 days of Phase-1, I will try to complete this bridge between grammar and in-memory structure after which storing data in memory will be done. Now coming on to phase-2 deliverables, I am planning to finish off with Storage on Disk. We have found a simplest solution to get this done(probably in a week or 2), and in the remaining days, we will check the working of this new feature. I will then migrate this code to 10.3 branch. (Phase-3 for unexpected delay) This project involve lots of thinking than actual coding, so I think I have maintained good pace throughout. I believe I will maintain this enthusiasm and confidence level in upcoming phases too. Hopefully, I will get a chance to continue with this project. Kind regards, Ankit Kumar

2 2

[Maria-developers] Fwd: ecbad0a07a8: MDEV-13189: Window functions crash when using INTERVAL
by Vicențiu Ciorbaru 28 Jun '17

28 Jun '17

Hi Sergey, Can you please review this change that makes window functions work with INTERVAL function? Thanks, Vicentiu ---------- Forwarded message --------- From: vicentiu <vicentiu(a)mariadb.org> Date: Wed, 28 Jun 2017 at 21:08 Subject: ecbad0a07a8: MDEV-13189: Window functions crash when using INTERVAL To: <commits(a)mariadb.org> revision-id: ecbad0a07a848c1f9f474c4c749c87b6bfe7986c (mariadb-10.2.6-74-gecbad0a07a8) parent(s): 31ba0fa48d27715e82258b1e74401093e0ee17a2 author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-06-28 21:07:55 +0300 message: MDEV-13189: Window functions crash when using INTERVAL Interval function makes use of Item_row. Item_row did not correctly mark with_window_func flag according to its arguments. Fix it by making Item_row aware of this flag. --- mysql-test/r/win.result | 14 ++++++++++++++ mysql-test/t/win.test | 9 +++++++++ sql/item_row.cc | 1 + 3 files changed, 24 insertions(+) diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result index 952bd82ac7f..7f8777ed491 100644 --- a/mysql-test/r/win.result +++ b/mysql-test/r/win.result @@ -3127,3 +3127,17 @@ NULL 1 0 1 0 1 2 0 1 drop table t1; +# +# MDEV-13189: Window functions crash when using INTERVAL function +# +create table t1(i int); +insert into t1 values (1),(2),(10),(20),(30); +select sum(i) over (order by i), interval(sum(i) over (order by i), 10, 20) +from t1; +sum(i) over (order by i) interval(sum(i) over (order by i), 10, 20) +1 0 +3 0 +13 1 +33 2 +63 2 +drop table t1; diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test index 45653b18682..aff717e3151 100644 --- a/mysql-test/t/win.test +++ b/mysql-test/t/win.test @@ -1914,3 +1914,12 @@ select max(i) over (order by i), max(i) over (order by i) is not null from t1; drop table t1; + +--echo # +--echo # MDEV-13189: Window functions crash when using INTERVAL function +--echo # +create table t1(i int); +insert into t1 values (1),(2),(10),(20),(30); +select sum(i) over (order by i), interval(sum(i) over (order by i), 10, 20) +from t1; +drop table t1; diff --git a/sql/item_row.cc b/sql/item_row.cc index ddbb0736d54..fc484f560ee 100644 --- a/sql/item_row.cc +++ b/sql/item_row.cc @@ -63,6 +63,7 @@ bool Item_row::fix_fields(THD *thd, Item **ref) } maybe_null|= item->maybe_null; with_sum_func= with_sum_func || item->with_sum_func; + with_window_func = with_window_func || item->with_window_func; with_field= with_field || item->with_field; with_subselect|= item->with_subselect; }

1 0

[Maria-developers] MDEV-13197 Parser refactoring for CREATE VIEW, TRIGGER, SP, UDF, EVENT
by Alexander Barkov 28 Jun '17

28 Jun '17

Hello Sanja, Can you please review a patch for MDEV-13197? It's a dependency change for MDEV-10591 Oracle-style packages, which I'm currently working on. The patch is for bb-10.2-ext. Thanks.

1 0

[Maria-developers] MDEV-12179: Per-engine mysql.gtid_slave_pos: auto-configuring/packaging
by Sergey Petrunia 26 Jun '17

26 Jun '17

Hello, Suppose there is a transactional storage engine that is shipped as a loadable module. The examples are MyRocks and TokuDB. I think it the default behavior for such engine after MDEV-12179 should be that the engine is listed in @@gtid_pos_auto_engines. That is, as soon as a transaction that uses the storage engine is replicated, mysql.slave_gtid_pos{_$engine} is created for it. gtid_pos_auto_engines makes it easy to do this manually, but it's difficult to achieve as default behavior. Here's an explanation why. Loadable storage engines are put into their separate packages, for example mariadb-plugin-rocksdb_10.2.7+maria-zesty_amd64.deb. The two important files in the package are : ./etc/mysql/mariadb.conf.d/rocksdb.cnf ./usr/lib/mysql/plugin/ha_rocksdb.so The first one is a .cnf that gets included into the server's cnf file. It would be nice to put there something like SET global gtid_pos_auto_engines= CONCAT(@@gtid_pos_auto_engines, 'rocksdb'); but that is not possible (it's a .cnf not .sql) Possible solutions: == [Ab]use the Storage Engine API == Make the storage engine put itself in @@gtid_pos_auto_engines. I think this is technically possible, but - It's a storage engine API abuse - how does one explain @@gtid_pos_auto_engines to the users? == Let gtid_pos_auto_engines include unknown engine == - Allow @@gtid_pos_auto_engines to list unknown engines - set the default to include all known transactional storage engines, that is it should be : gtid_pos_auto_engines='InnoDB,TokuDB,RocksDB' // (*) and the server should allow this even if it is built without TokuDB or RocksDB (*) -- Aria is not needed as it is not a "true" transactional storage engine as far as interplay with the binlog is concerned. == Let packaging use some other solution == I suppose it's possible for packaging system to run some script after a package has been installed. that script might find/adjust my.cnf file to have the desired @@gtid_pos_auto_engines setting. This solution might be complex and fragile, though. Any thoughts ? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

[Maria-developers] exact commands to produce similar mariadb release tarball
by Phil Sweeney 26 Jun '17

26 Jun '17

Hi there, I'm trying to do some custom builds of MariaDB for myself, but want to emulate the release builds as much as possible. I'm doing this on Ubuntu 16.04 LTS and following steps here: https://mariadb.com/kb/en/mariadb/creating-the-mariadb-binary-tarball/ However, my tarball ends up being 227MB compared to the downloads that are ~500MB, plus it won't start because it's missing jemalloc. Where can I find info about the environment and the commandline used to build the official releases e.g. mariadb-10.1.24-linux-glibc_214-x86_64.tar.gz (535.6MB) Thanks! Phil

2 2

Re: [Maria-developers] [Commits] ee73b33: MDEV-8840: ANALYZE FORMAT=JSON produces wrong data with BKA
by Sergey Petrunia 25 Jun '17

25 Jun '17

Hi Varun, On Sat, Jun 17, 2017 at 05:35:41PM +0530, Varun wrote: > revision-id: ee73b331d97d33ae7aa1403a4ac7312f2d261754 (mariadb-10.1.20-323-gee73b33) > parent(s): 056bab0880544d91ea67d18fe8db65b4f6625482 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-06-17 17:34:07 +0530 > message: > > MDEV-8840: ANALYZE FORMAT=JSON produces wrong data with BKA > > The issue was that r_loops, r_rows and r_filtered in ANALYZE FORMAT= JSON were not > calculated for the table on which we were performing the MRR scan in the BKA join > Fixed this by adding respective counter in the JOIN_CACHE_MRR::open and JOIN_CACHE::next > So I apply the patch and run this example (the dataset is from explain_json.test, also pasted below): MariaDB [j5]> analyze format=json select * from t3,t4 where t3.a=t4.a and (t4.c+1 < t3.b+1)\G *************************** 1. row *************************** ANALYZE: { "query_block": { "select_id": 1, "r_loops": 1, "r_total_time_ms": 0.8859, "table": { "table_name": "t3", "access_type": "ALL", "r_loops": 1, "rows": 10, "r_rows": 10, "r_total_time_ms": 0.0202, "filtered": 100, "r_filtered": 100, "attached_condition": "t3.a is not null" }, "block-nl-join": { "table": { "table_name": "t4", "access_type": "ref", "possible_keys": ["a"], "key": "a", "key_length": "5", "used_key_parts": ["a"], "ref": ["j5.t3.a"], "r_loops": 1, "rows": 1, "r_rows": 11, "r_total_time_ms": 0.0379, "filtered": 100, "r_filtered": 90.909 }, "buffer_type": "flat", "buffer_size": "256Kb", "join_type": "BKA", "mrr_type": "Rowid-ordered scan", "attached_condition": "t4.c + 1 < t3.b + 1", "r_filtered": 0 } } } I think, these value are wrong: > "r_rows": 11, > "r_filtered": 90.909 r_rows is actually 10, r_filtered=100 (no attached condition) I guess wrong r_filtered is caused by r_rows being wrong. ## Dataset: set storage_engine=myisam; set default_storage_engine=myisam; create table t1(a int); insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); create table t2(a int); insert into t2 select A.a + B.a* 10 + C.a * 100 from t1 A, t1 B, t1 C; create table t3(a int, b int); insert into t3 select a,a from t1; create table t4(a int, b int, c int, filler char(100), key (a,b)); insert into t4 select a,a,a, 'filler-data' from t2; set optimizer_switch='mrr=on'; set join_cache_level=6; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 62021f3: MDEV-12070 - Introduce thd_query_safe() from MySQL 5.7
by Sergei Golubchik 23 Jun '17

23 Jun '17

Hi, Sergey! On Jun 22, Sergey Vojtovich wrote: > revision-id: 62021f391a42c5577190aa43cb8ad91e56235b46 (mariadb-10.2.6-57-g62021f3) > parent(s): 557e1bd472612848a42e772c1fb6f8ed32ab33b4 > committer: Sergey Vojtovich > timestamp: 2017-06-22 17:15:10 +0400 > message: > > MDEV-12070 - Introduce thd_query_safe() from MySQL 5.7 > > Merged relevant part of MySQL revision: > https://github.com/mysql/mysql-server/commit/565d20b44f24fcc855dc616164d87b… > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index f8cf829..db65eb3 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -4529,6 +4529,32 @@ extern "C" LEX_STRING * thd_query_string (MYSQL_THD thd) > return(&thd->query_string.string); > } > > + > +/** > + Get the current query string for the thread. > + > + @param thd The MySQL internal thread pointer > + @param buf Buffer where the query string will be copied > + @param buflen Length of the buffer > + > + @return Length of the query > + > + @note This function is thread safe as the query string is > + accessed under mutex protection and the string is copied > + into the provided buffer. @see thd_query_string(). > +*/ > + > +extern "C" size_t thd_query_safe(MYSQL_THD thd, char *buf, size_t buflen) > +{ > + mysql_mutex_lock(&thd->LOCK_thd_data); > + size_t len= MY_MIN(buflen - 1, thd->query_length()); > + memcpy(buf, thd->query(), len); > + mysql_mutex_unlock(&thd->LOCK_thd_data); > + buf[len]= '\0'; > + return len; > +} Okay. But as neither thd_query_string() nor thd_query() are part of the plugin API, I'd suggest to remove them, they're inherently unsafe and should not be used. Few other engines use thd_query_string() need to be fixed too (but they bypass the plugin API and we don't promise stability for internal functions). Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] [Commits] 922f970: MDEV-12882 - Assertion failed in MDL_context::upgrade_shared_lock
by Sergei Golubchik 23 Jun '17

23 Jun '17

Hi, Sergey! On Jun 22, Sergey Vojtovich wrote: > revision-id: 922f9700a7fb084e3f7d6cff56e27e24fc603ed7 (mariadb-10.2.6-58-g922f970) > parent(s): 62021f391a42c5577190aa43cb8ad91e56235b46 > committer: Sergey Vojtovich > timestamp: 2017-06-22 17:35:36 +0400 > message: > > MDEV-12882 - Assertion failed in MDL_context::upgrade_shared_lock > > Relaxed assertion (in MySQL it was removed). > For "LOCK TABLES t1 WRITE CONCURRENT, t1 READ" upgrade lock to weakest > existing suitable lock, which is MDL_SHARED_NO_READ_WRITE. > > diff --git a/mysql-test/t/mdl.test b/mysql-test/t/mdl.test > new file mode 100644 > index 0000000..5a74ae5 > --- /dev/null > +++ b/mysql-test/t/mdl.test > @@ -0,0 +1,15 @@ Hm. I'd totally expect main.mdl test to exist for years :) > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 412cd1d..7c910e8 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2787,6 +2787,7 @@ static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) > ! table->prelocking_placeholder && > table->table->file->lock_count() == 0) > { > + enum enum_mdl_type lock_type; > /* > In case when LOCK TABLE ... READ LOCAL was issued for table with > storage engine which doesn't support READ LOCAL option and doesn't > @@ -2799,9 +2800,12 @@ static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) > deadlock_handler.init(); > thd->push_internal_handler(&deadlock_handler); > > + lock_type= table->table->mdl_ticket->get_type() == MDL_SHARED_WRITE ? > + MDL_SHARED_NO_READ_WRITE : MDL_SHARED_READ_ONLY; > + > bool result= thd->mdl_context.upgrade_shared_lock( > table->table->mdl_ticket, > - MDL_SHARED_READ_ONLY, > + lock_type, > thd->variables.lock_wait_timeout); This only works if you lock WRITE, then READ. If you do it the other way around, you'll get an error. Isn't it inconsistent? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] Condition pushdown into derived tables and outer joins.
by Sergey Petrunia 23 Jun '17

23 Jun '17

Hello Igor, While reviewing MDEV-12845, I've found this piece in sql_select.cc: while ((tbl= li++)) { /* Do not push conditions from where into materialized inner tables of outer joins: this is not valid. */ if (tbl->is_materialized_derived()) { /* Do not push conditions from where into materialized inner tables of outer joins: this is not valid. */ if (!tbl->is_inner_table_of_outer_join()) Q1: Is int meaningful to have two identical comments? Q2: It is not valid push parts of WHERE, but it is valid to push the parts of ON expression. An example: explain format=json select * from t21 LEFT JOIN (select a,b, COUNT(*) as CNT from t22 group by a,b) TBL ON (t21.a=TBL.a AND TBL.a<5); here, "TBL.a<5" can be pushed down into the subquery, but it is not. EXPLAIN EXTENDED confirms it: "query_block": { "select_id": 1, "const_condition": "1", "table": { "table_name": "t21", "access_type": "ALL", "rows": 10, "filtered": 100 }, "table": { "table_name": "<derived2>", "access_type": "ref", "possible_keys": ["key0"], "key": "key0", "key_length": "5", "used_key_parts": ["a"], "ref": ["test.t21.a"], "rows": 2, "filtered": 100, "attached_condition": "trigcond(trigcond(t21.a < 5 and t21.a is not null))", "materialized": { "query_block": { "select_id": 2, "filesort": { "sort_key": "t22.a, t22.b", "temporary_table": { "table": { "table_name": "t22", "access_type": "ALL", "rows": 10, "filtered": 100 } } } } } } Just in case, the commands to create the dataset were: create table ten(a int); insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); create table t21 (a int, b int, c int); insert into t21 select a,a,a from ten; create table t22 (a int, b int, c int); insert into t22 select a,a,a from ten; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Project Progress Report [Week 3]
by ANKIT KUMAR 20 Jun '17

20 Jun '17

Hi, In this week, we worked on in-memory structures:- Which representation to choose(prefix or expression tree), How to store values and how authentication will be done. I have prepared pseudocode to implement this subtask and have made implemented some part of code. Hopefully by 25th May, I will finish off with >50% of the project. I will declare Phase-2 deliverables next Monday(26 May). Kind regards, Ankit Kumar

2 1

Re: [Maria-developers] MDEV-12620 - set lock_wait_timeout = 1; flush tables with read lock;
by Sergei Golubchik 16 Jun '17

16 Jun '17

Hi, Sergey! On Jun 16, Sergey Vojtovich wrote: > revision-id: 056bab0880544d91ea67d18fe8db65b4f6625482 (mariadb-10.1.24-22-g056bab0) > parent(s): 58f87a41bd8de7370cc05c41977fadc685826c9e > committer: Sergey Vojtovich > timestamp: 2017-06-16 15:47:46 +0400 > message: > > MDEV-12620 - set lock_wait_timeout = 1;flush tables with read lock; > lock not released after timeout > > Release GRL if FLUSH TABLES phase failed. > > diff --git a/mysql-test/t/mdl_sync.test b/mysql-test/t/mdl_sync.test > index 8e80978..c479c2a 100644 > --- a/mysql-test/t/mdl_sync.test > +++ b/mysql-test/t/mdl_sync.test > @@ -4806,6 +4806,30 @@ disconnect con2; > disconnect con3; > > +--echo # > +--echo # MDEV-12620 - set lock_wait_timeout = 1;flush tables with read lock; > +--echo # lock not released after timeout > +--echo # > +CREATE TABLE t1(a INT) ENGINE=InnoDB; > +SET debug_sync='open_tables_after_open_and_process_table SIGNAL ready WAIT_FOR go'; > +send SELECT * FROM t1; > + > +connect (con1,localhost,root,,); > +SET debug_sync='now WAIT_FOR ready'; > +# lock_wait_timeout should be 0 in 10.3, so that we don't have to wait at all what's special in 10.3? > +SET lock_wait_timeout=1; > +--error ER_LOCK_WAIT_TIMEOUT > +FLUSH TABLES WITH READ LOCK; > +SET debug_sync='now SIGNAL go'; > + > +connection default; > +reap; > +SET debug_sync='RESET'; > +DROP TABLE t1; What should happen without a bug fix? DROP TABLE hangs? > + > +disconnect con1; > + > # Check that all connections opened by test cases in this file are really > # gone so execution of other tests won't be affected by their presence. > --source include/wait_until_count_sessions.inc > diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc > index 376dfeb..d68ce96 100644 > --- a/sql/sql_reload.cc > +++ b/sql/sql_reload.cc > @@ -253,7 +253,8 @@ bool reload_acl_and_cache(THD *thd, unsigned long long options, > NOTE: my_error() has been already called by reopen_tables() within > close_cached_tables(). > */ > - result= 1; > + thd->global_read_lock.unlock_global_read_lock(thd); > + return 1; > } Sure. Looks correct. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] MDEV-10306 Wrong results with combination of CONCAT, SUBSTR and CONVERT in subquery
by Alexander Barkov 16 Jun '17

16 Jun '17

Hello Sergei, Please review a fix for MDEV-10306. Thanks!

4 13

Re: [Maria-developers] [Commits] 01b2bb79bb78e42108e32af02f1418a660ca861d Fixed the bug mdev-13064.
by Oleksandr Byelkin 16 Jun '17

16 Jun '17

Hi, Igor! Am 15.06.2017 um 23:43 schrieb Igor Babaev: > commit 01b2bb79bb78e42108e32af02f1418a660ca861d > Author: Igor Babaev <igor(a)askmonty.org> > Commit: Igor Babaev <igor(a)askmonty.org> > > Fixed the bug mdev-13064. > > This is another attempt to fix the bug mdev-12992. > This patch introduces st_select_lex::context_analysis_place for > the place in SELECT where context analysis is currently performed. > It's similar to st_select_lex::parsing_place, but it is used at It is OK to push. > the preparation stage. > --- > mysql-test/r/func_group.result | 9 +++++++++ > mysql-test/t/func_group.test | 13 +++++++++++++ > sql/item.cc | 5 ++--- > sql/item_windowfunc.cc | 2 +- > sql/sql_lex.h | 1 + > sql/sql_select.cc | 33 ++++++++++++++++++--------------- > 6 files changed, 44 insertions(+), 19 deletions(-) > > diff --git a/mysql-test/r/func_group.result b/mysql-test/r/func_group.result > index cb97ea2..c4e991e 100644 > --- a/mysql-test/r/func_group.result > +++ b/mysql-test/r/func_group.result > @@ -2447,3 +2447,12 @@ DROP TABLE t1; > # > # End of 10.1 tests > # > +# > +# MDEV-13064: assertion `n < m_size' fails in Item::split_sum_func2() > +# > +create table t1 (i int) engine=MyISAM; > +insert into t1 value (1),(2); > +select count(*)+sleep(0) from t1; > +count(*)+sleep(0) > +2 > +drop table t1; > diff --git a/mysql-test/t/func_group.test b/mysql-test/t/func_group.test > index 1e75099..8bbc9e6 100644 > --- a/mysql-test/t/func_group.test > +++ b/mysql-test/t/func_group.test > @@ -1691,3 +1691,16 @@ DROP TABLE t1; > --echo # > --echo # End of 10.1 tests > --echo # > + > +--echo # > +--echo # MDEV-13064: assertion `n < m_size' fails in Item::split_sum_func2() > +--echo # > + > +create table t1 (i int) engine=MyISAM; > +insert into t1 value (1),(2); > + > +select count(*)+sleep(0) from t1; > + > +drop table t1; > + > + > diff --git a/sql/item.cc b/sql/item.cc > index 8db5cfc..df615b5 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -490,8 +490,7 @@ int Item::save_str_value_in_field(Field *field, String *result) > command => we should check thd->lex->current_select on zero (thd->lex > can be uninitialised) > */ > - if (thd->lex->current_select && > - thd->stmt_arena->is_stmt_prepare_or_first_sp_execute()) > + if (thd->lex->current_select) > { > enum_parsing_place place= > thd->lex->current_select->parsing_place; > @@ -5513,7 +5512,7 @@ bool Item_field::fix_fields(THD *thd, Item **reference) > > SELECT_LEX *select= thd->lex->current_select; > thd->change_item_tree(reference, > - select->parsing_place == IN_GROUP_BY && > + select->context_analysis_place == IN_GROUP_BY && > alias_name_used ? *rf->ref : rf); > > /* > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc > index 59a22c6..27b0072 100644 > --- a/sql/item_windowfunc.cc > +++ b/sql/item_windowfunc.cc > @@ -71,7 +71,7 @@ > { > DBUG_ASSERT(fixed == 0); > > - enum_parsing_place place= thd->lex->current_select->parsing_place; > + enum_parsing_place place= thd->lex->current_select->context_analysis_place; > > if (!(place == SELECT_LIST || place == IN_ORDER_BY)) > { > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index face3bc..0c55ffc 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -855,6 +855,7 @@ class st_select_lex: public st_select_lex_node > /* reserved for exists 2 in */ > uint select_n_reserved; > enum_parsing_place parsing_place; /* where we are parsing expression */ > + enum_parsing_place context_analysis_place; /* where we are in prepare */ > bool with_sum_func; /* sum function indicator */ > > ulong table_join_options; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 4c64cf2..6c56f01 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -648,17 +648,15 @@ void remove_redundant_subquery_clauses(st_select_lex *subq_select_lex) > > thd->lex->allow_sum_func|= (nesting_map)1 << select->nest_level; > > - save_place= thd->lex->current_select->parsing_place; > - thd->lex->current_select->parsing_place= IN_ORDER_BY; > + save_place= thd->lex->current_select->context_analysis_place; > + thd->lex->current_select->context_analysis_place= IN_ORDER_BY; > res= res || setup_order(thd, ref_pointer_array, tables, fields, all_fields, > order); > - thd->lex->current_select->parsing_place= save_place; > - thd->lex->allow_sum_func&= ~((nesting_map)1 << select->nest_level); > - save_place= thd->lex->current_select->parsing_place; > - thd->lex->current_select->parsing_place= IN_GROUP_BY; > + thd->lex->allow_sum_func&= ~((nesting_map)1 << select->nest_level); > + thd->lex->current_select->context_analysis_place= IN_GROUP_BY; > res= res || setup_group(thd, ref_pointer_array, tables, fields, all_fields, > group, hidden_group_fields); > - thd->lex->current_select->parsing_place= save_place; > + thd->lex->current_select->context_analysis_place= save_place; > thd->lex->allow_sum_func|= (nesting_map)1 << select->nest_level; > res= res || setup_windows(thd, ref_pointer_array, tables, fields, all_fields, > win_specs, win_funcs); > @@ -712,6 +710,7 @@ void remove_redundant_subquery_clauses(st_select_lex *subq_select_lex) > if (select_lex->handle_derived(thd->lex, DT_PREPARE)) > DBUG_RETURN(1); > > + thd->lex->current_select->context_analysis_place= NO_MATTER; > thd->lex->current_select->is_item_list_lookup= 1; > /* > If we have already executed SELECT, then it have not sense to prevent > @@ -801,12 +800,13 @@ void remove_redundant_subquery_clauses(st_select_lex *subq_select_lex) > > ref_ptrs= ref_ptr_array_slice(0); > > - enum_parsing_place save_place= thd->lex->current_select->parsing_place; > - thd->lex->current_select->parsing_place= SELECT_LIST; > + enum_parsing_place save_place= > + thd->lex->current_select->context_analysis_place; > + thd->lex->current_select->context_analysis_place= SELECT_LIST; > if (setup_fields(thd, ref_ptrs, fields_list, MARK_COLUMNS_READ, > &all_fields, 1)) > DBUG_RETURN(-1); > - thd->lex->current_select->parsing_place= save_place; > + thd->lex->current_select->context_analysis_place= save_place; > > if (setup_without_group(thd, ref_ptrs, tables_list, > select_lex->leaf_tables, fields_list, > @@ -22235,14 +22235,16 @@ int setup_order(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables, > List<Item> &fields, List<Item> &all_fields, ORDER *order, > bool from_window_spec) > { > - enum_parsing_place parsing_place= thd->lex->current_select->parsing_place; > + enum_parsing_place context_analysis_place= > + thd->lex->current_select->context_analysis_place; > thd->where="order clause"; > for (; order; order=order->next) > { > if (find_order_in_list(thd, ref_pointer_array, tables, order, fields, > all_fields, FALSE, from_window_spec)) > return 1; > - if ((*order->item)->with_window_func && parsing_place != IN_ORDER_BY) > + if ((*order->item)->with_window_func && > + context_analysis_place != IN_ORDER_BY) > { > my_error(ER_WINDOW_FUNCTION_IN_WINDOW_SPEC, MYF(0)); > return 1; > @@ -22284,7 +22286,8 @@ int setup_order(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables, > List<Item> &fields, List<Item> &all_fields, ORDER *order, > bool *hidden_group_fields, bool from_window_spec) > { > - enum_parsing_place parsing_place= thd->lex->current_select->parsing_place; > + enum_parsing_place context_analysis_place= > + thd->lex->current_select->context_analysis_place; > *hidden_group_fields=0; > ORDER *ord; > > @@ -22300,14 +22303,14 @@ int setup_order(THD *thd, Ref_ptr_array ref_pointer_array, TABLE_LIST *tables, > all_fields, TRUE, from_window_spec)) > return 1; > (*ord->item)->marker= UNDEF_POS; /* Mark found */ > - if ((*ord->item)->with_sum_func && parsing_place == IN_GROUP_BY) > + if ((*ord->item)->with_sum_func && context_analysis_place == IN_GROUP_BY) > { > my_error(ER_WRONG_GROUP_FIELD, MYF(0), (*ord->item)->full_name()); > return 1; > } > if ((*ord->item)->with_window_func) > { > - if (parsing_place == IN_GROUP_BY) > + if (context_analysis_place == IN_GROUP_BY) > my_error(ER_WRONG_PLACEMENT_OF_WINDOW_FUNCTION, MYF(0)); > else > my_error(ER_WINDOW_FUNCTION_IN_WINDOW_SPEC, MYF(0)); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] Review for MDEV-12666: CURRENT_ROLE() and DATABASE() does not work in a view
by Alexander Barkov 13 Jun '17

13 Jun '17

Hi Vicentiu, The patch looks fine. Thanks! I have only small comments below: > > [Commits] 974064cde3c: MDEV-12666: CURRENT_ROLE() and DATABASE() does not work in a view > vicentiu vicentiu at mariadb.org > Thu Jun 8 10:11:38 EEST 2017 > > Previous message (by thread): [Commits] 7f2d3c3: MDEV-11084 Select statement with partition selection against MyISAM table opens all partitions. > Next message (by thread): [Commits] d03abc71a49: MDEV-12609: Allow suppression of InnoDB log messages about reserving extents > Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] > > revision-id: 974064cde3c1cd1be9157bc11ce410604807f68b (mariadb-10.0.31-2-g974064cde3c) > parent(s): 54cd5bc703da08be45894495d7f3e2c8117617cd > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-06-08 10:10:07 +0300 > message: > > MDEV-12666: CURRENT_ROLE() and DATABASE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The same applies for the DATABASE function, as it can change as well. > > There is an additional problem with CURRENT_ROLE() when used in a > prepared statement. During prepared statement creation we used to set > the string_value of the function to the current role as well as the > null_value flag. During execution, if CURRENT_ROLE was not null, the > null_value flag was never set to not-null during fix_fields. > > Item_func_current_user however can never be NULL so it did not show this > problem in a view before. At the same time, the CURRENT_USER() can not > be changed between prepared statement execution and creation so the > implementation where the value is stored during fix_fields is > sufficient. > > Note also that DATABASE() function behaves differently during prepared > statements. See bug 25843 for details or commit > 7e0ad09edff587dadc3e9855fc81e1b7de8f2199 > <cut> > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -2344,6 +2344,7 @@ String *Item_func_database::val_str(String *str) > } > else > str->copy(thd->db, thd->db_length, system_charset_info); > + null_value= 0; > return str; > } > > @@ -2378,6 +2379,30 @@ bool Item_func_user::init(const char *user, const char *host) > } > > > +Item *Item_func_sysconst::safe_charset_converter(CHARSET_INFO *tocs) > +{ > + /* > + During view or prepared statement creation, the item should not > + make use of const_charset_converter as it would imply substitution > + with constant items which is not correct. Functions can have different > + values during view creation and view execution based on context. > + > + Return the identical item during view creation and prepare. > + */ > + if (current_thd->lex->context_analysis_only & > + (CONTEXT_ANALYSIS_ONLY_PREPARE | CONTEXT_ANALYSIS_ONLY_VIEW)) > + return this; > + return const_charset_converter(tocs, true, fully_qualified_func_name()); I propose to avoid code duplication and do like this: if (!Item_func_sysconst::const_item()) return this; return return const_charset_converter(tocs, true, fully_qualified_func_name()); > +} > + > +bool Item_func_sysconst::const_item() const > +{ > + if (current_thd->lex->context_analysis_only & > + (CONTEXT_ANALYSIS_ONLY_PREPARE | CONTEXT_ANALYSIS_ONLY_VIEW)) > + return false; > + return true; > +} > + > bool Item_func_user::fix_fields(THD *thd, Item **ref) > { > return (Item_func_sysconst::fix_fields(thd, ref) || > @@ -2403,20 +2428,25 @@ bool Item_func_current_role::fix_fields(THD *thd, Item **ref) > > Security_context *ctx= context->security_ctx > ? context->security_ctx : thd->security_ctx; > - > if (ctx->priv_role[0]) > { > if (str_value.copy(ctx->priv_role, strlen(ctx->priv_role), > system_charset_info)) > return 1; > - > str_value.mark_as_const(); > + null_value= maybe_null= 0; > return 0; > } > null_value= maybe_null= 1; > return 0; > } > > +String* Item_func_current_role::val_str(String * str) > +{ > + DBUG_ASSERT(fixed == 1); > + return null_value ? NULL : &str_value; > +} > + Why did you move val_str() from *.h to *.cc ? The method is just two lines, I'd leave it in *.h <cut>

1 0

[Maria-developers] MDEV-12962 Testing MDEV-10142
by Alexander Barkov 13 Jun '17

13 Jun '17

Hi Andrii, Thanks for looking into this. Please find my comments inline: > > > Below are mostly minor notes for which clarification is needed. > > Conceptual: are we sure that ORACLE mode should invalidate native MariaDB constructs? We need some consistency and in my understanding following should still work in Oracle mode: > ELSE / ELSEIF > % operation (if possible) I'm not sure. Monty, what do you think? > > > 2. MDEV-10343 sql_mode=ORACLE: Providing compatibility for basic SQL data types > > following are valid build-in datatypes declaration in oracle which cause syntax error at the moment (needs to be documented or created another task): > > these probably had to be implemented inside MDEV-10343 > > create or replace table t1(a binary_float); > create or replace table t1(a binary_double); > create or replace table t1(a long raw); > create or replace table t1(a NCLOB); > these need more effort or maybe just must be documented or separate task is needed > create or replace table t1(a varchar(30 byte)); > create or replace table t1(a varchar(30 char)); > create or replace table t1(a timestamp with local time zone); > create or replace table t1(a timestamp with time zone); > create or replace table t1(a interval day); > create or replace table t1(a interval year); > create or replace table t1(a BFILE); > create or replace table t1(a rowid); > create or replace table t1(a urowid(5)); > this should give an error (oracle will return ORA-02017: integer value required ) > MariaDB [test]> create or replace table t1(a number(2.2)); > Query OK, 0 rows affected (0.24 sec) There is a task for TIMESTAMP WITH TIME ZONE: MDEV-10018 Timestamp with time zone For other data types, should we create one task for every data type? > > > 3. MDEV-10411 Providing compatibility for basic PL/SQL constructs > 3.6 Assignment operator > It doesn't sound intuitive that one may write "wait_timeout:=a", but "a:=wait_timeout" shows syntax error. > > begin declare a int; begin a:=wait_timeout; end; end;; One needs to use @ when referring a system variable in the right side: a:=@wait_timeout; It's the same in the default mode: SET wait_timeount=100; but: SET xxx=@wait_timeout; Not sure if we should do anything with that. > > > 3.7 Variable declarations > Oracle allows NOT NULL in variable declaration. Maybe we should allow that syntax as well? > > begin declare a NOT NULL int; begin a:=1; a:=NULL; end; end;; I have created a task for this: MDEV-13078 NOT NULL routine variables > > > 10. MDEV-10585 EXECUTE IMMEDIATE > In ORACLE the command supports 'RETURNING INTO' clause, we should probably document the difference? > > sql_stmt := 'UPDATE emp SET sal = 2000 WHERE empno = :1 > > RETURNING sal INTO :2'; > > EXECUTE IMMEDIATE sql_stmt USING emp_id RETURNING INTO salary; In Oracle, EXECUTE IMMEDIATE also has the "INTO" clause: sql_stmt := 'SELECT * FROM emp WHERE empno = :id'; EXECUTE IMMEDIATE sql_stmt INTO emp_rec USING emp_id; Should we create two new tasks, for RETURNING INTO, and for INTO? > > 12. MDEV-10588 TRUNCATE TABLE t1 DROP|REUSE STORAGE > Following clauses should be allowed in TRUNCATE command as well: > [ PRESERVE MATERIALIZED VIEW LOG | PURGE MATERIALIZED VIEW LOG ] Should we create a task to ignore this syntax? > > 18. MDEV-10577 sql_mode=ORACLE: %TYPE in variable declarations > It is not clear what is expected to happen if table's column definition is altered after stored program creation. Nothing. Data type resolution is done during execution.

1 0

[Maria-developers] Progress report of the Project[Week 2]
by ANKIT KUMAR 12 Jun '17

12 Jun '17

Hi, In this week, I managed to do few contributions and finally completed subtask-1. These are few things that I did this week- 1. Extended CREATE USER syntax- Now there will be multiple authentication mechanisms per user. 2. Created tests for the newly implemented syntax and some tests for previously existing functonalities. Need to give final touch up to the code and will commit tomorrow. Now I will proceed to subtask-2 Week-3[TODO's]- 1. Implement in-memory structures for this new syntax-[Providing it to ACL_USER] 2. Will be able to write data in memory(By the end of Phase-1 evaluation). Kind regards, Ankit Kumar.

1 0

[Maria-developers] libmariadb.so.3 compatibility
by Michal Schorm 08 Jun '17

08 Jun '17

Hello, I'm currently quite confused about the *libmariadb* library. In MariaDB 10.1.24 there are: " libmysqlclient_r.so -> libmysqlclient.so " " libmysqlclient.so -> libmysqlclient.so.18 " " libmysqlclient.so.18 -> libmysqlclient.so.18.0.0 " " libmysqlclient.so.18.0.0 " In MariaDB 10.2.6 there are: " libmariadb.so -> libmariadb.so.3 " In MariaDB-connector-c v2.3.3 there are: " libmysqlclient.so -> libmariadb.so " " libmysqlclient_r.so -> libmariadb.so " " libmariadb.so -> libmariadb.so.2 " " libmariadb.so.2 " -- 1) is "libmariadb.so.2" compatibile with "libmysqlclient.so.18.0.0"? Both API and ABI? 2) is "libmariadb.so.3" compatibile with "libmysqlclient.so.18.0.0"? Both API and ABI? 3) is "libmariadb.so.2" compatibile with "libmariadb.so.3"? Both API and ABI? Many packages in Fedora depends on "libmysqlclient.so". I would like to pack 10.2 to Fedora, but I can't before all those packages move to the libmariadb library. It would be considerably easier transfer for me, if all those libraries were compatibile, so I could provide symlinks (simmilar as in the connector C), and provide time for all those packages in Fedora to move safely. -- Michal Schorm Associate Software Engineer Core Services - Databases Team Red Hat

2 2

Re: [Maria-developers] [Commits] 4d6b16f: Fixed the bug mdev-12838.
by Sergey Petrunia 07 Jun '17

07 Jun '17

Hello Igor, On Wed, May 31, 2017 at 10:45:07AM -0700, IgorBabaev wrote: > revision-id: 4d6b16f10f9991821c128c382d32eb85248b3926 (mariadb-5.5.56-14-g4d6b16f) > parent(s): b8405c853fa30002d164d5fe2b4f8ea8979c09b8 > author: Igor Babaev > committer: Igor Babaev > timestamp: 2017-05-31 10:45:07 -0700 > message: > > Fixed the bug mdev-12838. > ... > +bool JOIN_TAB::keyuse_is_valid_for_access_in_chosen_plan(JOIN *join, > + KEYUSE *keyuse) > +{ > + if (!access_from_tables_is_allowed(keyuse->used_tables, > + join->sjm_lookup_tables)) > + return false; > + if (join->sjm_scan_tables & table->map) > + return true; > + table_map keyuse_sjm_scan_tables= keyuse->used_tables & > + join->sjm_scan_tables; > + if (!keyuse_sjm_scan_tables) > + return true; > + uint sjm_tab_nr= 0; > + while (!(keyuse_sjm_scan_tables & 1 << sjm_tab_nr)) The above "1" is a 32-bit number, while it should have the same bit length as table_map (that is, 64). Please use "table_map(1)". BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 1

[Maria-developers] the Lock on supremum is neccessary ?
by jhx1008 07 Jun '17

07 Jun '17

hi everyone: I use sysbench to do some tests on MySQL 5.7.x, and then I got some problems with the lock added on supremum, I hope someone can give me some advice. my table like this: CREATE TABLE `sbtest1` ( `id` int(11) NOT NULL AUTO_INCREMENT, `k` int(11) NOT NULL DEFAULT '0', `c` char(120) NOT NULL DEFAULT '', `pad` char(60) NOT NULL DEFAULT '', PRIMARY KEY (`id`), ) ENGINE=InnoDB; secondary index is not neccessary, I do not create it!! I have two sessions: session1: begin; select * from sbtest1 where id=123456 for update session2: insert into sbtest1 values(123450, 0,'aaa','bbb'); the records in the table like below: (...),(123440, 264454, xxxxxx, xxxxx), (123456, 251028, xxxxx, xxxxxx),(....) in my opinion, the record with the id=123456 existed, so session1 add the X lock on this record, session2 add the insert intention lock between(123440, 123456), these two locks are not conflicted. session2 will not be blocked by session1. unfortunately, in my case, session2 is blocked by session1, what happens? when session2 is blocked, I do "select * from information_schema.INNODB_LOCKS" use another session, It shows : mysql> select * from information_schema.INNODB_LOCKS; +----------------+-------------+-----------+-----------+------------------+------------+------------+-----------+----------+------------------------+ | lock_id | lock_trx_id | lock_mode | lock_type | lock_table | lock_index | lock_space | lock_page | lock_rec | lock_data | +----------------+-------------+-----------+-----------+------------------+------------+------------+-----------+----------+------------------------+ | 2388:29:2772:1 | 2388 | X | RECORD | `mydb`.`sbtest1` | PRIMARY | 29 | 2772 | 1 | supremum pseudo-record | | 2387:29:2772:1 | 2387 | X | RECORD | `mydb`.`sbtest1` | PRIMARY | 29 | 2772 | 1 | supremum pseudo-record | +----------------+-------------+-----------+-----------+------------------+------------+------------+-----------+----------+------------------------+ both the two sessions want to add X lock on "supremum pseudo-record" in the same time. then I have debugged this instance(row0sel.cc::row_search_mvcc), I got that: when a record existed on the page for the first one, if you want to add the X Lock on it, the supremum of pre-page will be add X Lock(maybe next key lock, supremum can be add gap lock only) also. In my case, the records' primary key stored like this: (..., 123440, supremum), (infimum, 123456, ...)， the id=123456 is the first record of that page, when add X Lock, first, find the pre-record id=123440, and then add the X lock after this record(id=123440) until match the record witch id=123456, so the supremum is locked also...， because of this lock(supremum)， session2 is blocked. if id=123456 is not the first record of page, session2 will not be blocked. so my doubt is: in my case, the session1 lock the supremum is necessary? if it's necessary, the lock on supremum maybe resulting the difference even if two mysql instances have the same records, just as in a replication. the record located on the page not all the same even if two instances have the same table sturcture & data(if slave define the table compressed, or innodb page_size not set as the same as master, etc..). if lock on supremum is neccessary, two instances execute the same SQLs, master is OK, but slave conflicted? on master， the two transactions may be committed in one group, but on slave maybe conflicted, slave's parallell replication is OK? also in MySQL 5.7.x, the xa prepare recorded in binlog, if on the master, execute two xa transactions: xa start 'aaa'; delete from table_name where id=xxxx; xa end 'aaa';xa prepare 'aaa'; xa start 'bbb';insert into table_name values(xxxx); end 'bbb'; xa prepare 'bbb'; xa commit 'aaa'; xa commit 'bbb' on the master, these xa not conflicted, I can committed these transaction, but on the slave, maybe conflicted sometimes, the replication is broken. how to repeat: create a table with primary key(id auto_increment is better) insert some records in this table use 3 sessions to find the first record on one page(perhaps, there are many pages, any one is OK).how to? session1: begin; select * from table_name where id=xxx for update // id=xxx must be exist session2: select * from table_name where id=xxx for update session3: when session2 is blocked, execute: show engine innodb status\G; in the transaction section, you can find some details like this: ------- TRX HAS BEEN WAITING 2 SEC FOR THIS LOCK TO BE GRANTED: RECORD LOCKS space id 29 page no 2772 n bits 144 index PRIMARY of table `mydb`.`sbtest1` trx id 2388 lock_mode X insert intention waiting Record lock, heap no 1 PHYSICAL RECORD: n_fields 1; compact format; info bits 0 0: len 8; hex 73757072656d756d; asc supremum;; from the information "space id 29 page no 2772", we can find the page of this record. then test for next id until the page no become to 2773, remember this id, this record is the first one of page 2773 delete some records before this id witch get in step 3 use 2 sessions to do what i described I repeat it by sysbench(version 1.0). 1. use sysbench to create a table with 500,000 records: sysbench --table-size=500000 --tables=1 --threads=64 --create_secondary=false --db-driver=mysql --mysql-host=127.0.0.1 --mysql-port=3306 --mysql-user=root --mysql-db=mydb oltp_read_write.lua prepare 2. delete the records between(200000, 200057), delete from sbtest1 where id>200000 and id < 200057 3. two sessions: session1: begin;select * from sbtest1 where id=200057 for update; session2: insert into sbtest1 values(200050, 0,'a','a') session2 will be blocked by session1. Because this problem is related to the localtion of the record in the page, repeat it like my operations, do not use the compressed table and make sure the innodb_page_size = 16384. in my case, the Lock on supremum is neccessary? 2017-06-05 boyce@netease

2 4

[Maria-developers] Progress report of the Project
by ANKIT KUMAR 05 Jun '17

05 Jun '17

Hi, I am Working on MDEV-11340 and by the end of the first week of Coding Period I have managed to learn How to use Bison. Currently thinking about How to implement subtask-1 of my project. Kind Regards,

1 0

[Maria-developers] header change in MariaDB 10.2.6 and mysqlclient python binding
by Kazuhiko Shiozaki 30 May '17

30 May '17

Hi ! First of all, congratulations to MariaDB 10.2 GA. mysqlclient (python binding : https://pypi.python.org/pypi/mysqlclient/1.3.10 ) cannot be built with MariaDB 10.2.6 like the following : _mysql.c: In function '_mysql_ConnectionObject_ping': _mysql.c:1911:41: error: 'MYSQL' has no member named 'reconnect' if ( reconnect != -1 ) self->connection.reconnect = reconnect; ^ error: Setup script exited with error: command 'gcc' failed with exit status 1 This is because the change in include/mysql/mysql.h. In MariaDB 10.2.5, it is same as mariadb-10.2.5/include/mysql.h in the source tree. In MariaDB 10.2.6, it is same as mariadb-10.2.6/libmariadb/include/mysql.h in the source tree. Is this change intentional ? Should I contact mysqlclient developer for further fix of this issue ? Thanks in advance ! Kazuhiko

2 1

[Maria-developers] review for SP stack trace
by Alexander Barkov 30 May '17

30 May '17

Hello Jerome, here's Monty's review on your stack trace patch. Thank you very much. We're sorry for delay. > diff --git a/mysql-test/r/commit_1innodb.result b/mysql-test/r/commit_1innodb.result > index 1adba7b..8e40ec0 100644 > <cut> > > --- a/mysql-test/r/get_diagnostics.result > +++ b/mysql-test/r/get_diagnostics.result > @@ -595,7 +595,7 @@ SELECT var, @var; > END| > CALL p1(); > var @var > -1 1 > +2 2 > DROP PROCEDURE p1; > > What is the reason for this going from 1 to 2 ? > I would have understood if there would have been some warnings from before > in the code, but there wasn't. > > Does this come from the error from 'call p1()' earlier that does now have > a new 'note message'. > > If yes, it would be good to make this clear by adding a 'show > warnings' just before the above create procedure p1() line. > > # Setting TABLE_NAME is currently not implemented. > diff --git a/mysql-test/r/signal.result b/mysql-test/r/signal.result > index f05e357..a796cc6 100644 > --- a/mysql-test/r/signal_demo3.result > +++ b/mysql-test/r/signal_demo3.result > @@ -79,14 +79,23 @@ show warnings; > @@ -95,6 +104,4 @@ call proc_1(); > ERROR 45000: Oops in proc_1 > show warnings; > Level Code Message > -Error 1644 Oops in proc_5 > -Error 1644 Oops in proc_4 > -Error 1644 Oops in proc_3 > +Note 4068 At line 4 in demo.proc_3 > > Why has 3 error disappered and why is there a line number note for > demo.proc_3 but no error line? > > I assume this comes from the following line: > SET @@session.max_error_count = 5 > > The question is if max_error_count's should really be counted for notes too, > especially for "At line" notes. > The documentation for the variable says > "Specifies the maximum number of messages stored for display by" > > Still, the question is if we should include the 'At line' or not in counting. > (Not important for this patch, but good to think about) > > > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7484,3 +7484,5 @@ ER_UNKNOWN_SEQUENCES 42S02 > eng "Unknown SEQUENCE: '%-.300s'" > ER_UNKNOWN_VIEW 42S02 > eng "Unknown VIEW: '%-.300s'" > +ER_SP_STACK_TRACE > + eng "At line %d in %s" > > Should be %u as lineno is uint > > > Please add the "SHOW WARNINGS" and fix the error message and it's ok to push. > Thanks for a great addition to MariaDB!

2 2

[Maria-developers] Weekly Progress Report of the Project
by ANKIT KUMAR 29 May '17

29 May '17

Hi all, I am Ankit Kumar and I have been selected to work on allowing multiple authentication methods to same user. These are the few things I did this week- 1. Setted up Development environment on Windows. Resolved few issues which I was facing, some MariaDB members really helped me whenever I needed their help. 2. Read the Testing Framework Documentation of MariaDB in order to see how testing is performed. 3. I have been regularly communicating with the Mentor and other MariaDB Contributors in order in get deep insight of the problems that I would be working on during Coding Period. These are some basic things that I would need during official coding period, so I wont be wasting time on such things. Now I can get Started with my project. IRC Nick- AspiringMind

2 4

Re: [Maria-developers] GSoC NUMA support Project
by Daniel Black 29 May '17

29 May '17

On 29/05/17 06:03, Sumit Lakra wrote: > I think I am near done with dividing the buffer pool into the number of > nodes available in a NUMA machine. There is one thing left to do - to > check for the presence of 'libnuma' library in the server. Based on https://jira.mariadb.org/browse/MDEV-12924 I probably broke it in ab0e5031e41d0aa72dd77a6dced9eacc8be5c548 > As of now I > have explicitly defined HAVE_LIBNUMA in the > CMakeList.txt to check my code. CMakeList.txt in which directory? I'd suggest refering to publicly pushed code/branches for review. A branch on https://github.com/theGodlessLakra/server would be good. > Also, to test whether the control of execution reaches within a function > A, I tried to print out a message on the scree like 'fprintf(stderr, In > function A);' This seems to work in main(), my_init(), sys_var_init() > etc, but not in buf0buf.cc. The function which creates the buffer pool > is called by 'buf_pool_init()', which is called by > 'innobase_start_on_create_for_mysql()', which in turn is called by > 'innobase_init()', but I couldn't find where an d how is this function > called. Its called by function pointer -> see maria_declare_plugin in storage/innobase/handler/ha_innodb.cc which is called by the plugin initialization code. > Seems like the output to bot stdout and stderr is being > redirected somewhere and I can't find where (in code). reopen_fstreams in sql/{mysqld,log}.cc > Having this printing stuff is a quick way of testing the flow of control > and although my code is working, You can also run mysqld in gdb and the use "break" to set a breakpoint at a function/file:line and step through form there. > I would like to know about this > redirections if someone can tell me. add --verbose to the mysqld argument. > Also, which part of code calls the 'innobase_init()' function ? sql/plugin.cc:plugin_initialize in the call to plugin->init() > I have attached a screenshot of the server running on my NUMA emulated > pc with and without --numa-interleave=all. [ PID - 21016 without NUMA, > PID - 21058 with NUMA] > > At present it just ALLOCATES a buffer pool instance on each numa node. > Need to work on specific node binding and stuff.

1 0

[Maria-developers] GSoC NUMA support Project
by Sumit Lakra 28 May '17

28 May '17

I think I am near done with dividing the buffer pool into the number of nodes available in a NUMA machine. There is one thing left to do - to check for the presence of 'libnuma' library in the server. As of now I have explicitly defined HAVE_LIBNUMA in the CMakeList.txt to check my code. Also, to test whether the control of execution reaches within a function A, I tried to print out a message on the scree like 'fprintf(stderr, In function A);' This seems to work in main(), my_init(), sys_var_init() etc, but not in buf0buf.cc. The function which creates the buffer pool is called by 'buf_pool_init()', which is called by 'innobase_start_on_create_for_mysql()', which in turn is called by 'innobase_init()', but I couldn't find where an d how is this function called. Seems like the output to bot stdout and stderr is being redirected somewhere and I can't find where (in code). Having this printing stuff is a quick way of testing the flow of control and although my code is working, I would like to know about this redirections if someone can tell me. Also, which part of code calls the 'innobase_init()' function ? I have attached a screenshot of the server running on my NUMA emulated pc with and without --numa-interleave=all. [ PID - 21016 without NUMA, PID - 21058 with NUMA] At present it just ALLOCATES a buffer pool instance on each numa node. Need to work on specific node binding and stuff.

1 0

[Maria-developers] Building LibraOffice w/ MariaDB installed fails apt-get build-dep
by Howard Johnson 27 May '17

27 May '17

Hi, I'm having trouble with MariaDB as is described here: https://ask.libreoffice.org/en/question/96902/building-lo-w-mariadb-install… I suspect that there might be an issue with the virtual package or something like that. Any idea of what is going wrong or how to fix it? Thanks -- howard

2 1

Re: [Maria-developers] [Commits] df94429: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 26 May '17

26 May '17

Hi Varun, Ok to push. On Thu, May 25, 2017 at 07:41:50PM +0530, Varun wrote: > revision-id: df944293721a7ad93d115a58f2b574067d1dc79f (mariadb-10.1.20-282-gdf94429) > parent(s): a1b6128dedb4419db9fadaf94c356d3477d4e06f > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-25 19:40:08 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > mysql-test/r/innodb_ext_key.result | 81 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 51 ++++++++++++++++++++++++ > sql/table.cc | 39 +++++++++++++++++- > 3 files changed, 170 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..de1323e 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,86 @@ where index_date_updated= 10 and index_id < 800; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 range index_date_updated index_date_updated 13 NULL # Using index condition > drop table t0,t1,t2; > +# > +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +# was corrupted, server crashes in opt_sum_query > +set @save_innodb_file_format= @@innodb_file_format; > +set @save_innodb_large_prefix= @@innodb_large_prefix; > +set global innodb_file_format = BARRACUDA; > +set global innodb_large_prefix = ON; > +CREATE TABLE t1 ( > +pk INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1024), > +PRIMARY KEY (pk), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +MAX(t2.pk) > +2 > +drop table t1; > +CREATE TABLE t1 ( > +pk1 INT, > +pk2 INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1021), > +PRIMARY KEY (pk1,pk2), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["f2"], > + "key": "f2", > + "key_length": "3070", > + "used_key_parts": ["f2", "pk1"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "((t1.pk1 <= 5) and (t1.pk2 <= 5) and (t1.f2 = 'abc'))", > + "attached_condition": "(t1.f1 <= '3')" > + } > + } > +} > +drop table t1; > +CREATE TABLE t1 ( > +f2 INT, > +pk2 INT, > +f1 VARCHAR(3), > +pk1 VARCHAR(1000), > +PRIMARY KEY (pk1,pk2), > +KEY k1(pk1,f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(k1) where f2 <= 5 and pk2 <=5 and pk1 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["k1"], > + "key": "k1", > + "key_length": "3011", > + "used_key_parts": ["pk1", "f2", "pk2"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "((t1.f2 <= 5) and (t1.pk2 <= 5) and (t1.pk1 = 'abc'))", > + "attached_condition": "(t1.f1 <= '3')" > + } > + } > +} > +drop table t1; > set optimizer_switch=@save_ext_key_optimizer_switch; > +set global innodb_file_format = @save_innodb_file_format; > +set global innodb_large_prefix = @save_innodb_large_prefix; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/innodb_ext_key.test b/mysql-test/t/innodb_ext_key.test > index bf94b7d..c8acfc5 100644 > --- a/mysql-test/t/innodb_ext_key.test > +++ b/mysql-test/t/innodb_ext_key.test > @@ -778,5 +778,56 @@ where index_date_updated= 10 and index_id < 800; > > drop table t0,t1,t2; > > + > +--echo # > +--echo # MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +--echo # was corrupted, server crashes in opt_sum_query > + > +set @save_innodb_file_format= @@innodb_file_format; > +set @save_innodb_large_prefix= @@innodb_large_prefix; > +set global innodb_file_format = BARRACUDA; > +set global innodb_large_prefix = ON; > + > +CREATE TABLE t1 ( > + pk INT, > + f1 VARCHAR(3), > + f2 VARCHAR(1024), > + PRIMARY KEY (pk), > + KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > + > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +drop table t1; > + > +CREATE TABLE t1 ( > + pk1 INT, > + pk2 INT, > + f1 VARCHAR(3), > + f2 VARCHAR(1021), > + PRIMARY KEY (pk1,pk2), > + KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > + > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +drop table t1; > + > +CREATE TABLE t1 ( > +f2 INT, > +pk2 INT, > +f1 VARCHAR(3), > +pk1 VARCHAR(1000), > +PRIMARY KEY (pk1,pk2), > +KEY k1(pk1,f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(k1) where f2 <= 5 and pk2 <=5 and pk1 = 'abc' and f1 <= '3'; > +drop table t1; > + > set optimizer_switch=@save_ext_key_optimizer_switch; > +set global innodb_file_format = @save_innodb_file_format; > +set global innodb_large_prefix = @save_innodb_large_prefix; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/sql/table.cc b/sql/table.cc > index 37c0b63..a1e9ebf 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1721,6 +1721,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo= share->key_info; > uint primary_key= my_strcasecmp(system_charset_info, share->keynames.type_names[0], > primary_key_name) ? MAX_KEY : 0; > + KEY* key_first_info; > > if (primary_key >= MAX_KEY && keyinfo->flags & HA_NOSAME) > { > @@ -1800,19 +1801,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo->name_length+1); > } > > + if (!key) > + key_first_info= keyinfo; > + > if (ext_key_parts > share->key_parts && key) > { > KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part + > (keyinfo-1)->ext_key_parts; > uint add_keyparts_for_this_key= add_first_key_parts; > + uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0; > + Field *field; > > /* > Do not extend the key that contains a component > defined over the beginning of a field. > */ > for (i= 0; i < keyinfo->user_defined_key_parts; i++) > - { > + { > uint fieldnr= keyinfo->key_part[i].fieldnr; > + field= share->field[keyinfo->key_part[i].fieldnr-1]; > + > + if (field->null_ptr) > + len_null_byte= HA_KEY_NULL_LENGTH; > + > + if (field->type() == MYSQL_TYPE_BLOB || > + field->real_type() == MYSQL_TYPE_VARCHAR || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + length_bytes= HA_KEY_BLOB_LENGTH; > + } > + > + ext_key_length+= keyinfo->key_part[i].length + len_null_byte > + + length_bytes; > if (share->field[fieldnr-1]->key_length() != > keyinfo->key_part[i].length) > { > @@ -1821,6 +1841,23 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > } > } > > + if (add_keyparts_for_this_key) > + { > + for (i= 0; i < add_keyparts_for_this_key; i++) > + { > + uint pk_part_length= key_first_info->key_part[i].store_length; > + if (keyinfo->ext_key_part_map & 1<<i) > + { > + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + { > + add_keyparts_for_this_key= i; > + break; > + } > + ext_key_length+= pk_part_length; > + } > + } > + } > + > if (add_keyparts_for_this_key < (keyinfo->ext_key_parts - > keyinfo->user_defined_key_parts)) > { > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Cannot push branch to github
by Valentin Rakush 26 May '17

26 May '17

Hi all, I am trying to do first pull request to https://github.com/MariaDB/server but I cannot push my new branch to github. Git says "ERROR: Permission to MariaDB/server.git denied to alpes214. fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists." Should I talk to somebody to get access rights first? -- Kind regards, Valentin Rakush

2 3

Re: [Maria-developers] MDEV-12803 Improve function parameter data type control
by Alexander Barkov 26 May '17

26 May '17

Hello Alexey, Thanks for your feedback. Sending a new version, as discussed in this email and on IRC. Please find attached. - Removed Item_geometry_func_importer - Renamed Item_xxx_geometry_property to Item_xxx_args_geometry - Renamed Item_xxx_dyadic_operation to Item_xxx_args_geometry_geometry. This is the new hierarchy: Item_str_func Item_geometry_func Item_geometry_func_args_geometry Item_func_centroid Item_func_envelope Item_func_boundary Item_func_spatial_decomp Item_func_spatial_decomp_n Item_func_buffer Item_func_pointonsurface Item_func_geometry_from_text Item_func_geometry_from_wkb Item_func_geometry_from_json Item_func_point Item_func_spatial_collection Item_func_spatial_operation Item_binary_func_args_geometry Item_func_as_wkb Item_real_func Item_real_func_args_geometry Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_args_geometry_geometry Item_func_distance Item_long_func Item_long_func_args_geometry Item_func_issimple Item_func_isring Item_func_isclosed Item_func_dimension Item_func_numgeometries Item_func_numinteriorring Item_func_numpoints Item_func_srid Item_bool_func Item_bool_func_args_geometry Item_func_isempty Item_bool_func_args_geometry_geometry Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_args_geometry Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Thanks! On 05/26/2017 11:50 AM, Alexey Botchkov wrote: >>1. It will look like a C fragment in a C++ code. >> Why is it more natural than a virtual method? > > The Item hierarchy and 'check_type' hierarchy are parallel. > So it seems natural to me if we can assign an already-prepared > parameter-validation function to the type. > >> 2. It will make the Item size bigger. > But it will make Item** virtual tables smaller. > >> 4. It will need more CPU for m_check_arguments_function initialization. > Must be really small overhead. > >> Are you afraid that I'll have to modify class hierarchy? > Yes. My only concern is that the class hierarchy becomes more complicated > and with repeating code. > > But after a while i sort of used to it so now i'm fine with the virtual > method. > > >> We have a convention that functions return true on error and false on >> success. > For the check_something function returning TRUE or FALSE doesn't mean an > 'error'. > It's just the condition fits or doesn't. > But fine, our code has many examples of 'check' functions returning > results this way. > > Typenames discussed. > So otherwise i'm ok with the patch. > > Best regards. > HF > > On Wed, May 24, 2017 at 4:22 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hi Alexey, > > Thank you very much for your review! > > Please see my comments below: > > > On 05/23/2017 12:16 AM, Alexey Botchkov wrote: > > Firstly I'd say i don't like the idea of the 'virtual check_arguments()' > > function. > > I belive it's more natural to have a member 'm_check_arguments_function' > > that would be set in constructor (or be an argument to the constructor) > > and the non-virtual 'check_arguments()' that would just call that member. > > This would make Item class structure much nicer and would even save us > > memory. > > > > If you have good arguments for your approach, I have some > > comments/questions to your patch :) > > I'm not sure that a function member is better. > > 1. It will look like a C fragment in a C++ code. > Why is it more natural than a virtual method? > > 2. It will make the Item size bigger. > > 3. It will need more coding. > Now we have to declare only virtual functions. > With a function member, we'll have to do: > - declare function anyway > - add a code into constructors > > 4. It will need more CPU for m_check_arguments_function initialization. > On the contrary, having a virtual methods costs nothing, > except a small initialization in VMTs during mysqld startup. > > > What is your main concern? > Are you afraid that I'll have to modify class hierarchy > for other Item_xxx_func? It should not be necessary. > We'll just have a set of protected methods on the Item_func level, > for most typical cases. > > > > > > Why this change? > > +++ b/mysql-test/t/gis-rtree.test > > @@ -61,7 +61,7 @@ while ($1) > > let $2=10; > > while ($2) > > { > > - eval DELETE FROM t2 WHERE Within(g, > > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), Point($1 * 10, > > $2 * 10)))); > > + eval DELETE FROM t2 WHERE Within(g, > > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), 0))); > > SELECT count(*) FROM t2; > > dec $2; > > > It passed POINT() as the second argiment to GeometryFromWKB. > The second argument is for SRID. After my changes it's not allowed. > > > > } > > > > > > It looks weird when the 'check_something' functions returns FALSE if > > the argument fits the check. I'd make them returning TRUE in this case > > as people normally expect. > > We have a convention that functions return true on error and false on > success. > > Please have a look into: > > check_db_name() > check_for_broken_triggers() > check_unique_table() > > They all return true on error. > I did the same. > > > > > > +bool Item_func::check_argument_types_scalar(uint start, uint end) const > > +{ > > + for (uint i= start; i < end; i++) > > + { > > + DBUG_ASSERT(i < arg_count); > > > > In code like this i'd rather do > > bool Item_func::check_argument_types_scalar(uint start, uint end) const > > { > > DBUG_ASSERT(end < arg_count); > > > > for (uint i= start; i < end; i++) > > { > > // no DBUG_ASSERT here > > ... > > Agree. > > > > > > > > > Some typenames feel misleading. I'd suggest changes > > Item_real_func_geometry_property -> Item_real_func_arg_geometry > > Item_int_func_geometry_property -> Item_int_func_arg_geometry > > Item_bool_func_geometry_property -> Item_bool_func_arg_geometry > > Item_str_ascii_func_geometry_property -> Item_ascii_func_arg_geometry > > Item_geometry_func_geometry_property -> Item_geometry_func_arg_geometry > > This looks like a property. > The first argument is an object of some complex type (GEOMETRY in > our case). > The other arguments are simple type additional optional parameters. > > In C++ this would look like: > > geom->IsSimple(); > geom->Buffer(10); > > > OpenGIS also calls these methods "properties", > so do we in the documentation: > > https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html > <https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html> > > > > > > > Item_binary_func_geometry_property - do we really need this type? > > Right, there is only one binary string property: AsWKB(). > > I added Item_xxx_geometry_property for regularity > (to have a symmetric class hierarchy with other "properties" ). > > Here's the class structure: > > > Item_str_func > Item_geometry_func > Item_geometry_func_geometry_property > Item_func_centroid > Item_func_envelope > Item_func_boundary > Item_func_spatial_decomp > Item_func_spatial_decomp_n > Item_func_buffer > Item_func_pointonsurface > Item_geometry_func_importer > Item_func_geometry_from_text > Item_func_geometry_from_wkb > Item_func_geometry_from_json > Item_func_point > Item_func_spatial_collection > Item_func_spatial_operation > Item_binary_func_geometry_property > Item_func_as_wkb > Item_real_func > Item_real_func_geometry_property > Item_func_x > Item_func_y > Item_func_area > Item_func_glength > Item_real_func_geometry_dyadic_operation > Item_func_distance > Item_int_func > Item_int_func_geometry_property > Item_func_issimple > Item_func_isring > Item_func_isclosed > Item_func_dimension > Item_func_numgeometries > Item_func_numinteriorring > Item_func_numpoints > Item_func_srid > Item_func_gis_debug > Item_bool_func > Item_bool_func_geometry_property > Item_func_isempty > Item_bool_func_geometry_dyadic_operation > Item_func_spatial_relate > Item_str_ascii_func > Item_str_ascii_func_geometry_property > Item_func_as_wkt > Item_func_as_geojson > Item_func_geometry_type > > Btw, do you need a program to print this hierarchy? :) > I wrote it a few years ago, and use it almost every day. > > > > > Item_geometry_func_importer - do we need this type? > > I can remove this. It does not have shared methods indeed. > > > > > Item_real_func_geometry_dyadic_operation -> Item_real_func_arg_2_geometries > > Item_bool_func_geometry_dyadic_operation -> Item_bool_func_arg_2_geometries > > It's an operation on two geometries. > My names is quite self-descriptive. > Also, it tells that: > - there is no any other GEOMETRY arguments > - there can be optional non-GEOMETRY additional arguments > > > > > > > + bool check_arguments() const > > + { > > + DBUG_ASSERT(arg_count >= 2); > > + return check_argument_types_or_binary(&type_handler_geometry, 0, 2); > > > > the check_argument_types_or_binary() does the check of the arg_count so > > no need for the DBUG_ASSERT(). > > But i don't insist on removing this line :) > > I can remove it :) > > > > > > > On Mon, May 15, 2017 at 5:56 PM, Alexander Barkov <bar(a)mariadb.org <mailto:bar@mariadb.org> > > <mailto:bar@mariadb.org <mailto:bar@mariadb.org>>> wrote: > > > > Hello Alexey, > > > > can you please review a patch for MDEV-12803? > > > > Thanks! > > > > > >

1 0

Re: [Maria-developers] [Commits] 5ac7ee3: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 25 May '17

25 May '17

Hi Varun, Good to see that the feedback from the last time was addressed. There is another issue, though. Consider a case where some (but not necessarily all) PK columns are present in the secondary key: create table t1 ( pk1 VARCHAR(1000) not null, pk2 INT not null, col2 INT not null, KEY key1(pk1, col2), PRIMARY KEY(pk1, pk2) ) CHARSET utf8 engine=innodb; insert into t1 select a,a,a from ten; In this case, key1 still has a suffix, but the suffix only includes columns that are not already present in the key. That is, the "real" definition of key1 is (pk1, col2, pk2) and not (pk1, col2, pk1, pk2) . Let's try that. explain select * from t1 force index(key1) where pk1 < 'zz' and col2 <=2 and pk2 <=4; Without the patch, I have all 3 key parts used: | { "query_block": { "select_id": 1, "table": { "table_name": "t1", "access_type": "range", "possible_keys": ["key1"], "key": "key1", "key_length": "3010", "used_key_parts": ["pk1", "col2", "pk2"], "rows": 1, "filtered": 100, "attached_condition": "t1.pk1 <= '0' and t1.col2 <= 2 and t1.pk2 <= 4", "using_index": true } } } | With the patch, I only see the explicitly defined key parts used: | { "query_block": { "select_id": 1, "table": { "table_name": "t1", "access_type": "range", "possible_keys": ["key1"], "key": "key1", "key_length": "3006", "used_key_parts": ["pk1", "col2"], "rows": 1, "filtered": 100, "attached_condition": "t1.pk1 <= '0' and t1.col2 <= 2 and t1.pk2 <= 4", "using_index": true } } } | Which I think is wrong, because the key length never exceeds MAX_KEY_LEN. This can be easily fixed by appyling this patch over your patch: --- table.cc.varun 2017-05-24 18:30:39.076341082 +0300 +++ table.cc 2017-05-24 19:04:25.637740027 +0300 @@ -2164,10 +2164,13 @@ for (i= 0; i < add_keyparts_for_this_key; i++) { uint pk_part_length= key_first_info->key_part[i].store_length; - if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) + if (keyinfo->ext_key_part_map & 1<<i) { - add_keyparts_for_this_key= i; - break; + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) + { + add_keyparts_for_this_key= i; + break; + } } ext_key_length+= pk_part_length; } Do you agree with this change? Any amendments? If it's ok, please add this (together with the testcase) into your patch. I've checked MySQL 5.7 also, and they do something weird: ... insert into t1 select A.a + 1000*B.a, A.a + 1000*B.a, A.a + 1000*B.a from one_k A, ten B; explain format=json select * from t1 force index(key1) where pk1 <= '0' and col2 <=2 and pk2 <=4; EXPLAIN { "query_block": { "select_id": 1, "cost_info": { "query_cost": "1.41" }, "table": { "table_name": "t1", "access_type": "range", "possible_keys": [ "key1" ], "key": "key1", "used_key_parts": [ "pk1" ], "key_length": "3010", "rows_examined_per_scan": 1, "rows_produced_per_join": 0, "filtered": "11.11", "using_index": true, "cost_info": { "read_cost": "1.39", "eval_cost": "0.02", "prefix_cost": "1.41", "data_read_per_join": "335" }, "used_columns": [ "pk1", "pk2", "col2" ], "attached_condition": "((`test`.`t1`.`pk1` <= '0') and (`test`.`t1`.`col2` <= 2) and (`test`.`t1`.`pk2` <= 4))" } } } key_length is 3010, but used_key_parts only includes "pk1"... The numbers do not match. On Tue, May 23, 2017 at 07:08:48PM +0530, Varun wrote: > revision-id: 5ac7ee32a8e373d989bf9d665b5e01f770ac583f (mariadb-10.1.20-282-g5ac7ee3) > parent(s): a1b6128dedb4419db9fadaf94c356d3477d4e06f > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-23 19:00:15 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > mysql-test/r/innodb_ext_key.result | 51 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 38 ++++++++++++++++++++++++++++ > sql/table.cc | 36 ++++++++++++++++++++++++++- > 3 files changed, 124 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..c76660c 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,56 @@ where index_date_updated= 10 and index_id < 800; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 range index_date_updated index_date_updated 13 NULL # Using index condition > drop table t0,t1,t2; > +# > +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +# was corrupted, server crashes in opt_sum_query > +set @save_innodb_file_format= @@innodb_file_format; > +set @save_innodb_large_prefix= @@innodb_large_prefix; > +set global innodb_file_format = BARRACUDA; > +set global innodb_large_prefix = ON; > +CREATE TABLE t1 ( > +pk INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1024), > +PRIMARY KEY (pk), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +MAX(t2.pk) > +2 > +drop table t1; > +CREATE TABLE t1 ( > +pk1 INT, > +pk2 INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1021), > +PRIMARY KEY (pk1,pk2), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["f2"], > + "key": "f2", > + "key_length": "3070", > + "used_key_parts": ["f2", "pk1"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "((t1.pk1 <= 5) and (t1.pk2 <= 5) and (t1.f2 = 'abc'))", > + "attached_condition": "(t1.f1 <= '3')" > + } > + } > +} > +drop table t1; > set optimizer_switch=@save_ext_key_optimizer_switch; > +set global innodb_file_format = @save_innodb_file_format; > +set global innodb_large_prefix = @save_innodb_large_prefix; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/innodb_ext_key.test b/mysql-test/t/innodb_ext_key.test > index bf94b7d..adb032f 100644 > --- a/mysql-test/t/innodb_ext_key.test > +++ b/mysql-test/t/innodb_ext_key.test > @@ -778,5 +778,43 @@ where index_date_updated= 10 and index_id < 800; > > drop table t0,t1,t2; > > + > +--echo # > +--echo # MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +--echo # was corrupted, server crashes in opt_sum_query > + > +set @save_innodb_file_format= @@innodb_file_format; > +set @save_innodb_large_prefix= @@innodb_large_prefix; > +set global innodb_file_format = BARRACUDA; > +set global innodb_large_prefix = ON; > + > +CREATE TABLE t1 ( > + pk INT, > + f1 VARCHAR(3), > + f2 VARCHAR(1024), > + PRIMARY KEY (pk), > + KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > + > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +drop table t1; > + > +CREATE TABLE t1 ( > + pk1 INT, > + pk2 INT, > + f1 VARCHAR(3), > + f2 VARCHAR(1021), > + PRIMARY KEY (pk1,pk2), > + KEY(f2) > +) ENGINE=InnoDB CHARSET utf8 ROW_FORMAT= DYNAMIC; > + > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +drop table t1; > + > set optimizer_switch=@save_ext_key_optimizer_switch; > +set global innodb_file_format = @save_innodb_file_format; > +set global innodb_large_prefix = @save_innodb_large_prefix; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/sql/table.cc b/sql/table.cc > index 37c0b63..c8c9696 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1721,6 +1721,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo= share->key_info; > uint primary_key= my_strcasecmp(system_charset_info, share->keynames.type_names[0], > primary_key_name) ? MAX_KEY : 0; > + KEY* key_first_info; > > if (primary_key >= MAX_KEY && keyinfo->flags & HA_NOSAME) > { > @@ -1800,19 +1801,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo->name_length+1); > } > > + if (!key) > + key_first_info= keyinfo; > + > if (ext_key_parts > share->key_parts && key) > { > KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part + > (keyinfo-1)->ext_key_parts; > uint add_keyparts_for_this_key= add_first_key_parts; > + uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0; > + Field *field; > > /* > Do not extend the key that contains a component > defined over the beginning of a field. > */ > for (i= 0; i < keyinfo->user_defined_key_parts; i++) > - { > + { > uint fieldnr= keyinfo->key_part[i].fieldnr; > + field= share->field[keyinfo->key_part[i].fieldnr-1]; > + > + if (field->null_ptr) > + len_null_byte= HA_KEY_NULL_LENGTH; > + > + if (field->type() == MYSQL_TYPE_BLOB || > + field->real_type() == MYSQL_TYPE_VARCHAR || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + length_bytes= HA_KEY_BLOB_LENGTH; > + } > + > + ext_key_length+= keyinfo->key_part[i].length + len_null_byte > + + length_bytes; > if (share->field[fieldnr-1]->key_length() != > keyinfo->key_part[i].length) > { > @@ -1821,6 +1841,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > } > } > > + if (add_keyparts_for_this_key) > + { > + for (i= 0; i < add_keyparts_for_this_key; i++) > + { > + uint pk_part_length= key_first_info->key_part[i].store_length; > + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + { > + add_keyparts_for_this_key= i; > + break; > + } > + ext_key_length+= pk_part_length; > + } > + } > + > if (add_keyparts_for_this_key < (keyinfo->ext_key_parts - > keyinfo->user_defined_key_parts)) > { > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

Re: [Maria-developers] MDEV-12803 Improve function parameter data type control
by Alexander Barkov 24 May '17

24 May '17

Hi Alexey, Thank you very much for your review! Please see my comments below: On 05/23/2017 12:16 AM, Alexey Botchkov wrote: > Firstly I'd say i don't like the idea of the 'virtual check_arguments()' > function. > I belive it's more natural to have a member 'm_check_arguments_function' > that would be set in constructor (or be an argument to the constructor) > and the non-virtual 'check_arguments()' that would just call that member. > This would make Item class structure much nicer and would even save us > memory. > > If you have good arguments for your approach, I have some > comments/questions to your patch :) I'm not sure that a function member is better. 1. It will look like a C fragment in a C++ code. Why is it more natural than a virtual method? 2. It will make the Item size bigger. 3. It will need more coding. Now we have to declare only virtual functions. With a function member, we'll have to do: - declare function anyway - add a code into constructors 4. It will need more CPU for m_check_arguments_function initialization. On the contrary, having a virtual methods costs nothing, except a small initialization in VMTs during mysqld startup. What is your main concern? Are you afraid that I'll have to modify class hierarchy for other Item_xxx_func? It should not be necessary. We'll just have a set of protected methods on the Item_func level, for most typical cases. > > Why this change? > +++ b/mysql-test/t/gis-rtree.test > @@ -61,7 +61,7 @@ while ($1) > let $2=10; > while ($2) > { > - eval DELETE FROM t2 WHERE Within(g, > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), Point($1 * 10, > $2 * 10)))); > + eval DELETE FROM t2 WHERE Within(g, > Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), 0))); > SELECT count(*) FROM t2; > dec $2; It passed POINT() as the second argiment to GeometryFromWKB. The second argument is for SRID. After my changes it's not allowed. > } > > > It looks weird when the 'check_something' functions returns FALSE if > the argument fits the check. I'd make them returning TRUE in this case > as people normally expect. We have a convention that functions return true on error and false on success. Please have a look into: check_db_name() check_for_broken_triggers() check_unique_table() They all return true on error. I did the same. > > +bool Item_func::check_argument_types_scalar(uint start, uint end) const > +{ > + for (uint i= start; i < end; i++) > + { > + DBUG_ASSERT(i < arg_count); > > In code like this i'd rather do > bool Item_func::check_argument_types_scalar(uint start, uint end) const > { > DBUG_ASSERT(end < arg_count); > > for (uint i= start; i < end; i++) > { > // no DBUG_ASSERT here > ... Agree. > > > > Some typenames feel misleading. I'd suggest changes > Item_real_func_geometry_property -> Item_real_func_arg_geometry > Item_int_func_geometry_property -> Item_int_func_arg_geometry > Item_bool_func_geometry_property -> Item_bool_func_arg_geometry > Item_str_ascii_func_geometry_property -> Item_ascii_func_arg_geometry > Item_geometry_func_geometry_property -> Item_geometry_func_arg_geometry This looks like a property. The first argument is an object of some complex type (GEOMETRY in our case). The other arguments are simple type additional optional parameters. In C++ this would look like: geom->IsSimple(); geom->Buffer(10); OpenGIS also calls these methods "properties", so do we in the documentation: https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html > > Item_binary_func_geometry_property - do we really need this type? Right, there is only one binary string property: AsWKB(). I added Item_xxx_geometry_property for regularity (to have a symmetric class hierarchy with other "properties" ). Here's the class structure: Item_str_func Item_geometry_func Item_geometry_func_geometry_property Item_func_centroid Item_func_envelope Item_func_boundary Item_func_spatial_decomp Item_func_spatial_decomp_n Item_func_buffer Item_func_pointonsurface Item_geometry_func_importer Item_func_geometry_from_text Item_func_geometry_from_wkb Item_func_geometry_from_json Item_func_point Item_func_spatial_collection Item_func_spatial_operation Item_binary_func_geometry_property Item_func_as_wkb Item_real_func Item_real_func_geometry_property Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_geometry_dyadic_operation Item_func_distance Item_int_func Item_int_func_geometry_property Item_func_issimple Item_func_isring Item_func_isclosed Item_func_dimension Item_func_numgeometries Item_func_numinteriorring Item_func_numpoints Item_func_srid Item_func_gis_debug Item_bool_func Item_bool_func_geometry_property Item_func_isempty Item_bool_func_geometry_dyadic_operation Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_geometry_property Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Btw, do you need a program to print this hierarchy? :) I wrote it a few years ago, and use it almost every day. > > Item_geometry_func_importer - do we need this type? I can remove this. It does not have shared methods indeed. > > Item_real_func_geometry_dyadic_operation -> Item_real_func_arg_2_geometries > Item_bool_func_geometry_dyadic_operation -> Item_bool_func_arg_2_geometries It's an operation on two geometries. My names is quite self-descriptive. Also, it tells that: - there is no any other GEOMETRY arguments - there can be optional non-GEOMETRY additional arguments > > > + bool check_arguments() const > + { > + DBUG_ASSERT(arg_count >= 2); > + return check_argument_types_or_binary(&type_handler_geometry, 0, 2); > > the check_argument_types_or_binary() does the check of the arg_count so > no need for the DBUG_ASSERT(). > But i don't insist on removing this line :) I can remove it :) > > > On Mon, May 15, 2017 at 5:56 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Alexey, > > can you please review a patch for MDEV-12803? > > Thanks! > >

1 0

[Maria-developers] Oracle compatibility : Update with same source and target
by jerome brauge 23 May '17

23 May '17

Hello Alexander, In the same spirit of MDEV-12137, can you create a task for update with same source and target ? Regard, Jérôme.

2 3

[Maria-developers] MDEV-10142 - Replace function
by jerome brauge 23 May '17

23 May '17

Hello Alexander, Here is patch for replace function in oracle mode. Do I have to add a test for replication ? Regards, Jérôme.

2 4

Re: [Maria-developers] [Commits] 55a4858fe6f: MDEV-12666: CURRENT_ROLE() does not work in a view
by Alexander Barkov 23 May '17

23 May '17

Hi Vicentiu, It seems that Item_func_sysconst abuses const_charset_converter(). I have the following proposals: - Instead of adding a new code into Item::const_charset_converter(), can you please override Item_func_sysconst::safe_charset_converter()? The direction proposed by Sergei's looks good: Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { + if (thd->lex->context_analysis_only & CONTEXT_ANALYSIS_ONLY_VIEW) + return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } But it probably needs to be extended to handle prepared statement. At PREPARE-time it should not call const_charset_converter(). At EXECUTE-time it should be Ok to call const_charset_converter(). So can you please change Item_func_sysconst::const_item() to report false when in views and during PREPARE-time? And then change safe_charset_converter() as follows: Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { if (!const_item()) return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } - Please add tests for DATABASE() and CURRENT_USER - Please add tests for PS with changing context between PREPARE and EXECUTE. Thanks. On 05/22/2017 06:15 PM, vicentiu wrote: > revision-id: 55a4858fe6f3fdc63a072b93b9bc3d8e51dc5e94 (mariadb-10.0.30-76-g55a4858fe6f) > parent(s): 8d1827f5540a0def82b1e2cbc557245c3e5e14ca > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-05-22 17:06:01 +0300 > message: > > MDEV-12666: CURRENT_ROLE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The solution proposed is to not replace any function with an Item_null, > but instead use an Item_static_string_func. > > --- > .../suite/roles/current_role_view-12666.result | 48 ++++++++++++++++++++ > .../suite/roles/current_role_view-12666.test | 52 ++++++++++++++++++++++ > sql/item.cc | 13 +++++- > 3 files changed, 112 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/roles/current_role_view-12666.result b/mysql-test/suite/roles/current_role_view-12666.result > new file mode 100644 > index 00000000000..525748b4875 > --- /dev/null > +++ b/mysql-test/suite/roles/current_role_view-12666.result > @@ -0,0 +1,48 @@ > +CREATE USER has_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO has_role@'localhost'; > +CREATE ROLE test_role; > +GRANT test_role TO has_role@'localhost'; > +CREATE USER no_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO no_role@'localhost'; > +CREATE TABLE view_role_test ( > +id int primary key, > +role_name varchar(50) > +); > +INSERT INTO view_role_test VALUES (1, 'test_role'); > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +show create view v_view_role_test; > +View Create View character_set_client collation_connection > +v_view_role_test CREATE ALGORITHM=UNDEFINED DEFINER=`no_role`@`localhost` SQL SECURITY INVOKER VIEW `v_view_role_test` AS select `view_role_test`.`id` AS `id`,`view_role_test`.`role_name` AS `role_name` from `view_role_test` where (`view_role_test`.`role_name` = current_role()) latin1 latin1_swedish_ci > +SET ROLE test_role; > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test_2 > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +show create view v_view_role_test_2; > +View Create View character_set_client collation_connection > +v_view_role_test_2 CREATE ALGORITHM=UNDEFINED DEFINER=`no_role`@`localhost` SQL SECURITY INVOKER VIEW `v_view_role_test_2` AS select `view_role_test`.`id` AS `id`,`view_role_test`.`role_name` AS `role_name` from `view_role_test` where (`view_role_test`.`role_name` = current_role()) latin1 latin1_swedish_ci > +SELECT CURRENT_USER(); > +CURRENT_USER() > +root@localhost > +SELECT CURRENT_ROLE(); > +CURRENT_ROLE() > +test_role > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +id role_name > +1 test_role > +SELECT * FROM v_view_role_test; > +id role_name > +1 test_role > +drop user has_role@'localhost'; > +drop user no_role@'localhost'; > +drop role test_role; > +drop table view_role_test; > +drop view v_view_role_test; > +drop view v_view_role_test_2; > diff --git a/mysql-test/suite/roles/current_role_view-12666.test b/mysql-test/suite/roles/current_role_view-12666.test > new file mode 100644 > index 00000000000..86c74260b6e > --- /dev/null > +++ b/mysql-test/suite/roles/current_role_view-12666.test > @@ -0,0 +1,52 @@ > +CREATE USER has_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO has_role@'localhost'; > + > +CREATE ROLE test_role; > +GRANT test_role TO has_role@'localhost'; > + > +CREATE USER no_role@'localhost'; > +GRANT ALL PRIVILEGES ON *.* TO no_role@'localhost'; > + > +CREATE TABLE view_role_test ( > + id int primary key, > + role_name varchar(50) > + ); > + > +INSERT INTO view_role_test VALUES (1, 'test_role'); > + > + > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > + > +show create view v_view_role_test; > + > +SET ROLE test_role; > + > +CREATE OR REPLACE > +DEFINER = no_role@localhost > +SQL SECURITY INVOKER > +VIEW v_view_role_test_2 > +AS > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > + > +show create view v_view_role_test_2; > + > + > +SELECT CURRENT_USER(); > +SELECT CURRENT_ROLE(); > + > +SELECT * FROM view_role_test WHERE role_name = CURRENT_ROLE(); > +SELECT * FROM v_view_role_test; > + > + > +drop user has_role@'localhost'; > +drop user no_role@'localhost'; > +drop role test_role; > + > +drop table view_role_test; > +drop view v_view_role_test; > +drop view v_view_role_test_2; > diff --git a/sql/item.cc b/sql/item.cc > index 4ce8396f71e..2ee3a6d61dc 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -1263,9 +1263,20 @@ Item *Item::const_charset_converter(CHARSET_INFO *tocs, > DBUG_ASSERT(fixed); > StringBuffer<64>tmp; > String *s= val_str(&tmp); > - if (!s) > + > + /* Functions that return NULL during prepare must not be replaced with > + Item_null as they may return non-null values during execution. > + Ex: CURRENT_ROLE() during view creation time can be NULL if no role > + is set. */ > + if (!s && !func_name) > return new Item_null((char *) func_name, tocs); > > + if (!s) > + { > + tmp.set_ascii("", 0); > + s = &tmp; > + } > + > if (!needs_charset_converter(s->length(), tocs)) > { > if (collation.collation == &my_charset_bin && tocs != &my_charset_bin && > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits >

1 0

Re: [Maria-developers] [Commits] b400696: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 22 May '17

22 May '17

Hi Varun, Please find my input below. I would like again to voice my dissatisfaction that I have to point out coding style violations in pretty much every patch I get. The other error in the patch is also pretty basic, ok to have it once but I think it is reasonable to expect more polished patches to be submitted for code review. On Sat, May 20, 2017 at 01:50:42AM +0530, Varun wrote: > revision-id: b400696386529fa63e79f723bbb06c05f7bae820 (mariadb-10.2.2-416-gb400696) > parent(s): fff61e31eca83a0a9af7c30e2bcda8309e9c695a > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-20 01:49:34 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > mysql-test/r/innodb_ext_key.result | 45 ++++++++++++++++++++++++++++++++++++++ > mysql-test/t/innodb_ext_key.test | 31 ++++++++++++++++++++++++++ > sql/table.cc | 36 +++++++++++++++++++++++++++++- > 3 files changed, 111 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/innodb_ext_key.result b/mysql-test/r/innodb_ext_key.result > index 1305be8..66391d6 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1133,5 +1133,50 @@ where index_date_updated= 10 and index_id < 800; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t2 range index_date_updated index_date_updated 13 NULL # Using index condition > drop table t0,t1,t2; > +# > +# MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > +# was corrupted, server crashes in opt_sum_query > +CREATE TABLE t1 ( > +pk INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1024), > +PRIMARY KEY (pk), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8; > +INSERT INTO t1 VALUES (1,'foo','abc'),(2,'bar','def'); > +SELECT MAX(t2.pk) FROM t1 t2 INNER JOIN t1 t3 ON t2.f1 = t3.f1 WHERE t2.pk <= 4; > +MAX(t2.pk) > +2 > +drop table t1; > +CREATE TABLE t1 ( > +pk1 INT, > +pk2 INT, > +f1 VARCHAR(3), > +f2 VARCHAR(1021), > +PRIMARY KEY (pk1,pk2), > +KEY(f2) > +) ENGINE=InnoDB CHARSET utf8; > +INSERT INTO t1 VALUES (1,2,'2','abc'),(2,3,'3','def'); > +explain format= json > +select * from t1 force index(f2) where pk1 <= 5 and pk2 <=5 and f2 = 'abc' and f1 <= '3'; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "table": { > + "table_name": "t1", > + "access_type": "range", > + "possible_keys": ["f2"], > + "key": "f2", > + "key_length": "3070", > + "used_key_parts": ["f2", "pk1"], > + "rows": 1, > + "filtered": 100, > + "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'", > + "attached_condition": "t1.f1 <= '3'" > + } > + } > +} > +drop table t1; > set optimizer_switch=@save_ext_key_optimizer_switch; > SET SESSION STORAGE_ENGINE=DEFAULT; ... > diff --git a/sql/table.cc b/sql/table.cc > index 398383e..7d88eba 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2104,6 +2104,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > for (uint key=0 ; key < keys ; key++,keyinfo++) > { > uint usable_parts= 0; > + KEY* key_first_info; This variable may be used un-initialized. If I change the above line to be KEY* key_first_info=(KEY*)0x1; and run this statement: CREATE TABLE t1a ( pk INT, f1 VARCHAR(3), f2 VARCHAR(1024), f2a VARCHAR(1024), PRIMARY KEY (pk), KEY(f2), KEY(f2a) ) ENGINE=InnoDB CHARSET utf8; Then I get a crash. > keyinfo->name=(char*) share->keynames.type_names[key]; > keyinfo->name_length= strlen(keyinfo->name); > keyinfo->cache_name= > @@ -2118,19 +2119,38 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > keyinfo->name_length+1); > } > > + if(!key) Coding style: 'if (' ... > + key_first_info= keyinfo; > + > if (ext_key_parts > share->key_parts && key) > { > KEY_PART_INFO *new_key_part= (keyinfo-1)->key_part + > (keyinfo-1)->ext_key_parts; > uint add_keyparts_for_this_key= add_first_key_parts; > + uint length_bytes= 0, len_null_byte= 0, ext_key_length= 0; > + Field *field; > > /* > Do not extend the key that contains a component > defined over the beginning of a field. > */ > for (i= 0; i < keyinfo->user_defined_key_parts; i++) > - { > + { Coding style: wrong identation. > uint fieldnr= keyinfo->key_part[i].fieldnr; > + field= share->field[keyinfo->key_part[i].fieldnr-1]; > + > + if (field->null_ptr) > + len_null_byte= HA_KEY_NULL_LENGTH; > + > + if (field->type() == MYSQL_TYPE_BLOB || > + field->real_type() == MYSQL_TYPE_VARCHAR || > + field->type() == MYSQL_TYPE_GEOMETRY) > + { > + length_bytes= HA_KEY_BLOB_LENGTH; > + } > + > + ext_key_length+= keyinfo->key_part[i].length + len_null_byte > + + length_bytes; > if (share->field[fieldnr-1]->key_length() != > keyinfo->key_part[i].length) > { > @@ -2139,6 +2159,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > } > } > > + if (add_keyparts_for_this_key) > + { > + for (i= 0; i < add_keyparts_for_this_key; i++) > + { > + uint pk_part_length= key_first_info->key_part[i].store_length; > + if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + { > + add_keyparts_for_this_key= i; > + break; > + } > + ext_key_length+= pk_part_length; > + } > + } > + > if (add_keyparts_for_this_key < (keyinfo->ext_key_parts - > keyinfo->user_defined_key_parts)) > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 55a4858fe6f: MDEV-12666: CURRENT_ROLE() does not work in a view
by Sergei Golubchik 22 May '17

22 May '17

Hi, Vicentiu! Another possible approach (that doesn't necessarily penalizes other const functions that happen to return NULL) would be for Item_func_sysconst not to behave as const during view creation. Because they really aren't, sysconst functions can change between view creation and a select from the view. The fix would be, then Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) { + if (thd->lex->context_analysis_only & CONTEXT_ANALYSIS_ONLY_VIEW) + return Item_str_func::safe_charset_converter(thd, tocs); return const_charset_converter(thd, tocs, true, fully_qualified_func_name() } And possibly (not sure) other values of context_analysis_only would need a similar treatment here. On May 22, vicentiu wrote: > revision-id: 55a4858fe6f3fdc63a072b93b9bc3d8e51dc5e94 (mariadb-10.0.30-76-g55a4858fe6f) > parent(s): 8d1827f5540a0def82b1e2cbc557245c3e5e14ca > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-05-22 17:06:01 +0300 > message: > > MDEV-12666: CURRENT_ROLE() does not work in a view > > The problem lies in how CURRENT_ROLE is defined. The > Item_func_current_role inherits from Item_func_sysconst, which defines > a safe_charset_converter to be a const_charset_converter. > > During view creation, if there is no role previously set, the current_role() > function returns NULL. > > This is captured on item instantiation and the > const_charset_converter call subsequently returns an Item_null. > In turn, the function is replaced with Item_null and the view is > then created with an Item_null instead of Item_func_current_role. > > Without this patch, the first SHOW CREATE VIEW from the testcase would > have a where clause of WHERE role_name = NULL, while the second SHOW > CREATE VIEW would show a correctly created view. > > The solution proposed is to not replace any function with an Item_null, > but instead use an Item_static_string_func. > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MariaDB License Clarification
by Ryan Coe 19 May '17

19 May '17

Hello, My name is Ryan Coe and I've been maintaining the MariaDB package for Buildroot (https://buildroot.org) In commit 577915def8d39223aa23a06f898e11e82e72365a, COPYING.LESSER was removed. Our packaging currently states the MariaDB license as GPL-2.0 (server), GPL-2.0 with FLOSS exception (GPL client library), LGPL-2.0 (LGPL client library). Could I please get clarification on how the different components are licensed? Also, the GitHub page is still referencing COPYING.LESSER in the README. Thank you.

2 1

Re: [Maria-developers] strange IF in translog_flush_buffers()
by Sergei Golubchik 18 May '17

18 May '17

Hi, Sanja! On May 17, Oleksandr Byelkin wrote: > Hi! > > Actually code should work as it is, but it check unnecessary condition and > also very dificult to understand. I propose following fix (comment in it > explains why it work now and why condition is always TRUE) I cannot review it, this is between you and Monty. If you think it's ok, please push it (into 10.0). Or I can push it together with all other similar fixes. > diff --git a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c > index 86e51cc3526..e680e830703 100644 > --- a/storage/maria/ma_loghandler.c > +++ b/storage/maria/ma_loghandler.c > @@ -7789,8 +7789,23 @@ void translog_flush_buffers(TRANSLOG_ADDRESS *lsn, > translog_force_current_buffer_to_finish(); > translog_buffer_unlock(buffer); > } > - else if (log_descriptor.bc.buffer->prev_last_lsn != LSN_IMPOSSIBLE) > + else > { > + if (log_descriptor.bc.buffer->last_lsn == LSN_IMPOSSIBLE) > + { > + /* > + In this case both last_lsn & prev_last_lsn are LSN_IMPOSSIBLE > + otherwise it will go in the first IF because LSN_IMPOSSIBLE less > + then any real LSN and cmp_translog_addr(*lsn, > + log_descriptor.bc.buffer->prev_last_lsn) will be TRUE > + */ > + DBUG_ASSERT(log_descriptor.bc.buffer->prev_last_lsn == > + LSN_IMPOSSIBLE); > + DBUG_PRINT("info", ("There is no LSNs yet generated => do nothing")); > + translog_unlock(); > + DBUG_VOID_RETURN; > + } > + > /* fix lsn if it was horizon */ > *lsn= log_descriptor.bc.buffer->prev_last_lsn; > DBUG_PRINT("info", ("LSN to flush fixed to prev last lsn: (%lu,0x%lx)", > @@ -7799,13 +7814,6 @@ void translog_flush_buffers(TRANSLOG_ADDRESS *lsn, > TRANSLOG_BUFFERS_NO); > translog_unlock(); > } > - else if (log_descriptor.bc.buffer->last_lsn == LSN_IMPOSSIBLE) > - { > - DBUG_PRINT("info", ("There is no LSNs yet generated => do nothing")); > - translog_unlock(); > - DBUG_VOID_RETURN; > - } > - > /* flush buffers */ > *sent_to_disk= translog_get_sent_to_disk(); > if (cmp_translog_addr(*lsn, *sent_to_disk) > 0) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-12803 Improve function parameter data type control
by Alexander Barkov 15 May '17

15 May '17

Hello Alexey, can you please review a patch for MDEV-12803? Thanks!

1 0

[Maria-developers] String functions have their item->maybe_null=true unnecessarily, blocking optimizations
by Sergey Petrunia 15 May '17

15 May '17

Hi Alexander, I was looking at https://jira.mariadb.org/browse/MDEV-11893 and found this piece of code: bool Item_str_func::fix_fields(THD *thd, Item **ref) { bool res= Item_func::fix_fields(thd, ref); /* In Item_str_func::check_well_formed_result() we may set null_value flag on the same condition as in test() below. */ maybe_null= maybe_null || thd->is_strict_mode(); return res; } This has been introduced by this cset: https://github.com/MariaDB/server/commit/af22eb35e577ef17226faf662f2cffc470… Which says: Add Item_str_func::fix_fields() implementation, and set maybe_null to TRUE if we are in the SQL mode that requires some functions to return null even if they normally do not. The patch has only one example of CHAR() function doing that. At the same time, not having NOT-NULL attribute disallows query opimizations (see MDEV-11893 for an example). I think we should have this maybe_null= maybe_null || thd->is_strict_mode(); logic on a case-by-case basis only. I wanted to check with you - are there any other known string functions that, as the patch puts it "return null even if they normally do not"? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

[Maria-developers] MDEV-11340 Allow multiple alternative authentication methods for the same user
by serg＠mariadb.org 15 May '17

15 May '17

Hi, Ankit! I've started writing down my thoughts about MDEV-11340 "Allow multiple alternative authentication methods for the same user" See MDEV-11340 and feel free to comment on it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 8edbb1117a9: MDEV-12741: innodb.ibuf_not_empty failed in buildbot with "InnoDB: Trying to do I/O to a tablespace which does not exist"
by Marko Mäkelä 15 May '17

15 May '17

On Thu, May 11, 2017 at 11:25 AM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: 8edbb1117a9e1fd81fbd08b8f1d06c72efe38f44 (mariadb-10.2.5-168- > g8edbb1117a9) > parent(s): f4df8c00c957146f04db031f1f9f80aab229d1d2 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-05-11 11:15:37 +0300 > message: > > MDEV-12741: innodb.ibuf_not_empty failed in buildbot with "InnoDB: Trying > to do I/O to a tablespace which does not exist" > > Background thread is doing ibuf merge, in buf0rea.cc > buf_read_ibuf_merge_pages(). > It first tries to get page_size and if space is not found it deletes them, > but > as we do not hold any mutexes, space can be marked as stopped between that > and buf_read_page_low() for same space. This naturally leads seen error > message on log. > > buf_read_page_low(): Add parameter ignore_missing_space = false that > is passed to fil_io() > [snip] > @@ -178,7 +180,7 @@ buf_read_page_low( > > *err = fil_io( > request, sync, page_id, page_size, 0, page_size.physical(), > - dst, bpage); > + dst, bpage, ignore_missing_space); > > if (sync) { > thd_wait_end(NULL); > Can you please add the parameter to the IORequest? It would seem to be a more appropriate place for this. (Ideally, fil_io() would only take the request as a parameter, but we do not want to do such a big refactoring right now.) Marko

1 0

Re: [Maria-developers] [Commits] e6dabcd: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 11 May '17

11 May '17

Hi Varun, So I am debugging the testcase from the bug report. I reach these lines: > if(i) > ext_key_length+= key_part->length; (Why there is no indentation btw?) and I see: (gdb) p key_part->length $40 = 3072 3072=1024*3, that is, the NULL-byte and length bytes are NOT counted. The code then proceeds to check if pk_length + ext_key_length <= MAX_KEY_LENGTH but that check could produce a wrong result if NULL- and length- bytes are not counted. The second issue is that I don't see where ext_key_length is zero'ed. I try debugging this query: CREATE TABLE t2 ( pk INT, f1 VARCHAR(3), f2 VARCHAR(1024), f2a VARCHAR(1024), PRIMARY KEY (pk), KEY(f2), KEY(f2a) ) ENGINE=InnoDB CHARSET utf8; I put a breakpoint at this line: > if (share->use_ext_keys && i && !(keyinfo->flags & HA_NOSAME) > && (pk_length + ext_key_length <= MAX_KEY_LENGTH)) (84 chars! where's the coding style again?) and I observe: (gdb) print ext_key_length $45 = 6144 This is clearly more than any [possible] key length for the table. So, this particular patch is not a solution (even if the testcase happens to pass with it). On Mon, May 08, 2017 at 04:23:11PM +0530, Varun wrote: > revision-id: e6dabcd3a7e994feaa1a1dc5d0c1a0d621272f72 (mariadb-10.2.2-387-ge6dabcd) > parent(s): 39bce67b7aa27fc7e5458f36838148f6e0f4c3af > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-05-08 16:21:32 +0530 > message: > > MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff' > was corrupted, server crashes in opt_sum_query > > extended keys feature disabled if the length of extended key is longer than MAX_KEY_LEN > > --- > sql/table.cc | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/sql/table.cc b/sql/table.cc > index 398383e..554fc78 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -695,7 +695,7 @@ static bool create_key_infos(const uchar *strpos, const uchar *frm_image_end, > KEY_PART_INFO *key_part= NULL; > ulong *rec_per_key= NULL; > KEY_PART_INFO *first_key_part= NULL; > - uint first_key_parts= 0; > + uint first_key_parts= 0, pk_length=0, ext_key_length=0; > > if (!keys) > { > @@ -765,6 +765,7 @@ static bool create_key_infos(const uchar *strpos, const uchar *frm_image_end, > keyinfo->algorithm= first_keyinfo->algorithm; > if (new_frm_ver >= 3) > keyinfo->block_size= first_keyinfo->block_size; > + pk_length= keyinfo->key_length; > } > > keyinfo->key_part= key_part; > @@ -796,6 +797,8 @@ static bool create_key_infos(const uchar *strpos, const uchar *frm_image_end, > strpos+=7; > } > key_part->store_length=key_part->length; > + if(i) > + ext_key_length+= key_part->length; > } > > /* > @@ -805,7 +808,8 @@ static bool create_key_infos(const uchar *strpos, const uchar *frm_image_end, > keyinfo->ext_key_parts= keyinfo->user_defined_key_parts; > keyinfo->ext_key_flags= keyinfo->flags; > keyinfo->ext_key_part_map= 0; > - if (share->use_ext_keys && i && !(keyinfo->flags & HA_NOSAME)) > + if (share->use_ext_keys && i && !(keyinfo->flags & HA_NOSAME) > + && (pk_length + ext_key_length <= MAX_KEY_LENGTH)) > { > for (j= 0; > j < first_key_parts && keyinfo->ext_key_parts < MAX_REF_PARTS; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MDEV-10764 PL/SQL parser - Phase 2 / MDEV-12137
by jerome brauge 11 May '17

11 May '17

2 1

[Maria-developers] mariadb-java-client 1.6.0 does not work from yesterday update ?
by Krishnan, Gnanavel (Nokia - IN/Chennai) 11 May '17

11 May '17

Hi All, I was using mariadb java client jar version 1.6.0. but looks it is not present in release document Also from yesterday 1.6.0 jar gets updated not sure why Should use only 1.5.9 or 2.0.0 rc ? https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/1.6.0/ Thank you, Gnanavel.

1 0

Re: [Maria-developers] [Commits] 39bce67: MDEV-11196: Error:Run-Time Check Failure #2 - Stack around the variable 'key_buff'
by Sergey Petrunia 10 May '17

10 May '17

Hi Varun, First reaction: > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 84e4018..63636f9 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -3948,7 +3948,11 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, > key_part_info->fieldnr= field; > key_part_info->offset= (uint16) sql_field->offset; > key_part_info->key_type=sql_field->pack_flag; > - uint key_part_length= sql_field->key_length; > + uint key_part_length= MY_MAX(sql_field->key_length, > + sql_field->pack_length); What does the above do? It is not clear why MAX should be used. (Let's denote the compared variables X and Y. Can X>Y and X<Y both happen in practice? This requires a comment!) > + > + if (!(sql_field->flags & NOT_NULL_FLAG)) > + key_part_length++; > > if (column->length) > { I am concerned that the above may have an undesired effect: key_part_kength counts NULL-byte while previously it did not. Let's try this example: create table t1 (pk int not null primary key, col1 int, key(col1) ) engine=innodb; Run it, and put a breakpoint below on this code: /* Check if the key segment is partial, set the key flag accordingly */ if (key_part_length != sql_field->key_length) key_info->flags|= HA_KEY_HAS_PART_KEY_SEG; You will see that for filed 'col1' HA_KEY_HAS_PART_KEY_SEG flag is now set. This didn't happen before. Is this really intended? I wasn't able to find what exactly may get wrong, but if I run this query: explain select * from t1 where col1=1; then it hangs. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MDEV-12775 Reuse data type aggregation code for hybrid functions and UNION
by Alexander Barkov 10 May '17

10 May '17

Hello Sanja, Please review a patch for MDEV-12775, also fixing a number of bugs: - MDEV-9495 Wrong field type for a UNION of a signed and an unsigned INT expression - MDEV-9497 UNION and COALESCE produce different field types for DECIMAL+INT - MDEV-12594 UNION between fixed length double columns does not always preserve scale - MDEV-12595 UNION converts INT to BIGINT - MDEV-12599 UNION is not symmetric when mixing INT and CHAR Thanks!

1 0

[Maria-developers] galera source and direction
by Daniel Black 09 May '17

09 May '17

With Codership's galera repository showing spelling fixes and test case changes as the only changes this year, and Percona's galera repository streaming ahead with improvements, is Percona's galera becoming the new base? I did a quick test though and the mtr tests all pass with Percona's galera on the 10.2 branch head.

1 0

[Maria-developers] Google Summer of Code 2017 students
by Daniel Black 05 May '17

05 May '17

Folks I'd like to welcome our Google Summer of Code students. https://summerofcode.withgoogle.com/organizations/5889423829893120/#projects Ankit Kumar - Allow multiple alternative authentication methods for the same user mentor: Sergei Golubchik Daniil Medvedev - Full DECIMAL support in ColumnStore mentor: Andrew Hutchings Sumit Lakra - NUMA GSoC Project Proposal mentors: Daniel Black and Jan Lindström Galina Shalygina - Transformation of IN predicate into IN subquery based on implementation of table value constructor mentor: Igor Babaev Please make them welcome and we are looking forward to mentoring them to develop their contributions.

1 0

[Maria-developers] MDEV-7049 MySQL#74585 - InnoDB: Failing assertion: *mbmaxlen < 5 in file ha_innodb.cc line 1904
by Alexander Barkov 04 May '17

04 May '17

Hello Marko, InnoDB uses 5 bits to store the combination of mbminlen and mbmaxlen, and its calculated as: mbmaxlen * 5 + mbminlen Note, combinations with mbminlen=0 and mbmaxlen=0 are not possible. Also, combinations with mbminlen>mbmaxlen are not possible. This is a summary: mbminlen mbmaxlen 0 1 2 3 4 0 (0) (1) (2) (3) (4) 1 (5) 6 (7) (8) (9) 2 (10) 11 12 (13) (14) 3 (15) 16 17 18 (19) 4 (20) 21 22 23 24 I marked impossible numbers with parentheses. In the range 0..24: - 10 pairs are valid - 15 pairs are not valid It should be safe to use any impossible number to mean mbmaxlen=5 & mbminlen=1 It seems there is no really a need to extend the header to use separate 3 bits for both mbminlen and mbmaxlen.

2 2

[Maria-developers] MDEV-10142 - bb-10.2-compatibility
by jerome brauge 04 May '17

04 May '17

Hello Alexander, This is the patch for CHR(). Function CHR(n) returns the character having the binary equivalent to n in the database character set. Returns null on null input. Regard, Jérôme. PS : I share this code under terms of MCA. > -----Message d'origine----- > De : Alexander Barkov [mailto:bar@mariadb.org] > Envoyé : mercredi 26 avril 2017 06:12 > À : jerome brauge > Cc : MariaDB Developers > Objet : Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > > Hello Jerome, > > Thanks for working on this! > > On 04/25/2017 11:42 PM, jerome brauge wrote: > > Hello Alexander, > > Some times ago, we have had the below discussion. > > I made a patch following my first idea (it's enough from our point of view) : > replace empty string by null in literals and in make_empty_string. > > I think this patch can be complementary to your suggestion of adding > > an Item_func_eq_oracle (for data inserted in default sql_mode) > > > > This patch also contains following some functions changes: > > - replace ('ab','a',null) returns 'a' instead null > > - trim / ltrim / rtrim returns null when input only contains spaces > > - lpad /rpad can accept 2 args (default padding char is space) and if > > length equals to 0, returns null -substring : if start index is equal > > to 0, act as if it's equals to 1 -add a function chr() as a synonym of > > char() (but accept only one arg) -add a function lengthb() as a > > synonym of lentgh() -change the semantic of length for oracle (where > > length() is the character length and not the byte length) > > > > What do you think about this patch ? > > > > (Perhaps I have to make 2 patchs, one for empty string, and one for > > functions changes with separate test file for each modified function) > > Right, can you please split the patch into pieces? > I suggest even more pieces than two. One patch for one function. > I'll create MDEVs, one MDEV per function and put examples how Oracle > works. > > Can we do it in the following order: > > 1. LPAD / RPAD - making the third parameter optional. > This extension will be good for all sql_modes. > I checked IBM DB2, Oracle PostgreSQL, Firebird - they all have > the third parameter to LPAD/RPAD optional. > > 2. CHR(). > > 3. LENGTH() and LENGTHB(). > > > Then we can continue with REPLACE, TRIM. SUBSTRING in no particular > order, one patch per function. > > > Some general notes: > > > 1. Unify *.yy files > > Please put sql_mode dependent code into LEX rather than *.yy files directly. > We're going to unify the two files soon. See here for details: > MDEV-12518) Unify sql_yacc.yy and sql_yacc_ora.yy > > So instead of doing: > > | SUBSTRING '(' expr ',' expr ',' expr ')' > { > $$= new (thd->mem_root) Item_func_substr(thd, $3, $5, $7); > if ($$ == NULL) > MYSQL_YYABORT; > } > > > Please do something like this in both sql_yacc.yy and sql_yacc_ora.yy: > > > | SUBSTRING '(' expr ',' expr ',' expr ')' > { > if (!($$= Lex->make_item_func_substr(thd, $3, $5, $7))) > MYSQL_YYABORT; > } > > where LEX::make_item_func_substr() is a new method, creating either > Item_func_substr or Item_func_substr_oracle depending on sql_mode. > > The same applies to all other functions, and to text_literal. > > > 2. Don't use Item::is_null() when you call val_str() anyway. > > There are two reasons that: > - Performance > is_null() involves the second val_str() call, which can be resource consuming > in case of complex functions and subselect. > > - Side effect > Expressions can have some side effects. > For example, in case if the argument is a stored function which counts how > many times it was called and stores the counter in a user variable, or writes > something to a table on every execution. > If we do is_null() followed by val_str(), the side effect will done two times. > We need one time only. > > So instead of: > > String *Item_func_replace_oracle::val_str(String *str) { > String * res; > if (res = replace_value(str, > args[1]->is_null() ? &tmp_emtpystr : > rgs[1]->val_str(&tmp_value2), > args[2]->is_null() ? &tmp_emtpystr : > rgs[2]->val_str(&tmp_value3)) > > return (res->length() == 0) ? make_empty_result () : res; > else > return 0; > } > > > Please do something like this: > > String *Item_func_replace_oracle::val_str(String *str) { > String * res; > String *str1= args[1]->val_str(&tmp_value1); > String *str2= args[2]->val_str(&tmp_value2); > if (res= replace_value(str, > str1 ? str1 : &tmp_emptystr, > str2 ? str2 : &tmp_emptystr) > return (res->length() == 0) ? make_empty_result () : res; > else > return 0; > } > > > Thanks! > > > > > > > Best regards, > > Jérôme. > > > >> -----Message d'origine----- > >> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 23 > >> janvier 2017 12:49 À : jerome brauge Cc : MariaDB Developers Objet : > >> Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > >> > >> Hi Jerome, > >> > >> On 01/18/2017 01:22 PM, jerome brauge wrote: > >>> Hello Alexander, > >>> Sometime ago, when I ask you about plan for MDEV-10574, you replied : > >>> > >>>> The current plan is to do these transformations: > >>>> > >>>> 1. Transform Insert > >>>> - insert values ("") -> insert values (null) > >>>> > >>>> 2. Transform Select > >>>> > >>>> - where v=x => (v <> "" and V=X) > >>>> - where v is null => (v="" or v is null) > >>>> > >>>> We didn't plan to change functions yet. Thanks for bringing this up. > >>>> We'll discuss this. > >>> > >>> I've done some tests just by changing : > >>> - insert an Item_null instead of an Item_string when $1.length==0 in > >>> rule text_literal of sql_yacc_ora.yy > >>> - return null instead of an empty string in > >>> Item_str_func::make_empty_result > >>> > >>> My first tests seem promising. > >>> > >>> Of course this solution does not allow to "see" the records created > >>> with > >> empty strings as null values. > >>> I don't see the importance of being able to do this in a transparent way. > >>> We can explicitly select these row by adding rtrim on these columns. > >>> > >>> If you are interesting, I can begin to write a test to evaluate the > >>> coverage of > >> this solution. > >> > >> I'm afraid this will fix the behavior only for literals. > >> > >> This script: > >> > >> DROP TABLE t1; > >> CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10)); INSERT INTO t1 > VALUES > >> ('',''); SELECT COUNT(*) FROM t1 WHERE a=''; SELECT COUNT(*) FROM t1 > >> WHERE a=b; > >> > >> returns 0 for both SELECT queries in Oracle, and returns 1 for both > >> SELECT queries in MariaDB, Your approach will fix the first SELECT only. > >> The second will still return 1. > >> > >> > >> For now, I'm not sure how to do this correctly. Some ways possible: > >> > >> 1. We could add new classes, e.g. Item_func_eq_oracle as a pair for > >> Item_func_eq, which will handle both empty strings and NULLs inside > >> their > >> val_int() implementations. > >> > >> 2. Or there could be some after-processing on the created Item tree > >> which will replace all things like Item_func_eq to Item_cond_and with > >> Item_func_eq and Item_func_null_predicate passed as arguments. > >> > >> Currently I'm inclined to #1, because we don't have a good mechanism > >> to clone items to implement #2 correctly. We'd need such cloning to > >> pass arguments both to Item_func_eq and Item_func_null_predicate > correctly. > >> Some places in the code don't expect that the same arguments can be > >> passed to two different Item_func instances. > >> > >> On the other hand, implementing #1 will need more changes in the > >> optimizer. > >> > >> We were going to think on this later. > >> > >>> > >>> Best regard. > >>> Jérôme. > >>>

2 6

[Maria-developers] cte page
by Galina Shalygina 04 May '17

04 May '17

мои артиклы

4 5

[Maria-developers] Please review a patch for MDEV-12665 Hybrid functions do not preserve geometry type
by Alexander Barkov 02 May '17

02 May '17

Hello Alexey, Please review a joint patch fixing these two problems: MDEV-12665 Hybrid functions do not preserve geometry type MDEV-12560 Wrong data type for SELECT NULL UNION SELECT Point(1,1) Thanks!

1 0

[Maria-developers] Race Condition in Seconds behind master.
by Sachin Setiya 02 May '17

02 May '17

Hello Everyone! I setup a classical master-slave server. Master is constantly running query like this insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); insert into t1 values(1,1,1,1,1); flush logs; insert into t1 values(1,1,1,1,1); {there are arround million querys} Then I executed Show slave status (couple of times in a second), and found Seconds_behind_master is changing arbitrarily MariaDB [(none)]> show slave status\G *************************** 1. row *************************** Slave_IO_State: Queueing master event to the relay log Master_Host: localhost Master_User: root Master_Port: 16000 Connect_Retry: 60 Master_Log_File: master-bin.014595 Read_Master_Log_Pos: 4 Relay_Log_File: slave-relay-bin.029186 Relay_Log_Pos: 3557 Relay_Master_Log_File: master-bin.014593 Slave_IO_Running: Yes Slave_SQL_Running: Yes Replicate_Do_DB: Replicate_Ignore_DB: Replicate_Do_Table: Replicate_Ignore_Table: Replicate_Wild_Do_Table: Replicate_Wild_Ignore_Table: Last_Errno: 0 Last_Error: Skip_Counter: 0 Exec_Master_Log_Pos: 3257 Relay_Log_Space: 184375 Until_Condition: None Until_Log_File: Until_Log_Pos: 0 Master_SSL_Allowed: No Master_SSL_CA_File: Master_SSL_CA_Path: Master_SSL_Cert: Master_SSL_Cipher: Master_SSL_Key: Seconds_Behind_Master: 148 Master_SSL_Verify_Server_Cert: No Last_IO_Errno: 0 Last_IO_Error: Last_SQL_Errno: 0 Last_SQL_Error: Replicate_Ignore_Server_Ids: Master_Server_Id: 1 Master_SSL_Crl: Master_SSL_Crlpath: Using_Gtid: Current_Pos Gtid_IO_Pos: 0-1-145942 Replicate_Do_Domain_Ids: Replicate_Ignore_Domain_Ids: Parallel_Mode: conservative SQL_Delay: 0 SQL_Remaining_Delay: NULL Slave_SQL_Running_State: Reading event from the relay log 1 row in set (0.01 sec) MariaDB [(none)]> show slave status\G *************************** 1. row *************************** Slave_IO_State: Waiting for master to send event Master_Host: localhost Master_User: root Master_Port: 16000 Connect_Retry: 60 Master_Log_File: master-bin.014632 Read_Master_Log_Pos: 3257 Relay_Log_File: slave-relay-bin.029264 Relay_Log_Pos: 1548 Relay_Master_Log_File: master-bin.014632 Slave_IO_Running: Yes Slave_SQL_Running: Yes Replicate_Do_DB: Replicate_Ignore_DB: Replicate_Do_Table: Replicate_Ignore_Table: Replicate_Wild_Do_Table: Replicate_Wild_Ignore_Table: Last_Errno: 0 Last_Error: Skip_Counter: 0 Exec_Master_Log_Pos: 1248 Relay_Log_Space: 179996 Until_Condition: None Until_Log_File: Until_Log_Pos: 0 Master_SSL_Allowed: No Master_SSL_CA_File: Master_SSL_CA_Path: Master_SSL_Cert: Master_SSL_Cipher: Master_SSL_Key: Seconds_Behind_Master: 0 Master_SSL_Verify_Server_Cert: No Last_IO_Errno: 0 Last_IO_Error: Last_SQL_Errno: 0 Last_SQL_Error: Replicate_Ignore_Server_Ids: Master_Server_Id: 1 Master_SSL_Crl: Master_SSL_Crlpath: Using_Gtid: Current_Pos Gtid_IO_Pos: 0-1-146322 Replicate_Do_Domain_Ids: Replicate_Ignore_Domain_Ids: Parallel_Mode: conservative SQL_Delay: 0 SQL_Remaining_Delay: NULL Slave_SQL_Running_State: init 1 row in set (0.00 sec) MariaDB [(none)]> show slave status\G *************************** 1. row *************************** Slave_IO_State: Queueing master event to the relay log Master_Host: localhost Master_User: root Master_Port: 16000 Connect_Retry: 60 Master_Log_File: master-bin.014670 Read_Master_Log_Pos: 299 Relay_Log_File: slave-relay-bin.029336 Relay_Log_Pos: 3557 Relay_Master_Log_File: master-bin.014668 Slave_IO_Running: Yes Slave_SQL_Running: Yes Replicate_Do_DB: Replicate_Ignore_DB: Replicate_Do_Table: Replicate_Ignore_Table: Replicate_Wild_Do_Table: Replicate_Wild_Ignore_Table: Last_Errno: 0 Last_Error: Skip_Counter: 0 Exec_Master_Log_Pos: 3257 Relay_Log_Space: 184569 Until_Condition: None Until_Log_File: Until_Log_Pos: 0 Master_SSL_Allowed: No Master_SSL_CA_File: Master_SSL_CA_Path: Master_SSL_Cert: Master_SSL_Cipher: Master_SSL_Key: Seconds_Behind_Master: 150 Master_SSL_Verify_Server_Cert: No Last_IO_Errno: 0 Last_IO_Error: Last_SQL_Errno: 0 Last_SQL_Error: Replicate_Ignore_Server_Ids: Master_Server_Id: 1 Master_SSL_Crl: Master_SSL_Crlpath: Using_Gtid: Current_Pos Gtid_IO_Pos: 0-1-146692 Replicate_Do_Domain_Ids: Replicate_Ignore_Domain_Ids: Parallel_Mode: conservative SQL_Delay: 0 SQL_Remaining_Delay: NULL Slave_SQL_Running_State: Reading event from the relay log -- Regards Sachin Setiya Software Engineer at MariaDB

3 2

[Maria-developers] bb-10.2-ext / rpad lpad with 2 args
by jerome brauge 02 May '17

02 May '17

Hi Alexander, This is the patch for rpad/lpad with 2 args (call of make_empty_string() in val_str() is to prepare MDEV-10574). Regard, Jérôme. PS : I share this code under terms of MCA. > -----Message d'origine----- > De : Alexander Barkov [mailto:bar@mariadb.org] > Envoyé : mercredi 26 avril 2017 06:12 > À : jerome brauge > Cc : MariaDB Developers > Objet : Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > > Hello Jerome, > > Thanks for working on this! > > On 04/25/2017 11:42 PM, jerome brauge wrote: > > Hello Alexander, > > Some times ago, we have had the below discussion. > > I made a patch following my first idea (it's enough from our point of view) : > replace empty string by null in literals and in make_empty_string. > > I think this patch can be complementary to your suggestion of adding > > an Item_func_eq_oracle (for data inserted in default sql_mode) > > > > This patch also contains following some functions changes: > > - replace ('ab','a',null) returns 'a' instead null > > - trim / ltrim / rtrim returns null when input only contains spaces > > - lpad /rpad can accept 2 args (default padding char is space) and if > > length equals to 0, returns null -substring : if start index is equal > > to 0, act as if it's equals to 1 -add a function chr() as a synonym of > > char() (but accept only one arg) -add a function lengthb() as a > > synonym of lentgh() -change the semantic of length for oracle (where > > length() is the character length and not the byte length) > > > > What do you think about this patch ? > > > > (Perhaps I have to make 2 patchs, one for empty string, and one for > > functions changes with separate test file for each modified function) > > Right, can you please split the patch into pieces? > I suggest even more pieces than two. One patch for one function. > I'll create MDEVs, one MDEV per function and put examples how Oracle > works. > > Can we do it in the following order: > > 1. LPAD / RPAD - making the third parameter optional. > This extension will be good for all sql_modes. > I checked IBM DB2, Oracle PostgreSQL, Firebird - they all have > the third parameter to LPAD/RPAD optional. > > 2. CHR(). > > 3. LENGTH() and LENGTHB(). > > > Then we can continue with REPLACE, TRIM. SUBSTRING in no particular > order, one patch per function. > > > Some general notes: > > > 1. Unify *.yy files > > Please put sql_mode dependent code into LEX rather than *.yy files directly. > We're going to unify the two files soon. See here for details: > MDEV-12518) Unify sql_yacc.yy and sql_yacc_ora.yy > > So instead of doing: > > | SUBSTRING '(' expr ',' expr ',' expr ')' > { > $$= new (thd->mem_root) Item_func_substr(thd, $3, $5, $7); > if ($$ == NULL) > MYSQL_YYABORT; > } > > > Please do something like this in both sql_yacc.yy and sql_yacc_ora.yy: > > > | SUBSTRING '(' expr ',' expr ',' expr ')' > { > if (!($$= Lex->make_item_func_substr(thd, $3, $5, $7))) > MYSQL_YYABORT; > } > > where LEX::make_item_func_substr() is a new method, creating either > Item_func_substr or Item_func_substr_oracle depending on sql_mode. > > The same applies to all other functions, and to text_literal. > > > 2. Don't use Item::is_null() when you call val_str() anyway. > > There are two reasons that: > - Performance > is_null() involves the second val_str() call, which can be resource consuming > in case of complex functions and subselect. > > - Side effect > Expressions can have some side effects. > For example, in case if the argument is a stored function which counts how > many times it was called and stores the counter in a user variable, or writes > something to a table on every execution. > If we do is_null() followed by val_str(), the side effect will done two times. > We need one time only. > > So instead of: > > String *Item_func_replace_oracle::val_str(String *str) { > String * res; > if (res = replace_value(str, > args[1]->is_null() ? &tmp_emtpystr : > rgs[1]->val_str(&tmp_value2), > args[2]->is_null() ? &tmp_emtpystr : > rgs[2]->val_str(&tmp_value3)) > > return (res->length() == 0) ? make_empty_result () : res; > else > return 0; > } > > > Please do something like this: > > String *Item_func_replace_oracle::val_str(String *str) { > String * res; > String *str1= args[1]->val_str(&tmp_value1); > String *str2= args[2]->val_str(&tmp_value2); > if (res= replace_value(str, > str1 ? str1 : &tmp_emptystr, > str2 ? str2 : &tmp_emptystr) > return (res->length() == 0) ? make_empty_result () : res; > else > return 0; > } > > > Thanks! > > > > > > > Best regards, > > Jérôme. > > > >> -----Message d'origine----- > >> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 23 > >> janvier 2017 12:49 À : jerome brauge Cc : MariaDB Developers Objet : > >> Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > >> > >> Hi Jerome, > >> > >> On 01/18/2017 01:22 PM, jerome brauge wrote: > >>> Hello Alexander, > >>> Sometime ago, when I ask you about plan for MDEV-10574, you replied : > >>> > >>>> The current plan is to do these transformations: > >>>> > >>>> 1. Transform Insert > >>>> - insert values ("") -> insert values (null) > >>>> > >>>> 2. Transform Select > >>>> > >>>> - where v=x => (v <> "" and V=X) > >>>> - where v is null => (v="" or v is null) > >>>> > >>>> We didn't plan to change functions yet. Thanks for bringing this up. > >>>> We'll discuss this. > >>> > >>> I've done some tests just by changing : > >>> - insert an Item_null instead of an Item_string when $1.length==0 in > >>> rule text_literal of sql_yacc_ora.yy > >>> - return null instead of an empty string in > >>> Item_str_func::make_empty_result > >>> > >>> My first tests seem promising. > >>> > >>> Of course this solution does not allow to "see" the records created > >>> with > >> empty strings as null values. > >>> I don't see the importance of being able to do this in a transparent way. > >>> We can explicitly select these row by adding rtrim on these columns. > >>> > >>> If you are interesting, I can begin to write a test to evaluate the > >>> coverage of > >> this solution. > >> > >> I'm afraid this will fix the behavior only for literals. > >> > >> This script: > >> > >> DROP TABLE t1; > >> CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10)); INSERT INTO t1 > VALUES > >> ('',''); SELECT COUNT(*) FROM t1 WHERE a=''; SELECT COUNT(*) FROM t1 > >> WHERE a=b; > >> > >> returns 0 for both SELECT queries in Oracle, and returns 1 for both > >> SELECT queries in MariaDB, Your approach will fix the first SELECT only. > >> The second will still return 1. > >> > >> > >> For now, I'm not sure how to do this correctly. Some ways possible: > >> > >> 1. We could add new classes, e.g. Item_func_eq_oracle as a pair for > >> Item_func_eq, which will handle both empty strings and NULLs inside > >> their > >> val_int() implementations. > >> > >> 2. Or there could be some after-processing on the created Item tree > >> which will replace all things like Item_func_eq to Item_cond_and with > >> Item_func_eq and Item_func_null_predicate passed as arguments. > >> > >> Currently I'm inclined to #1, because we don't have a good mechanism > >> to clone items to implement #2 correctly. We'd need such cloning to > >> pass arguments both to Item_func_eq and Item_func_null_predicate > correctly. > >> Some places in the code don't expect that the same arguments can be > >> passed to two different Item_func instances. > >> > >> On the other hand, implementing #1 will need more changes in the > >> optimizer. > >> > >> We were going to think on this later. > >> > >>> > >>> Best regard. > >>> Jérôme. > >>>

2 4

[Maria-developers] MDEV-12617 CASE and CASE-alike hybrid functions do not preserve exact data types
by Alexander Barkov 28 Apr '17

28 Apr '17

Hi Sanja, can you please review a patch for MDEV-12617? Thanks!

1 0

[Maria-developers] MDEV-10142 - bb-10.2-compatibility / MDEV-10574
by jerome brauge 28 Apr '17

28 Apr '17

Hello Alexander, Sometime ago, when I ask you about plan for MDEV-10574, you replied : >The current plan is to do these transformations: > >1. Transform Insert >- insert values ("") -> insert values (null) > >2. Transform Select > >- where v=x => (v <> "" and V=X) >- where v is null => (v="" or v is null) > >We didn't plan to change functions yet. Thanks for bringing this up. >We'll discuss this. I've done some tests just by changing : - insert an Item_null instead of an Item_string when $1.length==0 in rule text_literal of sql_yacc_ora.yy - return null instead of an empty string in Item_str_func::make_empty_result My first tests seem promising. Of course this solution does not allow to "see" the records created with empty strings as null values. I don't see the importance of being able to do this in a transparent way. We can explicitly select these row by adding rtrim on these columns. If you are interesting, I can begin to write a test to evaluate the coverage of this solution. Best regard. Jérôme.

2 6

[Maria-developers] MDEV-10142 - bb-10.2-compatibility
by jerome brauge 27 Apr '17

27 Apr '17

Hello Alexander, This is the patch for length/lengthb. Regard, Jérôme. PS : I share this code under terms of MCA. > -----Message d'origine----- > De : Alexander Barkov [mailto:bar@mariadb.org] > Envoyé : mercredi 26 avril 2017 06:12 > À : jerome brauge > Cc : MariaDB Developers > Objet : Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > > Hello Jerome, > > Thanks for working on this! > > On 04/25/2017 11:42 PM, jerome brauge wrote: > > Hello Alexander, > > Some times ago, we have had the below discussion. > > I made a patch following my first idea (it's enough from our point of view) : > replace empty string by null in literals and in make_empty_string. > > I think this patch can be complementary to your suggestion of adding > > an Item_func_eq_oracle (for data inserted in default sql_mode) > > > > This patch also contains following some functions changes: > > - replace ('ab','a',null) returns 'a' instead null > > - trim / ltrim / rtrim returns null when input only contains spaces > > - lpad /rpad can accept 2 args (default padding char is space) and if > > length equals to 0, returns null -substring : if start index is equal > > to 0, act as if it's equals to 1 -add a function chr() as a synonym of > > char() (but accept only one arg) -add a function lengthb() as a > > synonym of lentgh() -change the semantic of length for oracle (where > > length() is the character length and not the byte length) > > > > What do you think about this patch ? > > > > (Perhaps I have to make 2 patchs, one for empty string, and one for > > functions changes with separate test file for each modified function) > > Right, can you please split the patch into pieces? > I suggest even more pieces than two. One patch for one function. > I'll create MDEVs, one MDEV per function and put examples how Oracle > works. > > Can we do it in the following order: > > 1. LPAD / RPAD - making the third parameter optional. > This extension will be good for all sql_modes. > I checked IBM DB2, Oracle PostgreSQL, Firebird - they all have > the third parameter to LPAD/RPAD optional. > > 2. CHR(). > > 3. LENGTH() and LENGTHB(). > > > Then we can continue with REPLACE, TRIM. SUBSTRING in no particular > order, one patch per function. > > > Some general notes: > > > 1. Unify *.yy files > > Please put sql_mode dependent code into LEX rather than *.yy files directly. > We're going to unify the two files soon. See here for details: > MDEV-12518) Unify sql_yacc.yy and sql_yacc_ora.yy > > So instead of doing: > > | SUBSTRING '(' expr ',' expr ',' expr ')' > { > $$= new (thd->mem_root) Item_func_substr(thd, $3, $5, $7); > if ($$ == NULL) > MYSQL_YYABORT; > } > > > Please do something like this in both sql_yacc.yy and sql_yacc_ora.yy: > > > | SUBSTRING '(' expr ',' expr ',' expr ')' > { > if (!($$= Lex->make_item_func_substr(thd, $3, $5, $7))) > MYSQL_YYABORT; > } > > where LEX::make_item_func_substr() is a new method, creating either > Item_func_substr or Item_func_substr_oracle depending on sql_mode. > > The same applies to all other functions, and to text_literal. > > > 2. Don't use Item::is_null() when you call val_str() anyway. > > There are two reasons that: > - Performance > is_null() involves the second val_str() call, which can be resource consuming > in case of complex functions and subselect. > > - Side effect > Expressions can have some side effects. > For example, in case if the argument is a stored function which counts how > many times it was called and stores the counter in a user variable, or writes > something to a table on every execution. > If we do is_null() followed by val_str(), the side effect will done two times. > We need one time only. > > So instead of: > > String *Item_func_replace_oracle::val_str(String *str) { > String * res; > if (res = replace_value(str, > args[1]->is_null() ? &tmp_emtpystr : > rgs[1]->val_str(&tmp_value2), > args[2]->is_null() ? &tmp_emtpystr : > rgs[2]->val_str(&tmp_value3)) > > return (res->length() == 0) ? make_empty_result () : res; > else > return 0; > } > > > Please do something like this: > > String *Item_func_replace_oracle::val_str(String *str) { > String * res; > String *str1= args[1]->val_str(&tmp_value1); > String *str2= args[2]->val_str(&tmp_value2); > if (res= replace_value(str, > str1 ? str1 : &tmp_emptystr, > str2 ? str2 : &tmp_emptystr) > return (res->length() == 0) ? make_empty_result () : res; > else > return 0; > } > > > Thanks! > > > > > > > Best regards, > > Jérôme. > > > >> -----Message d'origine----- > >> De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : lundi 23 > >> janvier 2017 12:49 À : jerome brauge Cc : MariaDB Developers Objet : > >> Re: MDEV-10142 - bb-10.2-compatibility / MDEV-10574 > >> > >> Hi Jerome, > >> > >> On 01/18/2017 01:22 PM, jerome brauge wrote: > >>> Hello Alexander, > >>> Sometime ago, when I ask you about plan for MDEV-10574, you replied : > >>> > >>>> The current plan is to do these transformations: > >>>> > >>>> 1. Transform Insert > >>>> - insert values ("") -> insert values (null) > >>>> > >>>> 2. Transform Select > >>>> > >>>> - where v=x => (v <> "" and V=X) > >>>> - where v is null => (v="" or v is null) > >>>> > >>>> We didn't plan to change functions yet. Thanks for bringing this up. > >>>> We'll discuss this. > >>> > >>> I've done some tests just by changing : > >>> - insert an Item_null instead of an Item_string when $1.length==0 in > >>> rule text_literal of sql_yacc_ora.yy > >>> - return null instead of an empty string in > >>> Item_str_func::make_empty_result > >>> > >>> My first tests seem promising. > >>> > >>> Of course this solution does not allow to "see" the records created > >>> with > >> empty strings as null values. > >>> I don't see the importance of being able to do this in a transparent way. > >>> We can explicitly select these row by adding rtrim on these columns. > >>> > >>> If you are interesting, I can begin to write a test to evaluate the > >>> coverage of > >> this solution. > >> > >> I'm afraid this will fix the behavior only for literals. > >> > >> This script: > >> > >> DROP TABLE t1; > >> CREATE TABLE t1 (a VARCHAR(10), b VARCHAR(10)); INSERT INTO t1 > VALUES > >> ('',''); SELECT COUNT(*) FROM t1 WHERE a=''; SELECT COUNT(*) FROM t1 > >> WHERE a=b; > >> > >> returns 0 for both SELECT queries in Oracle, and returns 1 for both > >> SELECT queries in MariaDB, Your approach will fix the first SELECT only. > >> The second will still return 1. > >> > >> > >> For now, I'm not sure how to do this correctly. Some ways possible: > >> > >> 1. We could add new classes, e.g. Item_func_eq_oracle as a pair for > >> Item_func_eq, which will handle both empty strings and NULLs inside > >> their > >> val_int() implementations. > >> > >> 2. Or there could be some after-processing on the created Item tree > >> which will replace all things like Item_func_eq to Item_cond_and with > >> Item_func_eq and Item_func_null_predicate passed as arguments. > >> > >> Currently I'm inclined to #1, because we don't have a good mechanism > >> to clone items to implement #2 correctly. We'd need such cloning to > >> pass arguments both to Item_func_eq and Item_func_null_predicate > correctly. > >> Some places in the code don't expect that the same arguments can be > >> passed to two different Item_func instances. > >> > >> On the other hand, implementing #1 will need more changes in the > >> optimizer. > >> > >> We were going to think on this later. > >> > >>> > >>> Best regard. > >>> Jérôme. > >>>

1 0

[Maria-developers] Please review MDEV-12588 Add Type_handler::type_handler_for_tmp_table() and Type_handler::type_handler_for_union()
by Alexander Barkov 25 Apr '17

25 Apr '17

Hello Sanja, Please review a patch for MDEV-12588. Thanks!

1 0

[Maria-developers] Please review MDEV-12426 Add Field::type_handler()
by Alexander Barkov 25 Apr '17

25 Apr '17

Hello Vicențiu, Please review a patch for: MDEV-12426 Add Field::type_handler() also fixing: MDEV-12432 Range optimizer for ENUM and SET does not return "Impossible WHERE" in some case Thank you!

2 2

[Maria-developers] MDEV-9217 Split Item::tmp_table_field_from_field_type() into virtual methods in Type_handler
by Alexander Barkov 24 Apr '17

24 Apr '17

Hello Sanja, Please review a patch for MDEV-9217. Thanks!

2 3