Prev: lock_timeout GUC patch - Review
Next: Some git conversion issues
From: Pavel Stehule on 20 Jul 2010 08:58
Hello

2010/7/16 Jan Urbański <wulczer(a)wulczer.org>:
> Hi,
>
> here's a review of the \sf and \ef [num] patch from
> http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19de29(a)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
>

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Pavel Stehule on 21 Jul 2010 08:43
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(a)wulczer.org>:
> Hi,
>
> here's a review of the \sf and \ef [num] patch from
> http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19de29(a)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
>
From: Dimitri Fontaine on 21 Jul 2010 17:04
Pavel Stehule <pavel.stehule(a)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

From: Robert Haas on 21 Jul 2010 23:01
On Fri, Jul 16, 2010 at 10:29 AM, Jan Urbański <wulczer(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= on 22 Jul 2010 18:56
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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3 4 5 6 7
Prev: lock_timeout GUC patch - Review
Next: Some git conversion issues