Re: [PATCH] Binding sql variables in dbpool

"Robert Galach" <[email protected]>
Newsgroups gmane.comp.mobile.kannel.devel
Message-ID <[email protected]> 
hi Alex,

> why in hell are you trying to append NULL pointers to the list?

No reason of course, this is a garbage I forgot. It is ugly, but does not
imply any problems.
Sorry, I made a mistake and diff'ed  dbpool_oracle. c which was working but
not clean version of file,
there is no need to keep bind_list at all, as there is no explicit calls to
any bindhp now.

Clean patch attached.

> I believe that this patch will get only very very small performance
advantage,
> because binds will get performance advantage only with prepared statements
> that is not a case in your patch.

Please run this query:

SELECT SQL_TEXT, PARSE_CALLS, EXECUTIONS
  FROM V$SQLAREA
  WHERE SQL_TEXT LIKE '%dlr%'

where dlr is the table-field from your config file.
Notice that DLR queries with bound variables are cached very well and parsed
_only_once_.
In my test system it looks like:

DELETE FROM dlr WHERE smsc=:1 AND timestamp=:2 AND destination=:3 AND ROWNUM
< 2
  4256    4256

INSERT INTO dlr (smsc, timestamp, src, destination, service, url, mask,
boxc_id, status)
  VALUES (:1, :2, :3, :4, :5, :6, :7, :8, 0)
  4275    4275

SELECT mask, service, url, src, destination, boxc_id FROM dlr
  WHERE smsc=:1 AND timestamp=:2 AND destination=:3 AND ROWNUM < 2
  4547    4547

Before this patch, V$SQLAREA was filled with thousands of rows with similar
statements
with differences only in literal parameters :( It has no matter in low load,
dedicated system.
But it is critical on production systems. Clean sql history is only way to
trace and optimize
inefficient queries. It also improve performance because Oracle is able to
effective use
cache mechanism and manage cache. You can use smaller SHARED_POOL.
Less number of unique objects in SHARED_POOL = faster SQL hashing and
finding.

Regards
Robert
db_pool_binding_variables_2.patch (application/octet-stream, 18.9 KB) 
Index: gwlib/dbpool.c
===================================================================
RCS file: /home/cvs/gateway/gwlib/dbpool.c,v
retrieving revision 1.8
diff -u -r1.8 dbpool.c
--- gwlib/dbpool.c	22 Jan 2004 14:08:25 -0000	1.8
+++ gwlib/dbpool.c	11 Mar 2004 20:01:49 -0000
@@ -61,6 +61,8 @@
  *      2003 Initial version.
  * Alexander Malysh <[email protected]>
  *      2003 Made dbpool more generic.
+ * Robert Ga³ach <[email protected]>
+ *      2004 Added support for binding variables.
  */
 
 #include "gwlib.h"
@@ -319,7 +321,7 @@
     return n;
 }
 
-int inline dbpool_conn_select(DBPoolConn *conn, const Octstr *sql, List **result)
+int inline dbpool_conn_select(DBPoolConn *conn, const Octstr *sql, List *binds, List **result)
 {
     if (sql == NULL || conn == NULL)
         return -1;
@@ -327,10 +329,10 @@
     if (conn->pool->db_ops->select == NULL)
         return -1; /* may be panic here ??? */
 
-    return conn->pool->db_ops->select(conn->conn, sql, result);
+    return conn->pool->db_ops->select(conn->conn, sql, binds, result);
 }
 
-int inline dbpool_conn_update(DBPoolConn *conn, const Octstr *sql)
+int inline dbpool_conn_update(DBPoolConn *conn, const Octstr *sql, List *binds)
 {
     if (sql == NULL || conn == NULL)
         return -1;
@@ -338,6 +340,6 @@
     if (conn->pool->db_ops->update == NULL)
         return -1; /* may be panic here ??? */
 
-    return conn->pool->db_ops->update(conn->conn, sql);
+    return conn->pool->db_ops->update(conn->conn, sql, binds);
 }
 #endif /* HAVE_DBPOOL */
