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
Attachments:
prettyprint.patchtext/x-patch; charset=iso-8859-1; name=prettyprint.patchDownload
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
Show quoted text
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 compactSELECT 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 compactSELECT 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 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
Attachments:
prettyprint.patchtext/x-patch; charset=iso-8859-1; name=prettyprint.patchDownload
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 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 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.viewsand 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
Attachments:
prettyprint.patchtext/x-patch; charset=iso-8859-1; name=prettyprint.patchDownload
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 true THEN 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.com
PG 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.com
PG 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