Re: SQLite3 patch

[Paul Querna <ch...@force-elite.com>]

Rick Keiner wrote:
> Attached is a patch that supports SQLite3 using the
> apr_dbd.h interface. Could it be included in the svn
> checkout?

I have put some comments inline with the patch bellow...

> ------------------------------------------------------------------------
> 
> Index: include/apu.h.in
> ===================================================================
> --- include/apu.h.in	(revision 164613)
> +++ include/apu.h.in	(working copy)
> @@ -79,6 +79,7 @@
>  
>  #define APU_HAVE_PGSQL         @apu_have_pgsql@
>  #define APU_HAVE_MYSQL         @apu_have_mysql@
> +#define APU_HAVE_SQLITE3       @apu_have_sqlite3@
>  #define APU_HAVE_SQLITE2       @apu_have_sqlite2@
>  
>  #define APU_HAVE_APR_ICONV     @have_apr_iconv@
> Index: configure.in
> ===================================================================
> --- configure.in	(revision 164613)
> +++ configure.in	(working copy)
> @@ -120,6 +120,7 @@
>  APU_CHECK_DBM
>  APU_CHECK_DBD
>  APU_CHECK_DBD_MYSQL
> +APU_CHECK_DBD_SQLITE3
>  APU_CHECK_DBD_SQLITE2
>  APU_FIND_EXPAT
>  APU_FIND_ICONV
> Index: dbd/apr_dbd_sqlite3.c
> ===================================================================
> --- dbd/apr_dbd_sqlite3.c	(revision 0)
> +++ dbd/apr_dbd_sqlite3.c	(revision 0)
> @@ -0,0 +1,431 @@
> +/* Copyright 2000-2005 The Apache Software Foundation or its licensors, as
> + * applicable.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include "apu.h"
> +
> +#if APU_HAVE_SQLITE3
> +
> +#include <ctype.h>
> +#include <stdlib.h>
> +
> +#include <sqlite3.h>
> +
> +#include "apr_strings.h"
> +#include "apr_time.h"
> +
> +#define MAX_RETRY_COUNT 15
> +#define MAX_RETRY_SLEEP 100000
> +

I still don't like the 'sleep()' call when it cannot get the connection
to the database.

15*1000000 / 1000000 = 1.5 seconds max sleeping

It seems like this is the standard solution for SQLite.  I wish we had a
good way to expose this in someway to the caller API.  Some applications
will not tolerate a 1.5 second delay.  It also makes debugging some
types of bugs very hard, since sometimes you will get a reply on the
first try, others it could take up to 1.5 seconds.

I guess it is a limitation of the Sqlite library, and there isn't much
we can do about it, but I would like a better way to control it.

Thoughts/Alternatives?

> +typedef struct apr_dbd_t apr_dbd_t;
> +typedef struct apr_dbd_results_t apr_dbd_results_t;
> +typedef struct apr_dbd_column_t apr_dbd_column_t;
> +typedef struct apr_dbd_row_t apr_dbd_row_t;
> +typedef struct {
> +	int errnum;
> +	apr_dbd_t *handle;
> +} apr_dbd_transaction_t;
> +
> +struct apr_dbd_t {
> +	sqlite3 *conn;
> +	apr_dbd_transaction_t *trans;
> +        apr_thread_mutex_t *mutex;
> +};
> +

Style nit, it should be 4 spaces for indentation. No Tabs.  It looks
like *mutex is at 8 spaces too.  I think the entire file uses tabs...
not a huge problem, but before it is committed, someone will have to
convert it.

> +struct apr_dbd_row_t {
> +	apr_dbd_results_t *res;
> +	apr_dbd_column_t **columns;
> +	apr_dbd_row_t  *next_row;
> +	int rownum;
> +} ;
> +
> +struct apr_dbd_column_t {
> +	char *name;
> +	char *value;
> +	int size;
> +	int type;
> +} ;
> +
> +struct apr_dbd_results_t {
> +	int random;
> +	sqlite3 *handle;
> +	sqlite3_stmt *stmt;
> +	apr_dbd_row_t *next_row;
> +	size_t sz;
> +	int tuples;
> +} ;
> +
> +
> +
> +typedef struct {
> +	const char *name;
> +	int prepared;
> +} apr_dbd_prepared_t;
> +
> +#define dbd_sqlite3_is_success(x) (((x) == SQLITE_DONE ) \
> +		|| ((x) == SQLITE_OK ))
> +
> +#define APR_DBD_INTERNAL
> +#include "apr_dbd.h"
> +
> +static int dbd_sqlite3_select(apr_pool_t *pool, apr_dbd_t *sql,
> +		apr_dbd_results_t **results,
> +		const char *query, int seek)
> +{
> +	sqlite3_stmt *stmt= NULL;
> +	const char   *tail = NULL;
> +	int i, ret, retry_count;
> +	size_t num_tuples=0;
> +	int increment =0;
> +        apr_dbd_row_t *row = NULL;
> +	apr_dbd_row_t *lastrow = NULL;
> +	apr_dbd_column_t *column;
> +	
> +	char *hold = NULL;
> +	
> +	apr_thread_mutex_lock(sql->mutex);
> +	
> +	ret = sqlite3_prepare(sql->conn, query, strlen(query), &stmt, &tail);
> +	if(!dbd_sqlite3_is_success(ret)) { 
> +	    apr_thread_mutex_unlock(sql->mutex);
> +	    return ret;
> +	}
> +	else {
> +		int column_count;
> + 		column_count = sqlite3_column_count(stmt);
> +		if (!*results) {
> +			*results = apr_pcalloc(pool, sizeof(apr_dbd_results_t));
> +		}
> +		(*results)->stmt = stmt;
> +		(*results)->sz = column_count;
> +		(*results)->random = seek;
> +		(*results)->next_row = 0;
> +		(*results)->tuples = 0;
> +		apr_pool_cleanup_register(pool, stmt, (void*)sqlite3_finalize,
> +					apr_pool_cleanup_null);

Should you be registering this cleanup if you didn't allocate it out of
pool?


> +		while (1) {
> +			ret = sqlite3_step((*results)->stmt);
> +			if(ret == SQLITE_BUSY) {
> +				if(retry_count++ > MAX_RETRY_COUNT) {
> +					apr_thread_mutex_unlock(sql->mutex);
> +					return -1;
> +				}
> +				apr_thread_mutex_unlock(sql->mutex);
> +				apr_sleep(MAX_RETRY_SLEEP);
> +			} else if(ret == SQLITE_ERROR || ret == SQLITE_MISUSE) {
> +				apr_thread_mutex_unlock(sql->mutex);
> +				return ret;
> +		        
> +			} else if (ret == SQLITE_ROW) {
> +				int length ;
> +				apr_dbd_column_t * col;
> +				row = apr_palloc(pool, sizeof(apr_dbd_row_t));
> +				row->res = *results;
> +				row->res->stmt = (*results)->stmt;
> +				increment = sizeof(apr_dbd_column_t *);
> +				length = increment * (*results)->sz ;
> +				row->columns = apr_palloc(pool, length);
> +				for (i=0; i < (*results)->sz; i++) {
> +					column = apr_palloc(pool, sizeof(apr_dbd_column_t));
> +					row->columns[i] = column;
> +					column->name = (char *)sqlite3_column_name((*results)->stmt, i);
> +				  	column->size = sqlite3_column_bytes((*results)->stmt, i);
> +					column->type = sqlite3_column_type((*results)->stmt, i);
> +					switch (column->type) {
> +							
> +
> +						case SQLITE_FLOAT:
> +							break;
> +						case SQLITE_INTEGER:
> +						case SQLITE_TEXT:
> +							hold = NULL;
> +  							hold = (char *)sqlite3_column_text((*results)->stmt, i);
> +  							if (hold) {
> +    								column->value = apr_palloc(pool, column->size + 1);
> +    								strncpy(column->value, hold, column->size +1);
> +  							}
> +							break;
> +						case SQLITE_BLOB:
> +							break;
> +						case SQLITE_NULL:
> +							break;
> +					}
> +					col = row->columns[i] ;
> +				}
> +				row->rownum = num_tuples++;
> +				row->next_row = 0;
> +				(*results)->tuples = num_tuples;
> +				if ((*results)->next_row == 0) {
> +					(*results)->next_row = row;
> +				}
> +				if (lastrow != 0) {
> +					lastrow->next_row =  row; 
> +				}
> +				lastrow = row;
> +			} else if (ret == SQLITE_DONE ) {
> +				apr_thread_mutex_unlock(sql->mutex);
> +				return 0;
> +			} else {
> +				apr_thread_mutex_unlock(sql->mutex);
> +				return ret;
> +			}
> +		}

Instead of calling return from within the while(1), personally, I would
prefer a do { ... } while (rv == OK), and have one 'return' at the end
of the function, that also does the mutex unlock once.


> +	}
> +	apr_thread_mutex_unlock(sql->mutex);
> +	return 0;
> +}
> + 
> +static int dbd_sqlite3_get_row(apr_pool_t *pool, apr_dbd_results_t *res,
> +		apr_dbd_row_t **rowp, int rownum)
> +{
> +	int ret, retry_count,  i = 0;
> +	apr_dbd_row_t *row;
> +	if (rownum == -1) {
> +		*rowp = res->next_row;
> +		if (*rowp == 0 ) return -1;
> +		res->next_row = (*rowp)->next_row;
> +		return 0;
> +	}
> +	if (rownum > res->tuples) {
> +		return -1;
> +	}
> +        *rowp = res->next_row;
> +	for (; *rowp != 0; i++ , *rowp=(*rowp)->next_row) {
> +		if (i  == rownum)  {
> +			return 0;
> +		}
> +	}
> +	
> +        return -1;
> +
> +}
> +
> +static const char *dbd_sqlite3_get_entry(const apr_dbd_row_t *row, int n)
> +{
> +	apr_dbd_column_t * column = row->columns[n];
> +	char * value = column->value;

This will crash when 'n' is out of range of the row.  The PgSQL DBD
calls PQgetvalue() directly, but this postgres function will return an
error if it is out of the range. (returning error = better than crashing...)

> +        return value;
> +}
> +
> +static const char *dbd_sqlite3_error(apr_dbd_t *sql, int n)
> +{
> +	return (const char*)sqlite3_errmsg(sql->conn);
> +}

Why the cast to (const char*)?

> +
> +static int dbd_sqlite3_query(apr_dbd_t *sql, int *nrows, const char *query)
> +{
> +	sqlite3_stmt *stmt= NULL;
> +	const char   *tail = NULL;
> +	int ret, retry_count=0, length=0;
> +        apr_status_t res;
> +	apr_pool_t *pool;
> +
> +	res = apr_pool_create(&pool, NULL);

You shouldn't create a pool from a NULL parent here. Create one from the
pool inside apr_dbd_t if a subpool is needed.

> +	if (res != APR_SUCCESS) {
> +	    return SQLITE_ERROR;
> +	}
> +	length = strlen(query);
> +	apr_thread_mutex_lock(sql->mutex);
> +
> +	do {
> +		ret = sqlite3_prepare(sql->conn, query, length, &stmt, &tail);
> +		if(ret != SQLITE_OK) {
> +			sqlite3_finalize(stmt);
> +			apr_thread_mutex_unlock(sql->mutex);
> +			return ret;
> +		}
> +
> +		ret = sqlite3_step(stmt);
> +		*nrows = sqlite3_changes(sql->conn);
> +		sqlite3_finalize(stmt);
> +		length -= (tail - query);
> +		query = tail;
> +	} while(length > 0);
> +
> +	if(dbd_sqlite3_is_success(ret)) {
> +		ret = 0;
> +	}
> +	apr_thread_mutex_unlock(sql->mutex);
> +	apr_pool_destroy(pool);
> +	return ret;
> +}
> +
> +static const char *dbd_sqlite3_escape(apr_pool_t *pool, const char *arg,
> +		apr_dbd_t *sql)
> +{
> +	char *ret = sqlite3_mprintf(arg);
> +	apr_pool_cleanup_register(pool, ret, (void*)sqlite3_free,
> +			apr_pool_cleanup_null);

I would almost prefer that we allocate this via a strdup into the pool,
rather than relying upon the sqlite3_free.

> +	return ret;
> +}
> +
> +static int dbd_sqlite3_prepare(apr_pool_t *pool, apr_dbd_t *sql,
> +		const char *query, const char *label,
> +		apr_dbd_prepared_t **statement)
> +{
> +	return APR_ENOTIMPL;
> +}
> +
> +static int dbd_sqlite3_pquery(apr_pool_t *pool, apr_dbd_t *sql,
> +		int *nrows, apr_dbd_prepared_t *statement,
> +		int nargs, const char **values)
> +{
> +	return APR_ENOTIMPL;
> +}
> +
> +static int dbd_sqlite3_pvquery(apr_pool_t *pool, apr_dbd_t *sql,
> +		int *nrows, apr_dbd_prepared_t *statement, ...)
> +{
> +	return APR_ENOTIMPL;
> +}
> +
> +static int dbd_sqlite3_pselect(apr_pool_t *pool, apr_dbd_t *sql,
> +		apr_dbd_results_t **results,
> +		apr_dbd_prepared_t *statement,
> +		int seek, int nargs, const char **values)
> +{
> +	return APR_ENOTIMPL;
> +}
> +
> +static int dbd_sqlite3_pvselect(apr_pool_t *pool, apr_dbd_t *sql,
> +		apr_dbd_results_t **results,
> +		apr_dbd_prepared_t *statement,
> +		int seek, ...)
> +{
> +	return APR_ENOTIMPL;
> +}
> +

For the prepared statements... it looks like SQLIte 2, SQLite 3, and
Mysql <=4.0 do not support them well enough.  For the DBD interface in
general, can we look at emulating them?

> +static int dbd_sqlite3_start_transaction(apr_pool_t *pool, apr_dbd_t *handle,
> +		apr_dbd_transaction_t **trans)
> +{
> +	int ret = 0;
> +	int nrows = 0;
> +
> +	ret = dbd_sqlite3_query(handle, &nrows, "BEGIN TRANSACTION;");
> +	if (!*trans) {
> +		*trans = apr_pcalloc(pool, sizeof(apr_dbd_transaction_t));
> +		(*trans)->handle = handle;
> +		handle->trans = *trans;
> +	}
> +
> +	return ret;
> +}
> +
> +static int dbd_sqlite3_end_transaction(apr_dbd_transaction_t *trans)
> +{
> +	int ret = 0;
> +	int nrows = 0;
> +
> +	if(trans) {
> +		ret = dbd_sqlite3_query(trans->handle, &nrows, "END TRANSACTION;");
> +		if (trans->errnum) {
> +			trans->errnum = 0;
> +			ret = dbd_sqlite3_query(trans->handle, &nrows, "ROLLBACK;");
> +		}
> +		else {
> +			ret = dbd_sqlite3_query(trans->handle, &nrows, "COMMIT;");
> +		}
> +		trans->handle->trans = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static apr_dbd_t *dbd_sqlite3_open(apr_pool_t *pool, const char *params)
> +{
> +	apr_dbd_t *sql  = NULL;
> +	sqlite3* conn = NULL;
> +        apr_status_t res;
> +	int sqlres;
> +	if(!params) return NULL;
> +	
> +        sqlres = sqlite3_open(params, &conn); 
> +	if(sqlres != SQLITE_OK) {
> +		sqlite3_close(conn); 
> +		return NULL;
> +	}
> +	/* should we register rand or power functions to the sqlite VM? */
> +	sql = apr_pcalloc (pool, sizeof (*sql)); 
> +	sql->conn = conn;
> +    	/* Create a mutex */
> +        res = apr_thread_mutex_create(&sql->mutex, APR_THREAD_MUTEX_NESTED,    /* nested */
> +                                  pool);

