On 22.12.2014 07:36, Tatsuo Ishii wrote:
> On 22.12.2014 00:28, Tomas Vondra wrote:
>>
>> (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"?
What about removing the concatenation logic, and just passing the whole
query to executeStatement2()? The queries are reasonably short, IMHO.
>> (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.
True. My point was that the to_regclass() is not exactly sufficient.
Maybe that's acceptable, maybe not.
>> (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.
So, how could the parameter be wrong? Or what about using PQgetisnull()?
>> (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.
I'm not sure. But if we use 'at least one of the tables exists' logic,
then I don't see a reason for msg1 and msg2. A single flag would be
sufficient.
>> 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.
I don't understand. I believe the code I proposed does exactly the same
thing as the patch, no? I certainly don't see any error messages in the
patch ...
>> (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.
Yes, but apparently pgbench.c does not do that. It's strange to have
prototypes for just two of many functions in the file.
Tomas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
