MYSQL_DATA_TRUNCATED error when using C API

[zllovesuki]

Attempted to use dolt's sql-server with PowerDNS's gmysqlbackend, but pdns returned error on startup:

Jul 25 02:55:49 Result field at row 33 column 0 has been truncated, we allocated 1 bytes but at least 3 was needed
Jul 25 02:55:49 Result field at row 33 column 1 has been truncated, we allocated 1 bytes but at least 9 was needed
Jul 25 02:55:49 Result field at row 33 column 2 has been truncated, we allocated 1 bytes but at least 67 was needed
Jul 25 02:55:49 Result field at row 33 column 3 has been truncated, we allocated 1 bytes but at least 6 was needed
Jul 25 02:55:49 Could not parse domain kind 'N' as one of 'MASTER', 'SLAVE' or 'NATIVE'. Setting zone kind to 'NATIVE'

Tracing the source code from pdns reveals https://github.com/PowerDNS/pdns/blob/master/modules/gmysqlbackend/smysql.cc#L298

Maybe dolt doesn't report the size correctly to the client?

(Related issue: PowerDNS/pdns#11803)
Relevant code section:
https://github.com/PowerDNS/pdns/blob/571146fa726d740ba86028f288c8e1724f04230f/modules/gmysqlbackend/smysql.cc#L237-L247

[zllovesuki]

This is potentially related to #3893

[timsehn]

We'll look at this today. Thanks!

[bpf120]

Hi @zllovesuki , we'd love to learn about your use case. If you'd like to share, come by our Discord or send me an email.

Thanks!

[fulghum]

Thanks for opening this one @zllovesuki. Good observation about the related issue with sending max field lengths over the wire – I think you're correct and I'm working on wiring up that metadata in responses now.

I quickly tried the repro steps you described in PowerDNS/pdns#11803, but I hit an error building the Docker image (build error). Let me know if you see anything obvious there, otherwise I'll keep digging into that error later this afternoon. If you're able to share a dump of any sample data to load into the powerdns db, that would also be helpful. I would love to get your repro running to make sure we get this fixed on the first try for you.

[zllovesuki]

@fulghum related issue with cc1plus being killed because of OOM: mlpack/mlpack#2775

Might want to try reducing the parallelism to 1: https://gist.github.com/zllovesuki/478a091f9fd338b20042ca9f25049dc7#file-dockerfile-L34

[zllovesuki]

Here's some sample SQL queries to quickly insert some test zones and records:

INSERT INTO `domains` (`id`,`name`,`master`,`last_check`,`type`,`notified_serial`,`account`) VALUES (1,'example.com',NULL,NULL,'NATIVE',NULL,NULL);
INSERT INTO `records` (`id`,`domain_id`,`name`,`type`,`content`,`ttl`,`prio`,`disabled`,`ordername`,`auth`) VALUES (1,1,'example.com','SOA','a.example.com hostmaster.example.com 2018092101 10800 3600 604800 600',86400,0,0,'',1);
INSERT INTO `records` (`id`,`domain_id`,`name`,`type`,`content`,`ttl`,`prio`,`disabled`,`ordername`,`auth`) VALUES (2,1,'example.com','NS','a.example.com',86400,0,0,'',1);
INSERT INTO `records` (`id`,`domain_id`,`name`,`type`,`content`,`ttl`,`prio`,`disabled`,`ordername`,`auth`) VALUES (3,1,'example.com','A','127.0.0.1',86400,0,0,'',1);

[zllovesuki]

Thanks for opening this one @zllovesuki. Good observation about the related issue with sending max field lengths over the wire – I think you're correct and I'm working on wiring up that metadata in responses now.

I quickly tried the repro steps you described in PowerDNS/pdns#11803, but I hit an error building the Docker image (build error). Let me know if you see anything obvious there, otherwise I'll keep digging into that error later this afternoon. If you're able to share a dump of any sample data to load into the powerdns db, that would also be helpful. I would love to get your repro running to make sure we get this fixed on the first try for you.

Also it might not be the max length in metadata that is causing issues. See https://github.com/PowerDNS/pdns/blob/571146fa726d740ba86028f288c8e1724f04230f/modules/gmysqlbackend/smysql.cc#L238. It takes the max(field max length, field length), which means in this case, both fields are zero in the metadata returned by dolt. Should also include the result's length as well (I think) in addition to max length.

[zllovesuki]

I can confidently say that it is caused by the metadata not being provided or being incomplete.

diff --git a/modules/gmysqlbackend/smysql.cc b/modules/gmysqlbackend/smysql.cc
index 8b3c80bbc..8fdc7b04b 100644
--- a/modules/gmysqlbackend/smysql.cc
+++ b/modules/gmysqlbackend/smysql.cc
@@ -238,6 +238,13 @@ public:
           unsigned long len = std::max(fields[i].max_length, fields[i].length) + 1;
           if (len > 128 * 1024)
             len = 128 * 1024; // LONGTEXT may tell us it needs 4GB!
+          if (len == 1) {
+            // workaround for dolt sql-server not returning proper metadata
+            len = 16 * 1024;
+            if (d_dolog) {
+              g_log << Logger::Warning << "Query " << ((long)(void*)this) << ": contains value length of 1, forcing to " << len << endl;
+            }
+          }
           d_res_bind[i].is_null = new my_bool[1];
           d_res_bind[i].error = new my_bool[1];
           d_res_bind[i].length = new unsigned long[1];

Monkey patching the code will make pdns happy.

[zllovesuki]

At the risk of spamming this issue, adding these command line options when running pdns will also show you query logging: --loglevel=7 --query-logging=yes

[fulghum]

Awesome!! Thanks for all these details @zllovesuki! This is all super, super helpful. I'll dig in and get you an update on my progress by the end of the day.

[fulghum]

Thanks for the hint about turning down the build parallelism. After that, I was able to get the repro up and running and was able to get the same error messages you got. I did a quick sanity check to always return 123 for the field max length and confirmed that I could not repro the problem with that code, so I'm also confident that we're focused on the right area.

After that, I've been digging through MySQL docs for each of the types and taking a first pass on providing this metadata. I'll need to take a second pass on that tomorrow and I also want to double check the Dolt implementation against MySQL's behavior for all the various types to make sure Dolt returns the same results. I'll keep working on that tomorrow, as well as coming up with a way to automate tests for this logic and keep you updated.

It sounds like you've got a short-term workaround for this bug with the monkey patching you mentioned above, but I wanted to double-check and make sure you weren't completely blocked on this. We should be able to get this released on Wednesday or so, but I'm happy to help come up with a short-term workaround or custom pre-release build if it'll help prevent you from being blocked.

Thanks again for reporting this one and for the great repro info to help us narrow in on the problem.

[zllovesuki]

Thanks for the hint about turning down the build parallelism. After that, I was able to get the repro up and running and was able to get the same error messages you got. I did a quick sanity check to always return 123 for the field max length and confirmed that I could not repro the problem with that code, so I'm also confident that we're focused on the right area.

After that, I've been digging through MySQL docs for each of the types and taking a first pass on providing this metadata. I'll need to take a second pass on that tomorrow and I also want to double check the Dolt implementation against MySQL's behavior for all the various types to make sure Dolt returns the same results. I'll keep working on that tomorrow, as well as coming up with a way to automate tests for this logic and keep you updated.

It sounds like you've got a short-term workaround for this bug with the monkey patching you mentioned above, but I wanted to double-check and make sure you weren't completely blocked on this. We should be able to get this released on Wednesday or so, but I'm happy to help come up with a short-term workaround or custom pre-release build if it'll help prevent you from being blocked.

Thanks again for reporting this one and for the great repro info to help us narrow in on the problem.

Interesting, my brain must have not working earlier. Since it is taking max(max field length, field length), providing max field length in metadata should be enough. Thank you for investing your time in this issue 👍

[fulghum]

Quick update from today's progress... I've got all the types updated to return the max field length and after digging through the MySQL source and docs and testing out all the different types, I think they're looking pretty good. I've started writing tests and will wrap that up tomorrow and hopefully get it out for review, too.

[fulghum]

This code cleaned up pretty nicely and I just requested a review from a teammate. I also retested against your repro setup and verified that this new version works and does not return the truncated data errors anymore.

I'll update again once I get this merged in and prepped for release.

[fulghum]

I got the fix merged into go-mysql-server and we'll pull it into Dolt today and run a release tonight or tomorrow. Thanks again for helping us find this gap and for all the helpful repro details!

[fulghum]

@zllovesuki – Dolt 0.40.21 was just released and it contains this change to send the max field length of the text serialized data in the response metadata. Check out version 0.40.21 when you get a chance and it should be working smoothly for you.

Please don't hesitate to open more issues with bugs or feature requests! We love to hear from customers.