Re: pgbench -f and vacuum - Mailing list pgsql-hackers

Hi,

On 21.12.2014 15:58, Tatsuo Ishii wrote:
>> On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii@postgresql.org> wrote:
>>>> If we care enough about that case to attempt the vacuum anyway
>>>> then we need to do something about the error message; either
>>>> squelch it or check for the existence of the tables before
>>>> attempting to vacuum. Since there's no way to squelch in the
>>>> server logfile, I think checking for the table is the right
>>>> answer.
>>>
>>> Fair enough. I will come up with "checking for table before
>>> vacuum" approach.
>>
>> +1 for this approach.
> 
> Here is the patch I promised.

First of all - I'm not entirely convinced the "IF EXISTS" approach is
somehow better than "-f implies -n" suggested before, but I don't have a
strong preference either.

Regarding the patch:

(1) I agree with Fabrizio that the 'executeStatement2' is not the best   naming as it does not show the 'if exists'
intent.


(2) The 'executeStatement2' API is a bit awkward as the signarure
   executeStatement2(PGconn *con, const char *sql, const char *table);
   suggests that the 'sql' command is executed when 'table' exists.   But that's not the case, because what actually
getsexecuted is   'sql table'.
 


(3) The 'is_table_exists' should be probably just 'table_exists'.


(4) The SQL used in is_table_exists to check table existence seems   slightly wrong, because 'to_regclass' works for
otherrelation   kinds, not just regular tables - it will match views for example.   While a conflict like that (having
anobject with the right name   but not a regular table) is rather unlikely I guess, a more correct   query would be
this:
     SELECT oid FROM pg_class WHERE relname = '%s' AND relkind = 'r';


(5) I'm not a libpq expert, but I don't see how the PQgetvalue() could   return anything except true/false, so the
       if (result == NULL)       {           PQclear(res);           return false;       }
   seems a bit pointless to me. But maybe it's necessary?


(6) The is_table_exists might be further simplified along these lines:
   static bool   is_table_exists(PGconn *con, const char *table)   {       PGresult    *res;       char
buf[1024];      char        *result;       bool        retval;
 
       snprintf(buf, sizeof(buf)-1,                    "SELECT to_regclass('%s') IS NULL", table);
       res = PQexec(con, buf);       if (PQresultStatus(res) != PGRES_TUPLES_OK)       {           return false;
}
       result = PQgetvalue(res, 0, 0);
       retval = (*result == 't');
       PQclear(res);
       return retval;   }


(7) I also find the coding in main() around line 3250 a bit clumsy. The   first reason is that it only checks existence
of'pgbench_branches'   and then vacuums three pgbench_tellers and pgbench_history in the   same block. If
pgbench_branchesdoes not exist, there will be no   message printed (but the tables will be vacuumed).
 
   The second reason is that the msg1, msg2 variables seem unnecessary.   IMHO this is a bit better:
   if (!is_no_vacuum)   {       if (is_table_exists(con, "pgbench_branches"))       {           fprintf(stderr,
"startingvacuum...");
 
           executeStatement2(con, "vacuum", "pgbench_branches");           executeStatement2(con, "vacuum",
"pgbench_tellers");          executeStatement2(con, "truncate", "pgbench_history");
 
           fprintf(stderr, "end.\n");       }
       if (do_vacuum_accounts)       {           if (is_table_exists(con, "pgbench_accounts"))           {
fprintf(stderr, "starting vacuum pgbench_accounts...");
 
               executeStatement(con,                                "vacuum analyze pgbench_accounts");
               fprintf(stderr, "end.\n");           }       }   }

(8) Also, I think it's not necessary to define function prototypes for   executeStatement2 and is_table_exists. It
certainlyis not   consistent with the other functions defined in pgbench.c (e.g.   there's no prototype for
executeStatement).Just delete the two   prototypes and move is_table_exists before executeStatement2.
 


regards
Tomas