|From:||Mark Wong <markwkm(at)gmail(dot)com>|
|To:||David Christensen <david(at)endpoint(dot)com>|
|Cc:||gabrielle <gorthx(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org|
|Subject:||Re: Explicit psqlrc|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
Is the patch in context diff format?
Does it apply cleanly to the current CVS HEAD?
Do all tests pass?
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
Read what the patch is supposed to do, and consider:
Does the patch actually implement that?
Do we want that?
Do we already have it?
Does it follow SQL spec, or the community-agreed behavior?
Does it include pg_dump support (if applicable)?
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?
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?
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
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
if (!options->nm_files % FILE_ALLOC_BLOCKS)
if (options->num_files > 1 && !((options->num_files - 1) % FILE_ALLOC_BLOCKS))
Does the patch slow down simple tests?
- not that we can tell.
If it claims to improve performance, does it?
Does it slow down other things?
- not that we can tell.
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?
Will it work on Windows/BSD etc?
Are the comments sufficient and accurate?
Does it do what it says, correctly?
Does it produce compiler warnings?
Can you make it crash?
- See above about the segfault.
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
Are there interdependencies that can cause problems?
- not that we are aware of
Did the reviewer cover all the things that kind of reviewer is supposed
|Next Message||KaiGai Kohei||2010-06-17 05:22:37||modular se-pgsql as proof-of-concept|
|Previous Message||Robert Haas||2010-06-17 02:03:36||Re: Keepalive for max_standby_delay|