Let drivers handle thread-safe access to the Database object in `AdbcConnectionInit`

[felipecrv]

What feature or improvement would you like to see?

Let drivers handle thread-safe access to the Database object in AdbcConnectionInit

This would make locking more fine-grained and remove the need for a pessimistic lock around the entire AdbcConnectionInit call.

For instance, this is how Rust's driver manager wraps the FFI call today:

    /// Initialize the given connection using the loaded driver.
    fn connection_init(
        &self,
        mut connection: ffi::FFI_AdbcConnection,
    ) -> Result<ffi::FFI_AdbcConnection> {
        let driver = self.ffi_driver();
        let mut database = self.inner.database.lock().unwrap();

        // ConnectionInit
        let mut error = ffi::FFI_AdbcError::with_driver(driver);
        let method = driver_method!(driver, ConnectionInit);
        let status = unsafe { method(&mut connection, &mut *database, &mut error) };
        check_status(status, error)?;

        Ok(connection)
    }

As connection initialization can mutate the database object that initializes the connection, a lock on self.inner.database must be acquired before the call.

A driver performing multi-step auth actions might be waiting on slow HTTP requests during the AdbcConnectionInit call. If the driver took responsibility of the locking, it could let these HTTP calls happen in parallel and only synchronize the actual mutations to the database object (e.g. caching obtained info for re-use).

[paleolimbot]

It seems like adbc.h suggests that concurrent access to the database is not supported?

arrow-adbc/c/include/arrow-adbc/adbc.h

Line 1214 in e045731

/// This must always be thread-safe (other operations are not), though

Creating a "database" is often cheap (sometimes it's just stored auth credentials or a URI)...is it possible to just create multiple databases and connect concurrently that way?

[felipecrv]

Creating one Database per connection goes against the reason for its existence: to serve as a cache and/or pool of connections. I don’t think it’s much to ask: some kind of API that makes connection creation parallelizable and based on some stateful creator controlled by the driver.

…

Message ID: ***@***.***>

[lidavidm]

arrow-adbc/c/include/arrow-adbc/adbc.h

Lines 34 to 36 in e045731

	/// In general, objects allow serialized access from multiple threads,
	/// but not concurrent access. Specific implementations may permit
	/// multiple threads.

I think the problem here is more that Rust has no way of knowing whether it is safe or not

[felipecrv]

arrow-adbc/c/include/arrow-adbc/adbc.h

Lines 34 to 36 in e045731

/// In general, objects allow serialized access from multiple threads,
/// but not concurrent access. Specific implementations may permit
/// multiple threads.
I think the problem here is more that Rust has no way of knowing whether it is safe or not

If thread-safety were a requirement we could perform an unsafe cast on the Rust side. Then thread-safety stops being guaranteed by the Rust type system and becomes guaranteed by the ADBC API requirements and drivers implementing it correctly.

[lidavidm]

Creating a "database" is often cheap (sometimes it's just stored auth credentials or a URI)...is it possible to just create multiple databases and connect concurrently that way?

I'm thinking the cleanest API might be to add "clone" methods for databases/connections. (A cloned database/connection would be the same object internally.) That lets drivers clearly opt in to being thread safe and in terms of Rust, lets you avoid having to make multiple mutable references to the same object.

I'm a little hesitant to require thread safety on all drivers.