Thread

review: psql: edit function, show function commands patch

Jan Urbański <wulczer@wulczer.org> — 2010-07-16T14:29:01Z

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.

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 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

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?

== End ==

Cheers,
Jan

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-07-20T12:58:24Z

Hello

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 disagree. When I working on servers of my customers there are some
default configuration - default editor is usually vi or vim. I cannot
change my preferred editor there and \ef n - can help me very much (I
know only one command for vi - :q :)). I am looking on KDE. There is
usual parameter --line.

I propose following solution - to add a system variable
PSQL_EDITOR_GOTOLINE_COMMAND that will contains a command for editors
without general +n navigation.

so you can set a PSQL_EDITOR_GOTOLINE_COMMAND='--line %d'

and when this string will be used, when will not be empty without default.


> 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 would personally not add the leading and trailing newlines to \sf output,
> but that's a question of taste.

Maybe better is using a title - so source code will use a format like
standard result set.

I'll look on it.

>
> 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

will be fiexed

>
> 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?
>

because I would not to use a magic constant. when lnptr is NULL, then
lineno is undefined.

Thank you very much

Pavel Stehule

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

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-07-21T12:43:46Z

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
>

Re: review: psql: edit function, show function commands patch

Dimitri Fontaine <dfontaine@hi-media.com> — 2010-07-21T21:04:18Z

Pavel Stehule <pavel.stehule@gmail.com> writes:
> ****  CREATE OR REPLACE FUNCTION public.foo()
> ****   RETURNS integer
> ****   LANGUAGE plpgsql
>    1  AS $function$ begin
>    2  return 10/0;
>    3  end;
> ****  $function$
>
> This is very trivial example - for more complex functions, the correct
> line numbering is more useful.

I completely agree with this, in-functions line numbering is a
must-have. I'd like psql to handle that better.

That said, I usually edit functions in Emacs on my workstation. I did
implement a linum-mode extension to show PL/pgSQL line numbers in
addition to the buffer line numbers in emacs, but it failed to work with
this "AS $function$ begin" on the same line example. It's fixed in the
attached, should there be any users of it.

Regards,
-- 
dim

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-07-22T03:01:08Z

On Fri, Jul 16, 2010 at 10:29 AM, Jan Urbański <wulczer@wulczer.org> wrote:
> 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

FWIW, I think this is all pretty useful.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Jan Urbański <wulczer@wulczer.org> — 2010-07-22T22:56:02Z

On 21/07/10 14:43, Pavel Stehule wrote:
> Hello
> 
> I am sending a actualised patch.

Hi, thanks!

> 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.

> ****  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=#

OK, that convinced me, and also others seem to like the feature. So I'll
just make code remarks this time.

In the \e handling code, if the file name was given and there is no line
number, an uninitialised variable will be passed to do_edit. I see that
you want to avoid passing a magic number to do_edit, but I think you
should just treat anything <0 as that magic value, initialise lineno
with -1 and simplify all these nested ifs.

Typo in a comment:
when wirst row of function -> when first row of function

I think the #define for DEFAULT_BODY_SEPARATOR should either be at the
beginning of the file (and then also used in \ef handling) or just
hardcoded in both places.

Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it?

Some lines have >80 characters.

If you agree that a -1 parameter do do_edit will mean "no lineno", then
I think you can change get_lineno_for_navigation to not take a
backslashResult argument and signal errors by returning -1.

In get_lineno_for_navigation you will have to protect the large comment
block with /*------ otherwise pgindent will reflow it.

PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs.

Uff, that's all from me, sorry for bringing up all these small issues, I
hope this will go in soon!

I'm going to change it to waiting on author again, mainly because of the
uninitialised variable in \d handling, the rest are just stylistic nitpicks.

Cheers,
Jan

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-07-23T00:31:41Z

On Thu, Jul 22, 2010 at 6:56 PM, Jan Urbański <wulczer@wulczer.org> wrote:
> the rest are just stylistic nitpicks.

But, if the patch author doesn't fix them, the committer has to, so
your nitpicking is much appreciated, at least by me!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-07-23T18:55:39Z

Hello

2010/7/23 Jan Urbański <wulczer@wulczer.org>:
> On 21/07/10 14:43, Pavel Stehule wrote:
>> Hello
>>
>> I am sending a actualised patch.
>
> Hi, thanks!
>
>> 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.
>
>> ****  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=#
>
> OK, that convinced me, and also others seem to like the feature. So I'll
> just make code remarks this time.
>
>
> In the \e handling code, if the file name was given and there is no line
> number, an uninitialised variable will be passed to do_edit. I see that
> you want to avoid passing a magic number to do_edit, but I think you
> should just treat anything <0 as that magic value, initialise lineno
> with -1 and simplify all these nested ifs.
>

fixed uninitialised variable. Second is little bit different - there
is three states, not only two - lineno is undefined, lineno is wrong
and lineno is correct. I would not to ignore a wrong lineno.

> Typo in a comment:
> when wirst row of function -> when first row of function

fixed

>
> I think the #define for DEFAULT_BODY_SEPARATOR should either be at the
> beginning of the file (and then also used in \ef handling) or just
> hardcoded in both places.

this means some like only local constant - see PARAMS_ARRAY_SIZE in
same file. It is used only inside a first_row_is_empty function. It's
not used outside this function.

>
> Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it?
>

no it is useless - fixed

> Some lines have >80 characters.

there are lot of longer lines - but I can't to modify other lines only
for formating. My code has max line about 90 characters (when it
doesn't respect more common form for some parts).

>
> If you agree that a -1 parameter do do_edit will mean "no lineno", then
> I think you can change get_lineno_for_navigation to not take a
> backslashResult argument and signal errors by returning -1.

there are a too much magic constants, so I prefer a cleaner form with
backslashResult. The code isn't longer and it reacts better on wrong
entered value - negative value is nonsense for this purpose.

>
> In get_lineno_for_navigation you will have to protect the large comment
> block with /*------ otherwise pgindent will reflow it.

done

>
> PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs.
>

I changed PSQL_NAVIGATION_COMMAND to PSQL_EDITOR_NAVIGATION_OPTION and
append a few lines - as I can - some one who can speak English has to
correct it.

>
> Uff, that's all from me, sorry for bringing up all these small issues, I
> hope this will go in soon!
>

It is your job :)

> I'm going to change it to waiting on author again, mainly because of the
> uninitialised variable in \d handling, the rest are just stylistic nitpicks.
>
> Cheers,
> Jan
>

attached updated patch

Thank you very much

Pavel Stehule

Re: review: psql: edit function, show function commands patch

Jan Urbański <wulczer@wulczer.org> — 2010-07-25T15:36:16Z

On 23/07/10 20:55, Pavel Stehule wrote:
> Hello
> 
> 2010/7/23 Jan Urbański <wulczer@wulczer.org>:
>> On 21/07/10 14:43, Pavel Stehule wrote:
>>> Hello
>>>
>>> I am sending a actualised patch.

OK, thanks. This time the only thing I'm not happy about is the error
message from doing:

\ef func 0
\e /etc/passwd xxx

which gives:

line number is unacceptable

where I think it should do:

\ef: line number is unacceptable
\e: line number is unacceptable

but that's too trivial to go through another round of review and I think
the committer can fix this easily.

The documentation likely needs some spelling fixes, but I'll leave that
to a native English speaker.

I'm setting this as ready for committer.

Thanks,
Jan

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-07-25T15:42:57Z

2010/7/25 Jan Urbański <wulczer@wulczer.org>:
> On 23/07/10 20:55, Pavel Stehule wrote:
>> Hello
>>
>> 2010/7/23 Jan Urbański <wulczer@wulczer.org>:
>>> On 21/07/10 14:43, Pavel Stehule wrote:
>>>> Hello
>>>>
>>>> I am sending a actualised patch.
>
> OK, thanks. This time the only thing I'm not happy about is the error
> message from doing:
>
> \ef func 0
> \e /etc/passwd xxx
>
> which gives:
>
> line number is unacceptable
>
> where I think it should do:
>
> \ef: line number is unacceptable
> \e: line number is unacceptable
>
> but that's too trivial to go through another round of review and I think
> the committer can fix this easily.
>
> The documentation likely needs some spelling fixes, but I'll leave that
> to a native English speaker.
>
> I'm setting this as ready for committer.

Thank you very much

Pavel
>
> Thanks,
> Jan
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-01T02:10:38Z

On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> I'm setting this as ready for committer.
>
> Thank you very much

I took a look at this tonight and am a bit mystified by the following bit:

+                       /*
+                        * PL doesn't calculate first row of function's body
+                        * when first row is empty. So checks first row, and
+                        * correct lineno when it is necessary.
+                        */

Is that true of any PL, or just some particular PL?  Is the behavior
described here a bug that we should fix, or is it, for some reason,
considered correct?  Is it documented in our documentation?

The implementation of first_row_is_empty() looks pretty kludgey, too.
It seems to me that it will fail completely if the text of the
function definition happens to contain $function$.

CREATE OR REPLACE FUNCTION boom() RETURNS text LANGUAGE plpgsql AS $$
BEGIN SELECT '$function$'; END $$;

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-01T02:46:41Z

Robert Haas <robertmhaas@gmail.com> writes:
> I took a look at this tonight and am a bit mystified by the following bit:

> +                       /*
> +                        * PL doesn't calculate first row of function's body
> +                        * when first row is empty. So checks first row, and
> +                        * correct lineno when it is necessary.
> +                        */

> Is that true of any PL, or just some particular PL?

plpgsql has an old bit of logic that deliberately ignores an initial
newline in the function body:

    /*----------
     * Hack: skip any initial newline, so that in the common coding layout
     *        CREATE FUNCTION ... AS $$
     *            code body
     *        $$ LANGUAGE plpgsql;
     * we will think "line 1" is what the programmer thinks of as line 1.
     *----------
     */
    if (*cur_line_start == '\r')
        cur_line_start++;
    if (*cur_line_start == '\n')
        cur_line_start++;

None of the other standard PLs do that AFAIK.

> Is it documented in our documentation?

I don't think so.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-01T04:15:09Z

2010/8/1 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> I'm setting this as ready for committer.
>>
>> Thank you very much
>
> I took a look at this tonight and am a bit mystified by the following bit:
>
> +                       /*
> +                        * PL doesn't calculate first row of function's body
> +                        * when first row is empty. So checks first row, and
> +                        * correct lineno when it is necessary.
> +                        */
>
> Is that true of any PL, or just some particular PL?  Is the behavior
> described here a bug that we should fix, or is it, for some reason,
> considered correct?  Is it documented in our documentation?

it is primary plpgsql issue.

>
> The implementation of first_row_is_empty() looks pretty kludgey, too.
> It seems to me that it will fail completely if the text of the
> function definition happens to contain $function$.
>
> CREATE OR REPLACE FUNCTION boom() RETURNS text LANGUAGE plpgsql AS $$
> BEGIN SELECT '$function$'; END $$;
>

I can enhance algorithm on client side - but it will not be a pretty
code - it better do it on server side - for example
pg_get_formated_functiondef ...

CREATE OR REPLACE FUNCTION pg_get_formated_function_def(in oid,
linenum bool:= false, OUT funcdef text, OUT first_line_isignored bool)

this can remove any ugly code

Regards

Pavel



> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-01T12:37:23Z

On Sun, Aug 1, 2010 at 12:15 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2010/8/1 Robert Haas <robertmhaas@gmail.com>:
>> On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>>> I'm setting this as ready for committer.
>>>
>>> Thank you very much
>>
>> I took a look at this tonight and am a bit mystified by the following bit:
>>
>> +                       /*
>> +                        * PL doesn't calculate first row of function's body
>> +                        * when first row is empty. So checks first row, and
>> +                        * correct lineno when it is necessary.
>> +                        */
>>
>> Is that true of any PL, or just some particular PL?  Is the behavior
>> described here a bug that we should fix, or is it, for some reason,
>> considered correct?  Is it documented in our documentation?
>
> it is primary plpgsql issue.

Yeah, that seems like a problem.  If your editor is vi, for example,
the following are the same number of keystrokes:

\ef foo 417<enter>
and
\ef foo<enter>417G

So the major advantage of the former over the latter is that you'll
(hopefully) end up on EXACTLY the right line rather than maybe being
off by a few lines.  With the current code, that won't necessarily
happen, because PL/pgsql (and any third-party PLs based on it) use one
line-numbering convention and other PLs don't necessarily include that
hack.  Admittedly, you'll probably only be off by one line instead of
three or four, so maybe people think that's good enough, but I'm not
totally convinced.  It seems like the easiest way to fix this would be
remove the hack from PL/pgsql, but I'm not sure how people feel about
that.  If we don't do that, I'm not sure there's any real good
solution here.

>> The implementation of first_row_is_empty() looks pretty kludgey, too.
>> It seems to me that it will fail completely if the text of the
>> function definition happens to contain $function$.
>>
>> CREATE OR REPLACE FUNCTION boom() RETURNS text LANGUAGE plpgsql AS $$
>> BEGIN SELECT '$function$'; END $$;
>>
>
> I can enhance algorithm on client side - but it will not be a pretty
> code - it better do it on server side - for example
> pg_get_formated_functiondef ...
>
> CREATE OR REPLACE FUNCTION pg_get_formated_function_def(in oid,
> linenum bool:= false, OUT funcdef text, OUT first_line_isignored bool)
>
> this can remove any ugly code

I don't really see why this can't be done on the client side - can't
you just scan until you find the second dollars sign and then see
whether the following character is a newline?  Seems like this could
be done in a very small number of lines of code using strchr().

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-01T13:20:20Z

2010/8/1 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Aug 1, 2010 at 12:15 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 2010/8/1 Robert Haas <robertmhaas@gmail.com>:
>>> On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>>>> I'm setting this as ready for committer.
>>>>
>>>> Thank you very much
>>>
>>> I took a look at this tonight and am a bit mystified by the following bit:
>>>
>>> +                       /*
>>> +                        * PL doesn't calculate first row of function's body
>>> +                        * when first row is empty. So checks first row, and
>>> +                        * correct lineno when it is necessary.
>>> +                        */
>>>
>>> Is that true of any PL, or just some particular PL?  Is the behavior
>>> described here a bug that we should fix, or is it, for some reason,
>>> considered correct?  Is it documented in our documentation?
>>
>> it is primary plpgsql issue.
>
> Yeah, that seems like a problem.  If your editor is vi, for example,
> the following are the same number of keystrokes:
>
> \ef foo 417<enter>
> and
> \ef foo<enter>417G
>

it not is too much important, navigation command is secondary
function. Primary functionality is showing of source code with correct
row numbers.

> So the major advantage of the former over the latter is that you'll
> (hopefully) end up on EXACTLY the right line rather than maybe being
> off by a few lines.  With the current code, that won't necessarily
> happen, because PL/pgsql (and any third-party PLs based on it) use one
> line-numbering convention and other PLs don't necessarily include that
> hack.  Admittedly, you'll probably only be off by one line instead of
> three or four, so maybe people think that's good enough, but I'm not
> totally convinced.  It seems like the easiest way to fix this would be
> remove the hack from PL/pgsql, but I'm not sure how people feel about
> that.  If we don't do that, I'm not sure there's any real good
> solution here.

.. :( without this feature - this patch has minimal value.

I don't believe so change plpgsql numbering is good way. It was
changed from good reasons. Because empty row is invisible but it isn't
empty for people, because there are " AS " keyword.

This patch must to working with plpgsql and have to work correctly.

>
>>> The implementation of first_row_is_empty() looks pretty kludgey, too.
>>> It seems to me that it will fail completely if the text of the
>>> function definition happens to contain $function$.
>>>
>>> CREATE OR REPLACE FUNCTION boom() RETURNS text LANGUAGE plpgsql AS $$
>>> BEGIN SELECT '$function$'; END $$;
>>>
>>
>> I can enhance algorithm on client side - but it will not be a pretty
>> code - it better do it on server side - for example
>> pg_get_formated_functiondef ...
>>
>> CREATE OR REPLACE FUNCTION pg_get_formated_function_def(in oid,
>> linenum bool:= false, OUT funcdef text, OUT first_line_isignored bool)
>>
>> this can remove any ugly code
>
> I don't really see why this can't be done on the client side - can't
> you just scan until you find the second dollars sign and then see
> whether the following character is a newline?  Seems like this could
> be done in a very small number of lines of code using strchr().

you have a true - it is possible, but still I am supply a parser on
client side. On server side I have all data in structured form.

but I don't would to complicate a possible commiting

so my plan

a) fix problem with ambiguous $function* like you proposed
b) fix problem with "first row excepting" - I can activate a detection
only for plpgsql language - I can identify LANGUAGE before.

all will be done on client

It is acceptable for you?

Regards

Pavel Stehule


> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-01T14:47:57Z

Pavel Stehule <pavel.stehule@gmail.com> writes:
> so my plan

> a) fix problem with ambiguous $function* like you proposed
> b) fix problem with "first row excepting" - I can activate a detection
> only for plpgsql language - I can identify LANGUAGE before.

Ick.  We should absolutely NOT have a client-side special case for plpgsql.

Personally I'd be fine with dropping the special case from the plpgsql
parser --- I don't believe that that behavior was ever discussed, much
less documented, and I doubt that many people rely on it or even know
it exists.  The need to count lines manually in function definitions is
far less than it was back when that kluge was put in.

If anyone can make a convincing case that it's a good idea to ignore
leading newlines, we should reimplement the behavior in such a way that
it applies across the board to all PLs (ie, make CREATE FUNCTION strip
a leading newline before storing the text).  However, then you'd have
issues about whether or when to put back the newline, so I'm not really
in favor of that route.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-01T15:09:44Z

On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> so my plan
>
>> a) fix problem with ambiguous $function* like you proposed
>> b) fix problem with "first row excepting" - I can activate a detection
>> only for plpgsql language - I can identify LANGUAGE before.
>
> Ick.  We should absolutely NOT have a client-side special case for plpgsql.
>
> Personally I'd be fine with dropping the special case from the plpgsql
> parser --- I don't believe that that behavior was ever discussed, much
> less documented, and I doubt that many people rely on it or even know
> it exists.

+1.

> The need to count lines manually in function definitions is
> far less than it was back when that kluge was put in.

Why?

> If anyone can make a convincing case that it's a good idea to ignore
> leading newlines, we should reimplement the behavior in such a way that
> it applies across the board to all PLs (ie, make CREATE FUNCTION strip
> a leading newline before storing the text).  However, then you'd have
> issues about whether or when to put back the newline, so I'm not really
> in favor of that route.

Ditto.

As a procedural note, if we decide to go this route, this should be
split into two patches - one that removes the line-numbering kludge,
and a second for the psql changes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-01T15:22:37Z

2010/8/1 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>> so my plan
>>
>>> a) fix problem with ambiguous $function* like you proposed
>>> b) fix problem with "first row excepting" - I can activate a detection
>>> only for plpgsql language - I can identify LANGUAGE before.
>>
>> Ick.  We should absolutely NOT have a client-side special case for plpgsql.
>>
>> Personally I'd be fine with dropping the special case from the plpgsql
>> parser --- I don't believe that that behavior was ever discussed, much
>> less documented, and I doubt that many people rely on it or even know
>> it exists.
>
> +1.
>
>> The need to count lines manually in function definitions is
>> far less than it was back when that kluge was put in.
>
> Why?
>
>> If anyone can make a convincing case that it's a good idea to ignore
>> leading newlines, we should reimplement the behavior in such a way that
>> it applies across the board to all PLs (ie, make CREATE FUNCTION strip
>> a leading newline before storing the text).  However, then you'd have
>> issues about whether or when to put back the newline, so I'm not really
>> in favor of that route.
>
> Ditto.
>
> As a procedural note, if we decide to go this route, this should be
> split into two patches - one that removes the line-numbering kludge,
> and a second for the psql changes.


ok - tomorrow I'll send a patch

Regards

Pavel
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-01T15:35:34Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The need to count lines manually in function definitions is
>> far less than it was back when that kluge was put in.

> Why?

That hack goes back to plpgsql's prehistory (it's there, though sans
comment, in plpgsql's scan.l 1.1).  We had none of the current support
for identifying error locations by cursor position and/or quoting part
of the source text back at you.  Let me illustrate what happened with
a simple syntax error in PG 7.0:

play=> create function fool() returns int as '
play'> begin
play'>   fool
play'> end' language 'plpgsql';
CREATE
play=> select fool();
NOTICE:  plpgsql: ERROR during compile of fool near line 2
ERROR:  missing ; at end of SQL statement
play=> 

