[Devel] Postgres reconnect patch
Bogdan-Andrei Iancu
bogdan at voice-system.ro
Fri Aug 5 10:10:42 CEST 2005
Hi Michael,
I will try to make fast overview, but as I'm not able to test the
postgres (no server :( ) I will appreciate if the some other postgres
users may test the patch before going into CVS.
regards,
Bogdan
Michael Ulitskiy wrote:
>Hi Klaus,
>
>I've seen the problem described by Klaus too, but first of all I was
>concerned that postgres module cannot reconnect to db after connection
>failure. It crashed openser.
>I've done some research on the module source code and I believe
>I've found a logical mistake in it.
>The issue is that parse_sql_url() function is supplied with the only copy
>of sql url and it corrupts it. So if the connection fails and module
>tries to reparse url it fails as CON_SQLURL(_h) is corrupted by first
>parsing.
>I've created a simple patch that corrects the problem. What it does it
>it introduce a temporary buffer with sql url string for parse_sql_url to work
>on. Also it makes so that original sql url is not deleted from connection structure
>until db_close() is called.
>Patch is attached.
>Also if in dbase.c in db_init() function you comment out the following:
>
>/*
> if (connect_db(res) < 0)
> {
> PLOG("db_init", "Error while trying to open database, FATAL\n")
> aug_free(res);
> return((db_con_t *) 0);
> }
>*/
>
>you'll get a "delay connect until used" feature that can be usefull due to lack
>of connection pool for postgres.
>Could please some of developers review this patch?
>
>Michael
>
>
>On Thursday 04 August 2005 11:39 am, Klaus Darilion wrote:
>
>
>>Hi!
>>
>>I had several situations in which openser crashed if the DB lookup
>>fails, e.g:
>>table does not exist (acc module)
>>wrong SQL query (lcr module)
>>
>>Whereas is some cases (wrong table permissions, avpops module) openser
>>keeps running.
>>
>>I do not know if these problems also occurs with mysql. If not, the
>>postgresql module would really need some review.
>>
>>regards,
>>klaus
>>
>>_______________________________________________
>>Devel mailing list
>>Devel at openser.org
>>http://openser.org/cgi-bin/mailman/listinfo/devel
>>
>>
>>
>
>
>
>------------------------------------------------------------------------
>
>diff -ur postgres.bak/con_postgres.h postgres/con_postgres.h
>--- postgres.bak/con_postgres.h 2005-08-04 14:09:11.000000000 -0400
>+++ postgres/con_postgres.h 2005-08-04 13:48:44.000000000 -0400
>@@ -60,4 +60,6 @@
> #define PLOG(f,s) LOG(L_ERR, "PG[%d] %s %s\n",__LINE__,f,s)
> #define DLOG(f,s) LOG(L_INFO, "PG[%d] %s %s\n",__LINE__,f,s)
>
>+#define SQLURL_LEN 256
>+
> #endif /* CON_POSTGRES_H */
>diff -ur postgres.bak/dbase.c postgres/dbase.c
>--- postgres.bak/dbase.c 2005-08-04 14:09:11.000000000 -0400
>+++ postgres/dbase.c 2005-08-04 14:13:44.000000000 -0400
>@@ -48,7 +48,7 @@
> static char sql_buf[SQL_BUF_LEN];
>
> static int submit_query(db_con_t* _h, const char* _s);
>-static int connect_db(db_con_t* _h, const char* _db_url);
>+static int connect_db(db_con_t* _h);
> static int disconnect_db(db_con_t* _h);
> static int free_query(db_con_t* _h);
>
>@@ -57,7 +57,6 @@
> **
> ** Arguments :
> ** db_con_t * as previously supplied by db_init()
>-** char *_db_url the database to connect to
> **
> ** Returns :
> ** 0 upon success
>@@ -71,9 +70,10 @@
> ** should clean up after itself.
> */
>
>-static int connect_db(db_con_t* _h, const char* _db_url)
>+static int connect_db(db_con_t* _h)
> {
> char* user, *password, *host, *port, *database;
>+ char urlbuf[SQLURL_LEN];
>
> if(! _h)
> {
>@@ -86,6 +86,11 @@
> DLOG("connect_db", "disconnect first!");
> disconnect_db(_h);
> }
>+
>+ if (!CON_SQLURL(_h)) {
>+ PLOG("connect_db","FATAL ERROR: no sql url!");
>+ return(-1);
>+ }
>
> /*
> ** CON_CONNECTED(_h) is now 0, set by disconnect_db()
>@@ -93,19 +98,19 @@
>
> /*
> ** Note :
>- ** Make a scratch pad copy of given SQL URL.
>+ ** Make a scratch pad copy of given SQL URL in urlbuf and work on it.
> ** all memory allocated to this connection is rooted
> ** from this.
> ** This is an important concept.
> ** as long as you always allocate memory using the function:
> ** mem = aug_alloc(size, CON_SQLURL(_h)) or
>- ** str = aug_strdup(string, CON_SQLURL(_h))
> ** where size is the amount of memory, then in the future
> ** when CON_SQLURL(_h) is freed (in the function disconnect_db())
> ** all other memory allocated in this manner is freed.
> ** this will keep memory leaks from happening.
> */
>- CON_SQLURL(_h) = aug_strdup((char *) _db_url, (char *) _h);
>+
>+ snprintf(urlbuf,SQLURL_LEN,"%s",CON_SQLURL(_h));
>
> /*
> ** get the connection parameters parsed from the db_url string
>@@ -115,14 +120,13 @@
> ** dbport : the port to connect to database on
> ** dbname : the name of the database
> */
>- if(parse_sql_url(CON_SQLURL(_h),
>+ if(parse_sql_url(urlbuf,
> &user,&password,&host,&port,&database) < 0)
> {
> char buf[256];
>- sprintf(buf, "Error while parsing %s", _db_url);
>+ sprintf(buf, "Error while parsing %s", CON_SQLURL(_h));
> PLOG("connect_db", buf);
>
>- aug_free(CON_SQLURL(_h));
> return -3;
> }
>
>@@ -137,7 +141,6 @@
> {
> PLOG("connect_db", PQerrorMessage(CON_CONNECTION(_h)));
> PQfinish(CON_CONNECTION(_h));
>- aug_free(CON_SQLURL(_h));
> return -4;
> }
>
>@@ -174,14 +177,6 @@
> return(0);
> }
>
>- /*
>- ** free lingering memory tree if it exists
>- */
>- if(CON_SQLURL(_h))
>- {
>- aug_free(CON_SQLURL(_h));
>- CON_SQLURL(_h) = (char *) 0;
>- }
>
> /*
> ** ignore if there is no current connection
>@@ -230,6 +225,11 @@
> db_con_t* res;
>
> DLOG("db_init", "entry");
>+ if (strlen(_sqlurl)>(SQLURL_LEN-1))
>+ {
>+ PLOG("db_init","ERROR: sql url too long");
>+ return ((db_con_t *) 0);
>+ }
>
> /*
> ** this is the root memory for this database connection.
>@@ -239,8 +239,9 @@
> res->tail = (unsigned long)aug_alloc
> (sizeof(struct con_postgres), (char*)res);
> memset((struct con_postgres*)res->tail, 0, sizeof(struct con_postgres));
>+ CON_SQLURL(res)=aug_strdup((char*)_sqlurl,(char *)res);
>
>- if (connect_db(res, _sqlurl) < 0)
>+ if (connect_db(res) < 0)
> {
> PLOG("db_init", "Error while trying to open database, FATAL\n");
> aug_free(res);
>@@ -274,6 +275,15 @@
> }
>
> disconnect_db(_h);
>+ /*
>+ ** free sql url memory if it exists
>+ */
>+ if(CON_SQLURL(_h))
>+ {
>+ aug_free(CON_SQLURL(_h));
>+ CON_SQLURL(_h) = (char *) 0;
>+ }
>+
> aug_free(_h);
>
> }
>@@ -475,7 +485,7 @@
> ** attempt to open the db.
> */
>
>- if((rv = connect_db(_h, CON_SQLURL(_h))) != 0)
>+ if((rv = connect_db(_h)) != 0)
> {
> /*
> ** our attempt to fix the connection failed
>@@ -485,6 +495,7 @@
> PLOG("begin_transaction",buf);
> return(rv);
> }
>+ PLOG("db_connect","successfully reconnected");
> }
> else
> {
>
>
>------------------------------------------------------------------------
>
>_______________________________________________
>Devel mailing list
>Devel at openser.org
>http://openser.org/cgi-bin/mailman/listinfo/devel
>
>
More information about the Devel
mailing list