Re: droped out precise time calculations in src/interfaces/libpq/fe-connect.c - Mailing list pgsql-hackers

Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
> > [ some convincing test cases that timeout=1 is not good ]
>
> >           remains.tv_sec = atoi(conn->connect_timeout);
> > +         if (remains.tv_sec == 1)
> > +             remains.tv_sec += 1;
> >           if (!remains.tv_sec)
> >           {
> >               conn->status = CONNECTION_BAD;
>
> On pure-paranoia grounds, I'd suggest the logic
>
> +        /* force a sane minimum delay */
> +         if (remains.tv_sec < 2)
> +             remains.tv_sec = 2;
>
> whereupon you could remove the failure check just below.

I think we should fail if they set the timeout to zero, rather than
cover it up by setting the delay to two.

Attached is a patch that implements most of what we have discussed:

    use time()
    get rid of timeval where not needed
    allow restart of select() to properly compute remaining time
    add 1 to timeout == 1
    pass finish time to pqWaitTimed

Patch applied.  I am applying it so it is in CVS and everyone can see
it.  I will keep modifying it until everyone likes it.  It is just
easier to do it that way when multiple people are reviewing it.  They
can jump in and make changes too.

--
  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
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.210
diff -c -c -r1.210 fe-connect.c
*** src/interfaces/libpq/fe-connect.c    15 Oct 2002 01:48:25 -0000    1.210
--- src/interfaces/libpq/fe-connect.c    16 Oct 2002 02:48:07 -0000
***************
*** 1052,1061 ****
  {
      PostgresPollingStatusType flag = PGRES_POLLING_WRITING;

!     time_t            finish_time = 0,
!                     current_time;
!     struct timeval    remains,
!                    *rp = NULL;

      if (conn == NULL || conn->status == CONNECTION_BAD)
          return 0;
--- 1052,1058 ----
  {
      PostgresPollingStatusType flag = PGRES_POLLING_WRITING;

!     time_t            finish_time = -1;

      if (conn == NULL || conn->status == CONNECTION_BAD)
          return 0;
***************
*** 1065,1084 ****
       */
      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;    /* We don't use subsecond timing */
!         rp = &remains;
!
          /* calculate the finish time based on start + timeout */
!         finish_time = time((time_t *) NULL) + remains.tv_sec;
      }

!     while (rp == NULL || remains.tv_sec > 0)
      {
          /*
           * Wait, if necessary.    Note that the initial state (just after
--- 1062,1082 ----
       */
      if (conn->connect_timeout != NULL)
      {
!         int timeout = atoi(conn->connect_timeout);
!
!         if (timeout == 0)
          {
              conn->status = CONNECTION_BAD;
              return 0;
          }
!         /* Rounding could cause connection to fail;we need at least 2 secs */
!         if (timeout == 1)
!             timeout++;
          /* calculate the finish time based on start + timeout */
!         finish_time = time(NULL) + timeout;
      }

!     while (finish_time == -1 || time(NULL) >= finish_time)
      {
          /*
           * Wait, if necessary.    Note that the initial state (just after
***************
*** 1094,1100 ****
                  return 1;        /* success! */

              case PGRES_POLLING_READING:
!                 if (pqWaitTimed(1, 0, conn, rp))
                  {
                      conn->status = CONNECTION_BAD;
                      return 0;
--- 1092,1098 ----
                  return 1;        /* success! */

              case PGRES_POLLING_READING:
!                 if (pqWaitTimed(1, 0, conn, finish_time))
                  {
                      conn->status = CONNECTION_BAD;
                      return 0;
***************
*** 1102,1108 ****
                  break;

              case PGRES_POLLING_WRITING:
!                 if (pqWaitTimed(0, 1, conn, rp))
                  {
                      conn->status = CONNECTION_BAD;
                      return 0;
--- 1100,1106 ----
                  break;

              case PGRES_POLLING_WRITING:
!                 if (pqWaitTimed(0, 1, conn, finish_time))
                  {
                      conn->status = CONNECTION_BAD;
                      return 0;
***************
*** 1119,1138 ****
           * Now try to advance the state machine.
           */
          flag = PQconnectPoll(conn);
-
-         /*
-          * If connecting timeout is set, calculate remaining time.
-          */
-         if (rp != NULL)
-         {
-             if (time(¤t_time) == -1)
-             {
-                 conn->status = CONNECTION_BAD;
-                 return 0;
-             }
-
-             remains.tv_sec = finish_time - current_time;
-         }
      }
      conn->status = CONNECTION_BAD;
      return 0;
--- 1117,1122 ----
Index: src/interfaces/libpq/fe-misc.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-misc.c,v
retrieving revision 1.83
diff -c -c -r1.83 fe-misc.c
*** src/interfaces/libpq/fe-misc.c    14 Oct 2002 18:11:17 -0000    1.83
--- src/interfaces/libpq/fe-misc.c    16 Oct 2002 02:48:10 -0000
***************
*** 779,789 ****
  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;
--- 779,789 ----
  int
  pqWait(int forRead, int forWrite, PGconn *conn)
  {
!     return pqWaitTimed(forRead, forWrite, conn, -1);
  }

  int
! pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time)
  {
      fd_set        input_mask;
      fd_set        output_mask;
***************
*** 820,826 ****
              FD_SET(conn->sock, &output_mask);
          FD_SET(conn->sock, &except_mask);

!         if (NULL != timeout)
          {
              /*
               *     select() may modify timeout argument on some platforms so
--- 820,826 ----
              FD_SET(conn->sock, &output_mask);
          FD_SET(conn->sock, &except_mask);

!         if (finish_time != -1)
          {
              /*
               *     select() may modify timeout argument on some platforms so
***************
*** 831,837 ****
               *    incorrect timings when select() is interrupted.
               *    bjm 2002-10-14
               */
!             tmp_timeout = *timeout;
              ptmp_timeout = &tmp_timeout;
          }
          if (select(conn->sock + 1, &input_mask, &output_mask,
--- 831,839 ----
               *    incorrect timings when select() is interrupted.
               *    bjm 2002-10-14
               */
!             if ((tmp_timeout.tv_sec = finish_time - time(NULL)) < 0)
!                 tmp_timeout.tv_sec = 0;  /* possible? */
!             tmp_timeout.tv_usec = 0;
              ptmp_timeout = &tmp_timeout;
          }
          if (select(conn->sock + 1, &input_mask, &output_mask,
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.59
diff -c -c -r1.59 libpq-int.h
*** src/interfaces/libpq/libpq-int.h    14 Oct 2002 17:15:11 -0000    1.59
--- src/interfaces/libpq/libpq-int.h    16 Oct 2002 02:48:12 -0000
***************
*** 340,346 ****
  extern int    pqFlush(PGconn *conn);
  extern int    pqSendSome(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);

--- 340,347 ----
  extern int    pqFlush(PGconn *conn);
  extern int    pqSendSome(PGconn *conn);
  extern int    pqWait(int forRead, int forWrite, PGconn *conn);
! extern int    pqWaitTimed(int forRead, int forWrite, PGconn *conn,
!                         time_t finish_time);
  extern int    pqReadReady(PGconn *conn);
  extern int    pqWriteReady(PGconn *conn);