Thread: Explicit psqlrc
I've now for the second time found myself wanting to specify an explicit psqlrc file instead of just parsing ~/.psqlrc, so attached is a patch that accepts the PSQLRC environment variable to control which psqlrc file is used. Any objections to this (obviously including documentation - this is just the trivial code) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
On Mar 4, 2010, at 4:00 PM, Magnus Hagander wrote: > I've now for the second time found myself wanting to specify an > explicit psqlrc file instead of just parsing ~/.psqlrc, so attached is > a patch that accepts the PSQLRC environment variable to control which > psqlrc file is used. > > Any objections to this (obviously including documentation - this is > just the trivial code) My bikeshed has a --psqlrc path/to/file, but +1 on the idea. Regards, David -- David Christensen End Point Corporation david@endpoint.com
2010/3/5 David Christensen <david@endpoint.com>: > > On Mar 4, 2010, at 4:00 PM, Magnus Hagander wrote: > >> I've now for the second time found myself wanting to specify an >> explicit psqlrc file instead of just parsing ~/.psqlrc, so attached is >> a patch that accepts the PSQLRC environment variable to control which >> psqlrc file is used. >> >> Any objections to this (obviously including documentation - this is >> just the trivial code) > > > My bikeshed has a --psqlrc path/to/file, but +1 on the idea. The main reason I went with environment variable is that it's the path-of-least-code :-) And it easily fullfills my use-cases for it, which has me launching interactive psql with completely different settings from a script. Do you have a use-case where --psqlrc would be more useful than an environment variable, or is it *only* bike-shedding? ;) -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
2010/3/5 Magnus Hagander <magnus@hagander.net>: > 2010/3/5 David Christensen <david@endpoint.com>: >> >> On Mar 4, 2010, at 4:00 PM, Magnus Hagander wrote: >> >>> I've now for the second time found myself wanting to specify an >>> explicit psqlrc file instead of just parsing ~/.psqlrc, so attached is >>> a patch that accepts the PSQLRC environment variable to control which >>> psqlrc file is used. >>> >>> Any objections to this (obviously including documentation - this is >>> just the trivial code) >> >> >> My bikeshed has a --psqlrc path/to/file, but +1 on the idea. > > The main reason I went with environment variable is that it's the > path-of-least-code :-) And it easily fullfills my use-cases for it, > which has me launching interactive psql with completely different > settings from a script. > > Do you have a use-case where --psqlrc would be more useful than an > environment variable, or is it *only* bike-shedding? ;) Just to be clear, the code difference isn't very large. Attached is a patch that does both PSQLRC and --psqlrc. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> 2010/3/5 David Christensen <david@endpoint.com>:
>> My bikeshed has a --psqlrc path/to/file, but +1 on the idea.
> Do you have a use-case where --psqlrc would be more useful than an
> environment variable, or is it *only* bike-shedding? ;)
The env variable solution seems a bit surprising to me. If we were
dealing with a libpq option it would make sense (to avoid having to pass
the info through other layers) but for a psql option it doesn't AFAICS.
regards, tom lane
2010/3/5 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> 2010/3/5 David Christensen <david@endpoint.com>: >>> My bikeshed has a --psqlrc path/to/file, but +1 on the idea. > >> Do you have a use-case where --psqlrc would be more useful than an >> environment variable, or is it *only* bike-shedding? ;) > > The env variable solution seems a bit surprising to me. If we were > dealing with a libpq option it would make sense (to avoid having to pass > the info through other layers) but for a psql option it doesn't AFAICS. Fair enough. It worked in my environment, but I can see your point. So I'll just strip that part out then :) -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On fre, 2010-03-05 at 11:30 +0100, Magnus Hagander wrote: > > Do you have a use-case where --psqlrc would be more useful than an > > environment variable, or is it *only* bike-shedding? ;) > > Just to be clear, the code difference isn't very large. Attached is a > patch that does both PSQLRC and --psqlrc. I can see the environment variable variant as an analogy to BASH_ENV, but what is the use case for the --psqlrc option? Wouldn't it be easier and more useful to just be able to process more than one file, say by specifying -f more than once?
Peter Eisentraut <peter_e@gmx.net> writes:
> I can see the environment variable variant as an analogy to BASH_ENV,
> but what is the use case for the --psqlrc option? Wouldn't it be easier
> and more useful to just be able to process more than one file, say by
> specifying -f more than once?
The analogy I was thinking about was psql -X, but I agree that it's
not obvious why this shouldn't be thought of as an additional -f file.
regards, tom lane
2010/3/6 Tom Lane <tgl@sss.pgh.pa.us>: > Peter Eisentraut <peter_e@gmx.net> writes: >> I can see the environment variable variant as an analogy to BASH_ENV, >> but what is the use case for the --psqlrc option? Wouldn't it be easier >> and more useful to just be able to process more than one file, say by >> specifying -f more than once? > > The analogy I was thinking about was psql -X, but I agree that it's > not obvious why this shouldn't be thought of as an additional -f file. Uh, I don't follow. When we use -f, we'll run the script and then exit. The whole point is to run it and *not* exit, since you are normally using it to set up the environment in psql. That said, it might certainly be useful to be able to process more than one file with -f, but that's a different usecase, I think. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> 2010/3/6 Tom Lane <tgl@sss.pgh.pa.us>:
>> The analogy I was thinking about was psql -X, but I agree that it's
>> not obvious why this shouldn't be thought of as an additional -f file.
> Uh, I don't follow. When we use -f, we'll run the script and then
> exit. The whole point is to run it and *not* exit, since you are
> normally using it to set up the environment in psql.
If we were going to support multiple -f options, it would be sensible
to interpret "-f -" as "read from stdin until EOF". Then you could
interleave prepared scripts and stdin, which could be pretty handy.
The default behavior would be equivalent to a single instance of "-f -",
and what you are looking for would be "-X -f substitute-psqlrc -f -".
I do think this is potentially cleaner and more general than the
--psqlrc switch. Maybe that should be reverted and the whole topic
reconsidered during 9.1 devel cycle.
regards, tom lane
2010/3/7 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> 2010/3/6 Tom Lane <tgl@sss.pgh.pa.us>: >>> The analogy I was thinking about was psql -X, but I agree that it's >>> not obvious why this shouldn't be thought of as an additional -f file. > >> Uh, I don't follow. When we use -f, we'll run the script and then >> exit. The whole point is to run it and *not* exit, since you are >> normally using it to set up the environment in psql. > > If we were going to support multiple -f options, it would be sensible > to interpret "-f -" as "read from stdin until EOF". Then you could > interleave prepared scripts and stdin, which could be pretty handy. > The default behavior would be equivalent to a single instance of "-f -", > and what you are looking for would be "-X -f substitute-psqlrc -f -". Right, that would work. Though it would be a lot more user-unfriendly for such a simple thing, IMHO. Also, "-f -" and just "psql" behaves different today (for example, in the showing of startup banners). So we couldn't do that without changing the behaviour of at least one of those. Which may not be a problem of course, but I'm sure someone will find a place to complain :) With your interleave, you mean things like "psql -f first.sql -f - -f second.sql"? That does sound like it could be handy - and also really dangerous :-) > I do think this is potentially cleaner and more general than the > --psqlrc switch. Maybe that should be reverted and the whole topic > reconsidered during 9.1 devel cycle. More general, definitely. But cleaner? I'd say rather the opposite. In the end, I don't see why we can't have both when the implementation is so trivial. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> 2010/3/7 Tom Lane <tgl@sss.pgh.pa.us>:
>> If we were going to support multiple -f options, it would be sensible
>> to interpret "-f -" as "read from stdin until EOF".
> Right, that would work. Though it would be a lot more user-unfriendly
> for such a simple thing, IMHO.
If the issue had come up even once before in psql's existence, I might
think that user-friendliness would be a good argument. As things stand,
I don't believe the average user will care about it in the least. I'd
be willing to lay long odds that the average user doesn't even have a
.psqlrc file, much less feel the need to override it. I'd rather see
"use a substitute psqlrc" be a behavior you can build out of existing
general-purpose switches than still another option that has to be
documented and remembered.
> Also, "-f -" and just "psql" behaves different today (for example, in
> the showing of startup banners).
Yes, there would be some things to think about there, which is why it's
a topic for a new devel cycle rather than something to shoehorn in
after the close of the last CF.
regards, tom lane
2010/3/7 Tom Lane <tgl@sss.pgh.pa.us>: > Magnus Hagander <magnus@hagander.net> writes: >> 2010/3/7 Tom Lane <tgl@sss.pgh.pa.us>: >>> If we were going to support multiple -f options, it would be sensible >>> to interpret "-f -" as "read from stdin until EOF". > >> Right, that would work. Though it would be a lot more user-unfriendly >> for such a simple thing, IMHO. > > If the issue had come up even once before in psql's existence, I might > think that user-friendliness would be a good argument. As things stand, > I don't believe the average user will care about it in the least. I'd > be willing to lay long odds that the average user doesn't even have a > .psqlrc file, much less feel the need to override it. I'd rather see > "use a substitute psqlrc" be a behavior you can build out of existing > general-purpose switches than still another option that has to be > documented and remembered. I've heard if a couple of times before, but I agree it's certainly not a much asked-for one. Most if it has been in off-list scenarios and people have probabliy just thought it's not a big enough feature to bother emailing about. >> Also, "-f -" and just "psql" behaves different today (for example, in >> the showing of startup banners). > > Yes, there would be some things to think about there, which is why it's > a topic for a new devel cycle rather than something to shoehorn in > after the close of the last CF. Fair enough. I expected it to be a small and noncontroversial thing, but since there are objections, I'll go revert it. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > >> Also, "-f -" and just "psql" behaves different today (for example, in > >> the showing of startup banners). > > > > Yes, there would be some things to think about there, which is why it's > > a topic for a new devel cycle rather than something to shoehorn in > > after the close of the last CF. > > Fair enough. I expected it to be a small and noncontroversial thing, > but since there are objections, I'll go revert it. Do you want to create a TODO entry for this? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
On Mar 7, 2010, at 9:22 AM, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> 2010/3/6 Tom Lane <tgl@sss.pgh.pa.us>: >>> The analogy I was thinking about was psql -X, but I agree that it's >>> not obvious why this shouldn't be thought of as an additional -f >>> file. > >> Uh, I don't follow. When we use -f, we'll run the script and then >> exit. The whole point is to run it and *not* exit, since you are >> normally using it to set up the environment in psql. > > If we were going to support multiple -f options, it would be sensible > to interpret "-f -" as "read from stdin until EOF". Then you could > interleave prepared scripts and stdin, which could be pretty handy. > The default behavior would be equivalent to a single instance of "-f > -", > and what you are looking for would be "-X -f substitute-psqlrc -f -". Here's an initial stab at supporting multiple -f's (not counting the interpretation of "-f -" as STDIN). There are also a few pieces that are up for interpretation, such as the propagation of the return value of the MainLoop(). Also, while this patch supports the single- transaction mode, it does so in a way that will break if one of the scripts include explicit BEGIN/COMMIT statements (although it is no different than the existing code in this regard). Regards, David -- David Christensen End Point Corporation david@endpoint.com
Attachment
On Mon, Mar 8, 2010 at 1:39 AM, David Christensen <david@endpoint.com> wrote: > > On Mar 7, 2010, at 9:22 AM, Tom Lane wrote: > >> Magnus Hagander <magnus@hagander.net> writes: >>> >>> 2010/3/6 Tom Lane <tgl@sss.pgh.pa.us>: >>>> >>>> The analogy I was thinking about was psql -X, but I agree that it's >>>> not obvious why this shouldn't be thought of as an additional -f file. >> >>> Uh, I don't follow. When we use -f, we'll run the script and then >>> exit. The whole point is to run it and *not* exit, since you are >>> normally using it to set up the environment in psql. >> >> If we were going to support multiple -f options, it would be sensible >> to interpret "-f -" as "read from stdin until EOF". Then you could >> interleave prepared scripts and stdin, which could be pretty handy. >> The default behavior would be equivalent to a single instance of "-f -", >> and what you are looking for would be "-X -f substitute-psqlrc -f -". > > Here's an initial stab at supporting multiple -f's (not counting the > interpretation of "-f -" as STDIN). There are also a few pieces that are up > for interpretation, such as the propagation of the return value of the > MainLoop(). Also, while this patch supports the single-transaction mode, it > does so in a way that will break if one of the scripts include explicit > BEGIN/COMMIT statements (although it is no different than the existing code > in this regard). I have added to this to the next CommitFest. ...Robert
On Sun, 2010-03-07 at 16:37 +0100, Magnus Hagander wrote: > With your interleave, you mean things like "psql -f first.sql -f - -f > second.sql"? That does sound like it could be handy - and also really > dangerous :-) Multiple -f support would be a good thing. As would mixed -f and -c options. What would be even better would be bringing across many of the concepts that are in pgbench, which has had the multiple file support for some time. -- Simon Riggs www.2ndQuadrant.com
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
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@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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.comPostgreSQL Development, 24x7 Support, Training and Services
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@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
On Mon, Jun 21, 2010 at 7:51 PM, gabrielle <gorthx@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@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
* Robert Haas (robertmhaas@gmail.com) wrote:
> So none of the above sounds like desired behavior to me... is that just me?
Yeah, I'm not really thrilled with this.. I mentioned earlier what I
thought would be a useful feature (basically, a switch which would
ignore the main psqlrc and turn on the various options that make sense
for a script), but that seems to have fallen to the wayside..
Thanks,
Stephen
On Mon, Jun 21, 2010 at 9:13 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> So none of the above sounds like desired behavior to me... is that just me? > > Yeah, I'm not really thrilled with this.. I mentioned earlier what I > thought would be a useful feature (basically, a switch which would > ignore the main psqlrc and turn on the various options that make sense > for a script), but that seems to have fallen to the wayside.. Well, that might be a good idea, too, but my expectation is that: psql -f one -f two -f three ought to behave in a manner fairly similar to: cat one two three > all psql -f all and it sounds like with this patch that's far from being the case. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Mon, 2010-06-21 at 20:53 -0400, Robert Haas wrote: > On Mon, Jun 21, 2010 at 7:51 PM, gabrielle <gorthx@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. That would be undesirable. > > 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@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. This is the existing behaviour. > So none of the above sounds like desired behavior to me... is that just me? Single transaction needs some help, but that's not the fault of this patch. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Well, that might be a good idea, too, but my expectation is that: > > psql -f one -f two -f three > > ought to behave in a manner fairly similar to: > > cat one two three > all > psql -f all > > and it sounds like with this patch that's far from being the case. Correct. Each is handled individually. Should I continue to check on ON_ERROR_ROLLBACK results, or bounce this back to the author? gabrielle
On Wed, Jun 23, 2010 at 9:17 AM, gabrielle <gorthx@gmail.com> wrote: > On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Well, that might be a good idea, too, but my expectation is that: >> >> psql -f one -f two -f three >> >> ought to behave in a manner fairly similar to: >> >> cat one two three > all >> psql -f all >> >> and it sounds like with this patch that's far from being the case. > > Correct. Each is handled individually. > > Should I continue to check on ON_ERROR_ROLLBACK results, or bounce > this back to the author? It doesn't hurt to continue to review and find other problems so that the author can try to fix them all at once, but it certainly seems clear that it's not ready to commit at this point, so it definitely needs to go back to the author for a rework. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Jun 22, 2010, at 1:34 AM, Simon Riggs wrote: > On Mon, 2010-06-21 at 20:53 -0400, Robert Haas wrote: >> On Mon, Jun 21, 2010 at 7:51 PM, gabrielle <gorthx@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. > > That would be undesirable. > >>> 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@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. > > This is the existing behaviour. > >> So none of the above sounds like desired behavior to me... is that just me? > > Single transaction needs some help, but that's not the fault of this > patch. I took a closer look at what was going on and what it would take to meet some of these expectations. In main() the patchadds BEGIN and COMMIT statements outside the call to process_file() in src/bin/psql/command.c. Here lies the previouslogic for handling single transaction. Since it remains, it appears that BEGIN can be issued twice before any statementsare executed if the single transaction switch is used. Plus there are other a couple of places that call thisparticular process_file() function, but I think those are straightforward cases to deal with. Regards, Mark
On Jun 23, 2010, at 5:36 PM, Mark Wong wrote: > On Jun 22, 2010, at 1:34 AM, Simon Riggs wrote: > >> On Mon, 2010-06-21 at 20:53 -0400, Robert Haas wrote: >>> On Mon, Jun 21, 2010 at 7:51 PM, gabrielle <gorthx@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. >> >> That would be undesirable. >> >>>> 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@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. >> >> This is the existing behaviour. >> >>> So none of the above sounds like desired behavior to me... is that just me? >> >> Single transaction needs some help, but that's not the fault of this >> patch. > > I took a closer look at what was going on and what it would take to meet some of these expectations. In main() the patchadds BEGIN and COMMIT statements outside the call to process_file() in src/bin/psql/command.c. Here lies the previouslogic for handling single transaction. Since it remains, it appears that BEGIN can be issued twice before any statementsare executed if the single transaction switch is used. Plus there are other a couple of places that call thisparticular process_file() function, but I think those are straightforward cases to deal with. Heh, not close enough. I was wrong about that scenario. I can see now that the single transaction flag is always set tofalse in process_file(). I think that turns the single transaction handling in process_file() into dead code with thepatch like this. Regards, Mark
On Wed, Jun 23, 2010 at 9:22 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jun 23, 2010 at 9:17 AM, gabrielle <gorthx@gmail.com> wrote: >> On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> Well, that might be a good idea, too, but my expectation is that: >>> >>> psql -f one -f two -f three >>> >>> ought to behave in a manner fairly similar to: >>> >>> cat one two three > all >>> psql -f all >>> >>> and it sounds like with this patch that's far from being the case. >> >> Correct. Each is handled individually. >> >> Should I continue to check on ON_ERROR_ROLLBACK results, or bounce >> this back to the author? > > It doesn't hurt to continue to review and find other problems so that > the author can try to fix them all at once, but it certainly seems > clear that it's not ready to commit at this point, so it definitely > needs to go back to the author for a rework. Since it has been over a month since this review was posted and no new version of the patch has appeared, I think we should mark this patch as Returned with Feedback. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Mon, 2010-07-19 at 23:40 -0400, Robert Haas wrote: > Since it has been over a month since this review was posted and no new > version of the patch has appeared, I think we should mark this patch > as Returned with Feedback. Mark posted a new patch 6 days ago, AFAICS. Not sure I see any benefit in doing as you propose anyway, or at least not without warning since it just confuses authors who may believe they have time while the commitfest is still on. Commitfests were created to help authors. We must continue to remember that 99% of patch authors are not full time and likely to find smooth responsiveness difficult. We should say "Will there be any further action on this patch?" -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
On Tue, Jul 20, 2010 at 3:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Mon, 2010-07-19 at 23:40 -0400, Robert Haas wrote: > >> Since it has been over a month since this review was posted and no new >> version of the patch has appeared, I think we should mark this patch >> as Returned with Feedback. > > Mark posted a new patch 6 days ago, AFAICS. Hmm. I can't find it in my mail, in my archives, or linked off the CommitFest application. Was it posted under a different subject line?Do you have a link to the archives? > Not sure I see any benefit in doing as you propose anyway, or at least > not without warning since it just confuses authors who may believe they > have time while the commitfest is still on. > > Commitfests were created to help authors. We must continue to remember > that 99% of patch authors are not full time and likely to find smooth > responsiveness difficult. > > We should say "Will there be any further action on this patch?" It isn't the purpose of CommitFests to provide patch authors with an unlimited right to have their patches reviewed. They exist, on the one hand, to make sure that patches don't linger forever without getting a review; and on the other hand, to create a defined time for each member of the community to set aside their own work and review/commit other people's patches. It is important that we have them, and it is also important that they end, so that reviews, committers, commitfest managers, etc. can stop working on the CF at some point and get back to their own work. In other words, CommitFests need to represent a balance between the needs of authors and the needs of patch reviewers and committers. Of course, anyone is always welcome to review a patch that has been posted, and a committer can decide to work on a patch at any time also. But if patch authors are entitled to resubmit a previously reviewed patch up until the very last CommitFest are *guaranteed* a possible review and commit even then, then the CommitFest will not end on time. Even if the CommitFest does end on time, more than 50% of the time between now and 9.1 feature freeze is CF-time - that is, time I'm supposed to be putting aside the work I'd like to get done to help other people get the work they'd like to do get done. I'm not really willing to increase that percentage much further. I feel it's important that we give everyone a fair shake, and I have devoted and will continue to devote a LOT of time to making sure that happens - but I want (and if I care to still be employed, need) some time to do my own work, too. To me, the definition of a fair shake is that people get 4-5 days to respond to review comments. This patch has had 33. It's not unfair to anyone to say, you know, since you didn't get around to updating this patch for over a month, you'll need to resubmit the updated version to the next CommitFest. If we have the resources to review and commit a late resubmission of some particular patch, that is great. But as of today, we still have 32 patches that need to be reviewed, many of which do not have a reviewer assigned or which have a reviewer assigned but have not yet had an initial review. Since there are 26 days left in the CommitFest and many of those patches will need multiple rounds of review and much discussion before we decide whether to commit them, send them back for rework, or reject them outright, that's pretty scary. To me, that's where we should be focusing our effort. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Tue, 2010-07-20 at 07:06 -0400, Robert Haas wrote: > To me, the definition of a fair shake is that people get 4-5 days to > respond to review comments. This patch has had 33. It's not unfair > to anyone to say, you know, since you didn't get around to updating > this patch for over a month, you'll need to resubmit the updated > version to the next CommitFest. If we have the resources to review > and commit a late resubmission of some particular patch, that is > great. But as of today, we still have 32 patches that need to be > reviewed, many of which do not have a reviewer assigned or which have > a reviewer assigned but have not yet had an initial review. Since > there are 26 days left in the CommitFest and many of those patches > will need multiple rounds of review and much discussion before we > decide whether to commit them, send them back for rework, or reject > them outright, that's pretty scary. To me, that's where we should be > focusing our effort. So focus your effort by leaving this alone until the end of the CF. Actively terminating things early doesn't help at all with the review work you mention above, but it looks good if we are measuring "cases resolved per day". Are we measuring that? If so, why? Who cares? Just leave them, and if no action at end of CF, boot them then. Saves loads of time on chasing up on people, interpreting what they say, worrying about it and minor admin. Closing early gains us nothing, though might close the door on useful work in progress. Net expected benefit is negative from acting early. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
On Tue, Jul 20, 2010 at 7:41 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > So focus your effort by leaving this alone until the end of the CF. > Actively terminating things early doesn't help at all with the review > work you mention above, but it looks good if we are measuring "cases > resolved per day". Are we measuring that? If so, why? Who cares? We don't formally measure that, but yeah, I definitely keep an eye on it. I've found that if you don't keep a sharp eye on that, you end up not done with the CommitFest is supposed to be over. I'd much rather boot patches for reasonable justification throughout the CommitFest than boot everything at the end whether there's a justification or not. > Closing early gains us nothing, though might close the door on useful > work in progress. IMHO, closing early LOSES us nothing. People are free to work on their patches whenever they'd like, and hopefully will. But pretending we're going to review them all no matter when they get resubmitted just makes people grumpy when they find out that we're not magical and can't. A further point is that it's very difficult to keep track of progress if the CF page reflects a whole bunch of supposedly "Waiting on Author" patches that are really quite thoroughly dead. On the other hand, if this patch was really resubmitted already and I missed it, as you suggested, that's a whole different situation. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Tue, 2010-07-20 at 07:49 -0400, Robert Haas wrote: > A further point is that it's very difficult to > keep track of progress if the CF page reflects a whole bunch of > supposedly "Waiting on Author" patches that are really quite > thoroughly dead. True, but the point under discussion is what to do if no reply is received from an author. That is something entirely different from a patch hitting a brick wall. We gain nothing by moving early on author-delay situations, so I suggest we don't. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
On Tue, Jul 20, 2010 at 8:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Tue, 2010-07-20 at 07:49 -0400, Robert Haas wrote: >> A further point is that it's very difficult to >> keep track of progress if the CF page reflects a whole bunch of >> supposedly "Waiting on Author" patches that are really quite >> thoroughly dead. > > True, but the point under discussion is what to do if no reply is > received from an author. That is something entirely different from a > patch hitting a brick wall. > > We gain nothing by moving early on author-delay situations, so I suggest > we don't. No, we gain something quite specific and tangible, namely, the expectation that patch authors will stay on top of their patches if they want them reviewed by the community. If that expectation doesn't seem important to you, feel free to try running a CommitFest without it. If you can make it work, I'll happily sign on. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> wrote: > we gain something quite specific and tangible, namely, the > expectation that patch authors will stay on top of their patches > if they want them reviewed by the community. Barring some operational emergency here, I'll be reviewing the status of all the open patches in the CF today. If I can't find any new posting by the author for the patch in question, I'll mark it Returned With Feedback. I'll probably be cracking the whip on a few others, one way or another. If anyone wonders why I don't do so for certain patches, it will probably be because I've had off-list messages about needing more time to do a proper review or being in transit and unable to do more than post short emails at the moment. I do request that all authors and reviewers make an effort to keep the CF app page up-to-date. If you're not sure all recent patches, reviews, and significant comment posts are reflected in the app for a patch for which you're an author or reviewer, please check as soon as possible and make it right; it's save time for me and will help keep the process moving smoothly. Thanks, all. -Kevin
On Tue, 2010-07-20 at 09:05 -0400, Robert Haas wrote: > On Tue, Jul 20, 2010 at 8:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > > On Tue, 2010-07-20 at 07:49 -0400, Robert Haas wrote: > >> A further point is that it's very difficult to > >> keep track of progress if the CF page reflects a whole bunch of > >> supposedly "Waiting on Author" patches that are really quite > >> thoroughly dead. > > > > True, but the point under discussion is what to do if no reply is > > received from an author. That is something entirely different from a > > patch hitting a brick wall. > > > > We gain nothing by moving early on author-delay situations, so I suggest > > we don't. > > No, we gain something quite specific and tangible, namely, the > expectation that patch authors will stay on top of their patches if > they want them reviewed by the community. If that expectation doesn't > seem important to you, feel free to try running a CommitFest without > it. If you can make it work, I'll happily sign on. I don't think so. We can assume people wrote a patch because they want it included in Postgres. Bumping them doesn't help them or us, since there is always an issue other than wish-to-complete. Not everybody is able to commit time in the way we do and we should respect that better. Authors frequently have to wait a long time for a review; why should reviewers not be as patient as authors must be? We should be giving authors as much leeway as possible, or they may not come back. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
Simon Riggs <simon@2ndQuadrant.com> wrote: > I don't think so. We can assume people wrote a patch because they > want it included in Postgres. Bumping them doesn't help them or > us, since there is always an issue other than wish-to-complete. > Not everybody is able to commit time in the way we do and we > should respect that better. Sure. If people mail me off-list about needing more time, I'm willing to accommodate, within reason. > Authors frequently have to wait a long time for a review; why > should reviewers not be as patient as authors must be? Let's keep this in perspective. We're talking about pushing review to less than two months away because of lack of author response for over a month. And should a patch appear before then, there's nothing that says an interested member of the community can't review it before the next CF. You, for example, would be free to review it at any time a patch might appear. > We should be giving authors as much leeway as possible, or they > may not come back. One phenomenon I've noticed is that sometimes a patch is submitted because an end user has solved their own problem for themselves, but wishes to share the solution with the community. They're not always motivated to go to the lengths required to polish it up to the standard required for inclusion in core. In such cases, unless someone with the time to do so finds it interesting enough to pick up, it is just going to drop. I hope such authors feel comfortable submitting their next effort, as it might be something which interests a larger audience than the previous effort. We should do what we can to ensure that they understand the dynamics of that. -Kevin
On Tue, Jul 20, 2010 at 9:23 AM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Robert Haas <robertmhaas@gmail.com> wrote: > >> we gain something quite specific and tangible, namely, the >> expectation that patch authors will stay on top of their patches >> if they want them reviewed by the community. > > Barring some operational emergency here, I'll be reviewing the > status of all the open patches in the CF today. If I can't find any > new posting by the author for the patch in question, I'll mark it > Returned With Feedback. I'll probably be cracking the whip on a few > others, one way or another. If anyone wonders why I don't do so for > certain patches, it will probably be because I've had off-list > messages about needing more time to do a proper review or being in > transit and unable to do more than post short emails at the moment. > > I do request that all authors and reviewers make an effort to keep > the CF app page up-to-date. If you're not sure all recent patches, > reviews, and significant comment posts are reflected in the app for > a patch for which you're an author or reviewer, please check as soon > as possible and make it right; it's save time for me and will help > keep the process moving smoothly. Thanks, I appreciate it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Jul 19, 2010, at 10:40 PM, Robert Haas wrote:
> On Wed, Jun 23, 2010 at 9:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Wed, Jun 23, 2010 at 9:17 AM, gabrielle <gorthx@gmail.com> wrote:
>>> On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> Well, that might be a good idea, too, but my expectation is that:
>>>>
>>>> psql -f one -f two -f three
>>>>
>>>> ought to behave in a manner fairly similar to:
>>>>
>>>> cat one two three > all
>>>> psql -f all
>>>>
>>>> and it sounds like with this patch that's far from being the case.
>>>
>>> Correct. Each is handled individually.
>>>
>>> Should I continue to check on ON_ERROR_ROLLBACK results, or bounce
>>> this back to the author?
>>
>> It doesn't hurt to continue to review and find other problems so that
>> the author can try to fix them all at once, but it certainly seems
>> clear that it's not ready to commit at this point, so it definitely
>> needs to go back to the author for a rework.
>
> Since it has been over a month since this review was posted and no new
> version of the patch has appeared, I think we should mark this patch
> as Returned with Feedback.
Sorry for the delays in response. This is fine; I think there are some semantic questions that should still be
resolvedat this point, particularly if we're moving toward supporting multiple -c and -f lines as expressed (an idea
thatI agree would be useful). Specifically:
1) Does the -1 flag with multiple files indicate a single transaction for all commands/files or that each command/file
isrun in its own transaction? I'd implemented this with the intent that all files were run in a single transaction,
butit's at least a bit ambiguous, and should probably be documented at the very least.
2) I had a question (expressed in the comments) about how the final error handling status should be reported/handled.
Ican see this affecting a number of things, particularly ON_ERROR_{STOP,ROLLBACK} behavior; specifically, if there was
anerror, it sounds like it should just abort processing of any other queued files/commands at this point (in the case
ofON_ERROR_STOP, at least).
3) With the switch to multiple intermixed -c and -f flags, the internal representation will essentially have to change
toa queue of structs; I think in that case, we'd want to modify the current .psqlrc handling to push a struct
representingthe .psqlrc file at the front of the queue, depending on the code paths that currently set this up. Are
thereany gotchas to this approach? (I'm looking essentially for odd code paths where say .psqlrc was not loaded
before,but now would be given the proper input of -c, -f file, -f -.)
I'll look more in-depth at the posted feedback and Mark's proposed patch.
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
On Tue, Jul 20, 2010 at 10:00 AM, David Christensen <david@endpoint.com> wrote:
> Sorry for the delays in response. This is fine; I think there are some semantic questions that should still be
resolvedat this point, particularly if we're moving toward supporting multiple -c and -f lines as expressed (an idea
thatI agree would be useful). Specifically:
>
> 1) Does the -1 flag with multiple files indicate a single transaction for all commands/files or that each
command/fileis run in its own transaction? I'd implemented this with the intent that all files were run in a single
transaction,but it's at least a bit ambiguous, and should probably be documented at the very least.
I think your implementation is right. Documentation is always good.
> 2) I had a question (expressed in the comments) about how the final error handling status should be reported/handled.
Ican see this affecting a number of things, particularly ON_ERROR_{STOP,ROLLBACK} behavior; specifically, if there was
anerror, it sounds like it should just abort processing of any other queued files/commands at this point (in the case
ofON_ERROR_STOP, at least).
Right. I think it should behave much as if you concatenated the files
and then ran psql on the result. Except with better reporting of
error locations, etc.
> 3) With the switch to multiple intermixed -c and -f flags, the internal representation will essentially have to
changeto a queue of structs; I think in that case, we'd want to modify the current .psqlrc handling to push a struct
representingthe .psqlrc file at the front of the queue, depending on the code paths that currently set this up. Are
thereany gotchas to this approach? (I'm looking essentially for odd code paths where say .psqlrc was not loaded
before,but now would be given the proper input of -c, -f file, -f -.)
>
> I'll look more in-depth at the posted feedback and Mark's proposed patch.
Well, IIRC, one of -c and -f suppresses psqlrc, and the other does
not. This doesn't seem very consistent to me, but I'm not sure
there's much to be done about it at this point. I guess if you use
whichever one suppresses psqlrc even once, it's suppressed, and
otherwise it's not. :-(
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
On Jul 20, 2010, at 9:05 AM, Robert Haas wrote:
> On Tue, Jul 20, 2010 at 10:00 AM, David Christensen <david@endpoint.com> wrote:
>> Sorry for the delays in response. This is fine; I think there are some semantic questions that should still be
resolvedat this point, particularly if we're moving toward supporting multiple -c and -f lines as expressed (an idea
thatI agree would be useful). Specifically:
>>
>> 1) Does the -1 flag with multiple files indicate a single transaction for all commands/files or that each
command/fileis run in its own transaction? I'd implemented this with the intent that all files were run in a single
transaction,but it's at least a bit ambiguous, and should probably be documented at the very least.
>
> I think your implementation is right. Documentation is always good.
>
>> 2) I had a question (expressed in the comments) about how the final error handling status should be
reported/handled. I can see this affecting a number of things, particularly ON_ERROR_{STOP,ROLLBACK} behavior;
specifically,if there was an error, it sounds like it should just abort processing of any other queued files/commands
atthis point (in the case of ON_ERROR_STOP, at least).
>
> Right. I think it should behave much as if you concatenated the files
> and then ran psql on the result. Except with better reporting of
> error locations, etc.
>
>> 3) With the switch to multiple intermixed -c and -f flags, the internal representation will essentially have to
changeto a queue of structs; I think in that case, we'd want to modify the current .psqlrc handling to push a struct
representingthe .psqlrc file at the front of the queue, depending on the code paths that currently set this up. Are
thereany gotchas to this approach? (I'm looking essentially for odd code paths where say .psqlrc was not loaded
before,but now would be given the proper input of -c, -f file, -f -.)
>>
>> I'll look more in-depth at the posted feedback and Mark's proposed patch.
>
> Well, IIRC, one of -c and -f suppresses psqlrc, and the other does
> not. This doesn't seem very consistent to me, but I'm not sure
> there's much to be done about it at this point. I guess if you use
> whichever one suppresses psqlrc even once, it's suppressed, and
> otherwise it's not. :-(
That seems sub-optimal; I can see people wanting to use this feature to do something like:
psql -c 'set work_mem = blah' -f script.sql
and then being surprised when it works differently than just `psql -f script.sql`.
psql -c "select 'starting'" -f file1.sql -c "select 'midway'" -f file2.sql -c "select 'done!'"
Although I wonder if the general usecase for .psqlrc is just in interactive mode; i.e., hypothetically if you're
runningscripts that are sensitive to environment, you're running with -X anyway; so maybe that's not that big of a
deal,as it's kind of an implied -X with multiple -c or -f commands. And if you really wanted it, we could add a flag
toexplicitly include .psqlrc (or the user could just specify -f path/to/psqlrc).
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
On Tue, Jul 20, 2010 at 11:23 AM, David Christensen <david@endpoint.com> wrote: >> Well, IIRC, one of -c and -f suppresses psqlrc, and the other does >> not. This doesn't seem very consistent to me, but I'm not sure >> there's much to be done about it at this point. I guess if you use >> whichever one suppresses psqlrc even once, it's suppressed, and >> otherwise it's not. :-( > > That seems sub-optimal; I can see people wanting to use this feature to do something like: > > psql -c 'set work_mem = blah' -f script.sql > > and then being surprised when it works differently than just `psql -f script.sql`. I agree... but then they might also be surprised if psql -c 'something' works differently from psql -c 'something' -f /dev/null > Although I wonder if the general usecase for .psqlrc is just in interactive mode; i.e., hypothetically if you're runningscripts that are sensitive to environment, you're running with -X anyway; so maybe that's not that big of a deal,as it's kind of an implied -X with multiple -c or -f commands. And if you really wanted it, we could add a flag toexplicitly include .psqlrc (or the user could just specify -f path/to/psqlrc). It's tempting to propose making .psqlrc apply only in interactive mode, period. But that would be an incompatibility with previous releases, and I'm not sure it's the behavior we want, either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Tue, Jul 20, 2010 at 4:06 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jul 20, 2010 at 3:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> On Mon, 2010-07-19 at 23:40 -0400, Robert Haas wrote: >> >>> Since it has been over a month since this review was posted and no new >>> version of the patch has appeared, I think we should mark this patch >>> as Returned with Feedback. >> >> Mark posted a new patch 6 days ago, AFAICS. > > Hmm. I can't find it in my mail, in my archives, or linked off the > CommitFest application. Was it posted under a different subject line? > Do you have a link to the archives? My bad, I didn't want to appear to hijack the patch from David. I have added the link to the commitfest app, and I'll give it here: http://archives.postgresql.org/message-id/AANLkTikFpzrTRl6392GhatQdwlCWQTXFdSMxh5CP51iv@mail.gmail.com Regads, Mark
On Tue, Jul 20, 2010 at 12:08 PM, Mark Wong <markwkm@gmail.com> wrote: > On Tue, Jul 20, 2010 at 4:06 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, Jul 20, 2010 at 3:20 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >>> On Mon, 2010-07-19 at 23:40 -0400, Robert Haas wrote: >>> >>>> Since it has been over a month since this review was posted and no new >>>> version of the patch has appeared, I think we should mark this patch >>>> as Returned with Feedback. >>> >>> Mark posted a new patch 6 days ago, AFAICS. >> >> Hmm. I can't find it in my mail, in my archives, or linked off the >> CommitFest application. Was it posted under a different subject line? >> Do you have a link to the archives? > > My bad, I didn't want to appear to hijack the patch from David. I > have added the link to the commitfest app, and I'll give it here: > > http://archives.postgresql.org/message-id/AANLkTikFpzrTRl6392GhatQdwlCWQTXFdSMxh5CP51iv@mail.gmail.com Oh, cool, I missed that, obviously. So I guess we need someone to review that version, then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Excerpts from Robert Haas's message of mar jul 20 11:48:29 -0400 2010: > > That seems sub-optimal; I can see people wanting to use this feature to do something like: > > > > psql -c 'set work_mem = blah' -f script.sql > > > > and then being surprised when it works differently than just `psql -f script.sql`. > > I agree... but then they might also be surprised if psql -c > 'something' works differently from psql -c 'something' -f /dev/null I think we should just make sure -X works, and have .psqlrc be read when it's not specified regardless of -f and -c switches. Otherwise it's just plain confusing.
On Jul 20, 2010, at 2:18 PM, Alvaro Herrera wrote: > Excerpts from Robert Haas's message of mar jul 20 11:48:29 -0400 2010: > >>> That seems sub-optimal; I can see people wanting to use this feature to do something like: >>> >>> psql -c 'set work_mem = blah' -f script.sql >>> >>> and then being surprised when it works differently than just `psql -f script.sql`. >> >> I agree... but then they might also be surprised if psql -c >> 'something' works differently from psql -c 'something' -f /dev/null > > I think we should just make sure -X works, and have .psqlrc be read when > it's not specified regardless of -f and -c switches. > > Otherwise it's just plain confusing. +1. Regards, David -- David Christensen End Point Corporation david@endpoint.com
On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: > It's tempting to propose making .psqlrc apply only in interactive > mode, period. But that would be an incompatibility with previous > releases, and I'm not sure it's the behavior we want, either. What is a use case for having .psqlrc be read in noninteractive use?
On Wed, Jul 21, 2010 at 10:24 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: >> It's tempting to propose making .psqlrc apply only in interactive >> mode, period. But that would be an incompatibility with previous >> releases, and I'm not sure it's the behavior we want, either. > > What is a use case for having .psqlrc be read in noninteractive use? Well, for example, if I hate the new ASCII format with a fiery passion that can never be quenched (and, by the way, I do), then I'd like this to apply: \pset linestyle old-ascii Even when I do this: psql -c '...whatever...' -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Jul 21, 2010, at 9:42 AM, Robert Haas wrote: > On Wed, Jul 21, 2010 at 10:24 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: >>> It's tempting to propose making .psqlrc apply only in interactive >>> mode, period. But that would be an incompatibility with previous >>> releases, and I'm not sure it's the behavior we want, either. >> >> What is a use case for having .psqlrc be read in noninteractive use? > > Well, for example, if I hate the new ASCII format with a fiery passion > that can never be quenched (and, by the way, I do), then I'd like this > to apply: > > \pset linestyle old-ascii > > Even when I do this: > > psql -c '...whatever...' Well, tossing out two possible solutions: 1) .psqlrc + .psql_profile (kinda like how bash separates out the interactive/non-interactive parts). Kinda yucky, but it'sa working solution. 2) have a flag which explicitly includes the psqlrc file in non-interactive use (perhaps if -x is available, use it for theanalogue to -X). Regards, David -- David Christensen End Point Corporation david@endpoint.com
On Wed, Jul 21, 2010 at 11:31 AM, David Christensen <david@endpoint.com> wrote: > > On Jul 21, 2010, at 9:42 AM, Robert Haas wrote: > >> On Wed, Jul 21, 2010 at 10:24 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>> On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: >>>> It's tempting to propose making .psqlrc apply only in interactive >>>> mode, period. But that would be an incompatibility with previous >>>> releases, and I'm not sure it's the behavior we want, either. >>> >>> What is a use case for having .psqlrc be read in noninteractive use? >> >> Well, for example, if I hate the new ASCII format with a fiery passion >> that can never be quenched (and, by the way, I do), then I'd like this >> to apply: >> >> \pset linestyle old-ascii >> >> Even when I do this: >> >> psql -c '...whatever...' > > > Well, tossing out two possible solutions: > > 1) .psqlrc + .psql_profile (kinda like how bash separates out the interactive/non-interactive parts). Kinda yucky, butit's a working solution. > > 2) have a flag which explicitly includes the psqlrc file in non-interactive use (perhaps if -x is available, use it forthe analogue to -X). Hmm. Well, that still doesn't solve the problem that -c and -f do different things with respect to psqlrc, does it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
On Wed, 2010-07-21 at 17:24 +0300, Peter Eisentraut wrote: > On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: > > It's tempting to propose making .psqlrc apply only in interactive > > mode, period. But that would be an incompatibility with previous > > releases, and I'm not sure it's the behavior we want, either. > > What is a use case for having .psqlrc be read in noninteractive use? Changing the historical defaults, such as error/exit behaviour, ensuring timing is on etc.. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Development, 24x7 Support, Training and Services
Excerpts from Peter Eisentraut's message of mié jul 21 10:24:26 -0400 2010: > On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: > > It's tempting to propose making .psqlrc apply only in interactive > > mode, period. But that would be an incompatibility with previous > > releases, and I'm not sure it's the behavior we want, either. > > What is a use case for having .psqlrc be read in noninteractive use? Even if there weren't one, why does it get applied to -f but not -c? They're both noninteractive.
On Jul 21, 2010, at 12:31 PM, Alvaro Herrera wrote:
> Excerpts from Peter Eisentraut's message of mié jul 21 10:24:26 -0400 2010:
>> On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote:
>>> It's tempting to propose making .psqlrc apply only in interactive
>>> mode, period. But that would be an incompatibility with previous
>>> releases, and I'm not sure it's the behavior we want, either.
>>
>> What is a use case for having .psqlrc be read in noninteractive use?
>
> Even if there weren't one, why does it get applied to -f but not -c?
> They're both noninteractive.
So not to let the thread drop, it appears that we're faced with the following situation:
1) The current behavior is inconsistent with the psqlrc handling of -c and -f.
2) The current behavior is still historical and we presumably want to maintain it.
I'm not sure of the cases where we're willing to break backwards compatibility, but I do know that it's not done
lightly. So perhaps for this specific patch, we'd need/want to punt supporting both -c's in conjunction with -f's, at
leastuntil we can resolve some of the ambiguities in what the actual behavior should be in each of these cases. This
couldstill be a followup patch for 9.1, if we get these issues resolved.
And as long as we're redesigning the bike shed, I think a better use case for supporting multiple sql files would be to
supportthem in such a way that you wouldn't need to provide explicit -f flags for each. Many programs utilize the '--'
tokenfor an "end of options" flag, with the rest of the arguments then becoming something special, such as filenames.
Sowhat about adding the interpretation that anything after '--' is interpreted as a filename? That will allow the use
ofshell wildcards to specify multiple files, and thus allow something like:
$ psql -U myuser mydatabase -- *.sql $ psql -- {pre-,,post-}migration.sql
while still being unambiguous with the current convention of having an unspecified argument be interpreted as a
databasename. This would make it possible to actually specify/use multiple files in a fashion that people are used to
doing,as opposed to having to explicitly type things out or do contortions will shell substitutions, etc.
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
On Wed, Jul 21, 2010 at 11:06 PM, David Christensen <david@endpoint.com> wrote: > > On Jul 21, 2010, at 12:31 PM, Alvaro Herrera wrote: > >> Excerpts from Peter Eisentraut's message of mié jul 21 10:24:26 -0400 2010: >>> On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: >>>> It's tempting to propose making .psqlrc apply only in interactive >>>> mode, period. But that would be an incompatibility with previous >>>> releases, and I'm not sure it's the behavior we want, either. >>> >>> What is a use case for having .psqlrc be read in noninteractive use? >> >> Even if there weren't one, why does it get applied to -f but not -c? >> They're both noninteractive. > > > So not to let the thread drop, it appears that we're faced with the following situation: Hmm. I thought we almost had consensus on changing the historical behavior of -c. If we do that, this all gets much simpler. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Kevin Grittner wrote: > > We should be giving authors as much leeway as possible, or they > > may not come back. > > One phenomenon I've noticed is that sometimes a patch is submitted > because an end user has solved their own problem for themselves, but > wishes to share the solution with the community. They're not always > motivated to go to the lengths required to polish it up to the > standard required for inclusion in core. In such cases, unless > someone with the time to do so finds it interesting enough to pick > up, it is just going to drop. I hope such authors feel comfortable > submitting their next effort, as it might be something which > interests a larger audience than the previous effort. We should do > what we can to ensure that they understand the dynamics of that. This brings up the larger issue of whether incomplete/unapplied patches are recorded on the TODO list or just ignored. We never really came up with a plan for that. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Simon Riggs wrote: > On Tue, 2010-07-20 at 09:05 -0400, Robert Haas wrote: > > On Tue, Jul 20, 2010 at 8:21 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > > > On Tue, 2010-07-20 at 07:49 -0400, Robert Haas wrote: > > >> A further point is that it's very difficult to > > >> keep track of progress if the CF page reflects a whole bunch of > > >> supposedly "Waiting on Author" patches that are really quite > > >> thoroughly dead. > > > > > > True, but the point under discussion is what to do if no reply is > > > received from an author. That is something entirely different from a > > > patch hitting a brick wall. > > > > > > We gain nothing by moving early on author-delay situations, so I suggest > > > we don't. > > > > No, we gain something quite specific and tangible, namely, the > > expectation that patch authors will stay on top of their patches if > > they want them reviewed by the community. If that expectation doesn't > > seem important to you, feel free to try running a CommitFest without > > it. If you can make it work, I'll happily sign on. > > I don't think so. We can assume people wrote a patch because they want > it included in Postgres. Bumping them doesn't help them or us, since > there is always an issue other than wish-to-complete. Not everybody is > able to commit time in the way we do and we should respect that better. > > Authors frequently have to wait a long time for a review; why should > reviewers not be as patient as authors must be? > > We should be giving authors as much leeway as possible, or they may not > come back. By marking patches as 'returned with feedback' long before the end of the commit-fest, we show feedback of how close we are to completing the commit-fest. If we keep patches in limbo status, it is unclear how close we are to CF completion. And, of course, the author can reactivate the patch just by replying. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Robert Haas wrote: > On Wed, Jul 21, 2010 at 11:06 PM, David Christensen <david@endpoint.com> wrote: > > > > On Jul 21, 2010, at 12:31 PM, Alvaro Herrera wrote: > > > >> Excerpts from Peter Eisentraut's message of mi? jul 21 10:24:26 -0400 2010: > >>> On tis, 2010-07-20 at 11:48 -0400, Robert Haas wrote: > >>>> It's tempting to propose making .psqlrc apply only in interactive > >>>> mode, period. ?But that would be an incompatibility with previous > >>>> releases, and I'm not sure it's the behavior we want, either. > >>> > >>> What is a use case for having .psqlrc be read in noninteractive use? > >> > >> Even if there weren't one, why does it get applied to -f but not -c? > >> They're both noninteractive. > > > > > > So not to let the thread drop, it appears that we're faced with the following situation: > > Hmm. I thought we almost had consensus on changing the historical > behavior of -c. If we do that, this all gets much simpler. Added to TODO: Consider having psql -c read .psqlrc, for consistency psql -f already reads .psqlrc -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +