Re: [HACKERS] pgbench: Skipping the creating primary keys afterinitialization

Hello Masahiko-san,

> Yeah, once custom initialization patch get committed we can extend it.
>
> Attached updated patch. I've incorporated the all comments from Fabien
> to it, and changed it to single letter version.

Patch applies and works.

A few comments and questions about the code and documentation:

Why not allow -I as a short option for --custom-initialize?

I do not think that the --custom-initialize option needs to appear as a 
specific synopsis in the documentation, as it is now a sub-option of -i.

checkCustomCmds: I would suggest to simplify test code with strchr
and to merge the two fprintf into one, something like:
  if (strchr("tdpfv", *cmd) == NULL) {     fprintf(stderr,            "....\n"        "....\n",        ...);     ...

Moreover there is already an error message later if checkCustomCmds fails, I think
it could be expanded and the two-line one in the function dropped 
entirely? It seems strange to have two level error messages for that.

Help message looks strange. I suggest something regexp-like:
 --custom-initialize=[tdvpf]+

I would suggest to put the various init* functions in a more logical order:
first create table, then load data, etc.

In case 0: do not exchange unlogged_tables & foreign_keys gratuiously.

After checking the initial code, I understand that the previous default 
was "tdvp", but you put "tdpv". I'm unsure whether vaccuum would do 
something to the indexes and that would make more sense. In doubt, I 
suggest to keep the previous default.

Maybe --foreign-keys should really do "tdvpf"?

I may be okay with disallowing --foreign-keys and --no-vacuum if --custom-init is used,
but then there is no need to test it again in init... I think that in any case 'f' and 'v'
should always trigger the corresponding initializations.

On the other hand, I think that it could be more pragmatic with these 
options, i.e. --foreign-keys could just append 'f' to the current command 
if not already there, and '--no-vacuum' could remove 'v' if there, on the 
fly, so that nothing would be banned. This would require to always have a 
malloced custom_init string. It would allow to remove the "foreign_keys" 
global variable. "is_no_vacuum" is probably still needed for benchmarking.
This way there would be no constraints and "is_custom_init" could be 
dropped as well.

-- 
Fabien.