Re: review: psql: edit function, show function commands patch - Mailing list pgsql-hackers

Hello

I am sending a actualised patch.

I understand to your criticism about line numbering. I have to agree.
With line numbering the patch is longer. I have a one significant
reason for it. There are not conformance between line numbers of
CREATE FUNCTION statement and line numbers of function's body. Raise
exception, syntactic errors use a function body line numbers. But
users doesn't see alone function's body. He see a CREATE FUNCTION
statement. What more - and this depend on programmer style sometimes
is necessary to correct line number with -1. Now I have enough
knowledges of plpgsql, and I am possible to see a problematic row, but
it little bit hard task for beginners. You can see.

****  CREATE OR REPLACE FUNCTION public.foo()
****   RETURNS integer
****   LANGUAGE plpgsql
****  AS $function$
   1  begin
   2    return 10/0;
   3  end;
****  $function$

postgres=# select foo();
ERROR:  division by zero
CONTEXT:  SQL statement "SELECT 10/0"
PL/pgSQL function "foo" line 2 at RETURN
postgres=#

****  CREATE OR REPLACE FUNCTION public.foo()
****   RETURNS integer
****   LANGUAGE plpgsql
   1  AS $function$ begin
   2  return 10/0;
   3  end;
****  $function$

postgres=# select foo();
ERROR:  division by zero
CONTEXT:  SQL statement "SELECT 10/0"
PL/pgSQL function "foo" line 2 at RETURN

This is very trivial example - for more complex functions, the correct
line numbering is more useful.

2010/7/16 Jan Urbański <wulczer@wulczer.org>:
> Hi,
>
> here's a review of the \sf and \ef [num] patch from
> http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19de29@mail.gmail.com
>
> == Formatting ==
>
> The patch has some small tabs/spaces and whitespace  issues and it applies
> with some offsets, I ran pgindent and rebased against HEAD, attaching the
> resulting patch for your convenience.
>
> == Functionality ==
>
> The patch adds the following features:
>  * \e file.txt num  ->  starts a editor for the current query buffer and
> puts the cursor on the [num] line
>  * \ef func num -> starts a editor for a function and puts the cursor on the
> [num] line
>  * \sf func -> shows a full CREATE FUNCTION statement for the function
>  * \sf+ func -> the same, but with line numbers
>  * \sf[+] func num -> the same, but only from line num onward
>
> It only touches psql, so no performance or backend stability worries.
>
> In my humble opinion, only the \sf[+] is interesting, because it gives you a
> copy/pasteable version of the function definition without opening up an
> editor, and I can find that useful (OTOH: you can set PSQL_EDITOR to cat and
> get the same effect with \ef... ok, just joking). Line numbers are an extra
> touch, personally it does not thrill me too much, but I've nothing against
> it.
>
> The number variants of \e and \ef work by simply executing $EDITOR +num
> file. I tried with some editors that came to my mind, and not all of them
> support it (most do, though):
>
>  * emacs and emacsclient work
>  * vi works
>  * nano works
>  * pico works
>  * mcedit works
>  * kwrite does not work
>  * kedit does not work
>
> not sure what other people (or for instance Windows people) use. Apart from
> no universal support from editors, it does not save that many keystrokes -
> at most a couple. In the end you can usually easily jump to the line you
> want once you are inside your dream editor.

I found, so there are a few editor for ms win with support for direct
line navigation. There isn't any standart. Next I tested kwrite and
KDE. There is usual a parameter --line. So you can you use a system
variable PSQL_NAVIGATION_COMMAND - for example - for KDE

PSQL_NAVIGATION_COMMAND="--line "

default is +n

>
> My recommendation would be to only integrate the \sf[+] part of the patch,
> which will have the additional benefit of making it much smaller and cleaner
> (will avoid the grotty splitting of the number from the function name, for
> instance). But I'm just another user out there, maybe others will find uses
> for the other cases.
>

I disagree. You cannot use a text editor command, because SQL
linenumbers are not equal to body line numbers.

> I would personally not add the leading and trailing newlines to \sf output,
> but that's a question of taste.
>
> Docs could use some small grammar fixes, but other than that they're fine.
>
> == Code ==
>
> In \sf code there just a strncmp, so this works:
> \sfblablabla funcname
>

fixed

> The error for an empty \sf is not great, it should probably look more like
> \sf: missing required argument
> following the examples of \pset, \copy or \prompt.
>
> Why is lnptr always being passed as a pointer? Looks like a unnecessary
> complication and one more variable to care about. Can't we just pass lineno?

fixed

I removed redundant code and appended a more comments/

Regards

Pavel Stehule

>
> == End ==
>
> Cheers,
> Jan
>