Thread: connect_timeout parameter in libpq
Hi there! I need to change current connectDBComplete() behavior ( hang if backend are not responding). Here is the patch. Please apply. *** fe-connect.c.orig Чт Авг 8 12:22:46 2002 --- fe-connect.c Пт Авг 9 14:57:18 2002 *************** *** 114,119 **** --- 114,122 ---- {"password", "PGPASSWORD", DefaultPassword, NULL, "Database-Password", "*", 20}, + {"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL, + "Connect-timeout", "", 10}, /* strlen( INT32_MAX) == 10 */ + {"dbname", "PGDATABASE", NULL, NULL, "Database-Name", "", 20}, *************** *** 307,312 **** --- 310,317 ---- conn->pguser = tmp ? strdup(tmp) : NULL; tmp = conninfo_getval(connOptions, "password"); conn->pgpass = tmp ? strdup(tmp) : NULL; + tmp = conninfo_getval(connOptions, "connect_timeout"); + conn->connect_timeout = tmp ? strdup(tmp) : NULL; #ifdef USE_SSL tmp = conninfo_getval(connOptions, "requiressl"); conn->require_ssl = tmp ? (tmp[0] == '1' ? true : false) : false; *************** *** 1050,1060 **** { PostgresPollingStatusType flag = PGRES_POLLING_WRITING; if (conn == NULL || conn->status == CONNECTION_BAD) return 0; ! for (;;) { /* * Wait, if necessary. Note that the initial state (just after * PQconnectStart) is to wait for the socket to select for --- 1055,1086 ---- { PostgresPollingStatusType flag = PGRES_POLLING_WRITING; + time_t finish_time, start_time; + struct timeval remains, *rp = NULL; + if (conn == NULL || conn->status == CONNECTION_BAD) return 0; ! if (conn->connect_timeout != NULL) { + remains.tv_sec = atoi(conn->connect_timeout); + if (!remains.tv_sec) + { + conn->status = CONNECTION_BAD; + return 0; + } + remains.tv_usec = 0; + rp = &remains; + } + + + while (rp == NULL || remains.tv_sec > 0) + { + if ((time_t)-1 == (start_time = time(NULL))) + { + conn->status = CONNECTION_BAD; + return 0; + } /* * Wait, if necessary. Note that the initial state (just after * PQconnectStart) is to wait for the socket to select for *************** *** 1069,1075 **** return 1; /* success! */ case PGRES_POLLING_READING: ! if (pqWait(1, 0, conn)) { conn->status = CONNECTION_BAD; return 0; --- 1095,1101 ---- return 1; /* success! */ case PGRES_POLLING_READING: ! if (pqWaitTimed(1, 0, conn, rp)) { conn->status = CONNECTION_BAD; return 0; *************** *** 1077,1083 **** break; case PGRES_POLLING_WRITING: ! if (pqWait(0, 1, conn)) { conn->status = CONNECTION_BAD; return 0; --- 1103,1109 ---- break; case PGRES_POLLING_WRITING: ! if (pqWaitTimed(0, 1, conn, rp)) { conn->status = CONNECTION_BAD; return 0; *************** *** 1094,1100 **** --- 1120,1135 ---- * Now try to advance the state machine. */ flag = PQconnectPoll(conn); + + if ((time_t)-1 == (finish_time = time(NULL))) + { + conn->status = CONNECTION_BAD; + return 0; + } + remains.tv_sec -= finish_time - start_time; } + conn->status = CONNECTION_BAD; + return 0; } /* ---------------- *************** *** 1929,1934 **** --- 1964,1971 ---- free(conn->pguser); if (conn->pgpass) free(conn->pgpass); + if (conn->connect_timeout) + free(conn->connect_timeout); /* Note that conn->Pfdebug is not ours to close or free */ if (conn->notifyList) DLFreeList(conn->notifyList); *** fe-misc.c.orig Чт Авг 8 16:33:34 2002 --- fe-misc.c Чт Авг 8 16:33:45 2002 *************** *** 748,753 **** --- 748,759 ---- int pqWait(int forRead, int forWrite, PGconn *conn) { + return pqWaitTimed( forRead, forWrite, conn, (struct timeval *) NULL); + } + + int + pqWaitTimed(int forRead, int forWrite, PGconn *conn, struct timeval *timeout) + { fd_set input_mask; fd_set output_mask; fd_set except_mask; *************** *** 770,777 **** if (forWrite) FD_SET(conn->sock, &output_mask); FD_SET(conn->sock, &except_mask); ! if (select(conn->sock + 1, &input_mask, &output_mask, &except_mask, ! (struct timeval *) NULL) < 0) { if (SOCK_ERRNO == EINTR) goto retry; --- 776,783 ---- if (forWrite) FD_SET(conn->sock, &output_mask); FD_SET(conn->sock, &except_mask); ! if (select(conn->sock + 1, &input_mask, &output_mask, ! &except_mask, timeout) < 0) { if (SOCK_ERRNO == EINTR) goto retry; *** libpq-int.h.orig Чт Авг 8 16:37:56 2002 --- libpq-int.h Чт Авг 8 16:37:37 2002 *************** *** 279,284 **** --- 279,286 ---- PQExpBufferData workBuffer; /* expansible string */ int client_encoding; /* encoding id */ + + char *connect_timeout; }; /* String descriptions of the ExecStatusTypes. *************** *** 324,329 **** --- 326,332 ---- extern int pqReadData(PGconn *conn); extern int pqFlush(PGconn *conn); extern int pqWait(int forRead, int forWrite, PGconn *conn); + extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn, struct timeval* timeout); extern int pqReadReady(PGconn *conn); extern int pqWriteReady(PGconn *conn); -- Regards Denis
Denis A Ustimenko <denis@oldham.ru> writes: > I need to change current connectDBComplete() behavior ( hang if > backend are not responding). Here is the patch. Please apply. Shouldn't such a patch include documentation updates? (And not only user-level documentation; this patch adds not even a single comment to explain what it's doing or why.) I'm also not thrilled with the way that the patch imposes the overhead of calculating the timeout whether the user wants it or not. The time() kernel calls should be skipped unless needed. A final comment is that the patch's timeout accuracy is quite poor, since time()'s result is quantized to seconds. gettimeofday() might be a better choice. Also it seems to assume that select() does not modify its timeout argument, which is not a portable assumption. On some platforms the timeout struct is decremented by the elapsed time. regards, tom lane
New version of patch. Documentation updates, accurate remaining time calculation etc. On Mon, Aug 12, 2002 at 11:35:17PM -0400, Tom Lane wrote: > Shouldn't such a patch include documentation updates? (And not > only user-level documentation; this patch adds not even a single > comment to explain what it's doing or why.) > > I'm also not thrilled with the way that the patch imposes the > overhead of calculating the timeout whether the user wants it or not. > The time() kernel calls should be skipped unless needed. > > A final comment is that the patch's timeout accuracy is quite poor, since > time()'s result is quantized to seconds. gettimeofday() might be a > better choice. Also it seems to assume that select() does not modify its > timeout argument, which is not a portable assumption. On some platforms > the timeout struct is decremented by the elapsed time. > > regards, tom lane *** postgresql-7.2.1/doc/src/sgml/libpq.sgml.orig Вт Авг 13 12:34:51 2002 --- postgresql-7.2.1/doc/src/sgml/libpq.sgml Вт Авг 13 14:18:15 2002 *************** *** 172,177 **** --- 172,186 ---- </varlistentry> <varlistentry> + <term><literal>connect_timeout</literal></term> + <listitem> + <para> + Time space in seconds given to connect routine. Zero or not set means infinite. + </para> + </listitem> + </varlistentry> + + <varlistentry> <term><literal>options</literal></term> <listitem> <para> *** postgresql-7.2.1/src/interfaces/libpq/fe-connect.c.orig Чт Авг 8 12:22:46 2002 --- postgresql-7.2.1/src/interfaces/libpq/fe-connect.c Вт Авг 13 15:50:43 2002 *************** *** 114,119 **** --- 114,122 ---- {"password", "PGPASSWORD", DefaultPassword, NULL, "Database-Password", "*", 20}, + {"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL, + "Connect-timeout", "", 10}, /* strlen( INT32_MAX) == 10 */ + {"dbname", "PGDATABASE", NULL, NULL, "Database-Name", "", 20}, *************** *** 307,312 **** --- 310,317 ---- conn->pguser = tmp ? strdup(tmp) : NULL; tmp = conninfo_getval(connOptions, "password"); conn->pgpass = tmp ? strdup(tmp) : NULL; + tmp = conninfo_getval(connOptions, "connect_timeout"); + conn->connect_timeout = tmp ? strdup(tmp) : NULL; #ifdef USE_SSL tmp = conninfo_getval(connOptions, "requiressl"); conn->require_ssl = tmp ? (tmp[0] == '1' ? true : false) : false; *************** *** 1050,1061 **** { PostgresPollingStatusType flag = PGRES_POLLING_WRITING; if (conn == NULL || conn->status == CONNECTION_BAD) return 0; ! for (;;) { /* * Wait, if necessary. Note that the initial state (just after * PQconnectStart) is to wait for the socket to select for * writing. --- 1055,1093 ---- { PostgresPollingStatusType flag = PGRES_POLLING_WRITING; + struct timeval remains, *rp = NULL, finish_time, start_time; + if (conn == NULL || conn->status == CONNECTION_BAD) return 0; ! /* ! * Prepare to time calculations, if connect_timeout isn't zero. ! */ ! if (conn->connect_timeout != NULL) { + remains.tv_sec = atoi(conn->connect_timeout); + if (!remains.tv_sec) + { + conn->status = CONNECTION_BAD; + return 0; + } + remains.tv_usec = 0; + rp = &remains; + } + + + while (NULL == rp || remains.tv_sec > 0 || remains.tv_sec == 0 && remains.tv_usec > 0) + { /* + * If connecting timeout is set, get current time. + */ + if ( NULL != rp && -1 == gettimeofday(&start_time, NULL)) + { + conn->status = CONNECTION_BAD; + return 0; + } + + /* * Wait, if necessary. Note that the initial state (just after * PQconnectStart) is to wait for the socket to select for * writing. *************** *** 1069,1075 **** return 1; /* success! */ case PGRES_POLLING_READING: ! if (pqWait(1, 0, conn)) { conn->status = CONNECTION_BAD; return 0; --- 1101,1107 ---- return 1; /* success! */ case PGRES_POLLING_READING: ! if (pqWaitTimed(1, 0, conn, rp)) { conn->status = CONNECTION_BAD; return 0; *************** *** 1077,1083 **** break; case PGRES_POLLING_WRITING: ! if (pqWait(0, 1, conn)) { conn->status = CONNECTION_BAD; return 0; --- 1109,1115 ---- break; case PGRES_POLLING_WRITING: ! if (pqWaitTimed(0, 1, conn, rp)) { conn->status = CONNECTION_BAD; return 0; *************** *** 1094,1100 **** --- 1126,1156 ---- * Now try to advance the state machine. */ flag = PQconnectPoll(conn); + + /* + * If connecting timeout is set, calculate remain time. + */ + if (NULL != rp) { + if (-1 == gettimeofday(&finish_time, NULL)) + { + conn->status = CONNECTION_BAD; + return 0; + } + if (0 > (finish_time.tv_usec -= start_time.tv_usec)) + { + remains.tv_sec++; + finish_time.tv_usec += 1000000; + } + if (0 > (remains.tv_usec -= finish_time.tv_usec)) + { + remains.tv_sec--; + remains.tv_usec += 1000000; + } + remains.tv_sec -= finish_time.tv_sec - start_time.tv_sec; + } } + conn->status = CONNECTION_BAD; + return 0; } /* ---------------- *************** *** 1929,1934 **** --- 1985,1992 ---- free(conn->pguser); if (conn->pgpass) free(conn->pgpass); + if (conn->connect_timeout) + free(conn->connect_timeout); /* Note that conn->Pfdebug is not ours to close or free */ if (conn->notifyList) DLFreeList(conn->notifyList); *** postgresql-7.2.1/src/interfaces/libpq/fe-misc.c.orig Чт Авг 8 16:33:34 2002 --- postgresql-7.2.1/src/interfaces/libpq/fe-misc.c Вт Авг 13 16:16:57 2002 *************** *** 748,757 **** --- 748,766 ---- int pqWait(int forRead, int forWrite, PGconn *conn) { + return pqWaitTimed( forRead, forWrite, conn, (const struct timeval *) NULL); + } + + int + pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval *timeout) + { fd_set input_mask; fd_set output_mask; fd_set except_mask; + struct timeval tmp_timeout; + struct timeval *ptmp_timeout = NULL; + if (conn->sock < 0) { printfPQExpBuffer(&conn->errorMessage, *************** *** 770,778 **** if (forWrite) FD_SET(conn->sock, &output_mask); FD_SET(conn->sock, &except_mask); ! if (select(conn->sock + 1, &input_mask, &output_mask, &except_mask, ! (struct timeval *) NULL) < 0) { if (SOCK_ERRNO == EINTR) goto retry; printfPQExpBuffer(&conn->errorMessage, --- 779,796 ---- if (forWrite) FD_SET(conn->sock, &output_mask); FD_SET(conn->sock, &except_mask); ! ! if (NULL != timeout) { + /* + * select may modify timeout argument on some platforms use copy + */ + tmp_timeout = *timeout; + ptmp_timeout = &tmp_timeout; + } + if (select(conn->sock + 1, &input_mask, &output_mask, + &except_mask, ptmp_timeout) < 0) + { if (SOCK_ERRNO == EINTR) goto retry; printfPQExpBuffer(&conn->errorMessage, *** postgresql-7.2.1/src/interfaces/libpq/libpq-int.h.orig Чт Авг 8 16:37:56 2002 --- postgresql-7.2.1/src/interfaces/libpq/libpq-int.h Вт Авг 13 14:37:54 2002 *************** *** 279,284 **** --- 279,286 ---- PQExpBufferData workBuffer; /* expansible string */ int client_encoding; /* encoding id */ + + char *connect_timeout; }; /* String descriptions of the ExecStatusTypes. *************** *** 324,329 **** --- 326,332 ---- extern int pqReadData(PGconn *conn); extern int pqFlush(PGconn *conn); extern int pqWait(int forRead, int forWrite, PGconn *conn); + extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval* timeout); extern int pqReadReady(PGconn *conn); extern int pqWriteReady(PGconn *conn); -- Regards Denis
Hi, there! I have posted patch two days ago. It has accurate timeout calculation, use gettimeofday(), includes documentation and so on accordind Tom's demand. Unfortunately I have recieved no comments. Why? Regards Denis On Mon, 12 Aug 2002, Tom Lane wrote: > Denis A Ustimenko <denis@oldham.ru> writes: > > I need to change current connectDBComplete() behavior ( hang if > > backend are not responding). Here is the patch. Please apply. > > Shouldn't such a patch include documentation updates? (And not > only user-level documentation; this patch adds not even a single > comment to explain what it's doing or why.) > > I'm also not thrilled with the way that the patch imposes the > overhead of calculating the timeout whether the user wants it or not. > The time() kernel calls should be skipped unless needed. > > A final comment is that the patch's timeout accuracy is quite poor, since > time()'s result is quantized to seconds. gettimeofday() might be a > better choice. Also it seems to assume that select() does not modify its > timeout argument, which is not a portable assumption. On some platforms > the timeout struct is decremented by the elapsed time. > > regards, tom lane
Uh, I wait a day or two to see if anyone comments, then I add it to the patch queue. I will do that now. --------------------------------------------------------------------------- Denis A Ustimenko wrote: > Hi, there! > > I have posted patch two days ago. It has accurate timeout > calculation, use gettimeofday(), includes documentation and so on > accordind Tom's demand. Unfortunately I have recieved no comments. > Why? > > Regards > Denis > > On Mon, 12 Aug 2002, Tom Lane wrote: > > > Denis A Ustimenko <denis@oldham.ru> writes: > > > I need to change current connectDBComplete() behavior ( hang if > > > backend are not responding). Here is the patch. Please apply. > > > > Shouldn't such a patch include documentation updates? (And not > > only user-level documentation; this patch adds not even a single > > comment to explain what it's doing or why.) > > > > I'm also not thrilled with the way that the patch imposes the > > overhead of calculating the timeout whether the user wants it or not. > > The time() kernel calls should be skipped unless needed. > > > > A final comment is that the patch's timeout accuracy is quite poor, since > > time()'s result is quantized to seconds. gettimeofday() might be a > > better choice. Also it seems to assume that select() does not modify its > > timeout argument, which is not a portable assumption. On some platforms > > the timeout struct is decremented by the elapsed time. > > > > regards, tom lane > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Your patch has been added to the PostgreSQL unapplied patches list at: http://candle.pha.pa.us/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --------------------------------------------------------------------------- Denis A Ustimenko wrote: > New version of patch. Documentation updates, accurate remaining time calculation etc. > > On Mon, Aug 12, 2002 at 11:35:17PM -0400, Tom Lane wrote: > > Shouldn't such a patch include documentation updates? (And not > > only user-level documentation; this patch adds not even a single > > comment to explain what it's doing or why.) > > > > I'm also not thrilled with the way that the patch imposes the > > overhead of calculating the timeout whether the user wants it or not. > > The time() kernel calls should be skipped unless needed. > > > > A final comment is that the patch's timeout accuracy is quite poor, since > > time()'s result is quantized to seconds. gettimeofday() might be a > > better choice. Also it seems to assume that select() does not modify its > > timeout argument, which is not a portable assumption. On some platforms > > the timeout struct is decremented by the elapsed time. > > > > regards, tom lane > > *** postgresql-7.2.1/doc/src/sgml/libpq.sgml.orig ?? ??? 13 12:34:51 2002 > --- postgresql-7.2.1/doc/src/sgml/libpq.sgml ?? ??? 13 14:18:15 2002 > *************** > *** 172,177 **** > --- 172,186 ---- > </varlistentry> > > <varlistentry> > + <term><literal>connect_timeout</literal></term> > + <listitem> > + <para> > + Time space in seconds given to connect routine. Zero or not set means infinite. > + </para> > + </listitem> > + </varlistentry> > + > + <varlistentry> > <term><literal>options</literal></term> > <listitem> > <para> > *** postgresql-7.2.1/src/interfaces/libpq/fe-connect.c.orig ?? ??? 8 12:22:46 2002 > --- postgresql-7.2.1/src/interfaces/libpq/fe-connect.c ?? ??? 13 15:50:43 2002 > *************** > *** 114,119 **** > --- 114,122 ---- > {"password", "PGPASSWORD", DefaultPassword, NULL, > "Database-Password", "*", 20}, > > + {"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL, > + "Connect-timeout", "", 10}, /* strlen( INT32_MAX) == 10 */ > + > {"dbname", "PGDATABASE", NULL, NULL, > "Database-Name", "", 20}, > > *************** > *** 307,312 **** > --- 310,317 ---- > conn->pguser = tmp ? strdup(tmp) : NULL; > tmp = conninfo_getval(connOptions, "password"); > conn->pgpass = tmp ? strdup(tmp) : NULL; > + tmp = conninfo_getval(connOptions, "connect_timeout"); > + conn->connect_timeout = tmp ? strdup(tmp) : NULL; > #ifdef USE_SSL > tmp = conninfo_getval(connOptions, "requiressl"); > conn->require_ssl = tmp ? (tmp[0] == '1' ? true : false) : false; > *************** > *** 1050,1061 **** > { > PostgresPollingStatusType flag = PGRES_POLLING_WRITING; > > if (conn == NULL || conn->status == CONNECTION_BAD) > return 0; > > ! for (;;) > { > /* > * Wait, if necessary. Note that the initial state (just after > * PQconnectStart) is to wait for the socket to select for > * writing. > --- 1055,1093 ---- > { > PostgresPollingStatusType flag = PGRES_POLLING_WRITING; > > + struct timeval remains, *rp = NULL, finish_time, start_time; > + > if (conn == NULL || conn->status == CONNECTION_BAD) > return 0; > > ! /* > ! * Prepare to time calculations, if connect_timeout isn't zero. > ! */ > ! if (conn->connect_timeout != NULL) > { > + remains.tv_sec = atoi(conn->connect_timeout); > + if (!remains.tv_sec) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + remains.tv_usec = 0; > + rp = &remains; > + } > + > + > + while (NULL == rp || remains.tv_sec > 0 || remains.tv_sec == 0 && remains.tv_usec > 0) > + { > /* > + * If connecting timeout is set, get current time. > + */ > + if ( NULL != rp && -1 == gettimeofday(&start_time, NULL)) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + > + /* > * Wait, if necessary. Note that the initial state (just after > * PQconnectStart) is to wait for the socket to select for > * writing. > *************** > *** 1069,1075 **** > return 1; /* success! */ > > case PGRES_POLLING_READING: > ! if (pqWait(1, 0, conn)) > { > conn->status = CONNECTION_BAD; > return 0; > --- 1101,1107 ---- > return 1; /* success! */ > > case PGRES_POLLING_READING: > ! if (pqWaitTimed(1, 0, conn, rp)) > { > conn->status = CONNECTION_BAD; > return 0; > *************** > *** 1077,1083 **** > break; > > case PGRES_POLLING_WRITING: > ! if (pqWait(0, 1, conn)) > { > conn->status = CONNECTION_BAD; > return 0; > --- 1109,1115 ---- > break; > > case PGRES_POLLING_WRITING: > ! if (pqWaitTimed(0, 1, conn, rp)) > { > conn->status = CONNECTION_BAD; > return 0; > *************** > *** 1094,1100 **** > --- 1126,1156 ---- > * Now try to advance the state machine. > */ > flag = PQconnectPoll(conn); > + > + /* > + * If connecting timeout is set, calculate remain time. > + */ > + if (NULL != rp) { > + if (-1 == gettimeofday(&finish_time, NULL)) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + if (0 > (finish_time.tv_usec -= start_time.tv_usec)) > + { > + remains.tv_sec++; > + finish_time.tv_usec += 1000000; > + } > + if (0 > (remains.tv_usec -= finish_time.tv_usec)) > + { > + remains.tv_sec--; > + remains.tv_usec += 1000000; > + } > + remains.tv_sec -= finish_time.tv_sec - start_time.tv_sec; > + } > } > + conn->status = CONNECTION_BAD; > + return 0; > } > > /* ---------------- > *************** > *** 1929,1934 **** > --- 1985,1992 ---- > free(conn->pguser); > if (conn->pgpass) > free(conn->pgpass); > + if (conn->connect_timeout) > + free(conn->connect_timeout); > /* Note that conn->Pfdebug is not ours to close or free */ > if (conn->notifyList) > DLFreeList(conn->notifyList); > *** postgresql-7.2.1/src/interfaces/libpq/fe-misc.c.orig ?? ??? 8 16:33:34 2002 > --- postgresql-7.2.1/src/interfaces/libpq/fe-misc.c ?? ??? 13 16:16:57 2002 > *************** > *** 748,757 **** > --- 748,766 ---- > int > pqWait(int forRead, int forWrite, PGconn *conn) > { > + return pqWaitTimed( forRead, forWrite, conn, (const struct timeval *) NULL); > + } > + > + int > + pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval *timeout) > + { > fd_set input_mask; > fd_set output_mask; > fd_set except_mask; > > + struct timeval tmp_timeout; > + struct timeval *ptmp_timeout = NULL; > + > if (conn->sock < 0) > { > printfPQExpBuffer(&conn->errorMessage, > *************** > *** 770,778 **** > if (forWrite) > FD_SET(conn->sock, &output_mask); > FD_SET(conn->sock, &except_mask); > ! if (select(conn->sock + 1, &input_mask, &output_mask, &except_mask, > ! (struct timeval *) NULL) < 0) > { > if (SOCK_ERRNO == EINTR) > goto retry; > printfPQExpBuffer(&conn->errorMessage, > --- 779,796 ---- > if (forWrite) > FD_SET(conn->sock, &output_mask); > FD_SET(conn->sock, &except_mask); > ! > ! if (NULL != timeout) > { > + /* > + * select may modify timeout argument on some platforms use copy > + */ > + tmp_timeout = *timeout; > + ptmp_timeout = &tmp_timeout; > + } > + if (select(conn->sock + 1, &input_mask, &output_mask, > + &except_mask, ptmp_timeout) < 0) > + { > if (SOCK_ERRNO == EINTR) > goto retry; > printfPQExpBuffer(&conn->errorMessage, > *** postgresql-7.2.1/src/interfaces/libpq/libpq-int.h.orig ?? ??? 8 16:37:56 2002 > --- postgresql-7.2.1/src/interfaces/libpq/libpq-int.h ?? ??? 13 14:37:54 2002 > *************** > *** 279,284 **** > --- 279,286 ---- > PQExpBufferData workBuffer; /* expansible string */ > > int client_encoding; /* encoding id */ > + > + char *connect_timeout; > }; > > /* String descriptions of the ExecStatusTypes. > *************** > *** 324,329 **** > --- 326,332 ---- > extern int pqReadData(PGconn *conn); > extern int pqFlush(PGconn *conn); > extern int pqWait(int forRead, int forWrite, PGconn *conn); > + extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval* timeout); > extern int pqReadReady(PGconn *conn); > extern int pqWriteReady(PGconn *conn); > > > > -- > Regards > Denis > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Do people think this is of general enough interest to be added to libpq? --------------------------------------------------------------------------- Denis A Ustimenko wrote: > New version of patch. Documentation updates, accurate remaining time calculation etc. > > On Mon, Aug 12, 2002 at 11:35:17PM -0400, Tom Lane wrote: > > Shouldn't such a patch include documentation updates? (And not > > only user-level documentation; this patch adds not even a single > > comment to explain what it's doing or why.) > > > > I'm also not thrilled with the way that the patch imposes the > > overhead of calculating the timeout whether the user wants it or not. > > The time() kernel calls should be skipped unless needed. > > > > A final comment is that the patch's timeout accuracy is quite poor, since > > time()'s result is quantized to seconds. gettimeofday() might be a > > better choice. Also it seems to assume that select() does not modify its > > timeout argument, which is not a portable assumption. On some platforms > > the timeout struct is decremented by the elapsed time. > > > > regards, tom lane > > *** postgresql-7.2.1/doc/src/sgml/libpq.sgml.orig ?? ??? 13 12:34:51 2002 > --- postgresql-7.2.1/doc/src/sgml/libpq.sgml ?? ??? 13 14:18:15 2002 > *************** > *** 172,177 **** > --- 172,186 ---- > </varlistentry> > > <varlistentry> > + <term><literal>connect_timeout</literal></term> > + <listitem> > + <para> > + Time space in seconds given to connect routine. Zero or not set means infinite. > + </para> > + </listitem> > + </varlistentry> > + > + <varlistentry> > <term><literal>options</literal></term> > <listitem> > <para> > *** postgresql-7.2.1/src/interfaces/libpq/fe-connect.c.orig ?? ??? 8 12:22:46 2002 > --- postgresql-7.2.1/src/interfaces/libpq/fe-connect.c ?? ??? 13 15:50:43 2002 > *************** > *** 114,119 **** > --- 114,122 ---- > {"password", "PGPASSWORD", DefaultPassword, NULL, > "Database-Password", "*", 20}, > > + {"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL, > + "Connect-timeout", "", 10}, /* strlen( INT32_MAX) == 10 */ > + > {"dbname", "PGDATABASE", NULL, NULL, > "Database-Name", "", 20}, > > *************** > *** 307,312 **** > --- 310,317 ---- > conn->pguser = tmp ? strdup(tmp) : NULL; > tmp = conninfo_getval(connOptions, "password"); > conn->pgpass = tmp ? strdup(tmp) : NULL; > + tmp = conninfo_getval(connOptions, "connect_timeout"); > + conn->connect_timeout = tmp ? strdup(tmp) : NULL; > #ifdef USE_SSL > tmp = conninfo_getval(connOptions, "requiressl"); > conn->require_ssl = tmp ? (tmp[0] == '1' ? true : false) : false; > *************** > *** 1050,1061 **** > { > PostgresPollingStatusType flag = PGRES_POLLING_WRITING; > > if (conn == NULL || conn->status == CONNECTION_BAD) > return 0; > > ! for (;;) > { > /* > * Wait, if necessary. Note that the initial state (just after > * PQconnectStart) is to wait for the socket to select for > * writing. > --- 1055,1093 ---- > { > PostgresPollingStatusType flag = PGRES_POLLING_WRITING; > > + struct timeval remains, *rp = NULL, finish_time, start_time; > + > if (conn == NULL || conn->status == CONNECTION_BAD) > return 0; > > ! /* > ! * Prepare to time calculations, if connect_timeout isn't zero. > ! */ > ! if (conn->connect_timeout != NULL) > { > + remains.tv_sec = atoi(conn->connect_timeout); > + if (!remains.tv_sec) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + remains.tv_usec = 0; > + rp = &remains; > + } > + > + > + while (NULL == rp || remains.tv_sec > 0 || remains.tv_sec == 0 && remains.tv_usec > 0) > + { > /* > + * If connecting timeout is set, get current time. > + */ > + if ( NULL != rp && -1 == gettimeofday(&start_time, NULL)) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + > + /* > * Wait, if necessary. Note that the initial state (just after > * PQconnectStart) is to wait for the socket to select for > * writing. > *************** > *** 1069,1075 **** > return 1; /* success! */ > > case PGRES_POLLING_READING: > ! if (pqWait(1, 0, conn)) > { > conn->status = CONNECTION_BAD; > return 0; > --- 1101,1107 ---- > return 1; /* success! */ > > case PGRES_POLLING_READING: > ! if (pqWaitTimed(1, 0, conn, rp)) > { > conn->status = CONNECTION_BAD; > return 0; > *************** > *** 1077,1083 **** > break; > > case PGRES_POLLING_WRITING: > ! if (pqWait(0, 1, conn)) > { > conn->status = CONNECTION_BAD; > return 0; > --- 1109,1115 ---- > break; > > case PGRES_POLLING_WRITING: > ! if (pqWaitTimed(0, 1, conn, rp)) > { > conn->status = CONNECTION_BAD; > return 0; > *************** > *** 1094,1100 **** > --- 1126,1156 ---- > * Now try to advance the state machine. > */ > flag = PQconnectPoll(conn); > + > + /* > + * If connecting timeout is set, calculate remain time. > + */ > + if (NULL != rp) { > + if (-1 == gettimeofday(&finish_time, NULL)) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + if (0 > (finish_time.tv_usec -= start_time.tv_usec)) > + { > + remains.tv_sec++; > + finish_time.tv_usec += 1000000; > + } > + if (0 > (remains.tv_usec -= finish_time.tv_usec)) > + { > + remains.tv_sec--; > + remains.tv_usec += 1000000; > + } > + remains.tv_sec -= finish_time.tv_sec - start_time.tv_sec; > + } > } > + conn->status = CONNECTION_BAD; > + return 0; > } > > /* ---------------- > *************** > *** 1929,1934 **** > --- 1985,1992 ---- > free(conn->pguser); > if (conn->pgpass) > free(conn->pgpass); > + if (conn->connect_timeout) > + free(conn->connect_timeout); > /* Note that conn->Pfdebug is not ours to close or free */ > if (conn->notifyList) > DLFreeList(conn->notifyList); > *** postgresql-7.2.1/src/interfaces/libpq/fe-misc.c.orig ?? ??? 8 16:33:34 2002 > --- postgresql-7.2.1/src/interfaces/libpq/fe-misc.c ?? ??? 13 16:16:57 2002 > *************** > *** 748,757 **** > --- 748,766 ---- > int > pqWait(int forRead, int forWrite, PGconn *conn) > { > + return pqWaitTimed( forRead, forWrite, conn, (const struct timeval *) NULL); > + } > + > + int > + pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval *timeout) > + { > fd_set input_mask; > fd_set output_mask; > fd_set except_mask; > > + struct timeval tmp_timeout; > + struct timeval *ptmp_timeout = NULL; > + > if (conn->sock < 0) > { > printfPQExpBuffer(&conn->errorMessage, > *************** > *** 770,778 **** > if (forWrite) > FD_SET(conn->sock, &output_mask); > FD_SET(conn->sock, &except_mask); > ! if (select(conn->sock + 1, &input_mask, &output_mask, &except_mask, > ! (struct timeval *) NULL) < 0) > { > if (SOCK_ERRNO == EINTR) > goto retry; > printfPQExpBuffer(&conn->errorMessage, > --- 779,796 ---- > if (forWrite) > FD_SET(conn->sock, &output_mask); > FD_SET(conn->sock, &except_mask); > ! > ! if (NULL != timeout) > { > + /* > + * select may modify timeout argument on some platforms use copy > + */ > + tmp_timeout = *timeout; > + ptmp_timeout = &tmp_timeout; > + } > + if (select(conn->sock + 1, &input_mask, &output_mask, > + &except_mask, ptmp_timeout) < 0) > + { > if (SOCK_ERRNO == EINTR) > goto retry; > printfPQExpBuffer(&conn->errorMessage, > *** postgresql-7.2.1/src/interfaces/libpq/libpq-int.h.orig ?? ??? 8 16:37:56 2002 > --- postgresql-7.2.1/src/interfaces/libpq/libpq-int.h ?? ??? 13 14:37:54 2002 > *************** > *** 279,284 **** > --- 279,286 ---- > PQExpBufferData workBuffer; /* expansible string */ > > int client_encoding; /* encoding id */ > + > + char *connect_timeout; > }; > > /* String descriptions of the ExecStatusTypes. > *************** > *** 324,329 **** > --- 326,332 ---- > extern int pqReadData(PGconn *conn); > extern int pqFlush(PGconn *conn); > extern int pqWait(int forRead, int forWrite, PGconn *conn); > + extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval* timeout); > extern int pqReadReady(PGconn *conn); > extern int pqWriteReady(PGconn *conn); > > > > -- > Regards > Denis > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes: > Do people think this is of general enough interest to be added to libpq? I had objections to that version, but didn't Denis submit a newer one? The feature per se seems reasonable enough. regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Do people think this is of general enough interest to be added to libpq? > > I had objections to that version, but didn't Denis submit a newer one? I have his new version in the patches archive: New version of patch. Documentation updates, accurate remaining time calculation etc. On Mon, Aug 12, 2002 at 11:35:17PM -0400, Tom Lane wrote: > Shouldn't such a patch include documentation updates? (And not > only user-level documentation; this patch adds not even a single > comment to explain what it's doing or why.) > The feature per se seems reasonable enough. OK. That's what I needed to know. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Do people think this is of general enough interest to be added to libpq? > > I had objections to that version, but didn't Denis submit a newer one? Sorry, I did quote the original patch because that was the one with the description. The new patch is in the patches queue. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. --------------------------------------------------------------------------- Denis A Ustimenko wrote: > New version of patch. Documentation updates, accurate remaining time calculation etc. > > On Mon, Aug 12, 2002 at 11:35:17PM -0400, Tom Lane wrote: > > Shouldn't such a patch include documentation updates? (And not > > only user-level documentation; this patch adds not even a single > > comment to explain what it's doing or why.) > > > > I'm also not thrilled with the way that the patch imposes the > > overhead of calculating the timeout whether the user wants it or not. > > The time() kernel calls should be skipped unless needed. > > > > A final comment is that the patch's timeout accuracy is quite poor, since > > time()'s result is quantized to seconds. gettimeofday() might be a > > better choice. Also it seems to assume that select() does not modify its > > timeout argument, which is not a portable assumption. On some platforms > > the timeout struct is decremented by the elapsed time. > > > > regards, tom lane > > *** postgresql-7.2.1/doc/src/sgml/libpq.sgml.orig ?? ??? 13 12:34:51 2002 > --- postgresql-7.2.1/doc/src/sgml/libpq.sgml ?? ??? 13 14:18:15 2002 > *************** > *** 172,177 **** > --- 172,186 ---- > </varlistentry> > > <varlistentry> > + <term><literal>connect_timeout</literal></term> > + <listitem> > + <para> > + Time space in seconds given to connect routine. Zero or not set means infinite. > + </para> > + </listitem> > + </varlistentry> > + > + <varlistentry> > <term><literal>options</literal></term> > <listitem> > <para> > *** postgresql-7.2.1/src/interfaces/libpq/fe-connect.c.orig ?? ??? 8 12:22:46 2002 > --- postgresql-7.2.1/src/interfaces/libpq/fe-connect.c ?? ??? 13 15:50:43 2002 > *************** > *** 114,119 **** > --- 114,122 ---- > {"password", "PGPASSWORD", DefaultPassword, NULL, > "Database-Password", "*", 20}, > > + {"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL, > + "Connect-timeout", "", 10}, /* strlen( INT32_MAX) == 10 */ > + > {"dbname", "PGDATABASE", NULL, NULL, > "Database-Name", "", 20}, > > *************** > *** 307,312 **** > --- 310,317 ---- > conn->pguser = tmp ? strdup(tmp) : NULL; > tmp = conninfo_getval(connOptions, "password"); > conn->pgpass = tmp ? strdup(tmp) : NULL; > + tmp = conninfo_getval(connOptions, "connect_timeout"); > + conn->connect_timeout = tmp ? strdup(tmp) : NULL; > #ifdef USE_SSL > tmp = conninfo_getval(connOptions, "requiressl"); > conn->require_ssl = tmp ? (tmp[0] == '1' ? true : false) : false; > *************** > *** 1050,1061 **** > { > PostgresPollingStatusType flag = PGRES_POLLING_WRITING; > > if (conn == NULL || conn->status == CONNECTION_BAD) > return 0; > > ! for (;;) > { > /* > * Wait, if necessary. Note that the initial state (just after > * PQconnectStart) is to wait for the socket to select for > * writing. > --- 1055,1093 ---- > { > PostgresPollingStatusType flag = PGRES_POLLING_WRITING; > > + struct timeval remains, *rp = NULL, finish_time, start_time; > + > if (conn == NULL || conn->status == CONNECTION_BAD) > return 0; > > ! /* > ! * Prepare to time calculations, if connect_timeout isn't zero. > ! */ > ! if (conn->connect_timeout != NULL) > { > + remains.tv_sec = atoi(conn->connect_timeout); > + if (!remains.tv_sec) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + remains.tv_usec = 0; > + rp = &remains; > + } > + > + > + while (NULL == rp || remains.tv_sec > 0 || remains.tv_sec == 0 && remains.tv_usec > 0) > + { > /* > + * If connecting timeout is set, get current time. > + */ > + if ( NULL != rp && -1 == gettimeofday(&start_time, NULL)) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + > + /* > * Wait, if necessary. Note that the initial state (just after > * PQconnectStart) is to wait for the socket to select for > * writing. > *************** > *** 1069,1075 **** > return 1; /* success! */ > > case PGRES_POLLING_READING: > ! if (pqWait(1, 0, conn)) > { > conn->status = CONNECTION_BAD; > return 0; > --- 1101,1107 ---- > return 1; /* success! */ > > case PGRES_POLLING_READING: > ! if (pqWaitTimed(1, 0, conn, rp)) > { > conn->status = CONNECTION_BAD; > return 0; > *************** > *** 1077,1083 **** > break; > > case PGRES_POLLING_WRITING: > ! if (pqWait(0, 1, conn)) > { > conn->status = CONNECTION_BAD; > return 0; > --- 1109,1115 ---- > break; > > case PGRES_POLLING_WRITING: > ! if (pqWaitTimed(0, 1, conn, rp)) > { > conn->status = CONNECTION_BAD; > return 0; > *************** > *** 1094,1100 **** > --- 1126,1156 ---- > * Now try to advance the state machine. > */ > flag = PQconnectPoll(conn); > + > + /* > + * If connecting timeout is set, calculate remain time. > + */ > + if (NULL != rp) { > + if (-1 == gettimeofday(&finish_time, NULL)) > + { > + conn->status = CONNECTION_BAD; > + return 0; > + } > + if (0 > (finish_time.tv_usec -= start_time.tv_usec)) > + { > + remains.tv_sec++; > + finish_time.tv_usec += 1000000; > + } > + if (0 > (remains.tv_usec -= finish_time.tv_usec)) > + { > + remains.tv_sec--; > + remains.tv_usec += 1000000; > + } > + remains.tv_sec -= finish_time.tv_sec - start_time.tv_sec; > + } > } > + conn->status = CONNECTION_BAD; > + return 0; > } > > /* ---------------- > *************** > *** 1929,1934 **** > --- 1985,1992 ---- > free(conn->pguser); > if (conn->pgpass) > free(conn->pgpass); > + if (conn->connect_timeout) > + free(conn->connect_timeout); > /* Note that conn->Pfdebug is not ours to close or free */ > if (conn->notifyList) > DLFreeList(conn->notifyList); > *** postgresql-7.2.1/src/interfaces/libpq/fe-misc.c.orig ?? ??? 8 16:33:34 2002 > --- postgresql-7.2.1/src/interfaces/libpq/fe-misc.c ?? ??? 13 16:16:57 2002 > *************** > *** 748,757 **** > --- 748,766 ---- > int > pqWait(int forRead, int forWrite, PGconn *conn) > { > + return pqWaitTimed( forRead, forWrite, conn, (const struct timeval *) NULL); > + } > + > + int > + pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval *timeout) > + { > fd_set input_mask; > fd_set output_mask; > fd_set except_mask; > > + struct timeval tmp_timeout; > + struct timeval *ptmp_timeout = NULL; > + > if (conn->sock < 0) > { > printfPQExpBuffer(&conn->errorMessage, > *************** > *** 770,778 **** > if (forWrite) > FD_SET(conn->sock, &output_mask); > FD_SET(conn->sock, &except_mask); > ! if (select(conn->sock + 1, &input_mask, &output_mask, &except_mask, > ! (struct timeval *) NULL) < 0) > { > if (SOCK_ERRNO == EINTR) > goto retry; > printfPQExpBuffer(&conn->errorMessage, > --- 779,796 ---- > if (forWrite) > FD_SET(conn->sock, &output_mask); > FD_SET(conn->sock, &except_mask); > ! > ! if (NULL != timeout) > { > + /* > + * select may modify timeout argument on some platforms use copy > + */ > + tmp_timeout = *timeout; > + ptmp_timeout = &tmp_timeout; > + } > + if (select(conn->sock + 1, &input_mask, &output_mask, > + &except_mask, ptmp_timeout) < 0) > + { > if (SOCK_ERRNO == EINTR) > goto retry; > printfPQExpBuffer(&conn->errorMessage, > *** postgresql-7.2.1/src/interfaces/libpq/libpq-int.h.orig ?? ??? 8 16:37:56 2002 > --- postgresql-7.2.1/src/interfaces/libpq/libpq-int.h ?? ??? 13 14:37:54 2002 > *************** > *** 279,284 **** > --- 279,286 ---- > PQExpBufferData workBuffer; /* expansible string */ > > int client_encoding; /* encoding id */ > + > + char *connect_timeout; > }; > > /* String descriptions of the ExecStatusTypes. > *************** > *** 324,329 **** > --- 326,332 ---- > extern int pqReadData(PGconn *conn); > extern int pqFlush(PGconn *conn); > extern int pqWait(int forRead, int forWrite, PGconn *conn); > + extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn, const struct timeval* timeout); > extern int pqReadReady(PGconn *conn); > extern int pqWriteReady(PGconn *conn); > > > > -- > Regards > Denis > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073