So you *had* to count lines, and do it accurately too, to figure out
even pretty simple syntax errors.

Personally, rather than sweat about what the exact definition of line
numbers is, I think we should be moving further in the direction of
being able to regurgitate source text to identify error locations.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-01T17:28:23Z

On Sun, Aug 1, 2010 at 11:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> The need to count lines manually in function definitions is
>>> far less than it was back when that kluge was put in.
>
>> Why?
>
> That hack goes back to plpgsql's prehistory (it's there, though sans
> comment, in plpgsql's scan.l 1.1).  We had none of the current support
> for identifying error locations by cursor position and/or quoting part
> of the source text back at you.  Let me illustrate what happened with
> a simple syntax error in PG 7.0:
>
> play=> create function fool() returns int as '
> play'> begin
> play'>   fool
> play'> end' language 'plpgsql';
> CREATE
> play=> select fool();
> NOTICE:  plpgsql: ERROR during compile of fool near line 2
> ERROR:  missing ; at end of SQL statement
> play=>
>
> So you *had* to count lines, and do it accurately too, to figure out
> even pretty simple syntax errors.
>
> Personally, rather than sweat about what the exact definition of line
> numbers is, I think we should be moving further in the direction of
> being able to regurgitate source text to identify error locations.

I basically agree with that; but on the other hand, in a large
PL/pgsql function, you may have very similar-looking text in multiple
places.  So line numbers are good, too: but then you weren't proposing
to remove those, I assume, just to augment them with additional
information.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-01T18:43:29Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Aug 1, 2010 at 11:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Personally, rather than sweat about what the exact definition of line
>> numbers is, I think we should be moving further in the direction of
>> being able to regurgitate source text to identify error locations.

> I basically agree with that; but on the other hand, in a large
> PL/pgsql function, you may have very similar-looking text in multiple
> places.  So line numbers are good, too: but then you weren't proposing
> to remove those, I assume, just to augment them with additional
> information.

Right.  I'm just suggesting that source text is better than line numbers
for exact position identification, so I don't see a lot of value in
preserving historical behaviors that change line numbers by one count
one way or another.  If some of the PLs used zero-based instead of
one-based line numbers, we'd be looking to standardize that not cater to
their individual idiosyncrasies.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-01T20:53:04Z

Hello

I am sending a modified patch - changes:

a) remove special row number handling of plpgsql (first patch)
b) more robust algorithm for header rows identification

Regards

Pavel Stehule

2010/8/1 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>> so my plan
>>
>>> a) fix problem with ambiguous $function* like you proposed
>>> b) fix problem with "first row excepting" - I can activate a detection
>>> only for plpgsql language - I can identify LANGUAGE before.
>>
>> Ick.  We should absolutely NOT have a client-side special case for plpgsql.
>>
>> Personally I'd be fine with dropping the special case from the plpgsql
>> parser --- I don't believe that that behavior was ever discussed, much
>> less documented, and I doubt that many people rely on it or even know
>> it exists.
>
> +1.
>
>> The need to count lines manually in function definitions is
>> far less than it was back when that kluge was put in.
>
> Why?
>
>> If anyone can make a convincing case that it's a good idea to ignore
>> leading newlines, we should reimplement the behavior in such a way that
>> it applies across the board to all PLs (ie, make CREATE FUNCTION strip
>> a leading newline before storing the text).  However, then you'd have
>> issues about whether or when to put back the newline, so I'm not really
>> in favor of that route.
>
> Ditto.
>
> As a procedural note, if we decide to go this route, this should be
> split into two patches - one that removes the line-numbering kludge,
> and a second for the psql changes.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-02T03:48:44Z

On Sun, Aug 1, 2010 at 4:53 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> a) remove special row number handling of plpgsql (first patch)

Committed.

> b) more robust algorithm for header rows identification

Have not gotten to this one yet.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-03T00:54:07Z

On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> b) more robust algorithm for header rows identification
>
> Have not gotten to this one yet.

I notIce that on WIN32 the default editor is notepad.exe and the
default editor navigation option is "/".  Does "notepad.exe /lineno
filename" actually work on Windows?  A quick Google search suggests
that the answer is "no", which seems somewhat unfortunate: it means
we'd be shipping "broken on Win32 out of the box".

http://www.robvanderwoude.com/commandlineswitches.php#Notepad

This is actually my biggest concern about this patch - that it may be
just too much of a hassle to actually make it work for people.  I just
tried setting $EDITOR to MacOS's TextEdit program, and it turns out
that TextEdit doesn't understand +.  I'm afraid that we're going to
end up with a situation where it only works for people using emacs or
vi, and everyone else ends up with a broken install (and, possibly, no
clear idea how to fix it).  Perhaps it would be better to accept \sf
and reject \sf+ and \ef <func> <lineno>.

Assuming we get past that threshold issue, I'm also wondering whether
the "navigation option" terminology is best; e.g. set
PSQL_EDITOR_NAVIGATION_OPTION to configure it.  It doesn't seem
terrible, but it doesn't seem great, either.  Anyone have a better
idea?

The docs are a little rough; they could benefit from some editing by a
fluent English speaker.  If anyone has time to work on this, it would
be much appreciated.

Instead of "line number is unacceptable", I think you should write
"invalid line number".

"dollar" should not be spelled "dolar".  "function" should not be
spelled "finction".

This change looks suspiciously like magic.  If it's actually safe, it
needs a comment explaining why:

-       sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
+       sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);

Attempting to compile with this patch applied emits a warning on my
machine; whether or not the warning is valid, you need to make it go
away:

command.c: In function ‘HandleSlashCmds’:
command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
command.c:1055: note: ‘bsep’ was declared here

Why does the \sf output have a trailing blank line?  This seems weird,
especially because \ef puts no such trailing blank line in the editor.

Instead of:

+       /* skip useles whitespaces */
+       while (c >= func)
+               if (isblank(*c))
+                       c--;
+               else
+                       break;

...wouldn't it be just as good to write:

while (c >= func && isblank(*c))
    c--;

(and similarly elsewhere)

In extract_separator, if you invert the sense of the first if-test,
then you can avoid having to indent the entire function contents.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-03T02:49:38Z

Robert Haas <robertmhaas@gmail.com> writes:
> This is actually my biggest concern about this patch - that it may be
> just too much of a hassle to actually make it work for people.  I just
> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
> that TextEdit doesn't understand +.  I'm afraid that we're going to
> end up with a situation where it only works for people using emacs or
> vi, and everyone else ends up with a broken install (and, possibly, no
> clear idea how to fix it).

[ disclaimer: I've not looked at the proposed patch yet ]

It seems like this ought to be fairly easily surmountable as long as
the patch is designed for failure.  The fallback position is just that
the line number does nothing, ie \ef foo just opens the editor and
doesn't try to position the cursor anywhere special; nobody can complain
about that because it's no worse than before.  What we need is to not
try to force positioning if we don't recognize the editor.  I'm tempted
to suggest forgetting about any user-configurable parameter and just
provide code that strcmp's the $EDITOR value to see if it recognizes the
editor name, otherwise do nothing.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-03T03:10:27Z

On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> This is actually my biggest concern about this patch - that it may be
>> just too much of a hassle to actually make it work for people.  I just
>> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
>> that TextEdit doesn't understand +.  I'm afraid that we're going to
>> end up with a situation where it only works for people using emacs or
>> vi, and everyone else ends up with a broken install (and, possibly, no
>> clear idea how to fix it).
>
> [ disclaimer: I've not looked at the proposed patch yet ]
>
> It seems like this ought to be fairly easily surmountable as long as
> the patch is designed for failure.

It isn't.

> The fallback position is just that
> the line number does nothing, ie \ef foo just opens the editor and
> doesn't try to position the cursor anywhere special; nobody can complain
> about that because it's no worse than before.  What we need is to not
> try to force positioning if we don't recognize the editor.

Supposing for the moment that we could make it work that way, that
would be reasonable.

> I'm tempted
> to suggest forgetting about any user-configurable parameter and just
> provide code that strcmp's the $EDITOR value to see if it recognizes the
> editor name, otherwise do nothing.

With all due respect, that sounds like an amazingly bad idea.  Surely,
we'll be forever getting patches to add $MYFAVORITEEDITOR to the list,
or complaints that it's not already included.  While this is
superficially a Nice Thing to Have and I would certainly support it if
+linenumber were relatively universal, it's really a pretty minor
convenience when you come right down to it, and I am not at all
convinced it is worth the hassle of trying to divine what piece of
syntax will equip the user's choice of editor with the necessary
amount of clue.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-03T03:17:18Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm tempted
>> to suggest forgetting about any user-configurable parameter and just
>> provide code that strcmp's the $EDITOR value to see if it recognizes the
>> editor name, otherwise do nothing.

> With all due respect, that sounds like an amazingly bad idea.  Surely,
> we'll be forever getting patches to add $MYFAVORITEEDITOR to the list,
> or complaints that it's not already included.

Well, yeah, that's the idea.  I say that beats a constant stream of
complaints that $MYFAVORITEEDITOR no longer works at all --- which
is what your concern was, no?

> While this is
> superficially a Nice Thing to Have and I would certainly support it if
> +linenumber were relatively universal, it's really a pretty minor
> convenience when you come right down to it, and I am not at all
> convinced it is worth the hassle of trying to divine what piece of
> syntax will equip the user's choice of editor with the necessary
> amount of clue.

The other approach we could take is that this whole thing is disabled by
default, and you have to set a psql variable EDITOR_LINENUMBER_SWITCH
to turn it on.  If you haven't read the documentation enough to find
out that variable exists, well, no harm no foul.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-03T03:34:02Z

On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm tempted
>>> to suggest forgetting about any user-configurable parameter and just
>>> provide code that strcmp's the $EDITOR value to see if it recognizes the
>>> editor name, otherwise do nothing.
>
>> With all due respect, that sounds like an amazingly bad idea.  Surely,
>> we'll be forever getting patches to add $MYFAVORITEEDITOR to the list,
>> or complaints that it's not already included.
>
> Well, yeah, that's the idea.  I say that beats a constant stream of
> complaints that $MYFAVORITEEDITOR no longer works at all --- which
> is what your concern was, no?

Well, it'd still work fine for \e foo.  It'll just blow up for \e foo
3.  My concern isn't really that things that which work now will break
so much as that this new feature will fail to work for a large
percentage of the people who try to use it, including virtually
everyone who tries to use it on Win32.

>> While this is
>> superficially a Nice Thing to Have and I would certainly support it if
>> +linenumber were relatively universal, it's really a pretty minor
>> convenience when you come right down to it, and I am not at all
>> convinced it is worth the hassle of trying to divine what piece of
>> syntax will equip the user's choice of editor with the necessary
>> amount of clue.
>
> The other approach we could take is that this whole thing is disabled by
> default, and you have to set a psql variable EDITOR_LINENUMBER_SWITCH
> to turn it on.  If you haven't read the documentation enough to find
> out that variable exists, well, no harm no foul.

That might be reasonable.  Right now the default behavior is to do
+line on Linux and /line on Windows.  But maybe a more sensible
default would be to fail with an error message that says "you have to
set thus-and-so variable if you want to use this feature".  That seems
better than sometimes working and sometimes failing depending on what
editor the user has configured.

A side question is whether this should be an environment variable or a
psql variable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-03T04:52:06Z

2010/8/3 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> b) more robust algorithm for header rows identification
>>
>> Have not gotten to this one yet.
>
> I notIce that on WIN32 the default editor is notepad.exe and the
> default editor navigation option is "/".  Does "notepad.exe /lineno
> filename" actually work on Windows?  A quick Google search suggests
> that the answer is "no", which seems somewhat unfortunate: it means
> we'd be shipping "broken on Win32 out of the box".
>
> http://www.robvanderwoude.com/commandlineswitches.php#Notepad

notapad supports nothing :( Microsoft doesn't use command line. I
found this syntax in pspad. Next other editor is notepad++. It use a
syntax -n. Wide used KDE editors use --line syntax

>
> This is actually my biggest concern about this patch - that it may be
> just too much of a hassle to actually make it work for people.  I just
> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
> that TextEdit doesn't understand +.  I'm afraid that we're going to
> end up with a situation where it only works for people using emacs or
> vi, and everyone else ends up with a broken install (and, possibly, no
> clear idea how to fix it).  Perhaps it would be better to accept \sf
> and reject \sf+ and \ef <func> <lineno>.
>

\sf+ is base - we really need a source output with line numbers. \ef
foo func is just user very friendly function. I agree, so this topic
have to be better documented

> Assuming we get past that threshold issue, I'm also wondering whether
> the "navigation option" terminology is best; e.g. set
> PSQL_EDITOR_NAVIGATION_OPTION to configure it.  It doesn't seem
> terrible, but it doesn't seem great, either.  Anyone have a better
> idea?


>
> The docs are a little rough; they could benefit from some editing by a
> fluent English speaker.  If anyone has time to work on this, it would
> be much appreciated.
>
> Instead of "line number is unacceptable", I think you should write
> "invalid line number".
>
> "dollar" should not be spelled "dolar".  "function" should not be
> spelled "finction".
>
> This change looks suspiciously like magic.  If it's actually safe, it
> needs a comment explaining why:
>
> -       sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
> +       sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
>
> Attempting to compile with this patch applied emits a warning on my
> machine; whether or not the warning is valid, you need to make it go
> away:
>
> command.c: In function ‘HandleSlashCmds’:
> command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
> command.c:1055: note: ‘bsep’ was declared here
>
> Why does the \sf output have a trailing blank line?  This seems weird,
> especially because \ef puts no such trailing blank line in the editor.
>
> Instead of:
>
> +       /* skip useles whitespaces */
> +       while (c >= func)
> +               if (isblank(*c))
> +                       c--;
> +               else
> +                       break;
>
> ...wouldn't it be just as good to write:
>
> while (c >= func && isblank(*c))
>    c--;
>
> (and similarly elsewhere)
>
> In extract_separator, if you invert the sense of the first if-test,
> then you can avoid having to indent the entire function contents.
>
> --

I'' recheck these issue


Pavel

> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-03T04:58:02Z

2010/8/3 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I'm tempted
>>>> to suggest forgetting about any user-configurable parameter and just
>>>> provide code that strcmp's the $EDITOR value to see if it recognizes the
>>>> editor name, otherwise do nothing.
>>
>>> With all due respect, that sounds like an amazingly bad idea.  Surely,
>>> we'll be forever getting patches to add $MYFAVORITEEDITOR to the list,
>>> or complaints that it's not already included.
>>
>> Well, yeah, that's the idea.  I say that beats a constant stream of
>> complaints that $MYFAVORITEEDITOR no longer works at all --- which
>> is what your concern was, no?
>
> Well, it'd still work fine for \e foo.  It'll just blow up for \e foo
> 3.  My concern isn't really that things that which work now will break
> so much as that this new feature will fail to work for a large
> percentage of the people who try to use it, including virtually
> everyone who tries to use it on Win32.
>
>>> While this is
>>> superficially a Nice Thing to Have and I would certainly support it if
>>> +linenumber were relatively universal, it's really a pretty minor
>>> convenience when you come right down to it, and I am not at all
>>> convinced it is worth the hassle of trying to divine what piece of
>>> syntax will equip the user's choice of editor with the necessary
>>> amount of clue.
>>
>> The other approach we could take is that this whole thing is disabled by
>> default, and you have to set a psql variable EDITOR_LINENUMBER_SWITCH
>> to turn it on.  If you haven't read the documentation enough to find
>> out that variable exists, well, no harm no foul.
>
> That might be reasonable.  Right now the default behavior is to do
> +line on Linux and /line on Windows.  But maybe a more sensible
> default would be to fail with an error message that says "you have to
> set thus-and-so variable if you want to use this feature".  That seems
> better than sometimes working and sometimes failing depending on what
> editor the user has configured.
>

I like this idea

> A side question is whether this should be an environment variable or a
> psql variable.
>
it can be just psql variable.

Pavel

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-03T09:04:02Z

attached updated patch

* don't use a default option for navigation in editor - user have to
set this option explicitly
* name for this psql variable is EDITOR_LINENUMBER_SWITCH -
* updated comments, doc and some issues described by Robert

Regards

Pavel Stehule

2010/8/3 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> b) more robust algorithm for header rows identification
>>
>> Have not gotten to this one yet.
>
> I notIce that on WIN32 the default editor is notepad.exe and the
> default editor navigation option is "/".  Does "notepad.exe /lineno
> filename" actually work on Windows?  A quick Google search suggests
> that the answer is "no", which seems somewhat unfortunate: it means
> we'd be shipping "broken on Win32 out of the box".
>
> http://www.robvanderwoude.com/commandlineswitches.php#Notepad
>
> This is actually my biggest concern about this patch - that it may be
> just too much of a hassle to actually make it work for people.  I just
> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
> that TextEdit doesn't understand +.  I'm afraid that we're going to
> end up with a situation where it only works for people using emacs or
> vi, and everyone else ends up with a broken install (and, possibly, no
> clear idea how to fix it).  Perhaps it would be better to accept \sf
> and reject \sf+ and \ef <func> <lineno>.
>
> Assuming we get past that threshold issue, I'm also wondering whether
> the "navigation option" terminology is best; e.g. set
> PSQL_EDITOR_NAVIGATION_OPTION to configure it.  It doesn't seem
> terrible, but it doesn't seem great, either.  Anyone have a better
> idea?
>
> The docs are a little rough; they could benefit from some editing by a
> fluent English speaker.  If anyone has time to work on this, it would
> be much appreciated.
>
> Instead of "line number is unacceptable", I think you should write
> "invalid line number".
>
> "dollar" should not be spelled "dolar".  "function" should not be
> spelled "finction".
>
> This change looks suspiciously like magic.  If it's actually safe, it
> needs a comment explaining why:
>
> -       sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
> +       sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
>
> Attempting to compile with this patch applied emits a warning on my
> machine; whether or not the warning is valid, you need to make it go
> away:
>
> command.c: In function ‘HandleSlashCmds’:
> command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
> command.c:1055: note: ‘bsep’ was declared here
>
> Why does the \sf output have a trailing blank line?  This seems weird,
> especially because \ef puts no such trailing blank line in the editor.
>
> Instead of:
>
> +       /* skip useles whitespaces */
> +       while (c >= func)
> +               if (isblank(*c))
> +                       c--;
> +               else
> +                       break;
>
> ...wouldn't it be just as good to write:
>
> while (c >= func && isblank(*c))
>    c--;
>
> (and similarly elsewhere)
>
> In extract_separator, if you invert the sense of the first if-test,
> then you can avoid having to indent the entire function contents.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-03T09:24:01Z

Hello

2010/8/3 Pavel Stehule <pavel.stehule@gmail.com>:
> attached updated patch
>
> * don't use a default option for navigation in editor - user have to
> set this option explicitly
> * name for this psql variable is EDITOR_LINENUMBER_SWITCH -
> * updated comments, doc and some issues described by Robert
>
> Regards
>
> Pavel Stehule

I found a small bug - the last patch better handle parsing lineno
after function descriptor

Regards

Pavel

>
> 2010/8/3 Robert Haas <robertmhaas@gmail.com>:
>> On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> b) more robust algorithm for header rows identification
>>>
>>> Have not gotten to this one yet.
>>
>> I notIce that on WIN32 the default editor is notepad.exe and the
>> default editor navigation option is "/".  Does "notepad.exe /lineno
>> filename" actually work on Windows?  A quick Google search suggests
>> that the answer is "no", which seems somewhat unfortunate: it means
>> we'd be shipping "broken on Win32 out of the box".
>>
>> http://www.robvanderwoude.com/commandlineswitches.php#Notepad
>>
>> This is actually my biggest concern about this patch - that it may be
>> just too much of a hassle to actually make it work for people.  I just
>> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
>> that TextEdit doesn't understand +.  I'm afraid that we're going to
>> end up with a situation where it only works for people using emacs or
>> vi, and everyone else ends up with a broken install (and, possibly, no
>> clear idea how to fix it).  Perhaps it would be better to accept \sf
>> and reject \sf+ and \ef <func> <lineno>.
>>
>> Assuming we get past that threshold issue, I'm also wondering whether
>> the "navigation option" terminology is best; e.g. set
>> PSQL_EDITOR_NAVIGATION_OPTION to configure it.  It doesn't seem
>> terrible, but it doesn't seem great, either.  Anyone have a better
>> idea?
>>
>> The docs are a little rough; they could benefit from some editing by a
>> fluent English speaker.  If anyone has time to work on this, it would
>> be much appreciated.
>>
>> Instead of "line number is unacceptable", I think you should write
>> "invalid line number".
>>
>> "dollar" should not be spelled "dolar".  "function" should not be
>> spelled "finction".
>>
>> This change looks suspiciously like magic.  If it's actually safe, it
>> needs a comment explaining why:
>>
>> -       sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
>> +       sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
>>
>> Attempting to compile with this patch applied emits a warning on my
>> machine; whether or not the warning is valid, you need to make it go
>> away:
>>
>> command.c: In function ‘HandleSlashCmds’:
>> command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
>> command.c:1055: note: ‘bsep’ was declared here
>>
>> Why does the \sf output have a trailing blank line?  This seems weird,
>> especially because \ef puts no such trailing blank line in the editor.
>>
>> Instead of:
>>
>> +       /* skip useles whitespaces */
>> +       while (c >= func)
>> +               if (isblank(*c))
>> +                       c--;
>> +               else
>> +                       break;
>>
>> ...wouldn't it be just as good to write:
>>
>> while (c >= func && isblank(*c))
>>    c--;
>>
>> (and similarly elsewhere)
>>
>> In extract_separator, if you invert the sense of the first if-test,
>> then you can avoid having to indent the entire function contents.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise Postgres Company
>>
>

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-03T11:20:32Z

Hello

I hope so I found and fixed last issue - the longer functions was
showed directly - without a pager.

Regards

Pavel

2010/8/3 Pavel Stehule <pavel.stehule@gmail.com>:
> Hello
>
> 2010/8/3 Pavel Stehule <pavel.stehule@gmail.com>:
>> attached updated patch
>>
>> * don't use a default option for navigation in editor - user have to
>> set this option explicitly
>> * name for this psql variable is EDITOR_LINENUMBER_SWITCH -
>> * updated comments, doc and some issues described by Robert
>>
>> Regards
>>
>> Pavel Stehule
>
> I found a small bug - the last patch better handle parsing lineno
> after function descriptor
>
> Regards
>
> Pavel
>
>>
>> 2010/8/3 Robert Haas <robertmhaas@gmail.com>:
>>> On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>>> b) more robust algorithm for header rows identification
>>>>
>>>> Have not gotten to this one yet.
>>>
>>> I notIce that on WIN32 the default editor is notepad.exe and the
>>> default editor navigation option is "/".  Does "notepad.exe /lineno
>>> filename" actually work on Windows?  A quick Google search suggests
>>> that the answer is "no", which seems somewhat unfortunate: it means
>>> we'd be shipping "broken on Win32 out of the box".
>>>
>>> http://www.robvanderwoude.com/commandlineswitches.php#Notepad
>>>
>>> This is actually my biggest concern about this patch - that it may be
>>> just too much of a hassle to actually make it work for people.  I just
>>> tried setting $EDITOR to MacOS's TextEdit program, and it turns out
>>> that TextEdit doesn't understand +.  I'm afraid that we're going to
>>> end up with a situation where it only works for people using emacs or
>>> vi, and everyone else ends up with a broken install (and, possibly, no
>>> clear idea how to fix it).  Perhaps it would be better to accept \sf
>>> and reject \sf+ and \ef <func> <lineno>.
>>>
>>> Assuming we get past that threshold issue, I'm also wondering whether
>>> the "navigation option" terminology is best; e.g. set
>>> PSQL_EDITOR_NAVIGATION_OPTION to configure it.  It doesn't seem
>>> terrible, but it doesn't seem great, either.  Anyone have a better
>>> idea?
>>>
>>> The docs are a little rough; they could benefit from some editing by a
>>> fluent English speaker.  If anyone has time to work on this, it would
>>> be much appreciated.
>>>
>>> Instead of "line number is unacceptable", I think you should write
>>> "invalid line number".
>>>
>>> "dollar" should not be spelled "dolar".  "function" should not be
>>> spelled "finction".
>>>
>>> This change looks suspiciously like magic.  If it's actually safe, it
>>> needs a comment explaining why:
>>>
>>> -       sys = pg_malloc(strlen(editorName) + strlen(fname) + 10 + 1);
>>> +       sys = pg_malloc(strlen(editorName) + strlen(fname) + 20 + 1);
>>>
>>> Attempting to compile with this patch applied emits a warning on my
>>> machine; whether or not the warning is valid, you need to make it go
>>> away:
>>>
>>> command.c: In function ‘HandleSlashCmds’:
>>> command.c:1055: warning: ‘bsep’ may be used uninitialized in this function
>>> command.c:1055: note: ‘bsep’ was declared here
>>>
>>> Why does the \sf output have a trailing blank line?  This seems weird,
>>> especially because \ef puts no such trailing blank line in the editor.
>>>
>>> Instead of:
>>>
>>> +       /* skip useles whitespaces */
>>> +       while (c >= func)
>>> +               if (isblank(*c))
>>> +                       c--;
>>> +               else
>>> +                       break;
>>>
>>> ...wouldn't it be just as good to write:
>>>
>>> while (c >= func && isblank(*c))
>>>    c--;
>>>
>>> (and similarly elsewhere)
>>>
>>> In extract_separator, if you invert the sense of the first if-test,
>>> then you can avoid having to indent the entire function contents.
>>>
>>> --
>>> Robert Haas
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise Postgres Company
>>>
>>
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-04T12:53:15Z

On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I hope so I found and fixed last issue - the longer functions was
> showed directly - without a pager.

As a matter of style, I suggest leaving bool *edited as the last
argument to do_edit() and inserting int lineno as the second-to-last
argument.

! 			int  lineno = -1;
[...]
! 					if (atoi(ln) < 1)
! 					{
! 						psql_error("invalid line number\n");
! 						status = PSQL_CMD_ERROR;
! 					}
! 					else
! 						lineno = atoi(ln);

Why call atoi(n) twice?  Can't you just write:

lineno = atoi(n);
if (lineno < 1) {
   ...error stuff...
}

I suggested writing psql_assert(datag != NULL) rather than just
psql_assert(datag).

Instead of: cannot use a editor navigation without setting
EDITOR_LINENUMBER_SWITCH variable
I suggest: cannot edit a specific line because the
EDITOR_LINENUMBER_SWITCH variable is not set

I don't find the name get_dg_tag() to be very mnemonic.  How about
get_function_dollarquote_tag()?

In help.c, it looks like you've only added one line but incremented
the pager count by three.

In this bit:

! 				dqtag = get_dq_tag(lines);
! 				if (dqtag)
! 				{
! 					free(dqtag);
! 					break;
! 				}
! 				else
! 					lineno++;

The "else" is unnecessary.  And just after that:

! 				if (end_of_line)
! 					lines = end_of_line + 1;
! 				else
! 					break;

You can write this more cleanly by saying if (!end_of_line) break;
lines = end_of_line + 1.

The following diff hunk (2213,2218) just adds a blank line and is
therefore unnecessary.  There's a similar hunk in the docs portion of
the patch.

In this part:

  			func = psql_scan_slash_option(scan_state,
  										  OT_WHOLE_LINE, NULL, true);
! 			lineno = extract_lineno_from_funcdesc(func, &status);
! 			
! 			if (status != PSQL_CMD_ERROR)
  			{
! 				if (!func)
! 				{
! 					/* set up an empty command to fill in */
! 					printfPQExpBuffer(query_buf,
! 									  "CREATE FUNCTION ( )\n"
! 									  " RETURNS \n"
! 									  " LANGUAGE \n"
! 									  " -- common options:  IMMUTABLE  STABLE  STRICT  SECURITY
DEFINER\n"
! 									  "AS $function$\n"
! 									  "\n$function$\n");
! 				}
! 				else if (!lookup_function_oid(pset.db, func, &foid))
! 				{
! 					/* error already reported */
! 					status = PSQL_CMD_ERROR;
! 				}
! 				else if (!get_create_function_cmd(pset.db, foid, query_buf))
! 				{
! 					/* error already reported */
! 					status = PSQL_CMD_ERROR;
! 				}
! 				if (func)
! 					free(func);
  			}

Why is it correct for if (func) free(func) to be inside the if (status
!= PSQL_CMD_ERROR) block?  It looks to me like func gets initialized
first, before status is set.

It seems unnecessary for extract_lineno_from_funcdesc() to return the
line number and have a separate out parameter to return a
backslashResult.  Can't you just return -1 to indicate an error?  (You
might need to move the if (!func) test at the top to the caller, but
that seems OK.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

David Fetter <david@fetter.org> — 2010-08-04T14:10:22Z

On Mon, Aug 02, 2010 at 11:34:02PM -0400, Robert Haas wrote:
> On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Well, it'd still work fine for \e foo.  It'll just blow up for \e
> foo 3.  My concern isn't really that things that which work now will
> break so much as that this new feature will fail to work for a large
> percentage of the people who try to use it, including virtually
> everyone who tries to use it on Win32.

That concern is a show-stopper.

> >> While this is superficially a Nice Thing to Have and I would
> >> certainly support it if +linenumber were relatively universal,
> >> it's really a pretty minor convenience when you come right down
> >> to it, and I am not at all convinced it is worth the hassle of
> >> trying to divine what piece of syntax will equip the user's
> >> choice of editor with the necessary amount of clue.
> >
> > The other approach we could take is that this whole thing is
> > disabled by default, and you have to set a psql variable
> > EDITOR_LINENUMBER_SWITCH to turn it on.  If you haven't read the
> > documentation enough to find out that variable exists, well, no
> > harm no foul.
> 
> That might be reasonable.  Right now the default behavior is to do
> +line on Linux and /line on Windows.  But maybe a more sensible
> default would be to fail with an error message that says "you have
> to set thus-and-so variable if you want to use this feature".  That
> seems better than sometimes working and sometimes failing depending
> on what editor the user has configured.
> 
> A side question is whether this should be an environment variable or
> a psql variable.

I'd say "yes."  As with $EDITOR/PSQL_EDITOR, there should be something
that looks for an overriding psql variable, drops through to look for
an environment variable, and then to a sane default, for some
reasonable value of "sane."  Perhaps this default could depend on OS
(Windows vs. Everything Else) to start with.

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
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

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

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-04T14:29:44Z

On Wed, Aug 4, 2010 at 10:10 AM, David Fetter <david@fetter.org> wrote:
> On Mon, Aug 02, 2010 at 11:34:02PM -0400, Robert Haas wrote:
>> On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > Robert Haas <robertmhaas@gmail.com> writes:
>> >> On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Well, it'd still work fine for \e foo.  It'll just blow up for \e
>> foo 3.  My concern isn't really that things that which work now will
>> break so much as that this new feature will fail to work for a large
>> percentage of the people who try to use it, including virtually
>> everyone who tries to use it on Win32.
>
> That concern is a show-stopper.

See downthread; I believe we have a resolution to this issue.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-04T14:35:25Z

Hello

updated patch attached

2010/8/4 Robert Haas <robertmhaas@gmail.com>:
> On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> I hope so I found and fixed last issue - the longer functions was
>> showed directly - without a pager.
>
> As a matter of style, I suggest leaving bool *edited as the last
> argument to do_edit() and inserting int lineno as the second-to-last
> argument.
>
> !                       int  lineno = -1;
> [...]
> !                                       if (atoi(ln) < 1)
> !                                       {
> !                                               psql_error("invalid line number\n");
> !                                               status = PSQL_CMD_ERROR;
> !                                       }
> !                                       else
> !                                               lineno = atoi(ln);
>
> Why call atoi(n) twice?  Can't you just write:
>
> lineno = atoi(n);
> if (lineno < 1) {
>   ...error stuff...
> }

fixed
>
> I suggested writing psql_assert(datag != NULL) rather than just
> psql_assert(datag).
>
> Instead of: cannot use a editor navigation without setting
> EDITOR_LINENUMBER_SWITCH variable
> I suggest: cannot edit a specific line because the
> EDITOR_LINENUMBER_SWITCH variable is not set

fixed
>
> I don't find the name get_dg_tag() to be very mnemonic.  How about
> get_function_dollarquote_tag()?
>

I used get_functiondef_dollarquote_tag

> In help.c, it looks like you've only added one line but incremented
> the pager count by three.
>

The original value for pager is probably wrong (not actual). I
rechecked real row numbers in external editor.

> In this bit:
>
> !                               dqtag = get_dq_tag(lines);
> !                               if (dqtag)
> !                               {
> !                                       free(dqtag);
> !                                       break;
> !                               }
> !                               else
> !                                       lineno++;
>
> The "else" is unnecessary.  And just after that:
>
> !                               if (end_of_line)
> !                                       lines = end_of_line + 1;
> !                               else
> !                                       break;
>
> You can write this more cleanly by saying if (!end_of_line) break;
> lines = end_of_line + 1.
>

fixed

> The following diff hunk (2213,2218) just adds a blank line and is
> therefore unnecessary.  There's a similar hunk in the docs portion of
> the patch.

fixed
>
> In this part:
>
>                        func = psql_scan_slash_option(scan_state,
>                                                                                  OT_WHOLE_LINE, NULL, true);
> !                       lineno = extract_lineno_from_funcdesc(func, &status);
> !
> !                       if (status != PSQL_CMD_ERROR)
>                        {
> !                               if (!func)
> !                               {
> !                                       /* set up an empty command to fill in */
> !                                       printfPQExpBuffer(query_buf,
> !                                                                         "CREATE FUNCTION ( )\n"
> !                                                                         " RETURNS \n"
> !                                                                         " LANGUAGE \n"
> !                                                                         " -- common options:  IMMUTABLE  STABLE  STRICT  SECURITY
> DEFINER\n"
> !                                                                         "AS $function$\n"
> !                                                                         "\n$function$\n");
> !                               }
> !                               else if (!lookup_function_oid(pset.db, func, &foid))
> !                               {
> !                                       /* error already reported */
> !                                       status = PSQL_CMD_ERROR;
> !                               }
> !                               else if (!get_create_function_cmd(pset.db, foid, query_buf))
> !                               {
> !                                       /* error already reported */
> !                                       status = PSQL_CMD_ERROR;
> !                               }
> !                               if (func)
> !                                       free(func);
>                        }
>
> Why is it correct for if (func) free(func) to be inside the if (status
> != PSQL_CMD_ERROR) block?  It looks to me like func gets initialized
> first, before status is set.

fixed

>
> It seems unnecessary for extract_lineno_from_funcdesc() to return the
> line number and have a separate out parameter to return a
> backslashResult.  Can't you just return -1 to indicate an error?  (You
> might need to move the if (!func) test at the top to the caller, but
> that seems OK.)

I can't to do it. There are three states
1) lineno is wrong
2) lineno is undefined
3) lineno is defined

@1 is solved via backslashResult, @2 lineno is negative, @3 lineno is positive

Regards

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-04T14:35:55Z

David Fetter <david@fetter.org> writes:
> On Mon, Aug 02, 2010 at 11:34:02PM -0400, Robert Haas wrote:
>> A side question is whether this should be an environment variable or
>> a psql variable.

> I'd say "yes."  As with $EDITOR/PSQL_EDITOR, there should be something
> that looks for an overriding psql variable, drops through to look for
> an environment variable, and then to a sane default, for some
> reasonable value of "sane."  Perhaps this default could depend on OS
> (Windows vs. Everything Else) to start with.

Well, the thing about $EDITOR is that it's a very-widely-understood
convention.  This one won't be, so the argument for making it an
environment variable seems pretty thin.  It'd be saner to set it in
your ~/.psqlrc file than to add another few nanoseconds to every
process launch you ever do.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Gregory Stark <gsstark@mit.edu> — 2010-08-05T00:50:53Z

On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Well, the thing about $EDITOR is that it's a very-widely-understood
> convention.  This one won't be, so the argument for making it an
> environment variable seems pretty thin.

Fwiw the +linenumber convention has been part of $EDITOR since
basically as long as vi has existed.


-- 
greg

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-05T01:32:35Z

On Wed, Aug 4, 2010 at 8:50 PM, Greg Stark <gsstark@mit.edu> wrote:
> On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, the thing about $EDITOR is that it's a very-widely-understood
>> convention.  This one won't be, so the argument for making it an
>> environment variable seems pretty thin.
>
> Fwiw the +linenumber convention has been part of $EDITOR since
> basically as long as vi has existed.

What precisely do you mean by that?  We've pretty much established
that the convention is nothing like universally accepted by the
editors that are out there.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-05T02:31:48Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Aug 4, 2010 at 8:50 PM, Greg Stark <gsstark@mit.edu> wrote:
>> On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Well, the thing about $EDITOR is that it's a very-widely-understood
>>> convention. This one won't be, so the argument for making it an
>>> environment variable seems pretty thin.
>> 
>> Fwiw the +linenumber convention has been part of $EDITOR since
>> basically as long as vi has existed.

> What precisely do you mean by that?  We've pretty much established
> that the convention is nothing like universally accepted by the
> editors that are out there.

More to the point, what I was saying is that there is no convention out
there for a second environment variable that tells programs calling
$EDITOR how to specify a linenumber argument.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-08T17:14:44Z

Pavel Stehule <pavel.stehule@gmail.com> writes:
> updated patch attached

What exactly is the point of the \sf command?  It seems like quite a lot
of added code for a feature that nobody has requested, and whose
definition is about as ad-hoc as could be.  Personally I'd much sooner
use \ef for looking at a function definition.  I think if \sf had been
submitted as a separate patch, rather than being snuck in with a feature
people do want, it wouldn't be accepted.

The current patch doesn't even compile warning-free :-(

command.c: In function `exec_command':
command.c:559: warning: `lineno' might be used uninitialized in this function
command.c: In function `editFile':
command.c:1729: warning: `editor_lineno_switch' might be used uninitialized in this function


			regards, tom lane

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-08T18:01:48Z

2010/8/8 Tom Lane <tgl@sss.pgh.pa.us>:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> updated patch attached
>
> What exactly is the point of the \sf command?  It seems like quite a lot
> of added code for a feature that nobody has requested, and whose
> definition is about as ad-hoc as could be.  Personally I'd much sooner
> use \ef for looking at a function definition.  I think if \sf had been
> submitted as a separate patch, rather than being snuck in with a feature
> people do want, it wouldn't be accepted.

I disagree. Now, you cannot to show a detail of function in well
readable form. Personally I prefer \sf+ form. Because I can see line
numbers, but \sf form is important for some copy paste operations. I
don't think, so \ef can replace \sf. It is based on my personal
experience. When I have to do some fast fix or fast decision I need to
see a source code of some functions (but I am in customer's
environment). Starting a external editor is slow and usually you can
not there to start your preferable editor.

If I return back then my first idea was to modify current \df command
to some practical form. Later in discussion was decided so new command
will be better.

>
> The current patch doesn't even compile warning-free :-(
>
> command.c: In function `exec_command':
> command.c:559: warning: `lineno' might be used uninitialized in this function
> command.c: In function `editFile':
> command.c:1729: warning: `editor_lineno_switch' might be used uninitialized in this function

there is some strange - it didn't find it in my environment. But I
recheck it tomorrow morning.

Regards

Pavel Stehule

>
>                        regards, tom lane
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-08T22:17:14Z

On Sun, Aug 8, 2010 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> updated patch attached
>
> What exactly is the point of the \sf command?  It seems like quite a lot
> of added code for a feature that nobody has requested, and whose
> definition is about as ad-hoc as could be.  Personally I'd much sooner
> use \ef for looking at a function definition.  I think if \sf had been
> submitted as a separate patch, rather than being snuck in with a feature
> people do want, it wouldn't be accepted.

I rather like \sf, actually; in fact, I think there's a decent
argument to be made that it's more useful than the line-numbering
stuff for \ef.  I don't particularly like the name "\sf", but that's
more because I think backslash commands are a fundamentally unscalable
approach to providing administrative functionality than because I
think there's a better option in this particular case.  It's rather
hard right now to get a function definition out of the database in
easily cut-and-pastable format.  pg_dump won't do it, unless you'd
like to dump the whole schema (I think we should add an option there,
too, actually).  Using \ef is reasonable but if the definition is more
than one page and you actually want to cut-and-paste it rather than
writing it to a file some place, it's not convenient.  (Hopefully you
understand the problem I'm talking about here: cut-and-paste can
scroll past one screen, but the editor doesn't actually write it out
that way; it displays it a page at a time.)  Now, admittedly, this is
only a minor convenience we're talking about: and if this get shot
down I won't cry into my beer, but I do think it's useful.

> The current patch doesn't even compile warning-free :-(
>
> command.c: In function `exec_command':
> command.c:559: warning: `lineno' might be used uninitialized in this function
> command.c: In function `editFile':
> command.c:1729: warning: `editor_lineno_switch' might be used uninitialized in this function

That obviously needs to be fixed.

(BTW, if you want to take this one instead of me, that's fine.
Otherwise, I'll get to it this week.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-09T03:38:25Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Aug 8, 2010 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What exactly is the point of the \sf command?

> I rather like \sf, actually; in fact, I think there's a decent
> argument to be made that it's more useful than the line-numbering
> stuff for \ef.  I don't particularly like the name "\sf", but that's
> more because I think backslash commands are a fundamentally unscalable
> approach to providing administrative functionality than because I
> think there's a better option in this particular case.  It's rather
> hard right now to get a function definition out of the database in
> easily cut-and-pastable format.

Um, but \sf *doesn't* give you anything that's usefully copy and
pasteable.  And if that were the goal, why doesn't it have an option to
write to a file?

But it's really the line numbers shoved in front that I'm on about here.
I can't see *any* use for that behavior except to figure out what part of
your function an error message with line number is referring to; and as
I said upthread, there are better ways to be attacking that problem.
If you've got a thousand-line function (yes, they're out there) do you
really want to be scrolling through \sf output to find out what line 714
is?

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-09T06:06:11Z

2010/8/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Aug 8, 2010 at 1:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> What exactly is the point of the \sf command?
>
>> I rather like \sf, actually; in fact, I think there's a decent
>> argument to be made that it's more useful than the line-numbering
>> stuff for \ef.  I don't particularly like the name "\sf", but that's
>> more because I think backslash commands are a fundamentally unscalable
>> approach to providing administrative functionality than because I
>> think there's a better option in this particular case.  It's rather
>> hard right now to get a function definition out of the database in
>> easily cut-and-pastable format.
>
> Um, but \sf *doesn't* give you anything that's usefully copy and
> pasteable.  And if that were the goal, why doesn't it have an option to
> write to a file?

there are not a line numbers. And you can't to use a result of \df+ too.

>
> But it's really the line numbers shoved in front that I'm on about here.
> I can't see *any* use for that behavior except to figure out what part of
> your function an error message with line number is referring to; and as
> I said upthread, there are better ways to be attacking that problem.
> If you've got a thousand-line function (yes, they're out there) do you
> really want to be scrolling through \sf output to find out what line 714

\sf supports a pager
\sf can show lines from entered number

so
\sf foo 700 -- show from line 700

Best regards

Pavel Stehule



> is?
>
>                        regards, tom lane
>

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-09T06:57:45Z

Hello

2010/8/8 Tom Lane <tgl@sss.pgh.pa.us>:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> updated patch attached
>
> What exactly is the point of the \sf command?  It seems like quite a lot
> of added code for a feature that nobody has requested, and whose
> definition is about as ad-hoc as could be.  Personally I'd much sooner
> use \ef for looking at a function definition.  I think if \sf had been
> submitted as a separate patch, rather than being snuck in with a feature
> people do want, it wouldn't be accepted.
>
> The current patch doesn't even compile warning-free :-(
>
> command.c: In function `exec_command':
> command.c:559: warning: `lineno' might be used uninitialized in this function
> command.c: In function `editFile':
> command.c:1729: warning: `editor_lineno_switch' might be used uninitialized in this function
>

This warnings depends on gcc version, probably :(. On new fedora I see
nothing. So updated patch attached - these variables are initialised
in declaration now.

Regards

Pavel Stehule

>
>                        regards, tom lane
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-09T10:51:08Z

On Sun, Aug 8, 2010 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Um, but \sf *doesn't* give you anything that's usefully copy and
> pasteable.

Works for me.

\sf ts_debug(regconfig, text)

> And if that were the goal, why doesn't it have an option to
> write to a file?

Well, you cut-and-paste from a terminal window, not a file, so that's
a slightly different problem, although perhaps also a good one to
solve.  But I'd rather see us solve that problem via some new pg_dump
functionality.

Hmm... or perhaps \sf should respect \o.  I notice that \d does.

> But it's really the line numbers shoved in front that I'm on about here.
> I can't see *any* use for that behavior except to figure out what part of
> your function an error message with line number is referring to; and as
> I said upthread, there are better ways to be attacking that problem.
> If you've got a thousand-line function (yes, they're out there) do you
> really want to be scrolling through \sf output to find out what line 714
> is?

Well, as Pavel points out, I guess you could use the "line number"
argument to \sf to start at around the place you're interested in,
athough I suspect that I would probably choose to use \ef in that
case.  I suspect \sf is in general most useful with somewhat shorter
functions (I'd copy and paste a 100 line function, perhaps, but for a
1000 line function I'd probably try to get the definition into a file
and scp it or whatever).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-09T11:40:40Z

2010/8/9 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Aug 8, 2010 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Um, but \sf *doesn't* give you anything that's usefully copy and
>> pasteable.
>
> Works for me.
>
> \sf ts_debug(regconfig, text)
>
>> And if that were the goal, why doesn't it have an option to
>> write to a file?
>
> Well, you cut-and-paste from a terminal window, not a file, so that's
> a slightly different problem, although perhaps also a good one to
> solve.  But I'd rather see us solve that problem via some new pg_dump
> functionality.
>
> Hmm... or perhaps \sf should respect \o.  I notice that \d does.

it's not a bad idea.

updated patch attached

>
>> But it's really the line numbers shoved in front that I'm on about here.
>> I can't see *any* use for that behavior except to figure out what part of
>> your function an error message with line number is referring to; and as
>> I said upthread, there are better ways to be attacking that problem.
>> If you've got a thousand-line function (yes, they're out there) do you
>> really want to be scrolling through \sf output to find out what line 714
>> is?
>
> Well, as Pavel points out, I guess you could use the "line number"
> argument to \sf to start at around the place you're interested in,
> athough I suspect that I would probably choose to use \ef in that
> case.  I suspect \sf is in general most useful with somewhat shorter
> functions (I'd copy and paste a 100 line function, perhaps, but for a
> 1000 line function I'd probably try to get the definition into a file
> and scp it or whatever).
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

David Wheeler <david@kineticode.com> — 2010-08-09T15:28:57Z

On Aug 8, 2010, at 8:38 PM, Tom Lane wrote:

> Um, but \sf *doesn't* give you anything that's usefully copy and
> pasteable.  And if that were the goal, why doesn't it have an option to
> write to a file?
> 
> But it's really the line numbers shoved in front that I'm on about here.
> I can't see *any* use for that behavior except to figure out what part of
> your function an error message with line number is referring to; and as
> I said upthread, there are better ways to be attacking that problem.
> If you've got a thousand-line function (yes, they're out there) do you
> really want to be scrolling through \sf output to find out what line 714
> is?

Suggestion:

\sf without line numbers
\sf+ with line numbers

Best,

David

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-09T15:30:24Z

2010/8/9 David E. Wheeler <david@kineticode.com>:
> On Aug 8, 2010, at 8:38 PM, Tom Lane wrote:
>
>> Um, but \sf *doesn't* give you anything that's usefully copy and
>> pasteable.  And if that were the goal, why doesn't it have an option to
>> write to a file?
>>
>> But it's really the line numbers shoved in front that I'm on about here.
>> I can't see *any* use for that behavior except to figure out what part of
>> your function an error message with line number is referring to; and as
>> I said upthread, there are better ways to be attacking that problem.
>> If you've got a thousand-line function (yes, they're out there) do you
>> really want to be scrolling through \sf output to find out what line 714
>> is?
>
> Suggestion:
>
> \sf without line numbers
> \sf+ with line numbers

it did it :)

Pavel

>
> Best,
>
> David
>

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-11T03:39:55Z

On Mon, Aug 9, 2010 at 7:40 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> updated patch attached

I spent some time cleaning this up tonight.  I think that the \e and
\ef portions are now ready to commit, but I am not quite happy with
the \sf stuff yet, so I've broken that out into a separate patch,
which is also attached.

Barring objections, I'll commit the \e and \ef portions of this in the
morning after one final read-through.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-11T03:58:12Z

Robert Haas <robertmhaas@gmail.com> writes:
> I spent some time cleaning this up tonight.  I think that the \e and
> \ef portions are now ready to commit, but I am not quite happy with
> the \sf stuff yet, so I've broken that out into a separate patch,
> which is also attached.

> Barring objections, I'll commit the \e and \ef portions of this in the
> morning after one final read-through.

The \e patch definitely needs another read-through.  I noticed a number
of comments that were still pretty poor English, and one ---
	/* skip header lines */
--- that seems just plain wrong.  The actual intent of that next bit is
to increase lineno to account for header lines, which is not well
conveyed by "skip".

BTW, at least in the usage in that loop, get_functiondef_dollarquote_tag
seems grossly overdesigned.  It would be clearer, shorter, and faster if
you just had a strncmp test for "AS $function" there.  Also, the entire
thing is subject to misbehavior in the case of \e (as opposed to \ef),
which really cannot safely assert() that it's reading the output of
pg_get_functiondef().  My inclination is to pull that part out of
do_edit and put it into \ef-specific code.

Also, there seemed to be some gratuitous inconsistency in the handling
of tests on line number variables, eg some places lineno > 0 and others
lineno >= 1.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-11T13:14:45Z

On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The \e patch definitely needs another read-through.  I noticed a number
> of comments that were still pretty poor English, and one ---
>        /* skip header lines */
> --- that seems just plain wrong.  The actual intent of that next bit is
> to increase lineno to account for header lines, which is not well
> conveyed by "skip".

Interestingly, I had already rewritten pretty much every comment in
the patch, and the entirety of the documentation, but I found a very
small number of stragglers this morning and made a few more
adjustments.  If you're still unhappy with it, you're going to need to
be more specific, or hack on it yourself.

> BTW, at least in the usage in that loop, get_functiondef_dollarquote_tag
> seems grossly overdesigned.  It would be clearer, shorter, and faster if
> you just had a strncmp test for "AS $function" there.

As far as I can see, the only purpose of that code is to support the
desire to have \sf+ display **** rather than a line number for the
lines that FOLLOW the function body.  But I'm wondering if we should
just forget about that and let the numbering run continuously from the
first "AS $function" line to end of file.  That would get rid of a
bunch of rather grotty code in the \sf patch, also.

> Also, the entire
> thing is subject to misbehavior in the case of \e (as opposed to \ef),
> which really cannot safely assert() that it's reading the output of
> pg_get_functiondef().  My inclination is to pull that part out of
> do_edit and put it into \ef-specific code.

Oh, for pity's sake.  I had thought that code WAS \ef-specific
(because it doesn't make any sense otherwise) but I see that you are
correct.

> Also, there seemed to be some gratuitous inconsistency in the handling
> of tests on line number variables, eg some places lineno > 0 and others
> lineno >= 1.

I think this is now fixed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-11T16:28:44Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, at least in the usage in that loop, get_functiondef_dollarquote_tag
>> seems grossly overdesigned. It would be clearer, shorter, and faster if
>> you just had a strncmp test for "AS $function" there.

> As far as I can see, the only purpose of that code is to support the
> desire to have \sf+ display **** rather than a line number for the
> lines that FOLLOW the function body.  But I'm wondering if we should
> just forget about that and let the numbering run continuously from the
> first "AS $function" line to end of file.  That would get rid of a
> bunch of rather grotty code in the \sf patch, also.

Oh?  Considering that in the standard pg_get_functiondef output, the
ending $function$ delimiter is always on the very last line, that sounds
pretty useless.  +1 for just numbering forward from the start line.

BTW, the last I looked, \sf+ was using what I thought to be a quite ugly
and poorly-considered formatting for the line number.  I would suggest
eight blanks for a header line and "%-7d " as the prefix format for a
numbered line.  The reason for making sure the prefix is 8 columns rather
than some other width is to not mess up tab-based formatting of the
function body.  I would also prefer a lot more visual separation between
the line number and the code than "%4d " will offer; and as for the
stars, they're just useless and distracting.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-11T16:38:08Z

On Wed, Aug 11, 2010 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> BTW, at least in the usage in that loop, get_functiondef_dollarquote_tag
>>> seems grossly overdesigned.  It would be clearer, shorter, and faster if
>>> you just had a strncmp test for "AS $function" there.
>
>> As far as I can see, the only purpose of that code is to support the
>> desire to have \sf+ display **** rather than a line number for the
>> lines that FOLLOW the function body.  But I'm wondering if we should
>> just forget about that and let the numbering run continuously from the
>> first "AS $function" line to end of file.  That would get rid of a
>> bunch of rather grotty code in the \sf patch, also.
>
> Oh?  Considering that in the standard pg_get_functiondef output, the
> ending $function$ delimiter is always on the very last line, that sounds
> pretty useless.  +1 for just numbering forward from the start line.

OK.

> BTW, the last I looked, \sf+ was using what I thought to be a quite ugly
> and poorly-considered formatting for the line number.  I would suggest
> eight blanks for a header line and "%-7d " as the prefix format for a
> numbered line.  The reason for making sure the prefix is 8 columns rather
> than some other width is to not mess up tab-based formatting of the
> function body.  I would also prefer a lot more visual separation between
> the line number and the code than "%4d " will offer; and as for the
> stars, they're just useless and distracting.

I don't have a strong preference, but that seems reasonable.  I
suggest that we punt the \sf portion of this patch back for rework for
the next CommitFest, and focus on getting the \e and \ef changes
committed.  I think the \sf code can be a lot simpler if we get rid of
the code that's intended to recognize the ending delimeter.

Another thought is that we might want to add a comment to
pg_get_functiondef() noting that anyone changing the output format
should be careful not to break the line-number-finding form of \ef in
the process.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-11T22:21:02Z

Robert Haas <robertmhaas@gmail.com> writes:
> ...  If you're still unhappy with it, you're going to need to
> be more specific, or hack on it yourself.

I'm doing another pass over this.  I notice that the documentation
claims the syntax of \e is "\e [FILE] [LINE]", but what is actually
implemented is "\e [FILE [LINE]]", ie it is not possible to specify a
line number without a file.  Now, it seems to me that specifying a line
number in the query buffer would actually be a pretty darn useful thing
to do, if you'd typed in a large query and the backend had spit back
"LINE 42: some problem or other".  So I think we should fix it so that
case works and the documentation isn't lying.  This would require
interpreting \e followed by a digit string as a line number not a file
... anybody have a problem with that?  If you're really eager to edit a
numerically-named file you could fake it out with "\e 1234 1".

BTW, there doesn't seem to be a need to do anything similar for \ef.
It does have the ability to omit a func name, but then you get a blank
CREATE FUNCTION template you're going to have to fill in, so there's
no advantage to positioning the cursor beyond the first line to start.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Robert Haas <robertmhaas@gmail.com> — 2010-08-11T23:53:08Z

On Wed, Aug 11, 2010 at 6:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ...  If you're still unhappy with it, you're going to need to
>> be more specific, or hack on it yourself.
>
> I'm doing another pass over this.  I notice that the documentation
> claims the syntax of \e is "\e [FILE] [LINE]", but what is actually
> implemented is "\e [FILE [LINE]]", ie it is not possible to specify a
> line number without a file.  Now, it seems to me that specifying a line
> number in the query buffer would actually be a pretty darn useful thing
> to do, if you'd typed in a large query and the backend had spit back
> "LINE 42: some problem or other".  So I think we should fix it so that
> case works and the documentation isn't lying.  This would require
> interpreting \e followed by a digit string as a line number not a file
> ... anybody have a problem with that?  If you're really eager to edit a
> numerically-named file you could fake it out with "\e 1234 1".

Or \e ./1234

It's a minor incompatibility, but it's probably reasonable to allow that.

> BTW, there doesn't seem to be a need to do anything similar for \ef.
> It does have the ability to omit a func name, but then you get a blank
> CREATE FUNCTION template you're going to have to fill in, so there's
> no advantage to positioning the cursor beyond the first line to start.

Hmm, OK.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-12T00:43:20Z

Robert Haas <robertmhaas@gmail.com> writes:
> I suggest that we punt the \sf portion of this patch back for rework for
> the next CommitFest, and focus on getting the \e and \ef changes
> committed.  I think the \sf code can be a lot simpler if we get rid of
> the code that's intended to recognize the ending delimeter.

I've committed the \e/\ef part after some further tweaking.  I concur
with marking the \sf part as Returned With Feedback.

> Another thought is that we might want to add a comment to
> pg_get_functiondef() noting that anyone changing the output format
> should be careful not to break the line-number-finding form of \ef in
> the process.

Done.

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-12T06:26:43Z

2010/8/11 Robert Haas <robertmhaas@gmail.com>:
> On Wed, Aug 11, 2010 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> BTW, at least in the usage in that loop, get_functiondef_dollarquote_tag
>>>> seems grossly overdesigned.  It would be clearer, shorter, and faster if
>>>> you just had a strncmp test for "AS $function" there.
>>
>>> As far as I can see, the only purpose of that code is to support the
>>> desire to have \sf+ display **** rather than a line number for the
>>> lines that FOLLOW the function body.  But I'm wondering if we should
>>> just forget about that and let the numbering run continuously from the
>>> first "AS $function" line to end of file.  That would get rid of a
>>> bunch of rather grotty code in the \sf patch, also.
>>
>> Oh?  Considering that in the standard pg_get_functiondef output, the
>> ending $function$ delimiter is always on the very last line, that sounds
>> pretty useless.  +1 for just numbering forward from the start line.
>
> OK.
>
>> BTW, the last I looked, \sf+ was using what I thought to be a quite ugly
>> and poorly-considered formatting for the line number.  I would suggest
>> eight blanks for a header line and "%-7d " as the prefix format for a
>> numbered line.  The reason for making sure the prefix is 8 columns rather
>> than some other width is to not mess up tab-based formatting of the
>> function body.  I would also prefer a lot more visual separation between
>> the line number and the code than "%4d " will offer; and as for the
>> stars, they're just useless and distracting.
>
> I don't have a strong preference, but that seems reasonable.  I
> suggest that we punt the \sf portion of this patch back for rework for
> the next CommitFest, and focus on getting the \e and \ef changes
> committed.  I think the \sf code can be a lot simpler if we get rid of
> the code that's intended to recognize the ending delimeter.
>

the proposed changes are not complex, and there are not reason to move
\sf to next commitfest. I am thinking about little bit simplification
- there can by only one cycle without two. After \e commiting there
are other complex code. If some code isn't  clean, then it is because
there are  \o and pager support.

> Another thought is that we might want to add a comment to
> pg_get_functiondef() noting that anyone changing the output format
> should be careful not to break the line-number-finding form of \ef in
> the process.
>

+1

Regards

Pavel Stehule

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Re: review: psql: edit function, show function commands patch

Pavel Stehule <pavel.stehule@gmail.com> — 2010-08-12T13:16:49Z

Hello

attached updated \sf implementation. It is little bit simplyfied with
support a pager and output forwarding. Formating was updated per Tom's
request.

Regards

Pavel Stehule

>
> BTW, the last I looked, \sf+ was using what I thought to be a quite ugly
> and poorly-considered formatting for the line number.  I would suggest
> eight blanks for a header line and "%-7d " as the prefix format for a
> numbered line.  The reason for making sure the prefix is 8 columns rather
> than some other width is to not mess up tab-based formatting of the
> function body.  I would also prefer a lot more visual separation between
> the line number and the code than "%4d " will offer; and as for the
> stars, they're just useless and distracting.
>
>                        regards, tom lane
>

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-13T19:56:09Z

Pavel Stehule <pavel.stehule@gmail.com> writes:
> attached updated \sf implementation. It is little bit simplyfied with
> support a pager and output forwarding.

The line number argument to this greatly complicates the code but
doesn't appear to me to have much practical use.  Why would you bother
with that?

			regards, tom lane

Re: review: psql: edit function, show function commands patch

Tom Lane <tgl@sss.pgh.pa.us> — 2010-08-14T14:09:04Z

Pavel Stehule <pavel.stehule@gmail.com> writes:
> attached updated \sf implementation. It is little bit simplyfied with
> support a pager and output forwarding. Formating was updated per Tom's
> request.

Applied with corrections --- mostly, fixing it to not trash the query
buffer, which would certainly not be expected behavior for such a
command.

Also, as previously mentioned, I took out the line number argument,
which seemed to me to have little if any use-case while substantially
complicating the code.  (It's not so much that it cost a lot in the
patch as submitted, it's that it *would* cost a lot if you were
honoring it in the pager calculation...)

One other thing: I took a look at the pg_get_functiondef() code and
realized that in fact it does NOT guarantee that the function body
will start with "AS $function...".  In languages with a nonnull
probin field, that's not what the line will look like.  I think though
that looking for just "AS " at the start of the line is sufficient
for our purposes here.  I will go change the \ef and \sf code for
that.

			regards, tom lane