odbc connector in sqlpp23

[autoantwort]

I am currently trying to migrate https://github.com/Erroneous1/sqlpp11-connector-odbc to sqlpp23.
All refactoring that were done here I have to understand and manually apply at the odbc connector which is a lot of work.
So I asked myself if it would be better to include the odbc connector code in this repo, so that further refactorings are also directly applied to the odbc connector code?

[rbock]

Thanks for reaching out!

Looking at the ODBC connector, it seems that it also did not follow some major developments in sqlpp11, like the move to header-only, or the changes in serialization. That does not speak for widespread interest in this connector? But I might be missing something, of course.

Irrespective of that, I think you will see that the migration should be rather straight forward since the fundamental concepts for connector libraries have not changed that much, they are just organized a bit differently in code now. Once you are done with the migration, we can consider including the ODBC connector in the main library.

A benefit of including it would be that the ODBC connector would be more readily available, of course.

However, as of now, I see two potential drawbacks from including it:

• The overall burden of maintaining the library would grow. I have to be mindful of my time.
• The continuous integration is running into timeouts every now and then already. Adding about 20% more tests would cause more issues. These would need to be resolved.

[autoantwort]

That does not speak for widespread interest in this connector?

Probably. ODBC is mostly Windows only and mostly MS Server which is maybe more enterprise only. We currently only support odbc with an internal DB lib but we want to migrate to something better.

I think you will see that the migration should be rather straight forward since the fundamental concepts for connector libraries have not changed that much

I have never worked with sqlpp23 before that and never touched connector code. We will see :D

The overall burden of maintaining the library would grow. I have to be mindful of my time.

That is definitely a point.

The continuous integration is running into timeouts every now and then already. Adding about 20% more tests would cause more issues.

Currently the lib only has one test, but this is probably not the solution :D

[MeanSquaredError]

I did need an ODBC connector for sqlpp23 in the past few years and even thought about porting the old connector from sqlpp11 to sqlpp23 myself, but never found the time, since it is a big task indeed. In the end I had to resort to using my own half-baked solution for Windows ODBC instead of sqlpp23.

As to why there are few people asking for an ODBC connector for sqlpp23, I don't know, but I suspect that it is a catch-22 situation, people see that there is no support for ODBC so they just move to other solutions without asking. And since there are no people asking for ODBC, everyone thinks that there is no interest and doesn't bother to port the old driver.

So kudos to @autoantwort for undertaking this, admittedly big, project!

As to whether it is worth including the project into sqlpp23, it is not my decicion to make and it is up to the library author to decide that. But I am pretty sure that even if it is not included in the library, there will be people appreciating the availability of this connector!

[rbock]

Thanks for the additional data point, @MeanSquaredError. And yeah, you might have a point about the catch-22.

@autoantwort Since you have the original odbc code available, I sincerely think that the hardest problems are already solved. Spending a focused day should be enough to have a first sqlpp23-version. From a quick look at the current odbc implementation, I'd say the sqlite3 connector is probably closest to see how things should look in the migrated version.

You will quickly see how to translate the old patterns into the new ones. For instance, bind_result_t::_bind_text_result and friends are now called bind_result::read_field.

In a first version, I would omit prepared statements, special clauses like insert_or, and special serialization. Just plain and simple queries.

Also, if you have any questions, do not hesitate to ask. I am happy to help.

[autoantwort]

Also, if you have any questions, do not hesitate to ask. I am happy to help.

Yeah a few :D

1. Some connectors have a text_result_t and some a bind_result_t and some both. My understanding of a bind_result_t would be that it binds the variables in such a way that the connector library (sqlite, mysql, ...) writes the data directly into the provided data structures, but the sqlite connector has a bind_result_t that does no binding at all? (Same with the odbc connector).
2. Are the variable addresses provided in bind_parameter stable and can be used by the connector library when using prepared statements?
3. When using the library I want to be able to support postgres and odbc at the same time. Iiuc I have to use specific database connectors when making requests? So I have to make everything everywhere a template or uses std::variant<sqlpp23::odbc::connection, sqlpp23::postgresql::connection> everywhere?

I sincerely think that the hardest problems are already solved. Spending a focused day should be enough to have a first sqlpp23-version

It seems that there are a few bugs. At least the prepared statement code was full of dangling references 😅

[rbock]

1. Some connectors have a text_result_t and some a bind_result_t and some both. [...]

It is not an exact science. The names came from the first connector (mysql) where variable binding actually happens. In other connectors, I call it text_result when the native library yields all results as some form of string. Otherwise, I call it bind_result. Maybe bin_result or binary_result would make more sense.

2. Are the variable addresses provided in bind_parameter stable [...]

Yes, but be careful. While the address of a string_view might be stable, the address of the string you can view through it might change when the parameter gets updated.

3. When using the library I want to be able to support postgres and odbc at the same time [...]

Oh, that's non-trivial. In danger of stating the obvious: I would try to limit the code that actually interacts with the connector to a minimum. And yes, that is easier said than done for sure. I don't have a good recipe.

I sincerely think that the hardest problems are already solved.

It seems that there are a few bugs.

OK, that makes it harder, of course. In my defense, I explicitly said I would omit prepared statements from a first version :-)

But maybe you are beyond the first version already?

[autoantwort]

Maybe bin_result or binary_result would make more sense.

I have renamed it to binary_result because this is what it actually does.

But maybe you are beyond the first version already?

No I currently try to understand what changes to the connection class interface were made.
The old code is like:

template <typename Insert>
size_t insert(const Insert& i) {
  _context_t context(*this);
  serialize(i, context);
  return insert_impl(context.str());
}

in the other connectors it is like

 template <typename Insert>
 insert_result _insert(const Insert& i) {
   context_t context(this);
   const auto query = to_sql_string(context, i);
   return insert_impl(query);
 }

Must I call it _insert or can I just keep it insert? And why does it call insert_impl? insert_impl is only called by this function, why not moving the body into _insert? This applies to a lot of functions.

[autoantwort]

Currently you can pass a driver_connection_config or a connection_config to the constructor of a connection, with the new design this is not possible anymore, right?

Edit: I will put a std::variant into the connection_config to model the old behavior

[rbock]

I have renamed it to binary_result because this is what it actually does.

👍

Must I call it _insert or can I just keep it insert? And why does it call insert_impl? insert_impl is only called by this function, why not moving the body into _insert? This applies to a lot of functions.

Yes, _insert. See include/sqlpp23/core/query/statement_handler.h. Whether you need the impl function is up to you. The core library does not need it.

Currently you can pass a driver_connection_config or [...]

Hmm. Not sure what you mean with driver_connection_config, but sounds like you know what to do by now?

[autoantwort]

Whether you need the impl function is up to you.

Ok 👍 I was just wondering because they all do it and it seems unnecessary.

Not sure what you mean with driver_connection_config, but sounds like you know what to do by now?

Yes I found a solution. The odbc connector has different connection config classes depending on how you want to connect.

I explicitly said I would omit prepared statements from a first version

It seems that the odbc connector always use prepared statements. Even for single shot queries they are used. I guess I will switch to the to_sql_string version.

[autoantwort]

3. Are the variable addresses provided in bind_parameter stable [...]

Yes, but be careful. While the address of a string_view might be stable, the address of the string you can view through it might change when the parameter gets updated.

The MySQL connector does

 void bind_parameter(size_t parameter_index, const std::string_view& value) {
   stmt_param_is_null[parameter_index] = false;
   MYSQL_BIND& param{stmt_params[parameter_index]};
   param.buffer_type = MYSQL_TYPE_STRING;
   param.buffer = const_cast<char*>(value.data());
   param.buffer_length = value.size();
   param.length = &param.buffer_length;
   param.is_null = &stmt_param_is_null[parameter_index].value;
   param.is_unsigned = false;
   param.error = nullptr;
 }

This indicates the string view always points to the same data and the string always has the same length, otherwise this would break or not?
When I want to bind a string param in odbc I have to give the API one fixed address which must be stable across executions, so it would be beneficial to know the maximum size when binding the param. The string must be null terminated, or one provides a pointer to an integer which holds the length of the current string (must be updated every time the bound string changed its length).

[MeanSquaredError]

Currently you can pass a driver_connection_config or a connection_config to the constructor of a connection, with the new design this is not possible anymore, right?

Edit: I will put a std::variant into the connection_config to model the old behavior

As an initial version I think it should be OK to use a variant.

I just looked at the current sqlpp23 code and it seems that in the future it shouldn't be that difficult to update sqlpp::normal_connection::normal_connection() so that it would accept any type of config object as long as that type is supported by the underlying connector-specific connection_base::connection_base(). This would allow one connection class to support multiple config object types.

Such a change would require a bit of tinkering since other parts of the library would be affected (e.g. the connection pool class would require some changes too), but is definitely doable. Just for the record: I am NOT volunteering to make that change, but just mentioning that it seems doable :-)

[MeanSquaredError]

This indicates the string view always points to the same data and the string always has the same length, otherwise this would break or not?

The MySQL documentation does not state it clearly, but I think that the caller of bind_parameter() must keep unchanged the underlying string pointed to by string_view until he finishes using that prepared statement. If the string is changed (e.g. if it is in the heap and its memory gets freed and then overwritten), then the prepared statement will use garbage instead of the string and as expected bad things will happen.

On a side note: I see a possibility for a minor cleanup in that code because const std::string_view& value is overcomplicated and should be replaced just with std::string_view value. That should save us one indirection and make the code slightly simpler and faster.

[autoantwort]

but I think that the caller of bind_parameter() must keep unchanged the underlying string pointed to by string_view until he finishes using that prepared statement.

How is this supposed to happen? For example here from https://github.com/rbock/sqlpp23/blob/main/docs/statement_execution.md:

// insert multiple rows based on a range of input values
for (const auto& input : input_values)
{
  // Set parameters
  prepared_insert.parameters.alpha = input.alpa;
  prepared_insert.parameters.cheese = input.beta;

  // Execute prepared statement with the current set of parameters
  db(prepared_insert);
}

[MeanSquaredError]

How is this supposed to happen? For example here from https://github.com/rbock/sqlpp23/blob/main/docs/statement_execution.md:

// insert multiple rows based on a range of input values
for (const auto& input : input_values)
{
// Set parameters
prepared_insert.parameters.alpha = input.alpa;
prepared_insert.parameters.cheese = input.beta;

// Execute prepared statement with the current set of parameters
db(prepared_insert);
}

I stand corrected: the caller of bind_parameter() must keep unchanged the underlying string until he (the caller) executes that prepared statement. In other words, when the prepared statement is executed all the bound string parameters must point to valid strings.

In your example this is definitely the case: the user sets prepared_insert.parameters.alpha and prepared_insert.parameters.cheese to point to some strings owned by input, and then immediately executes the prepared statement. The input variable is not destroyed until the closing }, so it and the strings owned by it (input.alpha and input.beta) are definitely valid when db(prepared_insert); is executed.