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: > ...". Fixed. > 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". Fixed. > Cleanup should have a message when it is executed. I suggest "cleaning > up...". Fixed. > 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"? Fixed. > > 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. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
pgbench_custom_initialization_v8.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
