On Thu, Aug 31, 2017 at 4:35 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
> Hello Masahiko-san,
>> [...] Personally I prefer "t" for table creation because "c" for create is
>> a generic word. We might want to have another initialization command that
>> creates something.
> Ok, good point.
> About the patch: applies, compiles, works for me. A few minor comments.

Thank you for dedicated reviewing this patch!

> While re-reading the documentation, I think that it should be "Set custom
> initialization steps". It could be "Require ..." when -I implied -i, but
> since -i is still required the sentence does not seem to apply as such.
> "Destroying any existing tables: ..." -> "Destroy existing pgbench tables:
> ...".


> I would suggest to add short expanded explanations in the term definition,
> next to the triggering letter, to underline the mnemonic. Something like:
>    c (cleanup)
>    t (table creation)
>    g (generate data)
>    v (vacuum)
>    p (primary key)
>    f (foreign key)

Nice idea, agreed.

> Also update the error message in checkCustomCommands to "ctgvpf".


> Cleanup should have a message when it is executed. I suggest "cleaning
> up...".


> Maybe add a comment in front of the array tables to say that the order is
> important, something like "tables in reverse foreign key dependencies
> order"?


> case 'I': ISTM that initialize_cmds is necessarily already allocated, thus I
> would not bother to test before pg_free.

Agreed, fixed.

Attached latest patch. Please review it.


Masahiko Sawada
NTT Open Source Software Center

Attachment: pgbench_custom_initialization_v8.patch
Description: Binary data

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to