Ack. I would prefer to avoid the nested mutex if possible, and go with
the OS Default.

> +        if (res != APR_SUCCESS) {
> +            return NULL;
> +        }
> +
> +	return sql;
> +}
> +
> +static apr_status_t dbd_sqlite3_close(apr_dbd_t *handle)
> +{
> +	sqlite3_close(handle->conn);
> +	apr_thread_mutex_destroy(handle->mutex);
> +	return APR_SUCCESS;
> +}
> +
> +static apr_status_t dbd_sqlite3_check_conn(apr_pool_t *pool,
> +		apr_dbd_t *handle)
> +{
> +	return (handle->conn != NULL)?APR_SUCCESS:APR_EGENERAL;
> +}
> +
I think the mysql and postgres DBDs do a more complicated check, like an
actual ping of the server.  Is there a way we can do this with sqlite?
(eg, 'SELECT 1')... The NULL check seems like a cheap way out.


> +static int dbd_sqlite3_select_db(apr_pool_t *pool, apr_dbd_t *handle,
> +		const char *name)
> +{
> +	return APR_ENOTIMPL;
> +}
> +
> +static void *dbd_sqlite3_native(apr_dbd_t *handle)
> +{
> +	return handle->conn;
> +}
> +
> +static int dbd_sqlite3_num_cols(apr_dbd_results_t* res)
> +{
> +	return res->sz;
> +}
> +
> +static int dbd_sqlite3_num_tuples(apr_dbd_results_t* res)
> +{
> +	return res->tuples;
> +}
> +
> +APU_DECLARE_DATA const apr_dbd_driver_t apr_dbd_sqlite3_driver = {
> +	"sqlite3",
> +	NULL,
> +	dbd_sqlite3_native,
> +	dbd_sqlite3_open,
> +	dbd_sqlite3_check_conn,
> +	dbd_sqlite3_close,
> +	dbd_sqlite3_select_db,
> +	dbd_sqlite3_start_transaction,
> +	dbd_sqlite3_end_transaction,
> +	dbd_sqlite3_query,
> +	dbd_sqlite3_select,
> +	dbd_sqlite3_num_cols,
> +	dbd_sqlite3_num_tuples,
> +	dbd_sqlite3_get_row,
> +	dbd_sqlite3_get_entry,
> +	dbd_sqlite3_error,
> +	dbd_sqlite3_escape,
> +	dbd_sqlite3_prepare,
> +	dbd_sqlite3_pvquery,
> +	dbd_sqlite3_pvselect,
> +	dbd_sqlite3_pquery,
> +	dbd_sqlite3_pselect,
> +};
> +#endif
> +
> Index: dbd/apr_dbd.c
> ===================================================================
> --- dbd/apr_dbd.c	(revision 164613)
> +++ dbd/apr_dbd.c	(working copy)
> @@ -68,6 +68,9 @@
>  #if APU_HAVE_PGSQL
>      DRIVER_LOAD("pgsql", apr_dbd_pgsql_driver, pool);
>  #endif
> +#if APU_HAVE_SQLITE3
> +    DRIVER_LOAD("sqlite3", apr_dbd_sqlite3_driver, pool);
> +#endif
>  #if APU_HAVE_SQLITE2
>      DRIVER_LOAD("sqlite2", apr_dbd_sqlite2_driver, pool);
>  #endif
> @@ -135,6 +138,7 @@
>                                         apr_pool_t *pool, const char *params,
>                                         apr_dbd_t **handle)
>  {
> +
>      *handle = driver->open(pool, params);
>      if (*handle == NULL) {
>          return APR_EGENERAL;
> Index: build/dbd.m4
> ===================================================================
> --- build/dbd.m4	(revision 164613)
> +++ build/dbd.m4	(working copy)
> @@ -119,9 +119,47 @@
>      APR_ADDTO(APRUTIL_LIBS,[-lmysqlclient_r])
>    fi
>  ])
> +dnl
> +AC_DEFUN([APU_CHECK_DBD_SQLITE3], [
> +  apu_have_sqlite3=0
>  AC_DEFUN([APU_CHECK_DBD_SQLITE2], [
>    apu_have_sqlite2=0
>  

I think the above is slightly wrong. The AC_DEFUN for sqlite2 should be
down bellow...

> +  AC_ARG_WITH([sqlite3], [
> +  --with-sqlite3=DIR         
> +  ], [
> +    apu_have_sqlite3=0
> +    if test "$withval" = "yes"; then
> +      AC_CHECK_HEADER(sqlite3.h, AC_CHECK_LIB(sqlite3, sqlite3_open, [apu_have_sqlite3=1]))
> +    elif test "$withval" = "no"; then
> +      apu_have_sqlite3=0
> +    else
> +      CPPFLAGS="-I$withval/include"
> +      LIBS="-L$withval/lib "
> +
> +      AC_MSG_NOTICE(checking for sqlite3 in $withval)
> +      AC_CHECK_HEADER(sqlite3.h, AC_CHECK_LIB(sqlite3, sqlite3_open, [apu_have_sqlite3=1]))
> +      if test "$apu_have_sqlite3" != "0"; then
> +        APR_ADDTO(APRUTIL_LDFLAGS, [-L$withval/lib])
> +        APR_ADDTO(APRUTIL_INCLUDES, [-I$withval/include])
> +      fi
> +    fi
> +  ], [
> +    apu_have_sqlite3=0
> +    AC_CHECK_HEADER(sqlite3.h, AC_CHECK_LIB(sqlite3, sqlite3_open, [apu_have_sqlite3=1]))
> +  ])
> +
> +  AC_SUBST(apu_have_sqlite3)
> +
> +  dnl Since we have already done the AC_CHECK_LIB tests, if we have it, 
> +  dnl we know the library is there.
> +  if test "$apu_have_sqlite3" = "1"; then
> +    APR_ADDTO(APRUTIL_EXPORT_LIBS,[-lsqlite3])
> +    APR_ADDTO(APRUTIL_LIBS,[-lsqlite3])
> +  fi
> +])
> +dnl
> +
>    AC_ARG_WITH([sqlite2], [
>    --with-sqlite2=DIR         
>    ], [