Explicit psqlrc
in [PgSql]
|
Prev: [PATCH] elimination of code duplication in DefineOpFamily()
Next: [HACKERS] trace_recovery_messages
From: Mark Wong on 16 Jun 2010 23:54 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(a)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(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Alvaro Herrera on 17 Jun 2010 14:50 Excerpts from Mark Wong's message of mié jun 16 23:54:52 -0400 2010: > ==Usability review== > Read what the patch is supposed to do, and consider: > Does the patch actually implement that? How does it play with ON_ERROR_STOP/ROLLBACK? -- Álvaro Herrera <alvherre(a)commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Simon Riggs on 18 Jun 2010 04:48 On Thu, 2010-06-17 at 14:50 -0400, Alvaro Herrera wrote: > Excerpts from Mark Wong's message of mié jun 16 23:54:52 -0400 2010: > > > ==Usability review== > > Read what the patch is supposed to do, and consider: > > Does the patch actually implement that? > > How does it play with ON_ERROR_STOP/ROLLBACK? Also, how does it play with --single-transaction. I would like multiple -c commands also, as well as a mix of -f and -c. Can we add that at the same time please? -- Simon Riggs www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: gabrielle on 21 Jun 2010 19:51 On Thu, 2010-06-17 at 14:50 -0400, Alvaro Herrera asked: > How does it play with ON_ERROR_STOP/ROLLBACK? With ON_ERROR_STOP=ON, psql issues an error when it encounters one, stops processing the file that contains the error, and then continues to process any remaining files. I'm still investigating ON_ERROR_ROLLBACK. I need to tinker with it some more before I say anything concrete. On Fri, Jun 18, 2010 at 1:48 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: > Also, how does it play with --single-transaction. That was buried in our original report :) "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)" To expand upon that a bit: when psql encounters a file that contains a BEGIN statement, you get the expected "WARNING: there is already a transaction in progress" message. The COMMIT at the end of that file (assuming the user doesn't forget it) generates a COMMIT. Commands after that commit, or in any remaining files to be processed, are dealt with according to the user's autocommit settings: - if autocommit is ON, statements in the remaining files are processed & committed; the implicit COMMIT at the end of the whole thing then generates a "WARNING: there is no transaction in progress" message - if autocommit is OFF, statements in the remaining files generate "ERROR: current transaction is aborted, commands ignored until end of transaction block" messages. > I would like multiple -c commands also, as well as a mix of -f and -c. > Can we add that at the same time please? I'll leave this one for someone else to answer. :) gabrielle -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 21 Jun 2010 20:53
On Mon, Jun 21, 2010 at 7:51 PM, gabrielle <gorthx(a)gmail.com> wrote: > On Thu, 2010-06-17 at 14:50 -0400, Alvaro Herrera asked: >> How does it play with ON_ERROR_STOP/ROLLBACK? > > With ON_ERROR_STOP=ON, psql issues an error when it encounters one, > stops processing the file that contains the error, and then continues > to process any remaining files. > > I'm still investigating ON_ERROR_ROLLBACK. �I need to tinker with it > some more before I say anything concrete. > > On Fri, Jun 18, 2010 at 1:48 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: >> Also, how does it play with --single-transaction. > That was buried in our original report :) "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)" > > To expand upon that a bit: �when psql encounters a file that contains > a BEGIN statement, you get the expected "WARNING: there is already a > transaction in progress" message. �The COMMIT at the end of that file > (assuming the user doesn't forget it) generates a COMMIT. �Commands > after that commit, or in any remaining files to be processed, are > dealt with according to the user's autocommit settings: > - if autocommit is ON, statements in the remaining files are processed > & committed; �the implicit COMMIT at the end of the whole thing then > generates a "WARNING: there is no transaction in progress" message > - if autocommit is OFF, statements in the remaining files generate > "ERROR: �current transaction is aborted, commands ignored until end of > transaction block" messages. So none of the above sounds like desired behavior to me... is that just me? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |