|From:||Tomas Vondra <tv(at)fuzzy(dot)cz>|
|Subject:||Re: pgbench -f and vacuum|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 21.12.2014 15:58, Tatsuo Ishii wrote:
>> On Sun, Dec 14, 2014 at 11:43 AM, Tatsuo Ishii <ishii(at)postgresql(dot)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
>>> 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
(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';
(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)
seems a bit pointless to me. But maybe it's necessary?
(6) The is_table_exists might be further simplified along these lines:
is_table_exists(PGconn *con, const char *table)
"SELECT to_regclass('%s') IS NULL", table);
res = PQexec(con, buf);
if (PQresultStatus(res) != PGRES_TUPLES_OK)
result = PQgetvalue(res, 0, 0);
retval = (*result == 't');
(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).
The second reason is that the msg1, msg2 variables seem unnecessary.
IMHO this is a bit better:
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");
if (is_table_exists(con, "pgbench_accounts"))
fprintf(stderr, "starting vacuum pgbench_accounts...");
"vacuum analyze pgbench_accounts");
(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.
|Next Message||Michael Paquier||2014-12-21 23:42:45||Re: TABLESAMPLE patch|
|Previous Message||Tatsuo Ishii||2014-12-21 23:25:45||Re: pgbench -f and vacuum|