Thread: pretty print viewdefs
[originally sent from wrong account :-( ]
The tiny patch attached fixes a long-standing peeve of mine (and at
least one of my clients'), namely that the target list printed in
viewdefs are unreadable.
example output now looks like this:
regression=# select pg_get_viewdef('shoe',true);
pg_get_viewdef
-----------------------------------------------
SELECT
sh.shoename,
sh.sh_avail,
sh.slcolor,
sh.slminlen,
sh.slminlen * un.un_fact AS slminlen_cm,
sh.slmaxlen,
sh.slmaxlen * un.un_fact AS slmaxlen_cm,
sh.slunit
FROM shoe_data sh, unit un
WHERE sh.slunit = un.un_name;
Is there any objection?
cheers
andrew
Index: src/backend/utils/adt/ruleutils.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ruleutils.c,v
retrieving revision 1.306
diff -c -r1.306 ruleutils.c
*** src/backend/utils/adt/ruleutils.c 1 Aug 2009 19:59:41 -0000 1.306
--- src/backend/utils/adt/ruleutils.c 26 Aug 2009 12:51:23 -0000
***************
*** 2665,2670 ****
--- 2665,2672 ----
appendStringInfoString(buf, sep);
sep = ", ";
+ appendContextKeyword(context, "", -PRETTYINDENT_STD,
+ PRETTYINDENT_STD, PRETTYINDENT_VAR);
colno++;
/*
Andrew Dunstan <andrew@dunslane.net> writes:
> The tiny patch attached fixes a long-standing peeve of mine (and at
> least one of my clients'), namely that the target list printed in
> viewdefs are unreadable.
Personally I think this will take up enough vertical space to make
things less readable on-screen, not more so. But that's just MHO.
It probably depends a lot on the sorts of views you tend to look at...
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> The tiny patch attached fixes a long-standing peeve of mine (and at >> least one of my clients'), namely that the target list printed in >> viewdefs are unreadable. >> > > Personally I think this will take up enough vertical space to make > things less readable on-screen, not more so. But that's just MHO. > It probably depends a lot on the sorts of views you tend to look at... > > > Well, I could work out if the bit that will be added to the line will run it over some limit (like 80 chars) and only put in the line break then, but it would involve a lot more code. When you're dealing with a view that has 40 or 50 fields, having them all run together over a dozen or two lines is just horrible. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> When you're dealing with a view that has 40 or 50 fields, having them
> all run together over a dozen or two lines is just horrible.
True, but is having them span a couple of screens vertically going to
be much better? There'll be a whole lot of wasted whitespace.
I'm not dead set against this change, just trying to consider
alternative viewpoints.
regards, tom lane
2009/8/26 Andrew Dunstan <andrew@dunslane.net>:
>
> [originally sent from wrong account :-( ]
>
>
> The tiny patch attached fixes a long-standing peeve of mine (and at least
> one of my clients'), namely that the target list printed in viewdefs are
> unreadable.
>
> example output now looks like this:
>
> regression=# select pg_get_viewdef('shoe',true);
> pg_get_viewdef
> -----------------------------------------------
> SELECT
> sh.shoename,
> sh.sh_avail,
> sh.slcolor,
> sh.slminlen,
> sh.slminlen * un.un_fact AS slminlen_cm,
> sh.slmaxlen,
> sh.slmaxlen * un.un_fact AS slmaxlen_cm,
> sh.slunit
> FROM shoe_data sh, unit un
> WHERE sh.slunit = un.un_name;
>
>
I am not sure - this should by task for client application. But Pg
should have some pretty print function - it is easy implemented there.
Personally, I prefere Celko's notation, it is little bit more compact
SELECT sh.shoename, sh.sh_avail, sh.slcolor, sh.slminlen, sh.slminlen * un.un_fact AS slminlen_cm,
sh.slmaxlen, sh.slmaxlen * un.un_fact AS slmaxlen_cm, sh.slunit FROM shoe_data sh, unit un WHERE sh.slunit
=un.un_name;
but, sure - this is my personal preference.
> Is there any objection?
I thing so default should be unformated with some pretty printing support.
regards
Pavel Stehule
>
> cheers
>
> andrew
>
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
Pavel Stehule wrote: > I am not sure - this should by task for client application. pg_get_viewdef() already has a pretty print mode, and this change would only affect output from that mode. Non-pretty printed output would be unchanged. My argument is that the pretty print mode for target lists is not at all pretty. I don't see why this has the be invented in every client. Then we'd have to do it in psql, pg_dump and so on. If any client doesn't like our pretty print output it can get the raw viewdef and do its own formatting. > But Pg > should have some pretty print function - it is easy implemented there. > Personally, I prefere Celko's notation, it is little bit more compact > > SELECT sh.shoename, sh.sh_avail, sh.slcolor, sh.slminlen, > sh.slminlen * un.un_fact AS slminlen_cm, sh.slmaxlen, > sh.slmaxlen * un.un_fact AS slmaxlen_cm, sh.slunit > FROM shoe_data sh, unit un > WHERE sh.slunit = un.un_name; > > but, sure - this is my personal preference. > To do that we would need to keep track of how much space was used on the line and how much space what we were adding would use. It's doable, but it's a lot more work. > >> Is there any objection? >> > > I thing so default should be unformated with some pretty printing support. > > > Please look at the function definition. You already have the option of formatted or unformatted output. cheers andrew
Andrew Dunstan wrote: > > [originally sent from wrong account :-( ] Andrew, you can login to the majordomo site and set your secondary address as an alias of this one. This means it'll recognize the other address and allow you to post from there without going through the moderator queue. Of course, that address will not receive any mail from majordomo. Note that if you do this, it will work automatically for all lists, not just -hackers, so it is a lot better than subscribing to each list and setting it "nomail". It only takes a minute (but you need your Majordomo list password, which can be emailed to you if you don't have it). http://www.postgresql.org/mailpref/pgsql-hackers -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Andrew Dunstan wrote: > >> [originally sent from wrong account :-( ] >> > > Andrew, you can login to the majordomo site and set your secondary > address as an alias of this one. This means it'll recognize the other > address and allow you to post from there without going through the > moderator queue. Of course, that address will not receive any mail from > majordomo. > > Thanks, that's one MD feature I didn't know about or had forgotten. Nice. cheers andrew
Andrew Dunstan wrote: > > >> But Pg >> should have some pretty print function - it is easy implemented there. >> Personally, I prefere Celko's notation, it is little bit more compact >> >> SELECT sh.shoename, sh.sh_avail, sh.slcolor, sh.slminlen, >> sh.slminlen * un.un_fact AS slminlen_cm, sh.slmaxlen, >> sh.slmaxlen * un.un_fact AS slmaxlen_cm, sh.slunit >> FROM shoe_data sh, unit un >> WHERE sh.slunit = un.un_name; >> >> but, sure - this is my personal preference. >> > > > To do that we would need to keep track of how much space was used on > the line and how much space what we were adding would use. It's > doable, but it's a lot more work. When initially implementing the pretty option, I ran into the same consideration. Back then, I decided not to try any line breaking on the column list. Instead, I treated the columns as "just a bunch of columns", laying the emphasis on the from-clause (with potentially many joined tables). So a pretty column formatting should still be white-space saving. Regards, Andreas
Andreas Pflug escribió: > When initially implementing the pretty option, I ran into the same > consideration. Back then, I decided not to try any line breaking on the > column list. Instead, I treated the columns as "just a bunch of > columns", laying the emphasis on the from-clause (with potentially many > joined tables). > So a pretty column formatting should still be white-space saving. It would be neat to have a way of detecting the client terminal's width (psql knows it; it'd have to pass it as an additional parameter) and output as many columns as fit on each line. This is a much more invasive change though. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Aug 26, 2009, at 9:02 AM, Andrew Dunstan wrote:
> The tiny patch attached fixes a long-standing peeve of mine (and at
> least one of my clients'), namely that the target list printed in
> viewdefs are unreadable.
>
> example output now looks like this:
>
> regression=# select pg_get_viewdef('shoe',true);
> pg_get_viewdef
> -----------------------------------------------
> SELECT
> sh.shoename,
> sh.sh_avail,
Did we kill the idea of trying to retain the original view
definition? Granted, that doesn't really help for SELECT *...
--
Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828
decibel wrote:
> On Aug 26, 2009, at 9:02 AM, Andrew Dunstan wrote:
>> The tiny patch attached fixes a long-standing peeve of mine (and at
>> least one of my clients'), namely that the target list printed in
>> viewdefs are unreadable.
>>
>> example output now looks like this:
>>
>> regression=# select pg_get_viewdef('shoe',true);
>> pg_get_viewdef
>> -----------------------------------------------
>> SELECT
>> sh.shoename,
>> sh.sh_avail,
>
>
> Did we kill the idea of trying to retain the original view definition?
> Granted, that doesn't really help for SELECT *...
>
That has nothing at all to do with the issue. The question is not about
whether to keep the original, it's about how to format the reconstructed
query.
cheers
andrew
On Wed, Aug 26, 2009 at 7:47 PM, Andrew Dunstan<andrew@dunslane.net> wrote: >> Did we kill the idea of trying to retain the original view definition? >> Granted, that doesn't really help for SELECT *... > > That has nothing at all to do with the issue. The question is not about > whether to keep the original, it's about how to format the reconstructed > query. I suspect Jim's thinking that if we keep the original we don't have to reconstruct the query ever. Unfortunately cases like "select *" -- and that's not the only case, think of columns that have been renamed -- throw a wrench in the works for that. I agree with Tom's concerns -- think of that guy who was bumping up against the 1600 column limit. At least if they're on one line you can still see the structure of the query albeit with a very very wide scrollbar... But for typical queries I do agree one per line is better. That is actually how I format my queries when they have complex expressions in the target list. Perhaps formatting one per line whenever there's an alias or the value is a complex expression but putting any unaliased columns (such as produced by select *) in a single line would be a good compromise? Incidentally, how does your patch format a complex subquery in the target list? but I think on balance this is probably better. In the extreme think of that guy a few days ago who was bumping up against the 1600 column limit. Assuming he had a few layers of nested subqueries his -- greg http://mit.edu/~gsstark/resume.pdf
Greg Stark <gsstark@mit.edu> writes:
> I agree with Tom's concerns -- think of that guy who was bumping up
> against the 1600 column limit. At least if they're on one line you can
> still see the structure of the query albeit with a very very wide
> scrollbar...
> But for typical queries I do agree one per line is better. That is
> actually how I format my queries when they have complex expressions in
> the target list. Perhaps formatting one per line whenever there's an
> alias or the value is a complex expression but putting any unaliased
> columns (such as produced by select *) in a single line would be a
> good compromise?
Yeah, I was wondering about adopting some rule like that too.
It would be pretty easy to adjust that loop so that columns that aren't
simple Vars are put on their own line, while Vars are allowed to share
a line. I dunno whether users would see that as inconsistent, though.
regards, tom lane
Tom Lane wrote: > Greg Stark <gsstark@mit.edu> writes: > >> I agree with Tom's concerns -- think of that guy who was bumping up >> against the 1600 column limit. At least if they're on one line you can >> still see the structure of the query albeit with a very very wide >> scrollbar... >> > > >> But for typical queries I do agree one per line is better. That is >> actually how I format my queries when they have complex expressions in >> the target list. Perhaps formatting one per line whenever there's an >> alias or the value is a complex expression but putting any unaliased >> columns (such as produced by select *) in a single line would be a >> good compromise? >> > > Yeah, I was wondering about adopting some rule like that too. > > It would be pretty easy to adjust that loop so that columns that aren't > simple Vars are put on their own line, while Vars are allowed to share > a line. I dunno whether users would see that as inconsistent, though. > Yeah, probably, I don't like it much. I do have a solution that wraps when running line length over 80 instead of on every col: SELECT sh.shoename, sh.sh_avail, sh.slcolor, sh.slminlen, sh.slminlen * un.un_fact AS slminlen_cm, sh.slmaxlen, sh.slmaxlen* un.un_fact AS slmaxlen_cm, sh.slunit FROM shoe_data sh, unit un WHERE sh.slunit = un.un_name; It's not a huge amount of code. Maybe we need a couple of extra pg_get_viewdef() variants. One to wrap on some provided line length, one to wrap on every column. psql could use the first, pg_dump could use the second. I really can't believe anyone wants a single line with 1600 column specs ... cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I do have a solution that wraps when running line length over 80 instead
> of on every col:
> SELECT sh.shoename, sh.sh_avail, sh.slcolor, sh.slminlen,
> sh.slminlen * un.un_fact AS slminlen_cm, sh.slmaxlen,
> sh.slmaxlen * un.un_fact AS slmaxlen_cm, sh.slunit
> FROM shoe_data sh, unit un
> WHERE sh.slunit = un.un_name;
> It's not a huge amount of code.
Well, let's see it? What do you do with expressions that don't fit?
> Maybe we need a couple of extra pg_get_viewdef() variants. One to wrap
> on some provided line length, one to wrap on every column. psql could
> use the first, pg_dump could use the second.
pg_dump doesn't use prettyprinting at all, and won't if I have anything
to say about it. But I could see teaching the psql \d views to pass
along whatever psql thinks the window width is.
regards, tom lane
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> I do have a solution that wraps when running line length over 80 instead
>> of on every col:
>>
>
>
>> SELECT sh.shoename, sh.sh_avail, sh.slcolor, sh.slminlen,
>> sh.slminlen * un.un_fact AS slminlen_cm, sh.slmaxlen,
>> sh.slmaxlen * un.un_fact AS slmaxlen_cm, sh.slunit
>> FROM shoe_data sh, unit un
>> WHERE sh.slunit = un.un_name;
>>
>
>
>> It's not a huge amount of code.
>>
>
> Well, let's see it? What do you do with expressions that don't fit?
>
See attached.
We don't apply the wrapping unless there has been a column printed on
the line (except for the SELECT line).
So we can run over the limit on a line, but if we do there's only one
column spec. I think that's acceptable.
>
>> Maybe we need a couple of extra pg_get_viewdef() variants. One to wrap
>> on some provided line length, one to wrap on every column. psql could
>> use the first, pg_dump could use the second.
>>
>
> pg_dump doesn't use prettyprinting at all, and won't if I have anything
> to say about it. But I could see teaching the psql \d views to pass
> along whatever psql thinks the window width is.
>
>
>
OK, but I'd still like to have the only one col per line variant available.
cheers
andrew
Index: src/backend/utils/adt/ruleutils.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ruleutils.c,v
retrieving revision 1.306
diff -c -r1.306 ruleutils.c
*** src/backend/utils/adt/ruleutils.c 1 Aug 2009 19:59:41 -0000 1.306
--- src/backend/utils/adt/ruleutils.c 26 Aug 2009 23:09:00 -0000
***************
*** 2649,2659 ****
{
StringInfo buf = context->buf;
char *sep;
! int colno;
ListCell *l;
sep = " ";
colno = 0;
foreach(l, targetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(l);
--- 2649,2669 ----
{
StringInfo buf = context->buf;
char *sep;
! int colno, linecol;
ListCell *l;
+ int save_len;
+ char *line_start;
+
+ line_start = strrchr(buf->data,'\n');
+ if (line_start == NULL)
+ line_start = buf->data;
+ else
+ line_start++;
+
sep = " ";
colno = 0;
+ linecol = 1;
foreach(l, targetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(l);
***************
*** 2666,2671 ****
--- 2676,2683 ----
appendStringInfoString(buf, sep);
sep = ", ";
colno++;
+ linecol++;
+ save_len = buf->len;
/*
* We special-case Var nodes rather than using get_rule_expr. This is
***************
*** 2703,2708 ****
--- 2715,2748 ----
if (attname == NULL || strcmp(attname, colname) != 0)
appendStringInfo(buf, " AS %s", quote_identifier(colname));
}
+
+ /* handle line overflow */
+ if (strlen(line_start) > 80 && linecol > 1 && PRETTY_INDENT(context))
+ {
+ StringInfoData thiscol;
+
+ initStringInfo(&thiscol);
+
+ /* save what we just added */
+ appendStringInfoString(&thiscol,buf->data + save_len);
+
+ /* wipe it out from the buffer */
+ buf->len = save_len;
+ buf->data[save_len] = '\0';
+
+ /* add a line break and reindent */
+ appendContextKeyword(context, "", -PRETTYINDENT_STD,
+ PRETTYINDENT_STD, PRETTYINDENT_VAR);
+
+ /* and now put back what we wiped out */
+ appendStringInfoString(buf,thiscol.data);
+
+ /* reset the counters */
+ line_start = strrchr(buf->data,'\n') + 1;
+ linecol = 0;
+
+ pfree(thiscol.data);
+ }
}
}
On Wed, Aug 26, 2009 at 11:49 PM, Andrew Dunstan<andrew@dunslane.net> wrote: > Maybe we need a couple of extra pg_get_viewdef() variants. One to wrap on > some provided line length, one to wrap on every column. psql could use the > first, pg_dump could use the second. > > I really can't believe anyone wants a single line with 1600 column specs ... Uhm, why not? People generally don't care about the list of columns at all if it's just generated by "select *". If they're reading it at all it's to see the WHERE clauses and FROM clauses and so on. I think wrapping it at 80 columns gets the worst of both worlds. Then you have dozens or even hundreds of lines just listing columns making it hard to see the rest of the query. At least if it's all on one line you can just not scroll to the right and see the rest of the query on your screen. An alternative to my previous compromise which might be more consistent: List the columns one per line if *any* of the columns has an alias or is a complex expression. If they're all simple columns then put them all one line. At least that way you won't have any weird column lists that switch back and forth between styles. -- greg http://mit.edu/~gsstark/resume.pdf
Greg Stark wrote: > At least if it's all on one line > you can just not scroll to the right and see the rest of the query on > your screen. > > > This is where the confusion arises. This is not possible on any terminal program I use - they don't scroll left and right, they wrap, and the result in this case is horrible. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Well, let's see it? What do you do with expressions that don't fit?
> See attached.
This isn't going to work as-is, because (a) buf->data can be moved
around by repalloc, and (b) you're not allowing for newlines being
introduced within the column expressions. You could probably fix it,
but given the lack of consensus for a line-length-based approach, I'm
not sure it's worth putting more effort into.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> Well, let's see it? What do you do with expressions that don't fit? >>> > > >> See attached. >> > > This isn't going to work as-is, because (a) buf->data can be moved > around by repalloc, and (b) you're not allowing for newlines being > introduced within the column expressions. You could probably fix it, > but given the lack of consensus for a line-length-based approach, I'm > not sure it's worth putting more effort into. > Yeah, it was just a prototype. I'll just provide for an pg_get_viewdef() variant that adopts my original approach, which I don't think suffers either of those problems. Surely that won't upset anyone, at least. It's what I really wanted anyway. cheers andrew
On Thu, Aug 27, 2009 at 1:00 AM, Andrew Dunstan<andrew@dunslane.net> wrote: > Greg Stark wrote: >> >> At least if it's all on one line >> you can just not scroll to the right and see the rest of the query on >> your screen. > > This is where the confusion arises. > > This is not possible on any terminal program I use - they don't scroll left > and right, they wrap, and the result in this case is horrible. Well then you need a better terminal? Or you need to do your programming in a text editor and not a terminal? Surely *any* significant view definition will overflow on a terminal? Incidentally I just tried \d information_schema.views and it *does* seem to put newlines after some of the target list items. After each of the CASE expressions it puts a newline. So you *already* get a mixture of some multiple items on a line and some one-per-line. So I think I'm back to my original suggestion, put any item with a complex expression or an alias on a line by itself. Any plain column names can be listed on a single line. -- greg http://mit.edu/~gsstark/resume.pdf
Greg Stark <gsstark@mit.edu> writes:
> Incidentally I just tried
> \d information_schema.views
> and it *does* seem to put newlines after some of the target list
> items. After each of the CASE expressions it puts a newline. So you
> *already* get a mixture of some multiple items on a line and some
> one-per-line.
Yeah, sufficiently complex expressions (sub-selects, for an obvious
case) will get internal pretty-printing that might include newlines.
regards, tom lane
Greg Stark wrote: > On Thu, Aug 27, 2009 at 1:00 AM, Andrew Dunstan<andrew@dunslane.net> wrote: > >> Greg Stark wrote: >> >>> At least if it's all on one line >>> you can just not scroll to the right and see the rest of the query on >>> your screen. >>> >> This is where the confusion arises. >> >> This is not possible on any terminal program I use - they don't scroll left >> and right, they wrap, and the result in this case is horrible. >> > > Well then you need a better terminal? Or you need to do your > programming in a text editor and not a terminal? I use emacs in its own window. And it too line wraps, although that can be changed (I'm not a fan of line truncation mode, frankly). But I also like to be able to read the viewdef, and I like to be able to cut and paste it so it is sanely editable. And I'm not alone. If there are embedded newlines in the column def that might be annoying, but what we have now sucks majorly for anything but the most trivial of target lists. cheers andrew
Tom Lane wrote:
> Greg Stark <gsstark@mit.edu> writes:
>
>> Incidentally I just tried
>> \d information_schema.views
>>
>
>
>> and it *does* seem to put newlines after some of the target list
>> items. After each of the CASE expressions it puts a newline. So you
>> *already* get a mixture of some multiple items on a line and some
>> one-per-line.
>>
>
> Yeah, sufficiently complex expressions (sub-selects, for an obvious
> case) will get internal pretty-printing that might include newlines.
>
OK, drawing this together, what I did was to go back closer to my
original idea, but put this in a separate function, so nobody would get
too upset ;-)
Here is what my function does, and also what the current "pretty
printing" does:
andrew=# select pg_get_viewdef_long('foo');
pg_get_viewdef_long
------------------------------
SELECT 'a'::text AS b,
( SELECT 1
FROM dual) AS x,
random() AS y,
CASE
WHEN true THEN 1
ELSE 0
END AS c,
1 AS d
FROM dual;
(1 row)
andrew=# select pg_get_viewdef('foo',true);
pg_get_viewdef
---------------------------------------------
SELECT 'a'::text AS b, ( SELECT 1
FROM dual) AS x, random() AS y,
CASE
WHEN true THEN 1
ELSE 0
END AS c, 1 AS d
FROM dual;
(1 row)
WIP Patch is attached. To complete it I would add a psql option to use
it, but maybe we should have a psql setting to enable it ... building
something extra into the \d* stuff looks a bit ugly, since we already
have a million options.
cheers
andrew
Index: src/include/catalog/pg_proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/catalog/pg_proc.h,v
retrieving revision 1.549
diff -c -r1.549 pg_proc.h
*** src/include/catalog/pg_proc.h 4 Aug 2009 04:04:12 -0000 1.549
--- src/include/catalog/pg_proc.h 27 Aug 2009 14:22:04 -0000
***************
*** 4071,4076 ****
--- 4071,4080 ----
DESCR("select statement of a view with pretty-print option");
DATA(insert OID = 2506 ( pg_get_viewdef PGNSP PGUID 12 1 0 0 f f f t f s 2 0 25 "26 16" _null_ _null_ _null_
_null_ pg_get_viewdef_ext _null_ _null_ _null_ ));
DESCR("select statement of a view with pretty-print option");
+ DATA(insert OID = 2336 ( pg_get_viewdef_long PGNSP PGUID 12 1 0 0 f f f t f s 1 0 25 "25" _null_ _null_ _null_
_null_ pg_get_viewdef_name_long _null_ _null_ _null_ ));
+ DESCR("select statement of a view with pretty-printing, columns line separated");
+ DATA(insert OID = 2337 ( pg_get_viewdef_long PGNSP PGUID 12 1 0 0 f f f t f s 1 0 25 "26" _null_ _null_ _null_
_null_ pg_get_viewdef_long _null_ _null_ _null_ ));
+ DESCR("select statement of a view with pretty-printing, columns line separated");
DATA(insert OID = 2507 ( pg_get_indexdef PGNSP PGUID 12 1 0 0 f f f t f s 3 0 25 "26 23 16" _null_ _null_
_null__null_ pg_get_indexdef_ext _null_ _null_ _null_ ));
DESCR("index description (full create statement or single expression) with pretty-print option");
DATA(insert OID = 2508 ( pg_get_constraintdef PGNSP PGUID 12 1 0 0 f f f t f s 2 0 25 "26 16" _null_ _null_ _null_
_null_ pg_get_constraintdef_ext _null_ _null_ _null_ ));
Index: src/include/utils/builtins.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/builtins.h,v
retrieving revision 1.338
diff -c -r1.338 builtins.h
*** src/include/utils/builtins.h 4 Aug 2009 16:08:36 -0000 1.338
--- src/include/utils/builtins.h 27 Aug 2009 14:22:05 -0000
***************
*** 582,589 ****
--- 582,591 ----
extern Datum pg_get_ruledef_ext(PG_FUNCTION_ARGS);
extern Datum pg_get_viewdef(PG_FUNCTION_ARGS);
extern Datum pg_get_viewdef_ext(PG_FUNCTION_ARGS);
+ extern Datum pg_get_viewdef_long(PG_FUNCTION_ARGS);
extern Datum pg_get_viewdef_name(PG_FUNCTION_ARGS);
extern Datum pg_get_viewdef_name_ext(PG_FUNCTION_ARGS);
+ extern Datum pg_get_viewdef_name_long(PG_FUNCTION_ARGS);
extern Datum pg_get_indexdef(PG_FUNCTION_ARGS);
extern Datum pg_get_indexdef_ext(PG_FUNCTION_ARGS);
extern char *pg_get_indexdef_string(Oid indexrelid);
Index: src/backend/utils/adt/ruleutils.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ruleutils.c,v
retrieving revision 1.306
diff -c -r1.306 ruleutils.c
*** src/backend/utils/adt/ruleutils.c 1 Aug 2009 19:59:41 -0000 1.306
--- src/backend/utils/adt/ruleutils.c 27 Aug 2009 14:22:05 -0000
***************
*** 71,80 ****
--- 71,83 ----
/* Pretty flags */
#define PRETTYFLAG_PAREN 1
#define PRETTYFLAG_INDENT 2
+ #define PRETTYFLAG_ONETARGET 4
/* macro to test if pretty action needed */
#define PRETTY_PAREN(context) ((context)->prettyFlags & PRETTYFLAG_PAREN)
#define PRETTY_INDENT(context) ((context)->prettyFlags & PRETTYFLAG_INDENT)
+ #define PRETTY_ONETARGET(context) \
+ ((context)->prettyFlags & PRETTYFLAG_ONETARGET)
/* ----------
***************
*** 350,355 ****
--- 353,369 ----
}
Datum
+ pg_get_viewdef_long(PG_FUNCTION_ARGS)
+ {
+ /* By OID */
+ Oid viewoid = PG_GETARG_OID(0);
+ int prettyFlags;
+
+ prettyFlags = PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_ONETARGET;
+ PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags)));
+ }
+
+ Datum
pg_get_viewdef_name(PG_FUNCTION_ARGS)
{
/* By qualified name */
***************
*** 381,386 ****
--- 395,416 ----
PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags)));
}
+ Datum
+ pg_get_viewdef_name_long(PG_FUNCTION_ARGS)
+ {
+ /* By qualified name */
+ text *viewname = PG_GETARG_TEXT_P(0);
+ int prettyFlags;
+ RangeVar *viewrel;
+ Oid viewoid;
+
+ prettyFlags = PRETTYFLAG_PAREN | PRETTYFLAG_INDENT | PRETTYFLAG_ONETARGET;
+ viewrel = makeRangeVarFromNameList(textToQualifiedNameList(viewname));
+ viewoid = RangeVarGetRelid(viewrel, false);
+
+ PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, prettyFlags)));
+ }
+
/*
* Common code for by-OID and by-name variants of pg_get_viewdef
*/
***************
*** 2651,2656 ****
--- 2681,2687 ----
char *sep;
int colno;
ListCell *l;
+ int save_len;
sep = " ";
colno = 0;
***************
*** 2666,2671 ****
--- 2697,2703 ----
appendStringInfoString(buf, sep);
sep = ", ";
colno++;
+ save_len = buf->len;
/*
* We special-case Var nodes rather than using get_rule_expr. This is
***************
*** 2703,2708 ****
--- 2735,2764 ----
if (attname == NULL || strcmp(attname, colname) != 0)
appendStringInfo(buf, " AS %s", quote_identifier(colname));
}
+
+ /* insert a line break if we don't already have one */
+ if ((PRETTY_ONETARGET(context)) && colno > 1 && buf->data[save_len] != '\n')
+ {
+ StringInfoData thiscol;
+
+ initStringInfo(&thiscol);
+
+ /* save what we just added */
+ appendStringInfoString(&thiscol,buf->data + save_len);
+
+ /* wipe it out from the buffer */
+ buf->len = save_len;
+ buf->data[save_len] = '\0';
+
+ /* add a line break and reindent */
+ appendContextKeyword(context, "", -PRETTYINDENT_STD,
+ PRETTYINDENT_STD, PRETTYINDENT_STD);
+
+ /* and now put back what we wiped out */
+ appendStringInfoString(buf,thiscol.data);
+
+ pfree(thiscol.data);
+ }
}
}
Andrew Dunstan <andrew@dunslane.net> writes:
> OK, drawing this together, what I did was to go back closer to my
> original idea, but put this in a separate function, so nobody would get
> too upset ;-)
This seems seriously ugly. Why don't you have the flag just driving
your original two-line addition?
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> OK, drawing this together, what I did was to go back closer to my >> original idea, but put this in a separate function, so nobody would get >> too upset ;-) >> > > This seems seriously ugly. Why don't you have the flag just driving > your original two-line addition? > > > I am confused. The original two line addition was already in effect driven by the PRETTY_INDENT flag, because the appendContextKeyword call would be effectively a no-op if that flag wasn't on. But apparently some people don't want each column on a separate line, as I do, even when it's pretty printed, so, since that's what I want, I provided for it in a separate function, but I made the code take account of the cases you and Greg mentioned, where it already begins a new line for the column def. So, what exactly is ugly? My code? I can believe that. I have since made it slightly simpler by using a pstrdup'ed string instead of an extra StringInfo object. The output? That's a matter of taste, but I don't see how it's less ugly than what's there now. The idea of a new function? I don't see how to get what I want without it unless we're prepared to upset some of the people who have objected to my proposal. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I am confused.
> The original two line addition was already in effect driven by the
> PRETTY_INDENT flag, because the appendContextKeyword call would be
> effectively a no-op if that flag wasn't on. But apparently some people
> don't want each column on a separate line, as I do, even when it's
> pretty printed, so, since that's what I want, I provided for it in a
> separate function, but I made the code take account of the cases you and
> Greg mentioned, where it already begins a new line for the column def.
What I was imagining was simply providing an additional pretty-print
flag that gives the alternatives of the current behavior, or the patch
you originally proposed that adds newlines between targetlist items all
the time. I don't think that you should complicate the behavior any
more than that.
Personally I would prefer the original patch to this one.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> I am confused. >> > > >> The original two line addition was already in effect driven by the >> PRETTY_INDENT flag, because the appendContextKeyword call would be >> effectively a no-op if that flag wasn't on. But apparently some people >> don't want each column on a separate line, as I do, even when it's >> pretty printed, so, since that's what I want, I provided for it in a >> separate function, but I made the code take account of the cases you and >> Greg mentioned, where it already begins a new line for the column def. >> > > What I was imagining was simply providing an additional pretty-print > flag that gives the alternatives of the current behavior, or the patch > you originally proposed that adds newlines between targetlist items all > the time. I don't think that you should complicate the behavior any > more than that. > > Personally I would prefer the original patch to this one. > > > OK, and how are we going to set that flag? Like I did, with a separate function? I assume you are in effect saying you don't mind if there is an occasional blank line in the output. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> OK, and how are we going to set that flag? Like I did, with a separate
> function?
I would be inclined to invent a variant of pg_get_viewdef with an
additional parameter rather than choosing a new function name, but
otherwise yeah. Or we could decide this isn't worth all the
trouble and just go back to your original patch. By the time you
get done with all the documentation and client-side hacking that
would be required, this patch is going to be a lot larger than it
seems worth.
> I assume you are in effect saying you don't mind if there is an
> occasional blank line in the output.
What blank line? I would expect prettyprinting of expressions to
sometimes insert an embedded newline, but not one at the beginning
or end. Do you have a counterexample?
regards, tom lane
Tom Lane wrote:
>> I assume you are in effect saying you don't mind if there is an
>> occasional blank line in the output.
>>
>
> What blank line? I would expect prettyprinting of expressions to
> sometimes insert an embedded newline, but not one at the beginning
> or end. Do you have a counterexample?
>
>
>
Yes, CASE expressions, as in my previously posted example:
SELECT 'a'::text AS b, ( SELECT 1 FROM dual) AS x, random() AS y, CASE WHEN
trueTHEN 1 ELSE 0 END AS c, 1 AS d FROM dual;
cheers
andrew
What happened to this? I didn't see it applied. --------------------------------------------------------------------------- Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > OK, and how are we going to set that flag? Like I did, with a separate > > function? > > I would be inclined to invent a variant of pg_get_viewdef with an > additional parameter rather than choosing a new function name, but > otherwise yeah. Or we could decide this isn't worth all the > trouble and just go back to your original patch. By the time you > get done with all the documentation and client-side hacking that > would be required, this patch is going to be a lot larger than it > seems worth. > > > I assume you are in effect saying you don't mind if there is an > > occasional blank line in the output. > > What blank line? I would expect prettyprinting of expressions to > sometimes insert an embedded newline, but not one at the beginning > or end. Do you have a counterexample? > > 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 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +
Bruce Momjian wrote: > What happened to this? I didn't see it applied. > > I got puzzled by some delphic comments, and then I got pulled into work of a higher priority, so it slipped down my list. Maybe we can pick it up again in 9.1. cheers andrew
Andrew Dunstan wrote: > > > > Bruce Momjian wrote: > > What happened to this? I didn't see it applied. > > > > > > I got puzzled by some delphic comments, and then I got pulled into work > of a higher priority, so it slipped down my list. > > Maybe we can pick it up again in 9.1. OK, should it be added to the TODO list? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +
Bruce Momjian wrote: > Andrew Dunstan wrote: > >> >> Bruce Momjian wrote: >> >>> What happened to this? I didn't see it applied. >>> >>> >>> >> I got puzzled by some delphic comments, and then I got pulled into work >> of a higher priority, so it slipped down my list. >> >> Maybe we can pick it up again in 9.1. >> > > > OK, should it be added to the TODO list? > > Sure. cheers andrew