Index: gwlib/dbpool.h
===================================================================
RCS file: /home/cvs/gateway/gwlib/dbpool.h,v
retrieving revision 1.6
diff -u -r1.6 dbpool.h
--- gwlib/dbpool.h	22 Jan 2004 14:08:25 -0000	1.6
+++ gwlib/dbpool.h	11 Mar 2004 20:01:49 -0000
@@ -177,8 +177,8 @@
  */
 void dbpool_conn_produce(DBPoolConn *conn);
 
-int inline dbpool_conn_select(DBPoolConn *conn, const Octstr *sql, List **result);
-int inline dbpool_conn_update(DBPoolConn *conn, const Octstr *sql);
+int inline dbpool_conn_select(DBPoolConn *conn, const Octstr *sql, List *binds, List **result);
+int inline dbpool_conn_update(DBPoolConn *conn, const Octstr *sql, List *binds);
 
 /*
  * Perfoms a check of all connections within the pool and tries to
Index: gwlib/dbpool_p.h
===================================================================
RCS file: /home/cvs/gateway/gwlib/dbpool_p.h,v
retrieving revision 1.4
diff -u -r1.4 dbpool_p.h
--- gwlib/dbpool_p.h	22 Jan 2004 14:08:25 -0000	1.4
+++ gwlib/dbpool_p.h	11 Mar 2004 20:01:49 -0000
@@ -91,16 +91,19 @@
      *           result is the list of rows each row will be stored also as list each column is stored as Octstr.
      * If someone has better idea please tell me ...
      *
-     * @params conn - database specific connection; sql - sql statement ; result - result will be saved here
+     * @params conn - database specific connection; sql - sql statement ; 
+     *         binds - list of Octstr values for binding holes in sql (NULL if no binds);
+     *         result - result will be saved here
      * @return 0 if all was fine ; -1 otherwise
      */
-    int (*select) (void *conn, const Octstr *sql, List **result);
+    int (*select) (void *conn, const Octstr *sql, List *binds, List **result);
     /*
      * Database specific update/insert/delete.
-     * @params conn - database specific connection ; sql - sql statement
+     * @params conn - database specific connection ; sql - sql statement;
+     *         binds - list of Octstr values for binding holes in sql (NULL if no binds);
      * @return #rows processed ; -1 if a error occurs
      */
-    int (*update) (void *conn, const Octstr *sql);
+    int (*update) (void *conn, const Octstr *sql, List *binds);
 };
 
 struct DBPool
Index: gwlib/dbpool_oracle.c
===================================================================
RCS file: /home/cvs/gateway/gwlib/dbpool_oracle.c,v
retrieving revision 1.4
diff -u -r1.4 dbpool_oracle.c
--- gwlib/dbpool_oracle.c	22 Jan 2004 14:08:25 -0000	1.4
+++ gwlib/dbpool_oracle.c	11 Mar 2004 20:01:50 -0000
@@ -63,6 +63,8 @@
  * believe me, just check it youself with valgrind ;)
  *
  * Alexander Malysh <[email protected]>
+ * Robert Ga³ach <[email protected]>
+ *      2004 Added support for binding variables.
  */
 
 #ifdef HAVE_ORACLE
@@ -70,7 +72,7 @@
 #include <oci.h>
 
 /* forward decl. */
-static int oracle_select(void *theconn, const Octstr *sql, List **res);
+static int oracle_select(void *theconn, const Octstr *sql, List *binds, List **res);
 
 struct ora_conn {
     /* environment handle */
@@ -82,7 +84,7 @@
 };
 
 /* This function prints the error */
-void oracle_checkerr(OCIError *errhp, sword status)
+static void oracle_checkerr(OCIError *errhp, sword status)
 {
     text errbuf[512];
     sb4 errcode = 0;
@@ -247,7 +249,7 @@
     /* TODO Check for appropriate OCI function */
     sql = octstr_create("SELECT 1 FROM DUAL");
 
-    ret = oracle_select(conn, sql, &res);
+    ret = oracle_select(conn, sql, NULL, &res);
     if (ret != -1 && list_len(res) > 0) {
         List *row = list_extract_first(res);
         list_destroy(row, octstr_destroy_item);
@@ -274,7 +276,7 @@
 }
 
 
-static int oracle_select(void *theconn, const Octstr *sql, List **res)
+static int oracle_select(void *theconn, const Octstr *sql, List *binds, List **res)
 {
     List *row;
     OCIStmt *stmt;
@@ -290,6 +292,7 @@
     };
     struct data_s *data;
     struct ora_conn *conn = (struct ora_conn*) theconn;
+    int binds_len = (binds ? list_len(binds) : 0);
 
     *res = NULL;
 
@@ -307,6 +310,21 @@
         OCIHandleFree(stmt, OCI_HTYPE_STMT);
         return -1;
     }
+
+    /* bind variables */
+    for (i = 0; i < binds_len; i++) {
+        OCIBind *bndhp = NULL;
+        Octstr *bind = list_get(binds, i);
+        status = OCIBindByPos(stmt, &bndhp, 
+                              conn->errhp, (i+1), (dvoid *) octstr_get_cstr(bind),
+                              (sword) octstr_len(bind)+1, SQLT_STR, (dvoid *) 0, (ub2 *)0,
+                              (ub2 *)0, (ub4)0, (ub4 *)0, OCI_DEFAULT);
+        if (OCI_SUCCESS != status) {
+            oracle_checkerr(conn->errhp, status);
+            OCIHandleFree(stmt, OCI_HTYPE_STMT);
+            return -1;
+        }
+    }
     /* execute our statement */
     status = OCIStmtExecute(conn->svchp, stmt, conn->errhp, 0, 0, NULL, NULL, 
                             OCI_DEFAULT);
@@ -439,13 +457,14 @@
 }
 
 
-static int oracle_update(void *theconn, const Octstr *sql)
+static int oracle_update(void *theconn, const Octstr *sql, List *binds)
 {
     OCIStmt *stmt;
     sword status;
-    ub4 rows = 0;
+    ub4 rows = 0, i;
     struct ora_conn *conn = (struct ora_conn*) theconn;
-
+    int binds_len = (binds ? list_len(binds) : 0);
+    
     /* allocate statement handle */
     status = OCIHandleAlloc(conn->envp, (dvoid**)&stmt, OCI_HTYPE_STMT, 0,0);
     if (OCI_SUCCESS != status) {
@@ -462,6 +481,22 @@
         return -1;
     }
     debug("dbpool.oracle",0,"OCIStmtPrepare done");
+   
+    /* bind variables */
+    for (i = 0; i < binds_len; i++) {
+        Octstr *bind = list_get(binds, i);
+        OCIBind *bndhp = NULL;
+        status = OCIBindByPos(stmt, &bndhp, 
+                              conn->errhp, (i+1), (dvoid *) octstr_get_cstr(bind),
+                              (sword) octstr_len(bind)+1, SQLT_STR, (dvoid *) 0, (ub2 *)0,
+                              (ub2 *)0, (ub4)0, (ub4 *)0, OCI_DEFAULT);
+        if (OCI_SUCCESS != status) {
+            oracle_checkerr(conn->errhp, status);
+            OCIHandleFree(stmt, OCI_HTYPE_STMT);
+            return -1;
+        }
+    }
+    
     /* execute our statement */
     status = OCIStmtExecute(conn->svchp, stmt, conn->errhp, 1, 0, NULL, NULL, 
                             /*OCI_DEFAULT*/ OCI_COMMIT_ON_SUCCESS);
@@ -481,7 +516,7 @@
     debug("dbpool.oracle",0,"rows processed = %d", rows);
 
     OCIHandleFree(stmt, OCI_HTYPE_STMT);
-
+    
     return (int) rows;
 }
 
Index: gw/dlr_oracle.c
===================================================================
RCS file: /home/cvs/gateway/gw/dlr_oracle.c,v
retrieving revision 1.5
diff -u -r1.5 dlr_oracle.c
--- gw/dlr_oracle.c	22 Jan 2004 14:08:24 -0000	1.5
+++ gw/dlr_oracle.c	11 Mar 2004 20:01:50 -0000
@@ -58,6 +58,8 @@
  * dlr_oracle.c - Oracle dlr storage implementation.
  *
  * Author: Alexander Malysh <[email protected]>, (C) 2003
+ * Robert Ga³ach <[email protected]>
+ *      2004 Rewrited sql queries to use binding variables.
  *
  * Copyright: See COPYING file that comes with this distribution
  */
