From: Marko Kreen Date: Wed, 28 Mar 2007 09:07:34 +0000 (+0000) Subject: ver 1.0.2 - fix 2 corner-case bugs X-Git-Tag: pgbouncer_1_0_2~3 X-Git-Url: http://git.postgresql.org/gitweb/static/gitweb.js?a=commitdiff_plain;h=fa93693505a2a5254331bcda212e843ca780485a;p=pgbouncer.git ver 1.0.2 - fix 2 corner-case bugs * libevent may report a deleted event inside same loop. Avoid socket reuse for one loop. * release_server() from disconnect_client() didnt look it the packet was actually sent. --- diff --git a/Makefile b/Makefile index 5e642f5..6152f45 100644 --- a/Makefile +++ b/Makefile @@ -14,6 +14,8 @@ DIRS = etc src debian # keep autoconf stuff separate -include config.mak +CFLAGS += -DDBGVER="\"compiled by <$${USER}@`hostname`> at `date '+%Y-%m-%d %H:%M:%S'`\"" + # calculate full-path values OBJS = $(SRCS:.c=.o) hdrs = $(addprefix $(srcdir)/src/, $(HDRS)) diff --git a/NEWS b/NEWS index 99c4fb0..8ab22bc 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,11 @@ +2007-03-28 - PgBouncer 1.0.2 - "Supersonic Spoon" + + * libevent may report a deleted event inside same loop. + Avoid socket reuse for one loop. + * release_server() from disconnect_client() didnt look + it the packet was actually sent. + 2007-03-15 - PgBouncer 1.0.1 - "Alien technology" = Fixes = diff --git a/configure.ac b/configure.ac index 24231bc..f422fb7 100644 --- a/configure.ac +++ b/configure.ac @@ -1,6 +1,6 @@ dnl Process this file with autoconf to produce a configure script. -AC_INIT(pgbouncer, 1.0.1) +AC_INIT(pgbouncer, 1.0.2) AC_CONFIG_SRCDIR(src/bouncer.h) AC_CONFIG_HEADER(config.h) diff --git a/debian/changelog b/debian/changelog index fd32287..0eb83bf 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +pgbouncer (1.0.2) unstable; urgency=low + + * 2 more bugs. + + -- Marko Kreen Wed, 28 Mar 2007 12:04:39 +0300 + pgbouncer (1.0.1) unstable; urgency=low * Couple hotfixes. diff --git a/src/admin.c b/src/admin.c index 2a8da8a..c3e1f16 100644 --- a/src/admin.c +++ b/src/admin.c @@ -687,7 +687,7 @@ static bool admin_show_version(PgSocket *admin) bool res; SEND_generic(res, admin, 'N', "ssss", "SNOTICE", "C00000", - "MPgBouncer version " PACKAGE_VERSION, ""); + "M" FULLVER, ""); if (res) res = admin_ready(admin, "SHOW"); return res; diff --git a/src/bouncer.h b/src/bouncer.h index 5d4fc85..4230e4a 100644 --- a/src/bouncer.h +++ b/src/bouncer.h @@ -24,15 +24,23 @@ #include +#ifdef DBGVER +#define FULLVER PACKAGE_NAME " version " PACKAGE_VERSION " (" DBGVER ")" +#else +#define FULLVER PACKAGE_NAME " version " PACKAGE_VERSION +#endif + /* each state corresponts to a list */ enum SocketState { CL_FREE, /* free_client_list */ + CL_JUSTFREE, /* justfree_client_list */ CL_LOGIN, /* login_client_list */ CL_WAITING, /* pool->waiting_client_list */ CL_ACTIVE, /* pool->active_client_list */ CL_CANCEL, /* pool->cancel_req_list */ SV_FREE, /* free_server_list */ + SV_JUSTFREE, /* justfree_server_list */ SV_LOGIN, /* pool->new_server_list */ SV_IDLE, /* pool->idle_server_list */ SV_ACTIVE, /* pool->active_server_list */ diff --git a/src/client.c b/src/client.c index 5abb916..b1ad1f6 100644 --- a/src/client.c +++ b/src/client.c @@ -341,7 +341,14 @@ bool client_proto(SBuf *sbuf, SBufEvent evtype, MBuf *pkt, void *arg) PgSocket *client = arg; Assert(!is_server_socket(client)); - Assert(client->state != SV_FREE); + Assert(client->sbuf.sock); + Assert(client->state != CL_FREE); + + if (client->state == CL_JUSTFREE) { + /* SBuf should catch the case */ + slog_warning(client, "state=CL_JUSTFREE, should not happen"); + return false; + } switch (evtype) { case SBUF_EV_CONNECT_OK: diff --git a/src/janitor.c b/src/janitor.c index 2840397..0278a52 100644 --- a/src/janitor.c +++ b/src/janitor.c @@ -223,7 +223,7 @@ static int per_loop_suspend(PgPool *pool) /* * this function is called for each event loop. */ -void per_loop_object_maint(void) +void per_loop_maint(void) { List *item; PgPool *pool; diff --git a/src/janitor.h b/src/janitor.h index e37c6e1..39b47d3 100644 --- a/src/janitor.h +++ b/src/janitor.h @@ -19,6 +19,6 @@ void janitor_setup(void); void config_postprocess(void); void resume_all(void); -void per_loop_object_maint(void); +void per_loop_maint(void); bool suspend_socket(PgSocket *sk); diff --git a/src/list.h b/src/list.h index 8bc2854..e02bb1d 100644 --- a/src/list.h +++ b/src/list.h @@ -112,6 +112,32 @@ static inline List *list_first(List *list) return list->next; } +/* put all elems in one list in the start of another list */ +static inline void list_prepend_list(List *src, List *dst) +{ + if (list_empty(src)) + return; + src->next->prev = dst; + src->prev->next = dst->next; + dst->next->prev = src->prev; + dst->next = src->next; + + src->next = src->prev = src; +} + +/* put all elems in one list in the end of another list */ +static inline void list_append_list(List *src, List *dst) +{ + if (list_empty(src)) + return; + src->next->prev = dst->prev; + src->prev->next = dst; + dst->prev->next = src->next; + dst->prev = src->prev; + + src->next = src->prev = src; +} + /* remove first elem from list and return with casting */ #define list_pop_type(list, typ, field) \ (list_empty(list) ? NULL \ @@ -151,6 +177,13 @@ struct StatList { const char *name; }; +static inline void statlist_inc_count(StatList *list, int val) +{ + list->cur_count += val; + if (list->cur_count > list->max_count) + list->max_count = list->cur_count; +} + #define STATLIST(var) StatList var = { {&var.head, &var.head}, 0, 0, #var } static inline void statlist_reset(StatList *list) @@ -161,25 +194,19 @@ static inline void statlist_reset(StatList *list) static inline void statlist_prepend(List *item, StatList *list) { list_prepend(item, &list->head); - list->cur_count ++; - if (list->cur_count > list->max_count) - list->max_count = list->cur_count; + statlist_inc_count(list, 1); } static inline void statlist_append(List *item, StatList *list) { list_append(item, &list->head); - list->cur_count ++; - if (list->cur_count > list->max_count) - list->max_count = list->cur_count; + statlist_inc_count(list, 1); } static inline void statlist_put_before(List *item, StatList *list, List *pos) { list_append(item, pos); - list->cur_count++; - if (list->cur_count > list->max_count) - list->max_count = list->cur_count; + statlist_inc_count(list, 1); } static inline void statlist_remove(List *item, StatList *list) @@ -227,6 +254,20 @@ static inline List *statlist_pop(StatList *list) return item; } +static inline void statlist_prepend_list(StatList *src, StatList *dst) +{ + list_prepend_list(&src->head, &dst->head); + statlist_inc_count(dst, src->cur_count); + src->cur_count = 0; +} + +static inline void statlist_append_list(StatList *src, StatList *dst) +{ + list_append_list(&src->head, &dst->head); + statlist_inc_count(dst, src->cur_count); + src->cur_count = 0; +} + static inline List *statlist_first(StatList *list) { return list_first(&list->head); diff --git a/src/main.c b/src/main.c index 9fdffc3..4f48eaa 100644 --- a/src/main.c +++ b/src/main.c @@ -400,7 +400,8 @@ static void main_loop_once(void) { reset_time_cache(); event_loop(EVLOOP_ONCE); - per_loop_object_maint(); + per_loop_maint(); + reuse_just_freed_objects(); } /* boot everything */ @@ -418,7 +419,7 @@ int main(int argc, char *argv[]) cf_verbose++; break; case 'V': - printf("%s version %s\n", PACKAGE_NAME, PACKAGE_VERSION); + printf("%s\n", FULLVER); return 0; case 'd': cf_daemon = 1; diff --git a/src/objects.c b/src/objects.c index e5e857f..69b4a28 100644 --- a/src/objects.c +++ b/src/objects.c @@ -36,6 +36,14 @@ STATLIST(free_client_list); STATLIST(free_server_list); STATLIST(login_client_list); +/* + * libevent may still report events when event_del() + * is called from somewhere else. So hide just freed + * PgSockets for one loop. + */ +STATLIST(justfree_client_list); +STATLIST(justfree_server_list); + /* how many client sockets are allocated */ static int absolute_client_count = 0; /* how many server sockets are allocated */ @@ -148,6 +156,9 @@ void change_client_state(PgSocket *client, SocketState newstate) case CL_FREE: statlist_remove(&client->head, &free_client_list); break; + case CL_JUSTFREE: + statlist_remove(&client->head, &justfree_client_list); + break; case CL_LOGIN: statlist_remove(&client->head, &login_client_list); break; @@ -169,9 +180,11 @@ void change_client_state(PgSocket *client, SocketState newstate) /* put to new location */ switch (client->state) { case CL_FREE: - /* use LIFO the keep cache warm */ statlist_prepend(&client->head, &free_client_list); break; + case CL_JUSTFREE: + statlist_append(&client->head, &justfree_client_list); + break; case CL_LOGIN: statlist_append(&client->head, &login_client_list); break; @@ -199,6 +212,9 @@ void change_server_state(PgSocket *server, SocketState newstate) case SV_FREE: statlist_remove(&server->head, &free_server_list); break; + case SV_JUSTFREE: + statlist_remove(&server->head, &justfree_server_list); + break; case SV_LOGIN: statlist_remove(&server->head, &pool->new_server_list); break; @@ -223,14 +239,16 @@ void change_server_state(PgSocket *server, SocketState newstate) /* put to new location */ switch (server->state) { case SV_FREE: - /* use LIFO the keep cache warm */ statlist_prepend(&server->head, &free_server_list); break; + case SV_JUSTFREE: + statlist_append(&server->head, &justfree_server_list); + break; case SV_LOGIN: statlist_append(&server->head, &pool->new_server_list); break; case SV_USED: - /* again, LIFO */ + /* use LIFO */ statlist_prepend(&server->head, &pool->used_server_list); break; case SV_TESTED: @@ -610,7 +628,7 @@ void disconnect_server(PgSocket *server, bool notify, const char *reason) sbuf_answer(&server->sbuf, pkt_term, sizeof(pkt_term)); sbuf_close(&server->sbuf); - change_server_state(server, SV_FREE); + change_server_state(server, SV_JUSTFREE); } /* drop client connection */ @@ -622,7 +640,8 @@ void disconnect_client(PgSocket *client, bool notify, const char *reason) case CL_ACTIVE: if (client->link) { PgSocket *server = client->link; - if (server->ready) { + /* ->ready may be set before all is sent */ + if (server->ready && sbuf_has_no_state(&server->sbuf)) { release_server(server); } else { server->link = NULL; @@ -649,7 +668,7 @@ void disconnect_client(PgSocket *client, bool notify, const char *reason) sbuf_close(&client->sbuf); - change_client_state(client, CL_FREE); + change_client_state(client, CL_JUSTFREE); } /* the pool needs new connection, if possible */ @@ -820,7 +839,7 @@ void forward_cancel_request(PgSocket *server) SEND_CancelRequest(res, server, req->cancel_key); - change_client_state(req, CL_FREE); + change_client_state(req, CL_JUSTFREE); } bool use_client_socket(int fd, PgAddr *addr, @@ -940,4 +959,24 @@ void tag_database_dirty(PgDatabase *db) } } +/* move objects from justfree_* to free_* lists */ +void reuse_just_freed_objects(void) +{ + List *tmp, *item; + PgSocket *sk; + + /* + * Obviously, if state would be set to *_FREE, + * they could be moved in one go. + */ + statlist_for_each_safe(item, &justfree_client_list, tmp) { + sk = container_of(item, PgSocket, head); + change_client_state(sk, CL_FREE); + } + statlist_for_each_safe(item, &justfree_server_list, tmp) { + sk = container_of(item, PgSocket, head); + change_server_state(sk, SV_FREE); + } +} + diff --git a/src/objects.h b/src/objects.h index 9d95ad5..317cf47 100644 --- a/src/objects.h +++ b/src/objects.h @@ -61,3 +61,5 @@ void for_each_server(PgPool *pool, void (*func)(PgSocket *sk)); void create_auth_cache(void); +void reuse_just_freed_objects(void); + diff --git a/src/sbuf.c b/src/sbuf.c index a3a636f..223005c 100644 --- a/src/sbuf.c +++ b/src/sbuf.c @@ -246,6 +246,10 @@ static void sbuf_send_cb(int sock, short flags, void *arg) { SBuf *sbuf = arg; + /* sbuf was closed before in this loop */ + if (!sbuf->sock) + return; + /* prepare normal situation for sbuf_recv_cb() */ sbuf->wait_send = 0; sbuf_wait_for_data(sbuf); @@ -272,6 +276,8 @@ static bool sbuf_send_pending(SBuf *sbuf) int res, avail; uint8 *pos; + Assert(sbuf->dst || !sbuf->send_remain); + try_more: /* how much data is available for sending */ avail = sbuf->recv_pos - sbuf->send_pos; @@ -280,6 +286,11 @@ try_more: if (avail == 0) return true; + if (sbuf->dst->sock == 0) { + log_error("sbuf_send_pending: no dst sock?"); + return false; + } + /* actually send it */ pos = sbuf->buf + sbuf->send_pos; res = safe_send(sbuf->dst->sock, pos, avail, 0); @@ -421,6 +432,10 @@ static void sbuf_recv_cb(int sock, short flags, void *arg) int free, ok; SBuf *sbuf = arg; + /* sbuf was closed before in this loop */ + if (!sbuf->sock) + return; + /* reading should be disabled when waiting */ Assert(sbuf->wait_send == 0); @@ -442,16 +457,18 @@ try_more: /* now handle it */ ok = sbuf_process_pending(sbuf); + if (!ok) + return; /* if the buffer is full, there can be more data available */ - if (ok && sbuf->recv_pos == cf_sbuf_len) + if (sbuf->recv_pos == cf_sbuf_len) goto try_more; /* clean buffer */ sbuf_try_resync(sbuf); /* notify proto that all is sent */ - if (sbuf->send_pos == sbuf->recv_pos && sbuf->pkt_remain == 0) + if (sbuf_has_no_state(sbuf)) sbuf_call_proto(sbuf, SBUF_EV_FLUSH); } diff --git a/src/server.c b/src/server.c index 9c6c28d..9f4ee34 100644 --- a/src/server.c +++ b/src/server.c @@ -230,6 +230,12 @@ bool server_proto(SBuf *sbuf, SBufEvent evtype, MBuf *pkt, void *arg) Assert(is_server_socket(server)); Assert(server->state != SV_FREE); + if (server->state == SV_JUSTFREE) { + /* SBuf should catch the case */ + slog_warning(server, "state=SV_JUSTFREE, should not happen"); + return false; + } + switch (evtype) { case SBUF_EV_RECV_FAILED: disconnect_server(server, false, "server conn crashed?");