On Tue, Aug 29, 2017 at 4:47 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote: > > Hello, > > Patch applies and works. > >>> I would like to insist a little bit: the name was declared as an int but >>> passed to init and used as a bool there before the patch. Conceptually >>> what >>> is meant is really a bool, and I see no added value bar saving a few >>> strokes >>> to have an int. ISTM that recent pgbench changes have started turning old >>> int-for-bool habits into using bool when bool is meant. >> >> >> Since is_no_vacuum is a existing one, if we follow the habit we should >> change other similar variables as well: is_init_mode, do_vacuum_accounts and >> debug. And I think we should change them in a separated patch. > > > Hmmm. The existing "is_no_vacuum" variable is typed *both* as int (in > "main") and as bool (in "init"), called by main (yuk!). I see no reason to > choose the bad one for the global:-) >
Yeah, I think this might be a good timing to re-consider int-for-bool habits in pgbench. If we decided to change is_no_vacuum to bool I want to change other similar variables as well. >> After more thought, I'm bit inclined to not have a short option for >> --custom-initialize because this option will be used for not primary >> cases. It would be better to save short options for future >> enhancements of pgbench. Thought? > > > I like it as is, especially as now the associated value is a simple and > short string, I think that it makes sense to have a simple and short option > to trigger it. Moreover -I stands cleanly for "initialization", and the > capital stands for something a little special which it is. Its to good to > miss. > > I think that the "-I" it should be added to the "--help" line, as it is done > with other short & long options. Okay, I'll leave it as of now. Maybe we can discuss later. > > Repeating "-I f" results in multiple foreign key constraints: > > Foreign-key constraints: > "pgbench_tellers_bid_fkey" FOREIGN KEY (bid) REFERENCES > pgbench_branches(bid) > "pgbench_tellers_bid_fkey1" FOREIGN KEY (bid) REFERENCES > pgbench_branches(bid) > "pgbench_tellers_bid_fkey2" FOREIGN KEY (bid) REFERENCES > pgbench_branches(bid) > "pgbench_tellers_bid_fkey3" FOREIGN KEY (bid) REFERENCES > pgbench_branches(bid) > > I wonder if this could be avoided easily? Maybe by setting the constraint > name explicitely so that the second one fails on the existing one, which is > fine, like for primary keys? Or adding a DROP CONSTRAINT IF EXISTS before > the CREATE CONSTRAINT, like for tables? Or doing nothing about it? I would > prefer the first option. Good point, I agree with first option. > > Maybe the initial cleanup (DROP TABLE) could be made an option added to the > default, so that cleaning up the database could be achieved with some > "pgbench -i -I c", instead of connecting and droping the tables one by one > which I have done quite a few times... What do you think? Yeah, I sometimes wanted that. Having the cleaning up tables option would be good idea. > Before it is definitely engraved, I'm thinking about the letters: > > c - cleanup > t - create table > d - data > p - primary key > f - foreign key > v - vacuum > > I think it is mostly okay, but it is the last time to think about it. Using > "d" for cleanup (drop) would mean finding another letter for filling in > data... maybe "g" for data generation? "c" may have been chosen for "create > table", but then would not be available for "cleanup". Thoughts? I'd say "g" for data generation would be better. Also, I'm inclined to add a command for the unlogged tables. How about this? c - cleanup t - create table u - create unlogged table g - data generation p - primary key f - foreign key v - vacuum Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
