Hi David, At a pdxpug gathering, we took a look at your patch to psql for supporting multiple -f's and put together some feedback:
REVIEW: Patch: support multiple -f options https://commitfest.postgresql.org/action/patch_view?id=286 ==Submission review== Is the patch in context diff format? yes Does it apply cleanly to the current CVS HEAD? yes Do all tests pass? yes Does it include reasonable tests, necessary doc patches, etc? - tests: inconclusive - docs: no: psql --help does not mention that you can use multiple -f switches; do we want it to? also, no doc update was included in the patch. ==Usability review== Read what the patch is supposed to do, and consider: Does the patch actually implement that? yes Do we want that? sure! Do we already have it? no Does it follow SQL spec, or the community-agreed behavior? NA Does it include pg_dump support (if applicable)? NA Are there dangers? - subject to the usual Dumb Mistakes (see "have all the bases been covered") Have all the bases been covered? Scenarios we came up with: - how does it handle wildcards, eg psql -f *.sql? Does not - this is a shell issue. psql is designed to take the database as the last argument; giving the -f option a wildcard expands the list, but does not assign multiple -f switches...so if you name one of your files the same as one of your databases, you could accidentally run updates you don't want to do. This is a known feature of psql, and has already bitten these reviewers in the butt on other occasions, so nothing to worry about here. - how does it handle the lack of a file? as expected: gabrie...@princess~/tmp/bin/:::--> ./psql -f ./psql: option requires an argument -- 'f' Try "psql --help" for more information. - how does it handle a non-existent file? as expected: gabrie...@princess~/tmp/bin/:::--> ./psql -f beer.sql beer.sql: No such file or directory - how does it handle files that don't contain valid sql? as expected, logs an error & carries on with the next file. ==Feature test== Apply the patch, compile it and test: Does the feature work as advertised? - Yes; and BEGIN-COMMIT statements within the files cause warnings when the command is wrapped in a transaction with the -1 switch (as specified in the patch submission). Are there corner cases the author has failed to consider? - none that we can think of Are there any assertion failures or crashes? - Mark got it to segfault due to bug in logic for allocating pointers for filenames. It appears the order of operations between '!' and '%' was not as intended, thus realloc() is never called and a seg fault may occur after 16 files are passed. There are a few ways to fix it, the one we played with to make minimum changes to the patch is to change: if (!options->nm_files % FILE_ALLOC_BLOCKS) to if (options->num_files > 1 && !((options->num_files - 1) % FILE_ALLOC_BLOCKS)) ==Performance review== Does the patch slow down simple tests? - not that we can tell. If it claims to improve performance, does it? N/A Does it slow down other things? - not that we can tell. ==Coding review== Read the changes to the code in detail and consider: Does it follow the project coding guidelines? - unnecessary whitespace on line 251? - inconsistent spacing between operators Are there portability issues? - untested Will it work on Windows/BSD etc? - untested Are the comments sufficient and accurate? Does it do what it says, correctly? - yes Does it produce compiler warnings? - no Can you make it crash? - See above about the segfault. ==Architecture review== Consider the changes to the code in the context of the project as a whole: Is everything done in a way that fits together coherently with other features/modules? - yes Are there interdependencies that can cause problems? - not that we are aware of ==Review review== Did the reviewer cover all the things that kind of reviewer is supposed to do? - AFAWK. Regards, Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
