New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement async rlm_sql and drivers #1913

Open
arr2036 opened this Issue Feb 14, 2017 · 9 comments

Comments

@arr2036
Member

arr2036 commented Feb 14, 2017

Need to verify which SQL backends offer access to the FD and provide support async querying.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Feb 14, 2017

Member

@pwdng investigated.

The Good

The Bad

  • Firebird (we can drop this as firebird provides an ODBC interface).
  • FreeTDS (we probably need to keep this).
  • DB2 (very small number of users, can probably drop this).
  • SQLite (we need to keep this)
Member

arr2036 commented Feb 14, 2017

@pwdng investigated.

The Good

The Bad

  • Firebird (we can drop this as firebird provides an ODBC interface).
  • FreeTDS (we probably need to keep this).
  • DB2 (very small number of users, can probably drop this).
  • SQLite (we need to keep this)

@arr2036 arr2036 changed the title from Investigate rlm_sql drivers for async I/O interfaces to Implement async rlm_sql and drivers Feb 14, 2017

@arr2036 arr2036 moved this from Backlog to In progress in Convert modules to asynchronous I/O Feb 14, 2017

@arr2036 arr2036 added this to the 4.0.0 release milestone Feb 14, 2017

@alandekok

This comment has been minimized.

Show comment
Hide comment
@alandekok

alandekok Feb 14, 2017

Member

My $0.02 is to create an rlm_sql_async, and go from there. The idea is that we can then prune it down to (e.g.) Postgres, and work on that code / API. Once that's done, add MySQL, etc.

We're probably OK with SQLite mostly as-is. File IO is blocking, but there isn't much you can do about that. And SQLite has file locking primitives so that only one thread at a time is accessing the DB.

Member

alandekok commented Feb 14, 2017

My $0.02 is to create an rlm_sql_async, and go from there. The idea is that we can then prune it down to (e.g.) Postgres, and work on that code / API. Once that's done, add MySQL, etc.

We're probably OK with SQLite mostly as-is. File IO is blocking, but there isn't much you can do about that. And SQLite has file locking primitives so that only one thread at a time is accessing the DB.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Feb 14, 2017

Member

Just need to markup that rlm_sql instance as blocking when we're either running with a blocking ODBC driver, or using FreeTDS or SQLite.

That means we can pass it off those calls to the blocking I/O threads or however we end up doing that.

Regarding copying the code. We could do. I think @pwdng is reviewing the code, so i guess it depends on whether there's breaking changes required.

Member

arr2036 commented Feb 14, 2017

Just need to markup that rlm_sql instance as blocking when we're either running with a blocking ODBC driver, or using FreeTDS or SQLite.

That means we can pass it off those calls to the blocking I/O threads or however we end up doing that.

Regarding copying the code. We could do. I think @pwdng is reviewing the code, so i guess it depends on whether there's breaking changes required.

@pwdng

This comment has been minimized.

Show comment
Hide comment
@pwdng

pwdng Feb 20, 2017

Collaborator

For ODBC & Oracle, it turns out the only non blocking option available is to use polling.
With previous versions of Oracle, it was possible to get the underlying file descriptor but this doesn't seem to be the case anymore and they don't recommend using the non blocking API anymore as polling is bad.
As for ODBC, there is a notification based system but it isn't available in either iODBC or UnixODBC. The remaining option here is to use polling for non blocking calls.

So true event driven non blocking will only be available for Postgres & MySQL.

Collaborator

pwdng commented Feb 20, 2017

For ODBC & Oracle, it turns out the only non blocking option available is to use polling.
With previous versions of Oracle, it was possible to get the underlying file descriptor but this doesn't seem to be the case anymore and they don't recommend using the non blocking API anymore as polling is bad.
As for ODBC, there is a notification based system but it isn't available in either iODBC or UnixODBC. The remaining option here is to use polling for non blocking calls.

So true event driven non blocking will only be available for Postgres & MySQL.

@pwdng

This comment has been minimized.

Show comment
Hide comment
@pwdng

pwdng Feb 20, 2017

Collaborator

Both MySQL and Postgres can only handle a single concurrent request per connection to the DB.
The initial thought of having a single connection per worker thread is therefore impossible.
We therefore need to keep a connection pool.

We can probably stick to a similar architecture to what's been done for rlm_rest and have a pool per thread.

Collaborator

pwdng commented Feb 20, 2017

Both MySQL and Postgres can only handle a single concurrent request per connection to the DB.
The initial thought of having a single connection per worker thread is therefore impossible.
We therefore need to keep a connection pool.

We can probably stick to a similar architecture to what's been done for rlm_rest and have a pool per thread.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Feb 20, 2017

Member

Sure, that's simple enough. Looks like it is worth writing that two tiered connection pool stuff, i'll see if I can find time tomorrow.

Member

arr2036 commented Feb 20, 2017

Sure, that's simple enough. Looks like it is worth writing that two tiered connection pool stuff, i'll see if I can find time tomorrow.

@alandekok alandekok added the v4.0.x label Feb 27, 2017

@pwdng

This comment has been minimized.

Show comment
Hide comment
@pwdng

pwdng Mar 8, 2017

Collaborator

Here's ongoing work to make rlm_sql async.
https://github.com/pwdng/freeradius-server/commits/async_sql

Ignore changes to postgres driver.
Goal here is to make the module compatible with non blocking drivers.
mod_authorize has been migrated.

Collaborator

pwdng commented Mar 8, 2017

Here's ongoing work to make rlm_sql async.
https://github.com/pwdng/freeradius-server/commits/async_sql

Ignore changes to postgres driver.
Goal here is to make the module compatible with non blocking drivers.
mod_authorize has been migrated.

@pwdng

This comment has been minimized.

Show comment
Hide comment
@pwdng

pwdng Mar 13, 2017

Collaborator

I have made some changes to my original commit and broken it up a bit to make it more readable.

Collaborator

pwdng commented Mar 13, 2017

I have made some changes to my original commit and broken it up a bit to make it more readable.

@pwdng

This comment has been minimized.

Show comment
Hide comment
@pwdng

pwdng Mar 13, 2017

Collaborator

See PR #1940

Collaborator

pwdng commented Mar 13, 2017

See PR #1940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment