Thread: [PATCH] "\ef " in psql

Refactoring pg_dump was more work than I had time to do right now, and I
wanted \ef to work, so I hacked up the attached (by copying dumpFunc and
its dependencies to src/bin/psql/dumpfunc.[ch]).

-- ams

Abhijit Menon-Sen <ams@oryx.com> writes:
> Refactoring pg_dump was more work than I had time to do right now, and I
> wanted \ef to work, so I hacked up the attached (by copying dumpFunc and
> its dependencies to src/bin/psql/dumpfunc.[ch]).

I doubt we'd consider accepting a patch done this way.
        regards, tom lane

At 2008-07-15 10:33:02 -0400, tgl@sss.pgh.pa.us wrote:
>
> I doubt we'd consider accepting a patch done this way.

Yes, it's much too ugly to live. I was posting it only for the record,
I should have made that clear.

-- ams

At 2008-07-15 20:28:39 +0530, ams@oryx.com wrote:
>
> > I doubt we'd consider accepting a patch done this way.
> 
> Yes, it's much too ugly to live.

Though I must say it would have been even MORE horrible to copy all this
code into the backend to make pg_get_functiondef(), notwithstanding the
extra utility of a generally-callable function.

But what I'm wondering, since Gavin said he once had a working version
of this patch (i.e. \ef) which he somehow lost, is how he approached the
problem at the time.

Gavin? Do you remember? Was it horrible?

-- ams

Abhijit Menon-Sen <ams@oryx.com> writes:
> Though I must say it would have been even MORE horrible to copy all this
> code into the backend to make pg_get_functiondef(), notwithstanding the
> extra utility of a generally-callable function.

FWIW, I just found myself forced to invent pg_get_function_arguments()
and pg_get_function_result(), because the TABLE function patch has
pushed the complexity of printing function argument and result types
well beyond the bounds of sanity.  (Pavel had hacked up pg_dump and
ignored psql's \df ...)  It wouldn't take a whole lot to convince me
that a pg_get_functiondef would be useful, although I don't foresee
either of those applications wanting to use it because of their
backward-compatibility constraints.
        regards, tom lane

At 2008-07-17 18:28:19 -0400, tgl@sss.pgh.pa.us wrote:
>
> It wouldn't take a whole lot to convince me that a pg_get_functiondef
> would be useful, although I don't foresee either of those applications
> wanting to use it because of their backward-compatibility constraints.

If the code lives in psql (as with my patch), then it has some chance of
working with older servers, but if you're happy with pg_get_functiondef,
then I'm happy enough to use it to get \ef working. I agree that pg_dump
wouldn't want to use it, of course, but I guess it doesn't matter very
much if \ef doesn't work on older servers.

What would the function return? "CREATE OR REPLACE FUNCTION ..."? Would
that be good enough for everyone who might want to call it?

(BTW, psql from 8.3 is already somewhat broken with 8.1:

archiveopteryx=> \d access_keys
ERROR:  column i.indisvalid does not exist

And 8.2 as well:

archiveopteryx=> \d access_keys
ERROR:  column t.tgconstraint does not exist
LINE 3: WHERE t.tgrelid = '16847' AND t.tgconstraint = 0                                     ^
Oh, I see they've both been fixed in CVS. Sorry for the noise.)

-- ams

Abhijit Menon-Sen <ams@oryx.com> writes:
> At 2008-07-17 18:28:19 -0400, tgl@sss.pgh.pa.us wrote:
>> It wouldn't take a whole lot to convince me that a pg_get_functiondef
>> would be useful, although I don't foresee either of those applications
>> wanting to use it because of their backward-compatibility constraints.

> What would the function return? "CREATE OR REPLACE FUNCTION ..."? Would
> that be good enough for everyone who might want to call it?

I think I'd go with CREATE FUNCTION for simplicity.  It would be easy
enough for something like \ef to splice in OR REPLACE before shipping
the command back to the server.
        regards, tom lane

On 7/23/08, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Abhijit Menon-Sen <ams@oryx.com> writes:
> > At 2008-07-17 18:28:19 -0400, tgl@sss.pgh.pa.us wrote:
>  >> It wouldn't take a whole lot to convince me that a pg_get_functiondef
>  >> would be useful, although I don't foresee either of those applications
>  >> wanting to use it because of their backward-compatibility constraints.
>
> > What would the function return? "CREATE OR REPLACE FUNCTION ..."? Would
>  > that be good enough for everyone who might want to call it?
>
> I think I'd go with CREATE FUNCTION for simplicity.  It would be easy
>  enough for something like \ef to splice in OR REPLACE before shipping
>  the command back to the server.

Please make it use full qualified names (schema.name) for both
function name and result types.  Current search_path juggling
the pg_dump does is major PITA.

-- 
marko

"Marko Kreen" <markokr@gmail.com> writes:
>> [ re pg_get_functiondef ]

> Please make it use full qualified names (schema.name) for both
> function name and result types.  Current search_path juggling
> the pg_dump does is major PITA.

Qualifying the function name seems like a good idea, but I'd advise
against tinkering with the datatype references.  It'll be hard to
do correctly and it will make things very substantially uglier.
Do you really want to show, eg, "pg_catalog.int4" rather than "integer"?

If you leave the backend code do what it wants to do here, the only
way that there would be a problem is if someone changed their
search_path in between pg_get_functiondef and trying to re-load the
function definition.  Which certainly ain't gonna happen for \ef,
and it seems a bit implausible for any other use-case either.
        regards, tom lane

At 2008-07-23 10:50:03 -0400, tgl@sss.pgh.pa.us wrote:
>
> > What would the function return? "CREATE OR REPLACE FUNCTION ..."?
> 
> I think I'd go with CREATE FUNCTION for simplicity.

OK, I have a mostly working pg_get_functiondef now, and some questions
about the remaining pieces:

1. Why is "prorows" a float4? Should I print it that way, i.e. "%f"?

2. Can I print the contents of "proconfig" as just "SET %s"? It seems  to work for the variables I've tried (e.g.
"DateStyle=iso"),but I  wonder if they'll always be quoted correctly (i.e., if the "split  on '='" thing pg_dump does
isnecessary for an 8.4 function).
 

3. Is there a function I can use from ruleutils.c to do dollar quoting  of prosrc/probin? If not, and I have to write
one,where should it  live?
 

4. What exactly is probin? Do I need to worry about it at all?

Thanks.

-- ams

+1 for CREATE OR REPLACE

...Robert

On Tue, Jul 29, 2008 at 02:21:18PM -0400, Robert Haas wrote:
> +1 for CREATE OR REPLACE
> 
> ...Robert

+1 for CREATE OR REPLACE from me, too :)

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

At 2008-07-29 15:42:27 +0530, ams@oryx.com wrote:
>
> OK, I have a mostly working pg_get_functiondef now, and some
> questions about the remaining pieces:

While I look for answers to those questions, here's the patch as it
stands now, in case anyone feels like reading through it.

-- ams

I have attached two patches:

- funcdef.diff implements pg_get_functiondef()
- edit.diff implements "\ef function" in psql based on (1).

Comments appreciated.

-- ams

On Jul 31, 2008, at 00:07, Abhijit Menon-Sen wrote:

> I have attached two patches:
>
> - funcdef.diff implements pg_get_functiondef()
> - edit.diff implements "\ef function" in psql based on (1).
>
> Comments appreciated.

+1

I like! The ability to easily edit a function on the fly in psql will  
be very welcome to DBAs I know. And I like the pg_get_functiondef()  
function, too, a that will simplify editing existing functions in  
other admin apps, like pgAdmin.

I'm starting to get really excited for 8.4. I can haz cheezburger?  
Oops, I mean, when does it ship? ;-P

Thanks,

David

On Mon, Aug 04, 2008 at 10:31:10AM -0700, David Wheeler wrote:
> On Jul 31, 2008, at 00:07, Abhijit Menon-Sen wrote:
>
>> I have attached two patches:
>>
>> - funcdef.diff implements pg_get_functiondef()
>> - edit.diff implements "\ef function" in psql based on (1).
>>
>> Comments appreciated.
>
> +1
>
> I like! The ability to easily edit a function on the fly in psql
> will be very welcome to DBAs I know. And I like the
> pg_get_functiondef()  function, too, a that will simplify editing
> existing functions in other admin apps, like pgAdmin.
>
> I'm starting to get really excited for 8.4. I can haz cheezburger?

You do understand you've just kicked off a discussion of shipping
PL/LOLCODE by default.

> Oops, I mean, when does it ship? ;-P

Christmas ;)

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Abhijit Menon-Sen <ams@oryx.com> writes:
> I have attached two patches:
> - funcdef.diff implements pg_get_functiondef()
> - edit.diff implements "\ef function" in psql based on (1).

I've applied this with some corrections (mostly around proper quoting)
and some outright editorialization:

* the psql command seemed to have some ideas about supplying a blank
CREATE OR REPLACE FUNCTION command for a nonexistent function, but this
didn't actually work.  In any case it seemed poorly thought out, because
you'd really need to pay some attention to *why* the regproc/regprocedure
lookup failed.  I just ripped it out for the moment.  I'm not averse to
the concept, if you can get it implemented properly.

* the way you had it set up, the CREATE OR REPLACE FUNCTION command
would be sent to the backend instantaneously upon return from the
editor, with no opportunity for the user to decide he didn't want his
changes applied.  This seemed unacceptably dangerous to me.  I changed
the exit code to PSQL_CMD_NEWEDIT instead of PSQL_CMD_SEND, which causes
the command to wait in the query buffer.  Unfortunately there's no
visual indication of that, other than a small change in the prompt
status.  It'd likely be better if we could get libreadline to redisplay
the query buffer contents --- anyone have an idea how to do that?
(I have some vague recollection that \e used to work that way, though
it definitely fails to do so now.)
        regards, tom lane

