Future improvements of sqlpp11

[MeanSquaredError]

@rbock Thank you for merging the connection pools into the main branch.

I made a list of things that may be improved in future versions of sqlpp.

BUGS:

• [DONE] One of the MySQL tests (sqlpp11.mysql.usage.DateTime) fails on my development machine. This is not related to the new connection pools code, because it failed before too. I am not sure of the exact reason, one (unlikely) reason could be because I am using the MySQL connector to connect to a MariaDB. I will try investigating and fixing it.
• [DONE] The MariaDB and SQLCipher connectors fail to build on my machine. I took a quick look and it seems that the cmake for these two has problems. I will investigate that in more detail.

IMPROVEMENTS OF THE CURRENT FUNCTIONALITY

• [DONE] Currently the connectors have different non-standard ways to check if the connection/handle is connected to the server. MySQL has a connection::is_valid() which essentially calls mysql_ping(). PostgreSQL just calls PQstatus() which checks passively if the remote server has closed the connection or if a previous request had an error. SQLite has nothing :-)
  Currently when the connection pool checks the handle status it just uses whatever functionality the connector supports which varies depending of the connector.
  So it really makes sense to standardize this. Each connection and connection_handle should have two methods:
  1. is_connected() checks if the connection has been established and the remote side has not closed the connection.
  2. ping_server() actively pings the server. For PostgreSQL and SQLite3 we can just use the approach of PHP's pg_ping() that sends SELECT 1 to the server and checks if there is an error.

Once we have standardized the is_connected() and ping_server() for connections, we can enhance slightly connection_pool::get() and make it accept a parameter which defines the type of the connection check ,e.g:

enum CHECK_TYPE
{
     CHECK_NONE,
     CHECK_CONNECTED,
     CHECK_PING
};

connection_type connection_pool::get (CHECK_TYPE check = CHECK_CONNECTED);

Then based on the requested check type it will perform the corresponding check before giving out the handle. Currently the MySQL connection pool performs a server ping before giving out a handle, which may be unnecessary if the MySQL server is reliable enough. So it makes sense to allow the user to choose the check type.

• Switching the testing framework to Catch2. sqlpp17 uses it, so it probably makes sense to use it in sqlpp11 too.

CODE CLEANUPS
These are a matter of taste, but it still probably makes sense to discuss and implement some/all of them.

• [DONE] The headers use old-style #ifdef inclusion guards. I think it makes sense to switch to #pragma once. It is non-standard but everyone supports it and besides sqlpp17 is already using it probably because the #ifdef guards are a headache :-)
• [DONE] Using brace-style (uniform) initialization in most places except where C++11 has problems, e.g. auto var = {value}. The C++ core guidelines by Bjarne Stroustrup recommend switching to brace initialization wherever possible, so it probably makes sense to try using it.
• [REJECTED] Moving the method definitions outside of class body. Arguably the class body should just be a description of the class interface and only have method declarations and all method definition should be made outside of the class. This would make the classes much simpler to read. That rule is not always possible to follow strictly, because in some cases we want to use auto as return type and let the compiler figure out the return type, so in these cases the method definition can be inside the class, but in many other cases they can be moved outside of the class. This will probably help mostly the connectors and connection handle classes.

FEATURES THAT REQUIRE C++17 or C++20
I am not sure what is the best way to handle these. Maybe just rename sqlpp11 to sqlpp20 and then implement these features there :-)

• std::optional support.
• std::chrono::zoned_time support.
• copying the improved good parts from sqlpp17. I did not look too much into the core code of sqlpp17, but I think it is improved, so maybe it makes sense to copy it to the new sqlpp20.
• C++20 modules support?
• C++20 concepts?

[rbock]

@rbock Thank you for merging the connection pools into the main branch.

Thanks for the thoughtful migration / implementation!

I made a list of things that may be improved in future versions of sqlpp.

👍

BUGS:

Thanks for looking into those.

IMPROVEMENTS OF THE CURRENT FUNCTIONALITY

* Currently the connectors have different non-standard ways to check if the connection/handle is connected to the server. 

That is a good observation. it certainly makes sense to unify and make use of that in the connection pool.

* Switching the testing framework to Catch2. sqlpp17 uses it, so it probably makes sense to use it in sqlpp11 too.

Not a priority in my view, but probably makes sense, yes.

CODE CLEANUPS These are a matter of taste, but it still probably makes sense to discuss and implement some/all of them.

* The headers use old-style #ifdef inclusion guards. I think it makes sense to switch to #pragma once. It is non-standard but everyone supports it and besides sqlpp17 is already using it probably because the #ifdef guards are a headache :-)

SGTM

* Using brace-style (uniform) initialization in most places except where C++11 has problems, e.g. `auto var = {value}`. The C++ core guidelines by Bjarne Stroustrup recommend switching to brace initialization wherever possible, so it probably makes sense to try using it.

Yes, I am in favor of that. There used to be some problems with older compiler versions (or my faulty understanding of the standard), but it is definitely worth trying to use more uniform initialization.

* Moving the method definitions outside of class body. 

That I am not a fan of. In my view, this creates a lot of unnecessary noise in the code, especially for templates.

FEATURES THAT REQUIRE C++17 or C++20 I am not sure what is the best way to handle these. Maybe just rename sqlpp11 to sqlpp20 and then implement these features there :-)

* std::optional support.

I have been debating with myself whether to create a simple c++11 version of std::optional and then use a type-alias depending on the C++ version. That would allow to implement dynamic queries in sqlp17 style.

* std::chrono::zoned_time support.

* copying the improved good parts from sqlpp17. I did not look too much into the core code of sqlpp17, but I think it is improved, so maybe it makes sense to copy it to the new sqlpp20.

* C++20 modules support?

* C++20 concepts?

I am not going to invest a lot of effort in sqlpp17 or sqlpp20 for now. As said elsewhere, I am looking forward to a time when reflection and generation are a thing in C++ :-)

Thanks for sharing your ideas! Looking forward to more detailed discussions and/or pull requests.

Cheers,
Roland

[MeanSquaredError]

* std::optional support.

I have been debating with myself whether to create a simple c++11 version of std::optional and then use a type-alias depending on the C++ version. That would allow to implement dynamic queries in sqlp17 style.

Using a custom, simplified version of std::optional makes sense. Actually, if keeping C++11 compatibility is required, then a custom std::optional is probably the only sane approach.

• std::chrono::zoned_time support.
  Actually Howard Hinnant's library supports its own version of zoned_time, so probably it is possible to use the same approach with it as with std::optional. date::zoned_time for C++17 and below and std::chrono::zoned_time with C++20 and newer

I am not going to invest a lot of effort in sqlpp17 or sqlpp20 for now. As said elsewhere, I am looking forward to a time when reflection and generation are a thing in C++ :-)

I am not sure how you plan to use the support of C++ reflection in sqlpp, but maybe you have seen taopq. They are somewhat similar to sqlpp in the sense that they are not an ORM but more of a database abstraction layer with type checking. They do support aggregate data types (e.g. structures) as query input parameters and results.
https://github.com/taocpp/taopq/blob/main/doc/Aggregate-Support.md

A while ago I looked into their implementation and it is done using structured binding (which obviously requires C++17). E.g. it seems to me that here they decompose the aggregate data and prepare it for sending to the database (e.g. as a query parameter):

https://github.com/taocpp/taopq/blob/main/include/tao/pq/internal/aggregate.hpp

That approach cannot be used directly in sqlpp11 as it requires C++17, but it might serve as food for thought.

[MeanSquaredError]

@rbock
I am planning to start work on the cleanup of the connector classes and the addition of is_connected() and ping_server() methods.

One thing that I noticed is that for PostgreSQL the connection_handle class is moved into a separate file (include/sqlpp11/postgresql/detail/connection_handle.h), while for MySQL and SQLite3 the connection handle code is a part of the corresponding connection.h file.

1. Should we move the connection handle code for MySQL and SQLite3 into separate files too?
2. The context/serializer classes are called differently for the different connectors. For SQLite3 it is sqlpp::sqlite3::serializer_t
   For MySQL it is sqlpp::mysql::serializer_t
   For PostgreSQL it is sqlpp::postgresql::context_t
   Should we rename all of them to context_t or serializer_t
3. Should we also move the context_t/serializer_t class definition into a separate file?
4. I see that some classes have the _t name suffix, e.g. the context_t/serializer_t have that suffix. For class-scope using-declarations it makes sense to alias the class names with the _t suffix as a hint that it is a type (and not a member). But what about the actual class names (not aliases) like sqlpp::mysql::serializer_t, should there be a _t suffix? I am fine with either approach as long as there is consistency.

[rbock]

@rbock I am planning to start work on the cleanup of the connector classes and the addition of is_connected() and ping_server() methods.

Looking forward to it, thanks!

One thing that I noticed is that for PostgreSQL the connection_handle class is moved into a separate file (include/sqlpp11/postgresql/detail/connection_handle.h), while for MySQL and SQLite3 the connection handle code is a part of the corresponding connection.h file.

Right. The different connector authors are clearly visible in some places. Streamlining these is certainly helping with readability.

1. Should we move the connection handle code for MySQL and SQLite3 into separate files too?

Yes, that makes sense, I think.

2. The context/serializer classes are called differently for the different connectors. For SQLite3 it is `sqlpp::sqlite3::serializer_t`
   For MySQL it is `sqlpp::mysql::serializer_t`
   For PostgreSQL it is `sqlpp::postgresql::context_t`
   Should we rename all of them to `context_t` or `serializer_t`

Yes. I am leaning to context_t.

3. Should we also move the context_t/serializer_t class definition into a separate file?

Maybe. If it can be done nicely, then yes. connection and context are quite closely coupled. I might be easier for them to stay in one file.

4. I see that some classes have the `_t` name suffix, e.g. the `context_t`/`serializer_t` have that suffix. For class-scope using-declarations it makes sense to alias the class names with the _t suffix as a hint that it is a type (and not a member). But what about the actual class names (not aliases) like sqlpp::mysql::serializer_t, should there be a _t suffix? I am fine with either approach as long as there is consistency.

There is a lack of consistency, indeed. Today, I'd say the _t suffix should be removed from all (or most) classes.

[MeanSquaredError]

@rbock
I started working on the connector code cleanup based on your feedback/suggestions. I also noticed that the MySQL connector code that parses date/time results can be simplified by using regular expressions, pretty much like we did with the PostgreSQL date/time-parsing code. Then maybe some of the PostgreSQL/MySQL date/time-parsing code can be unified further reducing the amount of total project code.

So for now I am planning the following PRs:

• Cleanup the connector code based on your input (currently working on it).
• Cleanup the MySQL date/time-parsing code using regular expressions.
• Add the support for connection and connection handle methods is_connected+ping_server and use them in connection_pool::get(CHECK_TYPE).

[CJCombrink]

@MeanSquaredError I hope you don't mind me chiming in here.
Just a comment: When you are cleaning up, perhaps look at the PR I merged for the PostgreSQL connection and see if there are any other places where it can be applied. I suspect the same type of change is needed for MySQL but it is something that I can't test, thus did not touch it.

[MeanSquaredError]

Actually I will split the first PR

* Cleanup the connector code based on your input (currently working on it).

into three separate PRs, one for MySQL, one for PostgreSQL and one for SQLite3. Having them all in one PR would make it too big and difficult to review

[MeanSquaredError]

@CJCombrink
After the connection pool code was merged into sqlpp11, all connection classes
sqlpp::[postgresql,mysq,sqlite3]::connection
are instantiations of sqlpp::normal_connection. Essentially in each connector namespace we have

using connection = sqlpp::normal_connection<connection_base>;

and thus all constructors for the connector classes are defined in sqlpp::normal_connection
When I created the constructors for that class I modeled them after the PostgreSQL connection constructors, so in a sense your PR is already applied to all the connectors.

[MeanSquaredError]

@rbock
I just wonder about the removal of the _t from the structures in the sqlpp namespace. We are going to have renames like

column_t -> column
case_t -> case

That might cause name collisions with variable names or C++ keywords (e.g. case)

Also we have

#define SQLPP_ALIAS_PROVIDER(name)                                           \
  struct name##_t                                                            \

Could that lead to name collisions too?

Maybe it still makes sense to have some special prefix/suffix for these special structures that are used to build the SQL expressions? E.g. the prefix expr_ or x_ or something else to prevent name collisions?

[rbock]

That might cause name collisions with variable names or C++ keywords (e.g. case)

True. Removing _t from names would be a lengthy and tedious project.

Re-reading what I wrote in the previous comment, I should clarify: I meant that if I wrote the library again, today, I would remove the _t from most classes.

But doing so in the existing library might not be worth it.

[MeanSquaredError]

@rbock
IMHO the amount of work is not that much of a problem. I am more worried about possible name clashes. If removing the _t suffixes is OK for you, then I will remove the _t suffixes from the class/structure names and keep them for the ones that clash with reserved names (e.g. sqlpp::case) or where they are a class-scoped alias, e.g.
sqlpp::mysql::connection_base::_handle_t

If that is OK with you, then I will go ahead and rename the classes/structs. What do you think?