@@ -91,12 +93,12 @@
     if (conn == NULL)
         return -1;
 
-    sql = octstr_format("SELECT count(*) FROM %s", octstr_get_cstr(fields->table));
+    sql = octstr_format("SELECT count(*) FROM %S", fields->table);
 #if defined(DLR_TRACE)
     debug("dlr.oracle", 0, "sql: %s", octstr_get_cstr(sql));
 #endif
 
-    if (dbpool_conn_select(conn, sql, &result) != 0) {
+    if (dbpool_conn_select(conn, sql, NULL, &result) != 0) {
         octstr_destroy(sql);
         dbpool_conn_produce(conn);
         return -1;
@@ -122,9 +124,9 @@
 
 static void dlr_add_oracle(struct dlr_entry *entry)
 {
-    Octstr *sql;
+    Octstr *sql, *os_mask;
     DBPoolConn *pconn;
-
+    List *binds = list_create();
     debug("dlr.oracle", 0, "adding DLR entry into database");
 
     pconn = dbpool_conn_consume(pool);
@@ -134,26 +136,32 @@
         return;
     }
 
-    sql = octstr_format("INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s, %s, %s) VALUES "
-                        "('%s', '%s', '%s', '%s', '%s', '%s', '%d', '%s', '%d')",
-                        octstr_get_cstr(fields->table), octstr_get_cstr(fields->field_smsc),
-                        octstr_get_cstr(fields->field_ts),
-                        octstr_get_cstr(fields->field_src), octstr_get_cstr(fields->field_dst),
-                        octstr_get_cstr(fields->field_serv), octstr_get_cstr(fields->field_url),
-                        octstr_get_cstr(fields->field_mask), octstr_get_cstr(fields->field_boxc),
-                        octstr_get_cstr(fields->field_status),
-                        octstr_get_cstr(entry->smsc), octstr_get_cstr(entry->timestamp), octstr_get_cstr(entry->source),
-                        octstr_get_cstr(entry->destination), octstr_get_cstr(entry->service), octstr_get_cstr(entry->url),
-                        entry->mask, octstr_get_cstr(entry->boxc_id), 0);
-
+    sql = octstr_format("INSERT INTO %S (%S, %S, %S, %S, %S, %S, %S, %S, %S) VALUES "
+                        "(:1, :2, :3, :4, :5, :6, :7, :8, 0)",
+                        fields->table, fields->field_smsc, fields->field_ts,
+                        fields->field_src, fields->field_dst, fields->field_serv, 
+                        fields->field_url, fields->field_mask, fields->field_boxc,
+                        fields->field_status);
+    os_mask = octstr_format("%d", entry->mask);
+    
+    list_append(binds, entry->smsc);         /* :1 */
+    list_append(binds, entry->timestamp);    /* :2 */
+    list_append(binds, entry->source);       /* :3 */
+    list_append(binds, entry->destination);  /* :4 */
+    list_append(binds, entry->service);      /* :5 */
+    list_append(binds, entry->url);          /* :6 */
+    list_append(binds, os_mask);             /* :7 */
+    list_append(binds, entry->boxc_id);      /* :8 */
 #if defined(DLR_TRACE)
     debug("dlr.oracle", 0, "sql: %s", octstr_get_cstr(sql));
 #endif
-    if (dbpool_conn_update(pconn, sql) == -1)
+    if (dbpool_conn_update(pconn, sql, binds) == -1)
         error(0, "DLR: ORACLE: Error while adding dlr entry for DST<%s>", octstr_get_cstr(entry->destination));
 
     dbpool_conn_produce(pconn);
     octstr_destroy(sql);
+    list_destroy(binds, NULL);
+    octstr_destroy(os_mask);
     dlr_entry_destroy(entry);
 }
 
@@ -161,6 +169,7 @@
 {
     Octstr *sql;
     DBPoolConn *pconn;
+    List *binds = list_create();
 
     debug("dlr.oracle", 0, "removing DLR from database");
 
@@ -169,18 +178,23 @@
     if (pconn == NULL)
         return;
 
-    sql = octstr_format("DELETE FROM %s WHERE %s='%s' AND %s='%s' AND ROWNUM < 2",
-                        octstr_get_cstr(fields->table), octstr_get_cstr(fields->field_smsc),
-                        octstr_get_cstr(smsc), octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts));
+    sql = octstr_format("DELETE FROM %S WHERE %S=:1 AND %S=:2 AND %S=:3 AND ROWNUM < 2",
+                        fields->table, fields->field_smsc,
+                        fields->field_ts, fields->field_dst);
+
+    list_append(binds, (Octstr *)smsc);      /* :1 */
+    list_append(binds, (Octstr *)ts);        /* :2 */
+    list_append(binds, (Octstr *)dst);       /* :3 */
 
 #if defined(DLR_TRACE)
     debug("dlr.oracle", 0, "sql: %s", octstr_get_cstr(sql));
 #endif
 
-    if (dbpool_conn_update(pconn, sql) == -1)
+    if (dbpool_conn_update(pconn, sql, binds) == -1)
         error(0, "DLR: ORACLE: Error while removing dlr entry for DST<%s>", octstr_get_cstr(dst));
 
     dbpool_conn_produce(pconn);
+    list_destroy(binds, NULL);
     octstr_destroy(sql);
 }
 
@@ -190,27 +204,33 @@
     DBPoolConn *pconn;
     List *result = NULL, *row;
     struct dlr_entry *res = NULL;
+    List *binds = list_create();
 
     pconn = dbpool_conn_consume(pool);
     if (pconn == NULL) /* should not happens, but sure is sure */
         return NULL;
 
-    sql = octstr_format("SELECT %s, %s, %s, %s, %s, %s FROM %s WHERE %s='%s' AND %s='%s' AND ROWNUM < 2",
-                        octstr_get_cstr(fields->field_mask), octstr_get_cstr(fields->field_serv),
-                        octstr_get_cstr(fields->field_url), octstr_get_cstr(fields->field_src),
-                        octstr_get_cstr(fields->field_dst), octstr_get_cstr(fields->field_boxc),
-                        octstr_get_cstr(fields->table), octstr_get_cstr(fields->field_smsc),
-                        octstr_get_cstr(smsc), octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts));
+    sql = octstr_format("SELECT %S, %S, %S, %S, %S, %S FROM %S WHERE %S=:1 AND %S=:2 AND %S=:3 AND ROWNUM < 2",
+                        fields->field_mask, fields->field_serv,
+                        fields->field_url, fields->field_src,
+                        fields->field_dst, fields->field_boxc,
+                        fields->table, fields->field_smsc,
+                        fields->field_ts, fields->field_dst);
+
+    list_append(binds, (Octstr *)smsc);      /* :1 */
+    list_append(binds, (Octstr *)ts);        /* :2 */
+    list_append(binds, (Octstr *)dst);       /* :3 */
 
 #if defined(DLR_TRACE)
     debug("dlr.oracle", 0, "sql: %s", octstr_get_cstr(sql));
 #endif
-    if (dbpool_conn_select(pconn, sql, &result) != 0) {
+    if (dbpool_conn_select(pconn, sql, binds, &result) != 0) {
         octstr_destroy(sql);
         dbpool_conn_produce(pconn);
         return NULL;
     }
     octstr_destroy(sql);
+    list_destroy(binds, NULL);
     dbpool_conn_produce(pconn);
 
 #define LO2CSTR(r, i) octstr_get_cstr(list_get(r, i))
@@ -237,8 +257,9 @@
 
 static void dlr_update_oracle(const Octstr *smsc, const Octstr *ts, const Octstr *dst, int status)
 {
-    Octstr *sql;
+    Octstr *sql, *os_status;
     DBPoolConn *pconn;
+    List *binds = list_create();
 
     debug("dlr.oracle", 0, "updating DLR status in database");
 
@@ -247,19 +268,48 @@
     if (pconn == NULL)
         return;
 
-    sql = octstr_format("UPDATE %s SET %s=%d WHERE %s='%s' AND %s='%s' AND ROWNUM < 2",
-                        octstr_get_cstr(fields->table),
-                        octstr_get_cstr(fields->field_status), status,
-                        octstr_get_cstr(fields->field_smsc), octstr_get_cstr(smsc),
-                        octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts));
-
+    sql = octstr_format("UPDATE %S SET %S=:1 WHERE %S=:2 AND %S=:3 AND %S=:4 AND ROWNUM < 2",
+                        fields->table, fields->field_status,
+                        fields->field_smsc, fields->field_ts, fields->field_dst);
+
+    os_status = octstr_format("%d", status);
+    list_append(binds, (Octstr *)os_status); /* :1 */
+    list_append(binds, (Octstr *)smsc);      /* :2 */
+    list_append(binds, (Octstr *)ts);        /* :3 */
+    list_append(binds, (Octstr *)dst);       /* :4 */
 #if defined(DLR_TRACE)
     debug("dlr.oracle", 0, "sql: %s", octstr_get_cstr(sql));
 #endif
-    if (dbpool_conn_update(pconn, sql) == -1)
+    if (dbpool_conn_update(pconn, sql, binds) == -1)
         error(0, "DLR: ORACLE: Error while updating dlr entry for DST<%s>", octstr_get_cstr(dst));
 
     dbpool_conn_produce(pconn);
+    list_destroy(binds, NULL);
+    octstr_destroy(os_status);
+    octstr_destroy(sql);
+}
+
+static void dlr_flush_oracle (void)
+{
+    Octstr *sql;
+    DBPoolConn *pconn;
+    int rows;
+
+    pconn = dbpool_conn_consume(pool);
+    /* just for sure */
+    if (pconn == NULL)
+        return;
+
+    sql = octstr_format("DELETE FROM DLR");
+#if defined(DLR_TRACE)
+    debug("dlr.oracle", 0, "sql: %s", octstr_get_cstr(sql));
+#endif
+    rows = dbpool_conn_update(pconn, sql, NULL);
+    if (rows == -1)
+        error(0, "DLR: ORACLE: Error while flushing dlr entries from database");
+    else
+        debug("dlr.oracle", 0, "Flushing %d DLR entries from database", rows);
+    dbpool_conn_produce(pconn);
     octstr_destroy(sql);
 }
 
@@ -270,7 +320,8 @@
     .dlr_add = dlr_add_oracle,
     .dlr_get = dlr_get_oracle,
     .dlr_remove = dlr_remove_oracle,
-    .dlr_update = dlr_update_oracle
+    .dlr_update = dlr_update_oracle,
+    .dlr_flush = dlr_flush_oracle
 };
 
 struct dlr_storage *dlr_init_oracle(Cfg *cfg)
Index: test/test_dbpool.c
===================================================================
RCS file: /home/cvs/gateway/test/test_dbpool.c,v
retrieving revision 1.6
diff -u -r1.6 test_dbpool.c
--- test/test_dbpool.c	22 Jan 2004 14:08:25 -0000	1.6
+++ test/test_dbpool.c	11 Mar 2004 20:01:50 -0000
@@ -186,11 +186,13 @@
 {
     DBPool *pool = arg;
     DBPoolConn *pconn = NULL;
+    /*
     struct ora_conn *conn;
-    int i;
     OCIStmt *stmt;
     sword status;
     ub2 stmt_type;
+    */
+    int i;
     List *result;
 
     for (i = 1; i <= queries; i++) {
@@ -199,19 +201,19 @@
         if (pconn == NULL)
             continue;
 #if 1 /* selects */
-        if (dbpool_conn_select(pconn, sql, &result) == 0) {
+        if (dbpool_conn_select(pconn, sql, NULL, &result) == 0) {
             long i,j;
             for (i=0; i < list_len(result); i++) {
                 List *row = list_get(result, i);
                 for (j=0; j < list_len(row); j++)
-                    debug("", 0, "col = %d   value = '%s'", j, octstr_get_cstr(list_get(row,j)));
+                    debug("", 0, "col = %ld   value = '%s'", j, octstr_get_cstr(list_get(row,j)));
                 list_destroy(row, octstr_destroy_item);
             }
         }
         list_destroy(result, NULL);
         dbpool_conn_produce(pconn);
 #else /* only updates */
-        debug("", 0, "rows processed = %d ", dbpool_conn_update(pconn, sql));
+        debug("", 0, "rows processed = %d ", dbpool_conn_update(pconn, sql, NULL));
         dbpool_conn_produce(pconn);
 #endif
     }
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.