Explicit psqlrc

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 Hagander
Me: 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/

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 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@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 Hagander
Me: 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 Hagander
Me: 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 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@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

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 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@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 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@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.com
PostgreSQL Development, 24x7 Support, Training and Services