review: psql: edit function, show function commands patch

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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