Thread: review: psql: edit function, show function commands patch
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
Attachment
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 >
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 >
Attachment
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
Attachment
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
Attachment
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 > > Thanks, > Jan >
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
thinksof 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 > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise Postgres Company >
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 > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise Postgres Company >
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 > EnterpriseDB: http://www.enterprisedb.com > The Enterprise Postgres Company >
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
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
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 >
Attachment
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
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
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
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
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
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
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 >
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 >
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 >
Attachment
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 >> >
Attachment
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 >>> >> >
Attachment
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
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
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
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
>
Attachment
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
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
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
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
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
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 >
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
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
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 >
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 >
Attachment
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
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 >
Attachment
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
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 >
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
Attachment
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
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
Attachment
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
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
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
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
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
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 >
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 >
Attachment
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
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