Re: Explicit psqlrc - Mailing list pgsql-hackers

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
mentionthat 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:
gabrielle@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:
gabrielle@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
betweenoperators
 
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