MariaDB v3.4.9 compatebility issue

[BlackDex]

Backend

Other

Diesel version

2.3.10

Diesel Features

mysql diesel_migrations

Operating System Version

GNU/Linux

Third party libraries

No response

What do you want to do?

Run migrations via mariadb-connector-c library v3.4.9.
MariaDB v3.4.9 seems to break some queries like min() and max() which the Diesel Migrations makes use of.
This causes the migrations to fail.

I have an example repo available here: https://github.com/BlackDex/mvp-issues/tree/diesel-mariadb-3.4.9
It shows that the min() and max() functions return empty, while with older versions they do return the expected values.

Compile time error

n/a

What code do you already have?

https://github.com/BlackDex/mvp-issues/tree/diesel-mariadb-3.4.9

Additional details

This issue is also discussed at Vaultwarden over here: dani-garcia/vaultwarden#7268

Checklist

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

[weiznich]

So I spend some time digging into this and as somewhat expected this is yet another breaking change from mariadb.

The problem is that they again overwrite the length values provided by diesel with numeric 0, now for dynamically sized types.

Mysql/Mariadb expects us to pass in a preallocated buffer for the data they return. For statically sized data like integers, etc we can know the size of the buffer in advance and will allocate a buffer of the right size. For dynamically sized data like strings that's not possible. We just pass in a smaller sized buffer in this case and expect libmysqlclient/libmariadb to return a "truncated" result:

diesel/diesel/src/mysql/connection/stmt/mod.rs

Lines 180 to 196 in 762a1a6

	pub(super) fn populate_row_buffers(&self, binds: &mut OutputBinds) -> QueryResult<Option<()>> {
	let next_row_result = unsafe { ffi::mysql_stmt_fetch(self.inner.stmt.as_ptr()) };
	if next_row_result < 0 {
	self.inner.did_an_error_occur().map(Some)
	} else {
	#[allow(clippy::cast_sign_loss)] // that's how it's supposed to be based on the API
	match next_row_result as libc::c_uint {
	ffi::MYSQL_NO_DATA => Ok(None),
	ffi::MYSQL_DATA_TRUNCATED => binds.populate_dynamic_buffers(self).map(Some),
	0 => {
	binds.update_buffer_lengths();
	Ok(Some(()))
	}
	_error => self.inner.did_an_error_occur().map(Some),
	}
	}
	}

This sets the length field in our BindData as that is provided as mutable pointer to the right size. (

diesel/diesel/src/mysql/connection/bind.rs

Line 172 in 762a1a6

length: libc::c_ulong,

)

We then use this information to resize the buffer to the correct size via

diesel/diesel/src/mysql/connection/bind.rs

Lines 77 to 89 in 762a1a6

	pub(super) fn populate_dynamic_buffers(&mut self, stmt: &StatementUse<'_>) -> QueryResult<()> {
	for (i, data) in self.0.data.iter_mut().enumerate() {
	data.did_numeric_overflow_occur()?;
	// This is safe because we are re-binding the invalidated buffers
	// at the end of this function
	unsafe {
	if let Some((mut bind, offset)) = data.bind_for_truncated_data() {
	stmt.fetch_column(&mut bind, i, offset)?
	} else {
	data.update_buffer_length()
	}
	}
	}

and

https://github.com/diesel-rs/diesel/blob/main/diesel/src/mysql/connection/bind.rs#L529-L590

In https://github.com/diesel-rs/diesel/blob/main/diesel/src/mysql/connection/bind.rs#L91 we repopulate the buffer with the now complete data. Before this call the length field is untouched on the diesel side, and still the right value for the buffer. After this call it's set to 0 by libmariadb in the problematic versions.

This field is then later used for dynamically sized types to determine the right size of the stored buffer:

diesel/diesel/src/mysql/connection/bind.rs

Lines 436 to 453 in 762a1a6

	let length = if let Some(length) = known_buffer_size_for_ffi_type(self.tpe) {
	debug_assert!(
	length <= self.capacity
	&& <usize as TryFrom<_>>::try_from(self.length)
	.expect("32bit integer fits in a usize")
	<= self.capacity,
	"Libmysqlclient reported a larger size for a fixed size buffer without setting the truncated flag. \n\
	This is a bug somewhere. Please open an issue with reproduction steps at \
	https://github.com/diesel-rs/diesel/issues/new \n\
	Calculated Length: {length}, Buffer Capacity: {}, Reported Length {}, Type: {:?}",
	self.capacity,
	self.length,
	self.tpe
	);
	length
	} else {
	self.length.try_into().expect("Usize is at least 32 bit")
	};

This now assumes that the buffer has a length of zero as that is what is reported by libmariadb, which in turn causes the other failures on top of it.

I'm honestly not sure why they even believe this is a sane change at all. This removes any indication about the buffer size for any subsequent call to that hits the right size by chance.

It seems possible to workaround this by just copying the relevant information on diesel side before the problematic call, but that's yet another mariadb specific workaround for a regression on their side. Before doing that I would at least fill an upstream issue and hear there reasoning for repeatably breaking stuff like that.

cc @robertsilen @ottok as you both tried repeatably to have Diesel declare compatibility with MariaDB. Such issues are exactly the reason why we have not done that yet.

cc also @grawlinson as this might be relevant for your packing as well.

[Narrat]

Thank you very much for the detailed analysis. Greatly appreciated as it involves something which isn't directly supported.
Up to now I wasn't sure if this is the same issue that got reported to mariadb (CONC-821) for the breaking commit or a separate one (stemming from the same change). But from my understanding reading this, it seems to be the same.
So this can be safely mentioned there? Which brings hopefully more attention to this regression...

[weiznich]

Feel free to mention it there as well.