Re: Explicit psqlrc

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:
gabrielle(at)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(at)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