On Sat, Sep 6, 2008 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> * the way you had it set up, the CREATE OR REPLACE FUNCTION command
> would be sent to the backend instantaneously upon return from the
> editor, with no opportunity for the user to decide he didn't want his
> changes applied.  This seemed unacceptably dangerous to me.  I changed
> the exit code to PSQL_CMD_NEWEDIT instead of PSQL_CMD_SEND, which causes
> the command to wait in the query buffer.

The principle of least astonishment suggests that \ef should behave in
the same way as \e.

With \e (which I use a lot), the command(s) are immediately executed
by the backend as soon as you write and exit from the editor.  I don't
find that dangerous, and anyone who uses \e will already be very much
accustomed to it.  If \ef did something different, it would just be
weird.

If you're not sure you want to execute the contents of your \e editor
session after all, you can always delete the semicolon, or everything
in the file, before quitting.

Cheers,
BJ

"Brendan Jurd" <direvus@gmail.com> writes:
> On Sat, Sep 6, 2008 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... I changed
>> the exit code to PSQL_CMD_NEWEDIT instead of PSQL_CMD_SEND, which causes
>> the command to wait in the query buffer.

> The principle of least astonishment suggests that \ef should behave in
> the same way as \e.

Quite.
        regards, tom lane

I wrote:
> * the psql command seemed to have some ideas about supplying a blank
> CREATE OR REPLACE FUNCTION command for a nonexistent function, but this
> didn't actually work.  In any case it seemed poorly thought out, because
> you'd really need to pay some attention to *why* the regproc/regprocedure
> lookup failed.  I just ripped it out for the moment.  I'm not averse to
> the concept, if you can get it implemented properly.

While I was out at dinner, the obvious solution presented itself: define
\ef with no argument as being the command that presents an empty CREATE
FUNCTION command template to fill in.  This isn't any more or less
typing than where I think you were going, and it eliminates all the
ambiguity about whether you meant to type a nonexistent function name
or just mistyped.
        regards, tom lane

I wrote:
> ... define
> \ef with no argument as being the command that presents an empty CREATE
> FUNCTION command template to fill in.

No complaints?  I'll go make that happen.

What about the general issue that neither \e nor \ef leave you with a
presentation of what's in the query buffer?  I haven't studied readline
enough to know if that's fixable.
        regards, tom lane

At 2008-09-06 14:58:25 -0400, tgl@sss.pgh.pa.us wrote:
>
> I wrote:
> > ... define
> > \ef with no argument as being the command that presents an empty
> > CREATE FUNCTION command template to fill in.
> 
> No complaints?  I'll go make that happen.

No complaints, it sounds fine to me.

> What about the general issue that neither \e nor \ef leave you with a
> presentation of what's in the query buffer?

I don't know how that can be fixed; but I agree with Brendan that it's
behaviour that people are used to, and that it can be left alone for
now.

-- ams

Tom Lane wrote:
> "Brendan Jurd" <direvus@gmail.com> writes:
> > On Sat, Sep 6, 2008 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> ... I changed
> >> the exit code to PSQL_CMD_NEWEDIT instead of PSQL_CMD_SEND, which causes
> >> the command to wait in the query buffer.
> 
> > The principle of least astonishment suggests that \ef should behave in
> > the same way as \e.
> 
> Quite.

So, are they consistent now or do we need another patch?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> "Brendan Jurd" <direvus@gmail.com> writes:
>>> The principle of least astonishment suggests that \ef should behave in
>>> the same way as \e.
>> 
>> Quite.

> So, are they consistent now or do we need another patch?

They are consistent, I just don't like either of them ;-)
        regards, tom lane

Abhijit Menon-Sen wrote:
> At 2008-09-06 14:58:25 -0400, tgl@sss.pgh.pa.us wrote:

> > What about the general issue that neither \e nor \ef leave you with a
> > presentation of what's in the query buffer?
> 
> I don't know how that can be fixed; but I agree with Brendan that it's
> behaviour that people are used to, and that it can be left alone for
> now.

As far as it works to not execute the query when the user exits without
saving the buffer, it should be OK.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Abhijit Menon-Sen wrote:
> > At 2008-09-06 14:58:25 -0400, tgl@sss.pgh.pa.us wrote:
> 
> > > What about the general issue that neither \e nor \ef leave you with a
> > > presentation of what's in the query buffer?
> > 
> > I don't know how that can be fixed; but I agree with Brendan that it's
> > behaviour that people are used to, and that it can be left alone for
> > now.
> 
> As far as it works to not execute the query when the user exits without
> saving the buffer, it should be OK.

Well, it works like \e now, which is good.  The only complexity is that
\e works differently depending on whether you use ';' or \g, meaning:
SELECT 1;\e

will execute the buffer on exit (saved or not), while
SELECT 1\g\e

will not execute the buffer on editor exit.  Our current \ef code does
not add a trailing semicolon to the CREATE FUNCTION buffer contents so
it works like the second case, which is probably the best we are going
to do.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +