Time Nick Message 19:47 cfnblx For those of you who don't Discord -- FYI, I submitted a PR for the MariaDB backend: https://github.com/minetest/minetest/pull/13266 19:48 cfnblx wsor / sfan5 have tagged it, and it has passed all checks. I pulled the current master just prior to pushing my commits, so it should be able to be merged in very simply 19:49 cfnblx I've had a couple friends look over to code to make sure it didn't introduce any errors, and everyone has given it a thumbs up. Hoping you guys will do the same. 19:54 Krock Desour: about #13264 - we're already drawing the tooltips ourselves (GUIFormSpecMenu::showTooltip). What's the difference in the PR? 19:54 pgimeno https://github.com/minetest/minetest/pull/13264 -- Draw irrlicht tooltips ourself by Desour 19:56 Krock cfnblx: what exactly is the motivation behind this change? better integration with existing database servers? 19:56 Desour Krock: irrlicht iguielements also have a tooltip (the one set by setToolTip). our table[] elements use this. these tooltips are usually drawn in the guienvironment 19:56 Desour see also the github code comment I made 19:57 Krock I saw the comment but did not see any such Irrlicht tooltips 19:58 ROllerozxa hover over any of the icons in the multiplayer server list? 19:58 Desour grep for setToolTip, it's done in gui/guiTable.cpp 19:58 Krock yes but I mean in the code. where exactly is it rendered 19:58 Desour ah, see the linked irrlicht PR 19:59 Krock oh. that's the part I missed. thanks. 20:03 cfnblx Krock: yes - to add MariaDB as an option for database backends. It fully supports map, auth, player, and mod storage in a single database and is fully tested. 20:04 Krock Desour: you could as well skip the tooltip drawing if `getSkin()->getColor(EGDC_TOOLTIP) == 0` in which case there would not be a text in first place.. but if we're not using the Irrlicht feature we could as well just purge everything as suggested 20:06 cfnblx Similar to PostgreSQL, I can't imagine many people would want to go through the trouble of setting it up for a client-only installation, so it's mostly intended for people running servers, especially those who may already have large investments into learning/running MySQL/MariaDB like myself. 20:06 Krock yes that makes entirely sense. thanks. 20:06 cfnblx Obviously I can continue to reintegrate my code and build from source, but it would be wonderful if next release could include my PR so I could actually use it on my public servers. 20:07 cfnblx (without all that extra trouble, I mean) 20:07 Krock problem is that a new release should be out for months already but it's still the same things blocking it 20:08 cfnblx Well, this one is hopefully a no-brainer as I've done all the work to make sure it's painless as possible for you guys. 20:09 Krock cfnblx: src/database/database-mariadb.h and .cpp lack an #if USE_MARIADB guard. compiling will fail if these headers are not found 20:09 Krock although I find it interesting the the build bot pased 20:10 Krock actually nvm it did not start in first place due to first-time contributor status 20:10 Krock (nvm x2 because the build bot status is not shown in the "commits" tab but it did eventually run in the past) 20:11 Krock either way I'll add it to the meeting points to decide its roadmap 20:11 cfnblx Uhm, no they're not 20:11 nrz_ Interesting, i should check query quality and schème 20:13 Desour Krock: FYI, I tried setting the font to NULL, but it falls back to the default font. I haven't tried setting the color to 0, as I wasn't sure if then the tooltips might be just uglily invisible. anyway, such solutions also seemed a bit hacky to me 20:13 cfnblx Wait, I stand corrected. .cpp does have the condition, .h does not 20:14 cfnblx database-postgre.h does not have the condition. Are you sure mine should? 20:14 Krock Desour: setting the color to 0 will make the tooltips render anyway. it was a suggestion to change the behavior in IrrlichtMt 20:14 ROllerozxa well technically the header won't be included if USE_MARIADB is false since it's walled off in server.cpp 20:14 Desour ah, I see 20:14 cfnblx right, so why do I need to add it to the .h? 20:15 Krock cfnblx: I did not check the include paths. if the header is protected from inclusion properly (where it is actually needed) it serves its purpose too 20:15 cfnblx yes, it is 20:15 ROllerozxa ok so redis' and leveldb's header files are gated but not postgre 20:15 cfnblx I spent a month on this project. It was not rushed, nor pushed prematuretly. 20:15 Krock src/map.cpp - it's included and protected there. good to know, thanks! 20:16 ROllerozxa that maybe should be sorted out 20:18 Krock yes there's some inconsistencies which might have contributed to my hasty remark about the guards. even still, there's way more inconsistent code in Minetes 20:18 MTDiscord The code is repeating the pattern try { ... } catch (sql::SQLException &e) { std::string msg = std::string("[MariaDB] Error: ") + e.what(); std::cout << msg << std::endl; throw DatabaseException(msg); } a lot. I'd consider looking into how to deduplicate this, using a macro as a last resort of all else fails. 20:18 MTDiscord s/of/if 20:19 cfnblx https://github.com/minetest/minetest/pull/13266/commits/54ad5654db1ba8fd191c12b72fa3725a073ac830 20:19 cfnblx resolved inconsistency 20:20 MTDiscord btw, does MariaDB not have a CREATE TABLE IF NOT EXISTS? because I see plenty of if (!tableExists(...)) createTable(...) in the code. 20:21 MTDiscord It does, I just chose to make it a separate query. 20:21 MTDiscord CREATE TABLE IF NOT EXISTS does in fact exist in mariadb, I believe 20:21 Krock the PR will be discussed this Sunday if the meeting takes place, and reviewers should appear automatically 20:21 MTDiscord why? 20:22 MTDiscord Why not? 20:22 MTDiscord Technically, my way is less expensive. 20:22 MTDiscord (bandwidth/network wise) 20:22 Krock does it matter for one-time queries? 20:22 MTDiscord not at all 20:23 MTDiscord it only sends a full table definition over the wire if the table actually doesn't exist 20:23 MTDiscord every query needs to grab and release a table lock 20:23 MTDiscord more queries is less expensive? 20:23 MTDiscord that's going to be 100x more expensive than a few extra bytes on the wire 20:23 MTDiscord yeah 20:23 MTDiscord It's a one time thing at startup. 20:23 MTDiscord yeah, and that's why I'd go for the shorter version that does less queries (if the tables don't exist) 20:24 MTDiscord But then you're sending longer queries for the life of the database 20:24 MTDiscord Mine sends one extra query the very first time you ever start a new world/database. 20:24 MTDiscord sounds like premature optimization 20:24 MTDiscord Your way would send a full table creation query every single startup for the life of the database. 20:24 MTDiscord yea if it's only done once, this doesn't matter much 20:24 MTDiscord yes, and that's completely negligible 20:25 MTDiscord Mine sends two once, then only a short "does this exist?" forever 20:25 MTDiscord which is why the shorter way should be preferred, IMO 20:25 MTDiscord @luatic how many production MariaDB servers are you running with hundreds of tables and millions of rows of data? 20:26 MTDiscord irrelevant authority argument immediately dismissed 20:26 MTDiscord My ways are tried and true 20:26 MTDiscord ... 20:26 MTDiscord I would hope it aborts as soon as it realises the table already exists, you'll save code lines, indentation levels and query count 20:26 MTDiscord You're welcome to argue on the merit of "what you think" if you prefer 20:26 Krock does network traffic of 500 characters matter when you're constantly retrieving mapblocks in the 16k+ magnitude from the database afterwards? 20:26 MTDiscord it doesn't 20:26 MTDiscord this is premature optimization at the cost of code quality 20:27 MTDiscord if you really want to shorten your queries, why don't you remove those NOT NULL constraints from x, y, z - they are redundant with the primary key constraint? :trollface: 20:29 MTDiscord or utf8mb4_unicode_ci everywhere 20:30 MTDiscord @caffeinatedblocks btw, all your error handling is using std::cout << msg << std::endl; - I think you should prefer using one of Minetest's logging streams, probably errorstream here. 20:34 MTDiscord Actually, I used that inconsistently it seems. 20:34 MTDiscord I intended to replace all of them with errorstream like I did in the Database_MariaDB constructor, but apparently I forgot that 20:36 Krock also you wouldn't need to first write the error message into `std::string` before feeding it into `errorstream` 20:37 Krock unless there's a log printing race condition that I am not aware of.. 20:37 MTDiscord allows concatenation with e.what() 20:37 MTDiscord (for thrown exception message) 20:37 Krock errorstream << "test test " << e.what() << std::endl; works just fine 20:37 MTDiscord I suppose I could just output to errorstream and throw a generic exception ("MariaDB fail") 20:38 MTDiscord I'm currently outputting full error message with e.what() to both cout/errorstream and also into the exception itself (which may or may not be useful) 20:40 Krock I see. the exceptions need to be re-thrown as a Minetest type.. 20:44 Krock In case you'd like to change it quickly in the future and not repeat too much code, you could resort to a copy&paste tool like Macros: https://pastebin.com/raw/nDhNsbHd (just mentioning in case you'd be interested) 20:47 MTDiscord I've opted to simply do what I said above 20:54 MTDiscord In following the same rules I've learned in marriage ("do you want to be RIGHT or HAPPY?"), here you go: 20:54 MTDiscord https://github.com/minetest/minetest/pull/13266/commits/c02a8377a9b630d71cac26e3ba9d71c64fc30dc0 20:55 MTDiscord Have I now appeased the powers that be? 20:59 Krock I should resort to github reviews again. They should leave me less headroom for premature/rushed comments. thanks for the errorstream change. The PR will be discussed at least this week again, possibly including other opinions 21:00 Krock it's easier to work with when there's an entire review than just random comments 21:02 MTDiscord I've removed the unnecessary separate existence check queries for database and tables, redirected all error output to errorstream, modified all exceptions to simply throw "MariaDB error" (since error is output to errorstream already anyway), and also removed the NOT NULL definitions as per luatic's comments 21:05 Krock I think that particular comment from luatic was not meant serious (considering the "trollface" reaction) 21:08 MTDiscord When your boss is fresh out of college, and all you have is more time and experience in the field than he's been alive, what else do you do? Argue with him? Nah, just shut up and do as asked. He'll figure it out eventually. 21:10 Krock I'd call it unprofessional like my initial blunder on the header guard 21:13 MTDiscord Personally, I'd have kept the NOT NULL. In my experience, indexes can change, and forgetting the implicit NOT NULL can result in catastrophe if/when that happens. 21:15 MTDiscord Well, personally, errorstream redirection aside, I'd have kept everything as-is. "Code quality" is not compromised by a if (!tableExists) -- if anything, it tells me exactly what is happening without having to scroll up to be sure "IF NOT EXISTS" was actually included in the statement. 21:16 proller somebody make benchmarks of all dbbackends ? 21:18 MTDiscord I benchmarked mine using an emerge script 21:19 MTDiscord SQLite/Postgre operates about 53% faster with an emerge from (-64, -64, -64) to (64, 64, 64), but all other benchmarks are/were purely observational/anecdotal 21:20 MTDiscord With regard to that additional latency, I spoke with the CTO of MariaDB and he opened this issue to have that addressed in a later release: https://jira.mariadb.org/browse/MDEV-30736 21:20 proller whats about redis or leveldb ? 21:20 MTDiscord I did not test either 21:21 MTDiscord I just compared to the default (sqlite) and the "competition" (postgre) 21:21 proller or "slow" and "very slow" ? 21:21 proller why do not try faster variants? 21:22 MTDiscord SQLite and Postgre are actually both on par with each other as far as the emerge goes. SQLite finished 2 seconds faster. 21:23 MTDiscord MariaDB, on the other hand, has the ability to run fully in memory like REDIS, unlike SQLite/Postgre (without using, for example, memfs/ramdisk) 21:23 MTDiscord does minetest really store blockdata in sql? good lord 21:24 MTDiscord i thought it was just for user/auth records, but i've never looked into it 21:24 proller 8) 21:24 MTDiscord (though I currently have it hard-coded for InnoDB, that could be easily changed to any of the other many supported storage engines, or you could even write your own storage engine) 21:26 MTDiscord With additional optimization tweaks (which make InnoDB less ACID compliant), you can also increase current performance by setting innodb_flush_log_at_trx_commit = 2 21:26 proller somebody should make some standard bench code for all available backends, for write-read thousands blocks and print times/speeds 21:27 MTDiscord That's difficult. 21:27 proller blockdata should not be stored in any sql and cloud bases 21:27 MTDiscord When working with the fellas over at MariaDB to develop this backend, we came up with some really creative tools to benchmark in a hypothetical sense... Minetest doesn't come anywhere optimal performance. 21:30 proller now leveldb should be fastest 21:31 proller and adding rocksdb may make faster 21:32 MTDiscord doesn't leveldb have... pretty serious reliability issues 21:32 MTDiscord I'd wanna know if rocksdb could solve them though, leveldb is in fact pretty neat 21:32 proller also it almost frozen development 21:33 proller ROllerozxa> thats why and i have hand crafted repair tool 21:34 proller it works fine if dont power off server every day 21:35 MTDiscord https://dpaste.com/4XU5EJQ98 21:35 MTDiscord Here's a script written by a fellow over at MariaDB 21:36 MTDiscord On my server, that takes 218643.792236 ms to complete, whereas emerge blocks (-64, -64, -64) (64, 64, 64) takes 282s 21:36 MTDiscord the extra 64 seconds are minetest overhead/latency 21:38 MTDiscord Then again, I seriously doubt anyone is running a server anywhere near mine in terms of hardware performance 21:39 MTDiscord My server's a bad mammajamma and has some seriously fast NVMes: https://dpaste.com/B4YFMCWTU 21:42 MTDiscord (and that's not even bare-metal performance... that includes over head from VMware's Paravirtual SCSI controller) 21:43 MTDiscord That said, if anyone comes up with some tools to benchmark all available backends, I'd be happy to try them out 🙂 21:56 pgimeno is there any DB backend that can keep each world database in the world directory, besides sqlite? 21:58 MTDiscord leveldb 21:58 MTDiscord but i wouldnt use it unless you love data loss/restoring from backups 21:59 pgimeno I'd consider firebird as a backend then