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 gets executed is
>     'sql table'.

Any replacement idea for "sql" and "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 other relation
>     kinds, not just regular tables - it will match views for example.
>     While a conflict like that (having an object 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';

This doesn't always work if schema search path is set.

> (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?

PQgetvalue could return NULL, if the parameter is wrong. I don't want
to raise segfault in any case.

> (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_branches does not exist, there will be no
>     message printed (but the tables will be vacuumed).

So we should check the existence of all pgbench_branches,
pgbench_tellers, pgbench_history and pgbench_accounts? Not sure if
it's worth the trouble.

>     The second reason is that the msg1, msg2 variables seem unnecessary.
>     IMHO this is a bit better:

This will behave differently from what pgbench it is now. If -f is not
specified and pgbench_* does not exist, then pgbech silently skipps
vacuum. Today pgbench raises error in this case.

>     if (!is_no_vacuum)
>     {
>         if (is_table_exists(con, "pgbench_branches"))
>         {
>             fprintf(stderr, "starting vacuum...");
> 
>             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 certainly is 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.

I think not having static function prototypes is not a good
custom. See other source code in PostgreSQL.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp