Re: Explicit psqlrc

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
Date: 2010-06-17 03:54:52
Message-ID: 20100616205452.4a3ead73@nghung
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: 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

==Submission review==
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

==Usability review==
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 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
==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)


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?
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?


In response to


Browse pgsql-hackers by date

  From Date Subject
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