Thread: bugfix - contrib/pgbench

DDLs was used instead of DDLAFTERs by mistake.


*** pgbench.c    Tue Nov  9 15:09:31 2004
--- pgbench.fix.c    Mon May 23 12:50:01 2005
*************** init(void)
*** 622,628 ****
      for (i = 0; i < (sizeof(DDLAFTERs) / sizeof(char *)); i++)
      {
          res = PQexec(con, DDLAFTERs[i]);
!         if (strncmp(DDLs[i], "drop", 4) && PQresultStatus(res) != PGRES_COMMAND_OK)
          {
              fprintf(stderr, "%s", PQerrorMessage(con));
              exit(1);
--- 622,628 ----
      for (i = 0; i < (sizeof(DDLAFTERs) / sizeof(char *)); i++)
      {
          res = PQexec(con, DDLAFTERs[i]);
!         if (strncmp(DDLAFTERs[i], "drop", 4) && PQresultStatus(res) != PGRES_COMMAND_OK)
          {
              fprintf(stderr, "%s", PQerrorMessage(con));
              exit(1);

---
ITAGAKI Takahiro <itagaki.takahiro@lab.ntt.co.jp>
NTT Cyber Space Laboratories
Nippon Telegraph and Telephone Corporation.

ITAGAKI Takahiro wrote:
> --- 622,628 ----
>       for (i = 0; i < (sizeof(DDLAFTERs) / sizeof(char *)); i++)
>       {
>           res = PQexec(con, DDLAFTERs[i]);
> !         if (strncmp(DDLAFTERs[i], "drop", 4) && PQresultStatus(res) != PGRES_COMMAND_OK)
>           {
>               fprintf(stderr, "%s", PQerrorMessage(con));
>               exit(1);

None of the DDL in DDLAFTERs begins with "drop", so ISTM the right fix
is to just remove the strncmp().

-Neil

Neil Conway <neilc@samurai.com> wrote:

> > --- 622,628 ----
> >       for (i = 0; i < (sizeof(DDLAFTERs) / sizeof(char *)); i++)
> >       {
> >           res = PQexec(con, DDLAFTERs[i]);
> > !         if (strncmp(DDLAFTERs[i], "drop", 4) && PQresultStatus(res) != PGRES_COMMAND_OK)
> >           {
> >               fprintf(stderr, "%s", PQerrorMessage(con));
> >               exit(1);
>
> None of the DDL in DDLAFTERs begins with "drop", so ISTM the right fix
> is to just remove the strncmp().

I certainly think so.

But this code seems to be an idiom,
"Check the result, but drop commands may be fail".
Drop commands might be added to DDLAFTERs some time, so we may leave it.

---
ITAGAKI Takahiro <itagaki.takahiro@lab.ntt.co.jp>
NTT Cyber Space Laboratories
Nippon Telegraph and Telephone Corporation.

ITAGAKI Takahiro wrote:
> But this code seems to be an idiom,
> "Check the result, but drop commands may be fail".
> Drop commands might be added to DDLAFTERs some time, so we may leave it.

Possibly, but I think checking for something that cannot occur is
confusing to the reader (and besides, it is not correct in general to
ignore errors from _all_ DDL commands that begin with "drop"). So I just
removed the strncmp(). Thanks for the report; this is fixed in HEAD.

BTW, there is probably room for some refactoring here: the pattern

     res = PQexec(con, "...");
     if (PQresultStatus(res) != PGRES_COMMAND_OK)
     {
         fprintf(stderr, "...");
         exit(1);
     }

occurs frequently. That could be refactored into an exec_sql_cmd()
function or similar that would only return on non-error. Or at the very
least, a fatal_error() function that takes the error message and then
does the exit(1).

-Neil