review: psql: edit function, show function commands patch
in [PgSql]
|
From: Robert Haas on 22 Jul 2010 20:31 On Thu, Jul 22, 2010 at 6:56 PM, Jan Urbański <wulczer(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 23 Jul 2010 14:55 Hello 2010/7/23 Jan UrbaÅski <wulczer(a)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
From: Robert Haas on 31 Jul 2010 22:10 On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule <pavel.stehule(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 31 Jul 2010 22:46 Robert Haas <robertmhaas(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 1 Aug 2010 00:15
2010/8/1 Robert Haas <robertmhaas(a)gmail.com>: > On Sun, Jul 25, 2010 at 11:42 AM, Pavel Stehule <pavel.stehule(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |