review: psql: edit function, show function commands patch
in [PgSql]
|
From: Robert Haas on 1 Aug 2010 08:37 On Sun, Aug 1, 2010 at 12:15 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > 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. Yeah, that seems like a problem. If your editor is vi, for example, the following are the same number of keystrokes: \ef foo 417<enter> and \ef foo<enter>417G So the major advantage of the former over the latter is that you'll (hopefully) end up on EXACTLY the right line rather than maybe being off by a few lines. With the current code, that won't necessarily happen, because PL/pgsql (and any third-party PLs based on it) use one line-numbering convention and other PLs don't necessarily include that hack. Admittedly, you'll probably only be off by one line instead of three or four, so maybe people think that's good enough, but I'm not totally convinced. It seems like the easiest way to fix this would be remove the hack from PL/pgsql, but I'm not sure how people feel about that. If we don't do that, I'm not sure there's any real good solution here. >> The implementation of first_row_is_empty() looks pretty kludgey, too. >> It seems to me that it will fail completely if the text of the >> function definition happens to contain $function$. >> >> CREATE OR REPLACE FUNCTION boom() RETURNS text LANGUAGE plpgsql AS $$ >> BEGIN SELECT '$function$'; END $$; >> > > I can enhance algorithm on client side - but it will not be a pretty > code - it better do it on server side - for example > pg_get_formated_functiondef ... > > CREATE OR REPLACE FUNCTION pg_get_formated_function_def(in oid, > linenum bool:= false, OUT funcdef text, OUT first_line_isignored bool) > > this can remove any ugly code I don't really see why this can't be done on the client side - can't you just scan until you find the second dollars sign and then see whether the following character is a newline? Seems like this could be done in a very small number of lines of code using strchr(). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- 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 09:20 2010/8/1 Robert Haas <robertmhaas(a)gmail.com>: > On Sun, Aug 1, 2010 at 12:15 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >> 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. > > Yeah, that seems like a problem. If your editor is vi, for example, > the following are the same number of keystrokes: > > \ef foo 417<enter> > and > \ef foo<enter>417G > it not is too much important, navigation command is secondary function. Primary functionality is showing of source code with correct row numbers. > So the major advantage of the former over the latter is that you'll > (hopefully) end up on EXACTLY the right line rather than maybe being > off by a few lines. With the current code, that won't necessarily > happen, because PL/pgsql (and any third-party PLs based on it) use one > line-numbering convention and other PLs don't necessarily include that > hack. Admittedly, you'll probably only be off by one line instead of > three or four, so maybe people think that's good enough, but I'm not > totally convinced. It seems like the easiest way to fix this would be > remove the hack from PL/pgsql, but I'm not sure how people feel about > that. If we don't do that, I'm not sure there's any real good > solution here. ... :( without this feature - this patch has minimal value. I don't believe so change plpgsql numbering is good way. It was changed from good reasons. Because empty row is invisible but it isn't empty for people, because there are " AS " keyword. This patch must to working with plpgsql and have to work correctly. > >>> The implementation of first_row_is_empty() looks pretty kludgey, too. >>> It seems to me that it will fail completely if the text of the >>> function definition happens to contain $function$. >>> >>> CREATE OR REPLACE FUNCTION boom() RETURNS text LANGUAGE plpgsql AS $$ >>> BEGIN SELECT '$function$'; END $$; >>> >> >> I can enhance algorithm on client side - but it will not be a pretty >> code - it better do it on server side - for example >> pg_get_formated_functiondef ... >> >> CREATE OR REPLACE FUNCTION pg_get_formated_function_def(in oid, >> linenum bool:= false, OUT funcdef text, OUT first_line_isignored bool) >> >> this can remove any ugly code > > I don't really see why this can't be done on the client side - can't > you just scan until you find the second dollars sign and then see > whether the following character is a newline? Seems like this could > be done in a very small number of lines of code using strchr(). you have a true - it is possible, but still I am supply a parser on client side. On server side I have all data in structured form. but I don't would to complicate a possible commiting so my plan a) fix problem with ambiguous $function* like you proposed b) fix problem with "first row excepting" - I can activate a detection only for plpgsql language - I can identify LANGUAGE before. all will be done on client It is acceptable for you? Regards Pavel Stehule > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise Postgres Company > -- 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 1 Aug 2010 10:47 Pavel Stehule <pavel.stehule(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 1 Aug 2010 11:09 On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > Pavel Stehule <pavel.stehule(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 1 Aug 2010 11:22
2010/8/1 Robert Haas <robertmhaas(a)gmail.com>: > On Sun, Aug 1, 2010 at 10:47 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: >> Pavel Stehule <pavel.stehule(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |