Re: 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/

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(at)endpoint(dot)com

2010/3/5 David Christensen <david(at)endpoint(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

2010/3/5 Magnus Hagander <magnus(at)hagander(dot)net>:
> 2010/3/5 David Christensen <david(at)endpoint(dot)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/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2010/3/5 David Christensen <david(at)endpoint(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2010/3/5 David Christensen <david(at)endpoint(dot)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 Hagander
Me: 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(at)gmx(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Peter Eisentraut <peter_e(at)gmx(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2010/3/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2010/3/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> 2010/3/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us>:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> 2010/3/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 Hagander
Me: 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(at)momjian(dot)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(at)hagander(dot)net> writes:
>> 2010/3/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)endpoint(dot)com

On Mon, Mar 8, 2010 at 1:39 AM, David Christensen <david(at)endpoint(dot)com> wrote:
>
> On Mar 7, 2010, at 9:22 AM, Tom Lane wrote:
>
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>
>>> 2010/3/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 mention that you can use multiple -f
switches; do we want it to? also, no doc update was included in the
patch.

==Usability review==
Read what the patch is supposed to do, and consider:
Does the patch actually implement that?
yes
Do we want that?
sure!
Do we already have it?
no
Does it follow SQL spec, or the community-agreed behavior?
NA
Does it include pg_dump support (if applicable)?
NA
Are there dangers?
- subject to the usual Dumb Mistakes (see "have all the bases
been covered") Have all the bases been covered?
Scenarios we came up with:
- how does it handle wildcards, eg psql -f *.sql?
Does not - this is a shell issue. psql is designed to
take the database as the last argument; giving the -f option a
wildcard expands the list, but does not assign multiple -f
switches...so if you name one of your files the same as one of your
databases, you could accidentally run updates you don't want to do.
This is a known feature of psql, and has already bitten these reviewers
in the butt on other occasions, so nothing to worry about here.
- how does it handle the lack of a file?
as expected:
gabrielle(at)princess~/tmp/bin/:::--> ./psql -f
./psql: option requires an argument -- 'f'
Try "psql --help" for more information.
- how does it handle a non-existent file?
as expected:
gabrielle(at)princess~/tmp/bin/:::--> ./psql -f beer.sql
beer.sql: No such file or directory
- how does it handle files that don't contain valid sql?
as expected, logs an error & carries on with the next
file.
==Feature test==
Apply the patch, compile it and test:
Does the feature work as advertised?
- Yes; and BEGIN-COMMIT statements within the files cause
warnings when the command is wrapped in a transaction with the -1
switch (as specified in the patch submission). Are there corner cases
the author has failed to consider?
- none that we can think of
Are there any assertion failures or crashes?
- Mark got it to segfault due to bug in logic for allocating
pointers for filenames. It appears the order of operations between '!'
and '%' was not as intended, thus realloc() is never called and a seg
fault may occur after 16 files are passed. There are a few ways to
fix it, the one we played with to make minimum changes to the patch is
to change:

if (!options->nm_files % FILE_ALLOC_BLOCKS)

to

if (options->num_files > 1 && !((options->num_files - 1) % FILE_ALLOC_BLOCKS))

==Performance review==
Does the patch slow down simple tests?
- not that we can tell.
If it claims to improve performance, does it?
N/A
Does it slow down other things?
- not that we can tell.

==Coding review==
Read the changes to the code in detail and consider:
Does it follow the project coding guidelines?
- unnecessary whitespace on line 251?
- inconsistent spacing between operators
Are there portability issues?
- untested
Will it work on Windows/BSD etc?
- untested
Are the comments sufficient and accurate?
Does it do what it says, correctly?
- yes
Does it produce compiler warnings?
- no
Can you make it crash?
- See above about the segfault.

==Architecture review==
Consider the changes to the code in the context of the project as a
whole: Is everything done in a way that fits together coherently with
other features/modules?
- yes
Are there interdependencies that can cause problems?
- not that we are aware of

==Review review==
Did the reviewer cover all the things that kind of reviewer is supposed
to do?
- AFAWK.

Regards,
Mark

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(at)commandprompt(dot)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.com
PostgreSQL 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(at)2ndquadrant(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)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(at)gmail(dot)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(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)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.com
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)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 patch adds BEGIN and COMMIT statements outside the call to process_file() in src/bin/psql/command.c. Here lies the previous logic for handling single transaction. Since it remains, it appears that BEGIN can be issued twice before any statements are executed if the single transaction switch is used. Plus there are other a couple of places that call this particular 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(at)gmail(dot)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(at)2ndquadrant(dot)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 patch adds BEGIN and COMMIT statements outside the call to process_file() in src/bin/psql/command.c. Here lies the previous logic for handling single transaction. Since it remains, it appears that BEGIN can be issued twice before any statements are executed if the single transaction switch is used. Plus there are other a couple of places that call this particular 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 to false in process_file(). I think that turns the single transaction handling in process_file() into dead code with the patch like this.

Regards,
Mark

On Wed, Jun 23, 2010 at 9:22 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 23, 2010 at 9:17 AM, gabrielle <gorthx(at)gmail(dot)com> wrote:
>> On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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.com
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jul 20, 2010 at 3:20 AM, Simon Riggs <simon(at)2ndquadrant(dot)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.com
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jul 20, 2010 at 7:41 AM, Simon Riggs <simon(at)2ndquadrant(dot)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.com
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jul 20, 2010 at 8:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)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.com
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)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(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> wrote:
>> On Wed, Jun 23, 2010 at 9:17 AM, gabrielle <gorthx(at)gmail(dot)com> wrote:
>>> On Mon, Jun 21, 2010 at 6:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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 resolved at this point, particularly if we're moving toward supporting multiple -c and -f lines as expressed (an idea that I 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 is 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.

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 at this point (in the case of ON_ERROR_STOP, at least).

3) With the switch to multiple intermixed -c and -f flags, the internal representation will essentially have to change to a queue of structs; I think in that case, we'd want to modify the current .psqlrc handling to push a struct representing the .psqlrc file at the front of the queue, depending on the code paths that currently set this up. Are there any 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(at)endpoint(dot)com

On Tue, Jul 20, 2010 at 10:00 AM, David Christensen <david(at)endpoint(dot)com> wrote:
> Sorry for the delays in response.  This is fine; I think there are some semantic questions that should still be resolved at this point, particularly if we're moving toward supporting multiple -c and -f lines as expressed (an idea that I 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 is 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 at this 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 change to a queue of structs; I think in that case, we'd want to modify the current .psqlrc handling to push a struct representing the .psqlrc file at the front of the queue, depending on the code paths that currently set this up.  Are there any 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(at)endpoint(dot)com> wrote:
>> Sorry for the delays in response. This is fine; I think there are some semantic questions that should still be resolved at this point, particularly if we're moving toward supporting multiple -c and -f lines as expressed (an idea that I 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 is 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 at this 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 change to a queue of structs; I think in that case, we'd want to modify the current .psqlrc handling to push a struct representing the .psqlrc file at the front of the queue, depending on the code paths that currently set this up. Are there any 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 running scripts 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 to explicitly include .psqlrc (or the user could just specify -f path/to/psqlrc).

Regards,

David
--
David Christensen
End Point Corporation
david(at)endpoint(dot)com

On Tue, Jul 20, 2010 at 11:23 AM, David Christensen <david(at)endpoint(dot)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 running scripts 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 to explicitly 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(at)gmail(dot)com> wrote:
> On Tue, Jul 20, 2010 at 3:20 AM, Simon Riggs <simon(at)2ndquadrant(dot)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(at)gmail(dot)com> wrote:
> On Tue, Jul 20, 2010 at 4:06 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Jul 20, 2010 at 3:20 AM, Simon Riggs <simon(at)2ndquadrant(dot)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(at)endpoint(dot)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(at)gmx(dot)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(at)gmx(dot)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'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 for the analogue to -X).

Regards,

David
--
David Christensen
End Point Corporation
david(at)endpoint(dot)com

On Wed, Jul 21, 2010 at 11:31 AM, David Christensen <david(at)endpoint(dot)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(at)gmx(dot)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'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 for the 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.com
PostgreSQL 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 least until we can resolve some of the ambiguities in what the actual behavior should be in each of these cases. This could still 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 support them in such a way that you wouldn't need to provide explicit -f flags for each. Many programs utilize the '--' token for an "end of options" flag, with the rest of the arguments then becoming something special, such as filenames. So what about adding the interpretation that anything after '--' is interpreted as a filename? That will allow the use of shell 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 database name. 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(at)endpoint(dot)com

On Wed, Jul 21, 2010 at 11:06 PM, David Christensen <david(at)endpoint(dot)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(at)momjian(dot)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(at)2ndquadrant(dot)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(at)momjian(dot)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(at)endpoint(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +