Re: [HACKERS] review: psql: edit function, show function commands patch
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/11 Robert Haas robertmh...@gmail.com: On Wed, Aug 11, 2010 at 12:28 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
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
*** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-08-12 02:40:59.0 +0200
--- ./doc/src/sgml/ref/psql-ref.sgml 2010-08-12 15:01:04.339404200 +0200
***
*** 2100,2105
--- 2100,2131
varlistentry
+ termliteral\sf[+] optional replaceable class=parameterfunction_description/ optional replaceable class=parameterline_number/ /optional /optional /literal/term
+
+ listitem
+ para
+ This command fetches and shows the definition of the named function,
+ in the form of a commandCREATE OR REPLACE FUNCTION/ command.
+ If literal+/literal is appended to the command name, then output
+ lines has a line number.
+ /para
+
+ para
+ The target function can be specified by name alone, or by name
+ and arguments, for example literalfoo(integer, text)/.
+ The argument types must be given if there is more
+ than one function of the same name.
+ /para
+
+ para
+ If a line number is specified, applicationpsql/application will
+ show the specified line as first line. Previous lines are skiped.
+ /para
+ /listitem
+ /varlistentry
+
+
+ varlistentry
termliteral\set [ replaceable class=parametername/replaceable [ replaceable class=parametervalue/replaceable [ ... ] ] ]/literal/term
listitem
*** ./src/bin/psql/command.c.orig 2010-08-12 02:40:59.0 +0200
--- ./src/bin/psql/command.c 2010-08-12 14:39:22.334403954 +0200
***
*** 32,37
--- 32,38
#ifdef USE_SSL
#include openssl/ssl.h
#endif
+ #include signal.h
#include portability/instr_time.h
***
*** 46,51
--- 47,53
#include input.h
#include large_obj.h
#include mainloop.h
+ #include pqsignal.h
#include print.h
#include psqlscan.h
#include settings.h
***
*** 1083,1088
--- 1085,1232
free(opt0);
}
+ /* \sf = show a function source code */
+ else if (strcmp(cmd, sf) == 0 || strcmp(cmd, sf+) == 0)
+ {
+ bool show_lineno;
+ int first_visible_line = -1;
+
+ show_lineno = (strcmp(cmd, sf+) == 0);
+
+ if (!query_buf)
+ {
+ psql_error(no query buffer\n);
+ status = PSQL_CMD_ERROR;
+ }
+ else
+ {
+ char *func;
+ Oid foid = InvalidOid;
+
+ func = psql_scan_slash_option(scan_state,
+ OT_WHOLE_LINE, NULL, true);
+ first_visible_line = strip_lineno_from_funcdesc(func);
+ if (first_visible_line == 0)
+ {
+ /* error already reported */
+ status = PSQL_CMD_ERROR;
+ }
+ else if (!func)
+ {
+ psql_error(missing a function name\n);
+ status = PSQL_CMD_ERROR;
+ }
+ 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);
+
+ if (status != PSQL_CMD_ERROR)
+ {
+ FILE *output;
+ bool is_pager = false;
+
+ /*
+ * Count a lines in function definition - it's used for opening
+ * a pager. Get a output stream - stdout, pager or forwarded output.
+ */
+ if (pset.queryFout == stdout)
+ {
+ int lc = 0;
+ const char *lines = query_buf-data;
+
+ while (*lines != '\0')
+ {
+ lc++;
+ /* find start of next line */
+ lines = strchr(lines, '\n');
+ if (!lines)
+ break;
+ lines++;
+ }
+
+ output = PageOutput(lc, pset.popt.topt.pager);
+ is_pager = output != stdout;
+ }
+ else
+ {
+ /* use a prepared query output, pager isn't activated */
+ output = pset.queryFout;
+ is_pager = false;
+ }
+
+ if (first_visible_line 0 || show_lineno)
+ {
+ bool is_header = true; /* true, when header lines is processed */
+ int lineno = 0;
+ char *lines = query_buf-data;
+
+ /*
+ * lineno 1 should correspond to the first line of the function
+ * body. We expect that pg_get_functiondef()
Re: [HACKERS] review: psql: edit function, show function commands patch
On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane t...@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 edit8-rmh-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Wed, Aug 11, 2010 at 12:28 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Wed, Aug 11, 2010 at 6:21 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Mon, Aug 9, 2010 at 7:40 AM, Pavel Stehule pavel.steh...@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 edit8-rmh.patch Description: Binary data sf.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/9 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Sun, Aug 8, 2010 at 1:14 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Hello 2010/8/8 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@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 *** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-08-03 09:00:48.384710383 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-08-03 10:44:57.312835131 +0200 *** *** 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1369,1380 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1369,1387 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). + /para /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional optional linenumber /optional /literal/term listitem para *** *** 1397,1402 --- 1404,1417 If no function is specified, a blank commandCREATE FUNCTION/ template is presented for editing. /para + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. It count lines from start of function body, not from + start of text (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). + /para /listitem /varlistentry *** *** 2116,2121 --- 2131,2148 varlistentry + termliteral\sf[+] replaceable class=parameterfunction_description/replaceable optional linenumber /optional /literal/term + + listitem + para + This command fetches and shows the definition of the named function, + in the form of a commandCREATE OR REPLACE FUNCTION/ command. + If the form literal\sf+/literal is used, then lines are numbered. + /para + /listitem + /varlistentry + + varlistentry termliteral\t/literal/term listitem para *** *** 2123,2128 --- 2150,2161 footer. This command is equivalent to literal\pset tuples_only/literal and is provided for convenience. /para + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. + /para /listitem /varlistentry *** *** 2459,2464 --- 2492,2511 /varlistentry varlistentry + termvarnameEDITOR_LINENUMBER_SWITCH/varname/term + listitem + para + Option used for navigation (go to line command) in external + editor. When it isn't defined, then you cannot to specify + line numbers for command\edit/command and command\ef/command + commands. On unix platforms are possible to use a 'option+/option' + or 'option--line /option'. The space after literalline/literal + is required. + /para + /listitem +
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 8, 2010 at 11:38 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/9 Robert Haas robertmh...@gmail.com: On Sun, Aug 8, 2010 at 11:38 PM, Tom Lane t...@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 *** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-08-03 09:00:48.384710383 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-08-03 10:44:57.312835131 +0200 *** *** 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1369,1380 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1369,1387 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). + /para /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional optional linenumber /optional /literal/term listitem para *** *** 1397,1402 --- 1404,1417 If no function is specified, a blank commandCREATE FUNCTION/ template is presented for editing. /para + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. It count lines from start of function body, not from + start of text (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). + /para /listitem /varlistentry *** *** 2116,2121 --- 2131,2148 varlistentry + termliteral\sf[+] replaceable class=parameterfunction_description/replaceable optional linenumber /optional /literal/term + + listitem + para + This command fetches and shows the definition of the named function, + in the form of a commandCREATE OR REPLACE FUNCTION/ command. + If the form literal\sf+/literal is used, then lines are numbered. + /para + /listitem + /varlistentry + + varlistentry termliteral\t/literal/term listitem para *** *** 2123,2128 --- 2150,2161 footer. This command is equivalent to literal\pset tuples_only/literal and is provided for convenience. /para + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. + /para /listitem /varlistentry *** *** 2459,2464 --- 2492,2511
Re: [HACKERS] review: psql: edit function, show function commands patch
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/9 David E. Wheeler da...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/8 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 8, 2010 at 1:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: On Sun, Aug 8, 2010 at 1:14 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule pavel.steh...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Mon, Aug 02, 2010 at 11:34:02PM -0400, Robert Haas wrote: On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane t...@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 da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Wed, Aug 4, 2010 at 10:10 AM, David Fetter da...@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 t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Hello
updated patch attached
2010/8/4 Robert Haas robertmh...@gmail.com:
On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule pavel.steh...@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)
Re: [HACKERS] review: psql: edit function, show function commands patch
David Fetter da...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Wed, Aug 4, 2010 at 8:50 PM, Greg Stark gsst...@mit.edu wrote: On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: On Wed, Aug 4, 2010 at 8:50 PM, Greg Stark gsst...@mit.edu wrote: On Wed, Aug 4, 2010 at 3:35 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
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 robertmh...@gmail.com: On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas robertmh...@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 *** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-08-03 09:00:48.384710383 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-08-03 10:44:57.312835131 +0200 *** *** 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1369,1380 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1369,1387 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). + /para /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional optional linenumber /optional /literal/term listitem para *** *** 1397,1402 --- 1404,1417 If no function is specified, a
Re: [HACKERS] review: psql: edit function, show function commands patch
Hello 2010/8/3 Pavel Stehule pavel.steh...@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 robertmh...@gmail.com: On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas robertmh...@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 *** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-08-03 09:00:48.384710383 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-08-03 10:44:57.312835131 +0200 *** *** 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1369,1380 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1369,1387 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). + /para /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional
Re: [HACKERS] review: psql: edit function, show function commands patch
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.steh...@gmail.com: Hello 2010/8/3 Pavel Stehule pavel.steh...@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 robertmh...@gmail.com: On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas robertmh...@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 *** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-08-03 09:00:48.384710383 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-08-03 10:44:57.312835131 +0200 *** *** 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1369,1380 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1369,1387 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor (The psql's variable varnameEDITOR_LINENUMBER_SWITCH/varname + have to be filled). +
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/3 Robert Haas robertmh...@gmail.com: On Sun, Aug 1, 2010 at 11:48 PM, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/3 Robert Haas robertmh...@gmail.com: On Mon, Aug 2, 2010 at 11:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Aug 2, 2010 at 10:49 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 1, 2010 at 12:15 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2010/8/1 Robert Haas robertmh...@gmail.com: On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule pavel.steh...@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 417enter and \ef fooenter417G 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/1 Robert Haas robertmh...@gmail.com: On Sun, Aug 1, 2010 at 12:15 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2010/8/1 Robert Haas robertmh...@gmail.com: On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule pavel.steh...@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 417enter and \ef fooenter417G 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/1 Robert Haas robertmh...@gmail.com: On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 1, 2010 at 11:35 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: On Sun, Aug 1, 2010 at 11:35 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
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 robertmh...@gmail.com: On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane t...@sss.pgh.pa.us wrote: Pavel Stehule pavel.steh...@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 *** ./src/pl/plpgsql/src/pl_scanner.c.orig 2010-02-26 03:01:35.0 +0100 --- ./src/pl/plpgsql/src/pl_scanner.c 2010-08-01 20:56:35.0 +0200 *** *** 519,537 cur_line_start = scanorig; cur_line_num = 1; - /*-- - * Hack: skip any initial newline, so that in the common coding layout - * CREATE FUNCTION ... AS $$ - * code body - * $$ LANGUAGE plpgsql; - * we will think line 1 is what the programmer thinks of as line 1. - *-- - */ - if (*cur_line_start == '\r') - cur_line_start++; - if (*cur_line_start == '\n') - cur_line_start++; - cur_line_end = strchr(cur_line_start, '\n'); } --- 519,524 *** ./src/test/regress/expected/domain.out.orig 2008-06-11 23:53:49.0 +0200 --- ./src/test/regress/expected/domain.out 2010-08-01 20:57:33.0 +0200 *** *** 436,442 end$$ language plpgsql; select doubledecrement(3); -- fail because of implicit null assignment ERROR: domain pos_int does not allow null values ! CONTEXT: PL/pgSQL function doubledecrement line 2 during statement block local variable initialization create or replace function doubledecrement(p1 pos_int) returns pos_int as $$ declare v pos_int := 0; begin --- 436,442 end$$ language plpgsql; select doubledecrement(3); -- fail because of implicit null assignment ERROR: domain pos_int does not allow null values ! CONTEXT: PL/pgSQL function doubledecrement line 3 during statement block local variable initialization create or replace function doubledecrement(p1 pos_int) returns pos_int as $$ declare v pos_int := 0; begin *** *** 444,450 end$$ language plpgsql; select doubledecrement(3); -- fail at initialization assignment ERROR: value for domain pos_int violates check constraint pos_int_check ! CONTEXT: PL/pgSQL function doubledecrement line 2 during statement block local variable initialization create or replace function doubledecrement(p1 pos_int) returns pos_int as $$ declare v pos_int := 1; begin --- 444,450 end$$ language plpgsql; select doubledecrement(3); -- fail at initialization assignment ERROR: value for domain pos_int violates check constraint pos_int_check ! CONTEXT: PL/pgSQL function doubledecrement line 3 during statement block local variable initialization create or replace function doubledecrement(p1 pos_int) returns pos_int as $$ declare v pos_int := 1; begin *** *** 457,463 ERROR: value for domain pos_int violates check constraint pos_int_check select doubledecrement(1); -- fail at assignment to v ERROR: value for domain pos_int violates check constraint pos_int_check ! CONTEXT: PL/pgSQL function doubledecrement line 3 at assignment select doubledecrement(2); -- fail at return ERROR: value for domain pos_int violates check constraint pos_int_check CONTEXT: PL/pgSQL function doubledecrement while casting return value to function's return type --- 457,463 ERROR: value for domain pos_int violates check constraint pos_int_check select doubledecrement(1); -- fail at assignment to v ERROR: value for domain pos_int violates check constraint pos_int_check ! CONTEXT: PL/pgSQL function doubledecrement line 4 at assignment select doubledecrement(2); -- fail at return ERROR: value
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Aug 1, 2010 at 4:53 PM, Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Robert Haas robertmh...@gmail.com writes: I took a look at this tonight and am a bit mystified by the following bit: + /* +* PL doesn't calculate first row of function's body +* when first row is empty. So checks first row, and +* correct lineno when it is necessary. +*/ Is that true of any PL, or just some particular PL? plpgsql has an old bit of logic that deliberately ignores an initial newline in the function body: /*-- * Hack: skip any initial newline, so that in the common coding layout *CREATE FUNCTION ... AS $$ *code body *$$ LANGUAGE plpgsql; * we will think line 1 is what the programmer thinks of as line 1. *-- */ if (*cur_line_start == '\r') cur_line_start++; if (*cur_line_start == '\n') cur_line_start++; None of the other standard PLs do that AFAIK. Is it documented in our documentation? I don't think so. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/8/1 Robert Haas robertmh...@gmail.com: On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On 23/07/10 20:55, Pavel Stehule wrote: Hello 2010/7/23 Jan Urbański wulc...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
2010/7/25 Jan Urbański wulc...@wulczer.org: On 23/07/10 20:55, Pavel Stehule wrote: Hello 2010/7/23 Jan Urbański wulc...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Hello 2010/7/23 Jan Urbański wulc...@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 *** ./doc/src/sgml/ref/psql-ref.sgml.orig 2010-07-20 05:54:19.0 +0200 --- ./doc/src/sgml/ref/psql-ref.sgml 2010-07-23 20:46:49.746690828 +0200 *** *** 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1339,1345 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1369,1380 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1369,1386 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. + /para /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional optional linenumber /optional /literal/term listitem para *** *** 1397,1402 --- 1403,1415 If no function is specified, a blank commandCREATE FUNCTION/ template is presented for editing. /para + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved
Re: [HACKERS] review: psql: edit function, show function commands patch
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 2return
10/0; 3 end; $function$
postgres=# select foo(); ERROR: division by zero CONTEXT: SQL
statement SELECT 10/0 PL/pgSQL function foo line 2 at RETURN
postgres=#
OK, that convinced me, and also others seem to like the feature. So I'll
just make code remarks this time.
In the \e handling code, if the file name was given and there is no line
number, an uninitialised variable will be passed to do_edit. I see that
you want to avoid passing a magic number to do_edit, but I think you
should just treat anything 0 as that magic value, initialise lineno
with -1 and simplify all these nested ifs.
Typo in a comment:
when wirst row of function - when first row of function
I think the #define for DEFAULT_BODY_SEPARATOR should either be at the
beginning of the file (and then also used in \ef handling) or just
hardcoded in both places.
Any reason why DEFAULT_NAVIGATION_COMMAND has a space in front of it?
Some lines have 80 characters.
If you agree that a -1 parameter do do_edit will mean no lineno, then
I think you can change get_lineno_for_navigation to not take a
backslashResult argument and signal errors by returning -1.
In get_lineno_for_navigation you will have to protect the large comment
block with /*-- otherwise pgindent will reflow it.
PSQL_NAVIGATION_COMMAND needs to be documented in the SGML docs.
Uff, that's all from me, sorry for bringing up all these small issues, I
hope this will go in soon!
I'm going to change it to waiting on author again, mainly because of the
uninitialised variable in \d handling, the rest are just stylistic nitpicks.
Cheers,
Jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Thu, Jul 22, 2010 at 6:56 PM, Jan Urbański wulc...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
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 2return 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 wulc...@wulczer.org: Hi, here's a review of the \sf and \ef [num] patch from http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19d...@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 editfce.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Pavel Stehule pavel.steh...@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
dim-pgsql.el
Description: pgsql setup for emacs
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
On Fri, Jul 16, 2010 at 10:29 AM, Jan Urbański wulc...@wulczer.org wrote: The patch adds the following features: * \e file.txt num - starts a editor for the current query buffer and puts the cursor on the [num] line * \ef func num - starts a editor for a function and puts the cursor on the [num] line * \sf func - shows a full CREATE FUNCTION statement for the function * \sf+ func - the same, but with line numbers * \sf[+] func num - the same, but only from line num onward It only touches psql, so no performance or backend stability worries. In my humble opinion, only the \sf[+] is interesting, because it gives you a FWIW, I think this is all pretty useful. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] review: psql: edit function, show function commands patch
Hello 2010/7/16 Jan Urbański wulc...@wulczer.org: Hi, here's a review of the \sf and \ef [num] patch from http://archives.postgresql.org/message-id/162867791003290927y3ca44051p80e697bc6b19d...@mail.gmail.com == Formatting == The patch has some small tabs/spaces and whitespace issues and it applies with some offsets, I ran pgindent and rebased against HEAD, attaching the resulting patch for your convenience. == Functionality == The patch adds the following features: * \e file.txt num - starts a editor for the current query buffer and puts the cursor on the [num] line * \ef func num - starts a editor for a function and puts the cursor on the [num] line * \sf func - shows a full CREATE FUNCTION statement for the function * \sf+ func - the same, but with line numbers * \sf[+] func num - the same, but only from line num onward It only touches psql, so no performance or backend stability worries. In my humble opinion, only the \sf[+] is interesting, because it gives you a copy/pasteable version of the function definition without opening up an editor, and I can find that useful (OTOH: you can set PSQL_EDITOR to cat and get the same effect with \ef... ok, just joking). Line numbers are an extra touch, personally it does not thrill me too much, but I've nothing against it. The number variants of \e and \ef work by simply executing $EDITOR +num file. I tried with some editors that came to my mind, and not all of them support it (most do, though): * emacs and emacsclient work * vi works * nano works * pico works * mcedit works * kwrite does not work * kedit does not work not sure what other people (or for instance Windows people) use. Apart from no universal support from editors, it does not save that many keystrokes - at most a couple. In the end you can usually easily jump to the line you want once you are inside your dream editor. I disagree. When I working on servers of my customers there are some default configuration - default editor is usually vi or vim. I cannot change my preferred editor there and \ef n - can help me very much (I know only one command for vi - :q :)). I am looking on KDE. There is usual parameter --line. I propose following solution - to add a system variable PSQL_EDITOR_GOTOLINE_COMMAND that will contains a command for editors without general +n navigation. so you can set a PSQL_EDITOR_GOTOLINE_COMMAND='--line %d' and when this string will be used, when will not be empty without default. My recommendation would be to only integrate the \sf[+] part of the patch, which will have the additional benefit of making it much smaller and cleaner (will avoid the grotty splitting of the number from the function name, for instance). But I'm just another user out there, maybe others will find uses for the other cases. I would personally not add the leading and trailing newlines to \sf output, but that's a question of taste. Maybe better is using a title - so source code will use a format like standard result set. I'll look on it. Docs could use some small grammar fixes, but other than that they're fine. == Code == In \sf code there just a strncmp, so this works: \sfblablabla funcname will be fiexed The error for an empty \sf is not great, it should probably look more like \sf: missing required argument following the examples of \pset, \copy or \prompt. Why is lnptr always being passed as a pointer? Looks like a unnecessary complication and one more variable to care about. Can't we just pass lineno? because I would not to use a magic constant. when lnptr is NULL, then lineno is undefined. Thank you very much Pavel Stehule == End == Cheers, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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/162867791003290927y3ca44051p80e697bc6b19d...@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 *** doc/src/sgml/ref/psql-ref.sgml --- /tmp/CUVdHd_psql-ref.sgml 2010-07-16 13:31:53.362662393 +0200 *** *** 1329,1335 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional/literal/term listitem para --- 1329,1335 varlistentry ! termliteral\edit/literal (or literal\e/literal) literaloptional replaceable class=parameterfilename/replaceable /optional optional linenumber /optional/literal/term listitem para *** *** 1359,1370 systems, filenamenotepad.exe/filename on Windows systems. /para /tip /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional/literal/term listitem para --- 1359,1376 systems, filenamenotepad.exe/filename on Windows systems. /para /tip + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. + /para /listitem /varlistentry varlistentry ! termliteral\ef optional replaceable class=parameterfunction_description/replaceable /optional optional linenumber /optional /literal/term listitem para *** *** 1387,1392 --- 1393,1405 If no function is specified, a blank commandCREATE FUNCTION/ template is presented for editing. /para + + para + If replaceable class=parameterlinenumber/replaceable is + specified, then cursor is moved on this line after start of + editor. It count lines from start of function body, not from + start of text. + /para /listitem /varlistentry *** *** 2106,2111 --- 2119,2136 varlistentry + termliteral\sf[+] replaceable class=parameterfunction_description/replaceable optional linenumber
