Re: [HACKERS] reprise: pretty print viewdefs
On 01/13/2012 02:50 PM, Andrew Dunstan wrote:
On 01/13/2012 12:31 AM, Hitoshi Harada wrote:
So my conclusion is it's better than nothing, but we could do
better job here.
From timeline perspective, it'd be ok to apply this patch and improve
more later in 9.3+.
I agree, let's look at the items other than the target list during
9.3. But I do think this addresses the biggest pain point.
Actually, it turns out to be very simple to add wrapping logic for the
FROM clause, as in the attached updated patch, and I think we should do
that for this round.
cheers
andrew
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 13758,13764 SELECT pg_type_is_visible('myschema.widget'::regtype);
row
entryliteralfunctionpg_get_viewdef(parameterview_name/parameter, parameterpretty_bool/)/function/literal/entry
entrytypetext/type/entry
!entryget underlying commandSELECT/command command for view (emphasisdeprecated/emphasis)/entry
/row
row
entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter)/function/literal/entry
--- 13758,13765
row
entryliteralfunctionpg_get_viewdef(parameterview_name/parameter, parameterpretty_bool/)/function/literal/entry
entrytypetext/type/entry
!entryget underlying commandSELECT/command command for view,
! lines with fields are wrapped to 80 columns if pretty_bool is true (emphasisdeprecated/emphasis)/entry
/row
row
entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter)/function/literal/entry
***
*** 13768,13774 SELECT pg_type_is_visible('myschema.widget'::regtype);
row
entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter, parameterpretty_bool/)/function/literal/entry
entrytypetext/type/entry
!entryget underlying commandSELECT/command command for view/entry
/row
row
entryliteralfunctionpg_options_to_table(parameterreloptions/parameter)/function/literal/entry
--- 13769,13782
row
entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter, parameterpretty_bool/)/function/literal/entry
entrytypetext/type/entry
!entryget underlying commandSELECT/command command for view,
! lines with fields are wrapped to 80 columns if pretty_bool is true/entry
! /row
! row
!entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter, parameterwrap_int/)/function/literal/entry
!entrytypetext/type/entry
!entryget underlying commandSELECT/command command for view,
! wrapping lines with fields as specified, pretty printing is implied/entry
/row
row
entryliteralfunctionpg_options_to_table(parameterreloptions/parameter)/function/literal/entry
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***
*** 73,78
--- 73,80
#define PRETTYFLAG_PAREN 1
#define PRETTYFLAG_INDENT 2
+ #define PRETTY_WRAP_DEFAULT 79
+
/* macro to test if pretty action needed */
#define PRETTY_PAREN(context) ((context)-prettyFlags PRETTYFLAG_PAREN)
#define PRETTY_INDENT(context) ((context)-prettyFlags PRETTYFLAG_INDENT)
***
*** 136,141 static SPIPlanPtr plan_getrulebyoid = NULL;
--- 138,144
static const char *query_getrulebyoid = SELECT * FROM pg_catalog.pg_rewrite WHERE oid = $1;
static SPIPlanPtr plan_getviewrule = NULL;
static const char *query_getviewrule = SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND rulename = $2;
+ static int pretty_wrap = PRETTY_WRAP_DEFAULT;
/* GUC parameters */
bool quote_all_identifiers = false;
***
*** 381,386 pg_get_viewdef_ext(PG_FUNCTION_ARGS)
--- 384,406
}
Datum
+ pg_get_viewdef_wrap(PG_FUNCTION_ARGS)
+ {
+ /* By OID */
+ Oid viewoid = PG_GETARG_OID(0);
+ int wrap = PG_GETARG_INT32(1);
+ int prettyFlags;
+ char *result;
+
+ /* calling this implies we want pretty printing */
+ prettyFlags = PRETTYFLAG_PAREN | PRETTYFLAG_INDENT;
+ pretty_wrap = wrap;
+ result = pg_get_viewdef_worker(viewoid, prettyFlags);
+ pretty_wrap = PRETTY_WRAP_DEFAULT;
+ PG_RETURN_TEXT_P(string_to_text(result));
+ }
+
+ Datum
pg_get_viewdef_name(PG_FUNCTION_ARGS)
{
/* By qualified name */
***
*** 3013,3018 get_target_list(List *targetList, deparse_context *context,
--- 3033,3039
char *sep;
int colno;
ListCell *l;
+ boollast_was_multiline = false;
sep = ;
colno = 0;
***
*** 3021,3026 get_target_list(List *targetList, deparse_context *context,
--- 3042,3051
TargetEntry *tle = (TargetEntry *) lfirst(l);
char *colname;
char *attname;
+ StringInfoData targetbuf;
+ int leading_nl_pos = -1;
+ char *trailing_nl;
+ int pos;
Re: [HACKERS] reprise: pretty print viewdefs
On 01/13/2012 12:31 AM, Hitoshi Harada wrote: So my conclusion is it's better than nothing, but we could do better job here. From timeline perspective, it'd be ok to apply this patch and improve more later in 9.3+. I agree, let's look at the items other than the target list during 9.3. But I do think this addresses the biggest pain point. Thanks for the review. 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
Re: [HACKERS] reprise: pretty print viewdefs
On Tue, Dec 27, 2011 at 8:02 AM, Andrew Dunstan and...@dunslane.net wrote:
Updated, with docs and tests. Since the docs mark the versions of
pg_get_viewdef() that take text as the first param as deprecated, I removed
that variant of the new flavor. I left adding extra psql support to another
day - I think this already does a good job of cleaning it up without any
extra adjustments.
I'll add this to the upcoming commitfest.
I've looked at (actually tested with folks in reviewfest, so not so in
detail :P) the patch. It applies with some hunks and compiles cleanly,
documentation and tests are prepared. make install passed without
fails.
$ patch -p1 ~/Downloads/viewdef.patch
patching file doc/src/sgml/func.sgml
Hunk #1 succeeded at 13736 (offset -22 lines).
Hunk #2 succeeded at 13747 (offset -22 lines).
patching file src/backend/utils/adt/ruleutils.c
patching file src/include/catalog/pg_proc.h
Hunk #1 succeeded at 3672 (offset -43 lines).
patching file src/include/utils/builtins.h
Hunk #1 succeeded at 604 (offset -20 lines).
patching file src/test/regress/expected/polymorphism.out
Hunk #1 succeeded at 1360 (offset -21 lines).
patching file src/test/regress/expected/rules.out
patching file src/test/regress/expected/with.out
patching file src/test/regress/sql/rules.sql
The change of the code is in only ruleutiles.c, which looks sane to
me, with some trailing white spaces. I wonder if it's worth working
more than on target list, namely like FROM clause, expressions.
For example, pg_get_viewdef('pg_stat_activity', true).
# select pg_get_viewdef('pg_stat_activity'::regclass, true);
pg_get_viewdef
--
SELECT s.datid, d.datname, s.procpid, s.usesysid, u.rolname AS
usename,
+
s.application_name, s.client_addr, s.client_hostname,
s.client_port,
+
s.backend_start, s.xact_start, s.query_start, s.waiting,
s.current_query
+
FROM pg_database d, pg_stat_get_activity(NULL::integer) s(datid,
procpid, usesysid, application_name, current_query, waiting,
xact_start, query_start, backend_start, client_addr, client_hostname,
client_port), pg_authid u+
WHERE s.datid = d.oid AND s.usesysid = u.oid;
(1 row)
It doesn't help much although the SELECT list is wrapped within 80
characters. For expressions, I mean:
=# select pg_get_viewdef('v', true);
pg_get_viewdef
-
SELECT a.a, a.a - 1 AS b, a.a - 2 AS c, a.a - 3 AS d,
+
a.a / 1 AS long_long_name, a.a * 1000 AS bcd,
+
CASE
+
WHEN a.a = 1 THEN 1
+
WHEN (a.a + a.a + a.a + a.a + a.a + a.a + a.a + a.a + a.a
+ a.a + a.a + a.a) = 2 THEN 2+
ELSE 10
+
END AS what_about_case_expression
+
FROM generate_series(1, 100) a(a);
(1 row)
So my conclusion is it's better than nothing, but we could do better
job here. From timeline perspective, it'd be ok to apply this patch
and improve more later in 9.3+.
Thanks,
--
Hitoshi Harada
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reprise: pretty print viewdefs
On 12/22/2011 06:14 PM, Andrew Dunstan wrote:
On 12/22/2011 06:11 PM, Andrew Dunstan wrote:
On 12/22/2011 02:17 PM, Andrew Dunstan wrote:
On 12/22/2011 01:05 PM, Tom Lane wrote:
Maybe, though I fear it might complicate the ruleutils code a bit.
You'd probably have to build the output for a column first and then
see how long it is before deciding whether to insert a newline.
In short, I don't mind trying to make this work better, but I think it
will take more work than a two-line patch.
OK. Let me whip something up. I had already come to the conclusion
you did about how best to do this.
Here's a WIP patch. At least it's fairly localized, but as expected
it's rather more than 2 lines :-). Sample output:
regression=# \pset format unaligned
Output format is unaligned.
regression=# select pg_get_viewdef('shoelace',true);
pg_get_viewdef
SELECT s.sl_name, s.sl_avail, s.sl_color, s.sl_len, s.sl_unit,
s.sl_len * u.un_fact AS sl_len_cm
FROM shoelace_data s, unit u
WHERE s.sl_unit = u.un_name;
(1 row)
regression=#
I just had an idea. We could invent a flavor of pg_get_viewdef() that
took an int as the second param, that would be the wrap width. For
the boolean case as above, 80 would be implied. 0 would mean always
wrap. psql could be made to default to the window width, or maybe
window width - 1, but we could provide a psql setting that would
override it.
This time with patch.
Updated, with docs and tests. Since the docs mark the versions of
pg_get_viewdef() that take text as the first param as deprecated, I
removed that variant of the new flavor. I left adding extra psql support
to another day - I think this already does a good job of cleaning it up
without any extra adjustments.
I'll add this to the upcoming commitfest.
cheers
andrew
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2e06346..0418591 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -13758,7 +13758,8 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
row
entryliteralfunctionpg_get_viewdef(parameterview_name/parameter, parameterpretty_bool/)/function/literal/entry
entrytypetext/type/entry
- entryget underlying commandSELECT/command command for view (emphasisdeprecated/emphasis)/entry
+ entryget underlying commandSELECT/command command for view,
+ lines with fields are wrapped to 80 columns if pretty_bool is true (emphasisdeprecated/emphasis)/entry
/row
row
entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter)/function/literal/entry
@@ -13768,7 +13769,14 @@ SELECT pg_type_is_visible('myschema.widget'::regtype);
row
entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter, parameterpretty_bool/)/function/literal/entry
entrytypetext/type/entry
- entryget underlying commandSELECT/command command for view/entry
+ entryget underlying commandSELECT/command command for view,
+ lines with fields are wrapped to 80 columns if pretty_bool is true/entry
+ /row
+ row
+ entryliteralfunctionpg_get_viewdef(parameterview_oid/parameter, parameterwrap_int/)/function/literal/entry
+ entrytypetext/type/entry
+ entryget underlying commandSELECT/command command for view,
+ wrapping lines with fields as specified, pretty printing is implied/entry
/row
row
entryliteralfunctionpg_options_to_table(parameterreloptions/parameter)/function/literal/entry
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 29df748..5b774f0 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -73,6 +73,8 @@
#define PRETTYFLAG_PAREN 1
#define PRETTYFLAG_INDENT 2
+#define PRETTY_WRAP_DEFAULT 79
+
/* macro to test if pretty action needed */
#define PRETTY_PAREN(context) ((context)-prettyFlags PRETTYFLAG_PAREN)
#define PRETTY_INDENT(context) ((context)-prettyFlags PRETTYFLAG_INDENT)
@@ -136,6 +138,7 @@ static SPIPlanPtr plan_getrulebyoid = NULL;
static const char *query_getrulebyoid = SELECT * FROM pg_catalog.pg_rewrite WHERE oid = $1;
static SPIPlanPtr plan_getviewrule = NULL;
static const char *query_getviewrule = SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND rulename = $2;
+static int pretty_wrap = PRETTY_WRAP_DEFAULT;
/* GUC parameters */
bool quote_all_identifiers = false;
@@ -381,6 +384,23 @@ pg_get_viewdef_ext(PG_FUNCTION_ARGS)
}
Datum
+pg_get_viewdef_wrap(PG_FUNCTION_ARGS)
+{
+ /* By OID */
+ Oid viewoid = PG_GETARG_OID(0);
+ int wrap = PG_GETARG_INT32(1);
+ int prettyFlags;
+ char *result;
+
+ /* calling this implies we want pretty printing */
+ prettyFlags = PRETTYFLAG_PAREN | PRETTYFLAG_INDENT;
+ pretty_wrap = wrap;
+ result = pg_get_viewdef_worker(viewoid, prettyFlags);
+ pretty_wrap = PRETTY_WRAP_DEFAULT;
+ PG_RETURN_TEXT_P(string_to_text(result));
+}
+
+Datum
Re: [HACKERS] reprise: pretty print viewdefs
On 12/24/2011 02:26 PM, Greg Stark wrote: On Thu, Dec 22, 2011 at 5:52 PM, Andrew Dunstanand...@dunslane.net wrote: I've looked at that, and it was discussed a bit previously. It's more complex because it requires that we keep track of (or calculate) where we are on the line, You might try a compromise, just spit out all the columns on one line *unless* either the previous or next column is longer than something like 30 columns. So if you have a long list of short columns it just gets wrapped by your terminal but if you have complex expressions like CASE expressions or casts or so on they go on a line by themselves. I think that sounds too complex, honestly. Here's what I have working: /* * If the field we're adding already has a leading newline * or wrap mode is disabled (pretty_wrap 0), don't add one. * Otherwise, add one, plus some indentation, * if either the new field would cause an * overflow or the last field had a multiline spec. */ Here's an illustration: http://developer.postgresql.org/~adunstan/pg_get_viewdef.png 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
Re: [HACKERS] reprise: pretty print viewdefs
On Thu, Dec 22, 2011 at 5:52 PM, Andrew Dunstan and...@dunslane.net wrote: I've looked at that, and it was discussed a bit previously. It's more complex because it requires that we keep track of (or calculate) where we are on the line, You might try a compromise, just spit out all the columns on one line *unless* either the previous or next column is longer than something like 30 columns. So if you have a long list of short columns it just gets wrapped by your terminal but if you have complex expressions like CASE expressions or casts or so on they go on a line by themselves. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reprise: pretty print viewdefs
Robert Haas robertmh...@gmail.com writes: I *still* spend a lot of time editing in a 25x80 window. 80 is a good choice whatever the screen size, because it's about the most efficient text width as far as eyes movements are concerned: the eye is much better at going top-bottom than left-right. That's also why printed papers still pick shorter columns and website design often do, too. If it's physically tiring to read your text, very few people will. Now, 25 lines… maybe you should tweak your terminal setups, or consider editing in a more comfortable environment :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reprise: pretty print viewdefs
On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstan and...@dunslane.net wrote: The simple solution I originally proposed to put a line feed and some space before every target field in pretty print mode. This is a two line patch. The downsides are a) maybe not everyone will like the change and b) it will produce superfluous newlines, e.g. before CASE expressions. With regard to (a), specifically, you won't like this change if your column names are things like bob and sam, because you'll burn through an inordinate amount of vertical space. On the other hand, I agree that you'll probably find it a big improvement if they are things like nc.nspname::information_schema.sql_identifier as udt_schema. It has always seemed to me that a sensible strategy here would be to try to produce output that looks good in 80 columns, on the assumption that (1) everyone has at least that much horizontal space to play with and (2) people who do won't necessarily mind it if we don't use all of that horizontal space when pretty-printing SQL. However, it is possible that I am living in the dark ages. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reprise: pretty print viewdefs
On 12/22/2011 12:18 PM, Robert Haas wrote: On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstanand...@dunslane.net wrote: The simple solution I originally proposed to put a line feed and some space before every target field in pretty print mode. This is a two line patch. The downsides are a) maybe not everyone will like the change and b) it will produce superfluous newlines, e.g. before CASE expressions. With regard to (a), specifically, you won't like this change if your column names are things like bob and sam, because you'll burn through an inordinate amount of vertical space. On the other hand, I agree that you'll probably find it a big improvement if they are things like nc.nspname::information_schema.sql_identifier as udt_schema. It has always seemed to me that a sensible strategy here would be to try to produce output that looks good in 80 columns, on the assumption that (1) everyone has at least that much horizontal space to play with and (2) people who do won't necessarily mind it if we don't use all of CASE WHEN nco.nspname IS NOT NULL THEN current_database() ELSE NULL::name END::information_schema.sql_identifier AS collation_catalog, nco.nspname::information_schema.sql_identifier AS collation_schema, I'd still be much happier with a that horizontal space when pretty-printing SQL. However, it is possible that I am living in the dark ages. I've looked at that, and it was discussed a bit previously. It's more complex because it requires that we keep track of (or calculate) where we are on the line, and where we would be after adding the new text. But I could live with it if others could. It would certainly be an improvement. But that's still going to leave things that will look odd, such as a CASE expression's final line being filled up to 80 columns with more fields. My preference would be for a newline as a visual clue to where each column spec starts. I used to try to be conservative about vertical space, but in these days of scrollbars and screens not limited to 24 or 25 lines (Yes, kids, that's what some of us grew up with) that seems a bit old-fashioned. One of the arguments for KR style braces used to be that it used one less line than BSD style. Maybe that made some sense 20 or so years ago, although I didn't really buy it even then, but it makes very little sense to me now. 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
Re: [HACKERS] reprise: pretty print viewdefs
On 12/22/2011 12:52 PM, Andrew Dunstan wrote: On 12/22/2011 12:18 PM, Robert Haas wrote: On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstanand...@dunslane.net wrote: The simple solution I originally proposed to put a line feed and some space before every target field in pretty print mode. This is a two line patch. The downsides are a) maybe not everyone will like the change and b) it will produce superfluous newlines, e.g. before CASE expressions. With regard to (a), specifically, you won't like this change if your column names are things like bob and sam, because you'll burn through an inordinate amount of vertical space. On the other hand, I agree that you'll probably find it a big improvement if they are things like nc.nspname::information_schema.sql_identifier as udt_schema. It has always seemed to me that a sensible strategy here would be to try to produce output that looks good in 80 columns, on the assumption that (1) everyone has at least that much horizontal space to play with and (2) people who do won't necessarily mind it if we don't use all ofCASE WHEN nco.nspname IS NOT NULL THEN current_database() ELSE NULL::name END::information_schema.sql_identifier AS collation_catalog, nco.nspname::information_schema.sql_identifier AS collation_schema, I'd still be much happier with a that horizontal space when pretty-printing SQL. However, it is possible that I am living in the dark ages. I've looked at that, and it was discussed a bit previously. It's more complex because it requires that we keep track of (or calculate) where we are on the line, and where we would be after adding the new text. But I could live with it if others could. It would certainly be an improvement. But that's still going to leave things that will look odd, such as a CASE expression's final line being filled up to 80 columns with more fields. My preference would be for a newline as a visual clue to where each column spec starts. I used to try to be conservative about vertical space, but in these days of scrollbars and screens not limited to 24 or 25 lines (Yes, kids, that's what some of us grew up with) that seems a bit old-fashioned. One of the arguments for KR style braces used to be that it used one less line than BSD style. Maybe that made some sense 20 or so years ago, although I didn't really buy it even then, but it makes very little sense to me now. Wow. My editor or my fingers seem to have screwed up here. Something got CP'd into the middle of the quoted text that shouldn't have. *sigh* 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
Re: [HACKERS] reprise: pretty print viewdefs
On Thu, Dec 22, 2011 at 12:52 PM, Andrew Dunstan and...@dunslane.net wrote: I used to try to be conservative about vertical space, but in these days of scrollbars and screens not limited to 24 or 25 lines (Yes, kids, that's what some of us grew up with) that seems a bit old-fashioned. One of the arguments for KR style braces used to be that it used one less line than BSD style. Maybe that made some sense 20 or so years ago, although I didn't really buy it even then, but it makes very little sense to me now. I *still* spend a lot of time editing in a 25x80 window. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reprise: pretty print viewdefs
Robert Haas robertmh...@gmail.com writes: On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstan and...@dunslane.net wrote: The simple solution I originally proposed to put a line feed and some space before every target field in pretty print mode. This is a two line patch. The downsides are a) maybe not everyone will like the change and b) it will produce superfluous newlines, e.g. before CASE expressions. With regard to (a), specifically, you won't like this change if your column names are things like bob and sam, because you'll burn through an inordinate amount of vertical space. Yeah. I'm not exactly thrilled with (b), either, if it's a consequence of a change whose only excuse for living is to make the output look nicer. Random extra newlines don't look nicer to me. It has always seemed to me that a sensible strategy here would be to try to produce output that looks good in 80 columns, Maybe, though I fear it might complicate the ruleutils code a bit. You'd probably have to build the output for a column first and then see how long it is before deciding whether to insert a newline. In short, I don't mind trying to make this work better, but I think it will take more work than a two-line patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] reprise: pretty print viewdefs
On 12/22/2011 01:05 PM, Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstanand...@dunslane.net wrote: The simple solution I originally proposed to put a line feed and some space before every target field in pretty print mode. This is a two line patch. The downsides are a) maybe not everyone will like the change and b) it will produce superfluous newlines, e.g. before CASE expressions. With regard to (a), specifically, you won't like this change if your column names are things like bob and sam, because you'll burn through an inordinate amount of vertical space. Yeah. I'm not exactly thrilled with (b), either, if it's a consequence of a change whose only excuse for living is to make the output look nicer. Random extra newlines don't look nicer to me. It has always seemed to me that a sensible strategy here would be to try to produce output that looks good in 80 columns, Maybe, though I fear it might complicate the ruleutils code a bit. You'd probably have to build the output for a column first and then see how long it is before deciding whether to insert a newline. In short, I don't mind trying to make this work better, but I think it will take more work than a two-line patch. OK. Let me whip something up. I had already come to the conclusion you did about how best to do this. 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
Re: [HACKERS] reprise: pretty print viewdefs
On 12/22/2011 02:17 PM, Andrew Dunstan wrote:
On 12/22/2011 01:05 PM, Tom Lane wrote:
Maybe, though I fear it might complicate the ruleutils code a bit.
You'd probably have to build the output for a column first and then
see how long it is before deciding whether to insert a newline.
In short, I don't mind trying to make this work better, but I think it
will take more work than a two-line patch.
OK. Let me whip something up. I had already come to the conclusion you
did about how best to do this.
Here's a WIP patch. At least it's fairly localized, but as expected it's
rather more than 2 lines :-). Sample output:
regression=# \pset format unaligned
Output format is unaligned.
regression=# select pg_get_viewdef('shoelace',true);
pg_get_viewdef
SELECT s.sl_name, s.sl_avail, s.sl_color, s.sl_len, s.sl_unit,
s.sl_len * u.un_fact AS sl_len_cm
FROM shoelace_data s, unit u
WHERE s.sl_unit = u.un_name;
(1 row)
regression=#
I just had an idea. We could invent a flavor of pg_get_viewdef() that
took an int as the second param, that would be the wrap width. For the
boolean case as above, 80 would be implied. 0 would mean always wrap.
psql could be made to default to the window width, or maybe window width
- 1, but we could provide a psql setting that would override it.
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
Re: [HACKERS] reprise: pretty print viewdefs
On 12/22/2011 06:11 PM, Andrew Dunstan wrote:
On 12/22/2011 02:17 PM, Andrew Dunstan wrote:
On 12/22/2011 01:05 PM, Tom Lane wrote:
Maybe, though I fear it might complicate the ruleutils code a bit.
You'd probably have to build the output for a column first and then
see how long it is before deciding whether to insert a newline.
In short, I don't mind trying to make this work better, but I think it
will take more work than a two-line patch.
OK. Let me whip something up. I had already come to the conclusion
you did about how best to do this.
Here's a WIP patch. At least it's fairly localized, but as expected
it's rather more than 2 lines :-). Sample output:
regression=# \pset format unaligned
Output format is unaligned.
regression=# select pg_get_viewdef('shoelace',true);
pg_get_viewdef
SELECT s.sl_name, s.sl_avail, s.sl_color, s.sl_len, s.sl_unit,
s.sl_len * u.un_fact AS sl_len_cm
FROM shoelace_data s, unit u
WHERE s.sl_unit = u.un_name;
(1 row)
regression=#
I just had an idea. We could invent a flavor of pg_get_viewdef() that
took an int as the second param, that would be the wrap width. For the
boolean case as above, 80 would be implied. 0 would mean always wrap.
psql could be made to default to the window width, or maybe window
width - 1, but we could provide a psql setting that would override it.
This time with patch.
cheers
andrew
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***
*** 3013,3018 get_target_list(List *targetList, deparse_context *context,
--- 3013,3019
char *sep;
int colno;
ListCell *l;
+ boollast_was_multiline = false;
sep = ;
colno = 0;
***
*** 3021,3028 get_target_list(List *targetList, deparse_context *context,
TargetEntry *tle = (TargetEntry *) lfirst(l);
char *colname;
char *attname;
! if (tle-resjunk)
continue; /* ignore junk entries */
appendStringInfoString(buf, sep);
--- 3022,3033
TargetEntry *tle = (TargetEntry *) lfirst(l);
char *colname;
char *attname;
+ StringInfoData targetbuf;
+ int leading_nl_pos = -1;
+ char *trailing_nl;
+ int i;
! if (tle-resjunk)
continue; /* ignore junk entries */
appendStringInfoString(buf, sep);
***
*** 3030,3035 get_target_list(List *targetList, deparse_context *context,
--- 3035,3049
colno++;
/*
+ * Put the new field spec into targetbuf so we can
+ * decide after we've got it whether or not it needs
+ * to go on a new line.
+ */
+
+ initStringInfo(targetbuf);
+ context-buf = targetbuf;
+
+ /*
* We special-case Var nodes rather than using get_rule_expr. This is
* needed because get_rule_expr will display a whole-row Var as
* foo.*, which is the preferred notation in most contexts, but at
***
*** 3063,3070 get_target_list(List *targetList, deparse_context *context,
if (colname) /* resname could be NULL */
{
if (attname == NULL || strcmp(attname, colname) != 0)
! appendStringInfo(buf, AS %s, quote_identifier(colname));
}
}
}
--- 3077,3139
if (colname) /* resname could be NULL */
{
if (attname == NULL || strcmp(attname, colname) != 0)
! appendStringInfo(targetbuf, AS %s, quote_identifier(colname));
! }
!
! /* Does the new field start with whitespace plus a new line? */
!
! for (i=0; i targetbuf.len; i++)
! {
! if (targetbuf.data[i] == '\n')
! {
! leading_nl_pos = i;
! break;
! }
! if (targetbuf.data[i] ' ')
! break;
! }
!
! context-buf = buf;
!
! /* Locate the start of the current line in the buffer */
!
! trailing_nl = (strrchr(buf-data,'\n'));
! if (trailing_nl == NULL)
! trailing_nl = buf-data;
! else
! trailing_nl++;
!
! /*
! * If the field we're adding already has a newline
! * don't add one. Otherwise, add one, plus some
! * indentation. if either the new field would
! * cause an 80 column overflow or the last field
! * had a multiline spec.
! */
!
! if (leading_nl_pos == -1
! ((strlen(trailing_nl) + strlen(targetbuf.data) 79) ||
! last_was_multiline))
! {
! appendContextKeyword(context, , -PRETTYINDENT_STD,
! PRETTYINDENT_STD, PRETTYINDENT_VAR);
}
+
+ /* Add the new field */
+
+ appendStringInfoString(buf, targetbuf.data);
+
+
+ /* Keep track of this fieLd's status for next interation */
+
+ if (strchr(targetbuf.data + leading_nl_pos + 1,'\n') != NULL)
+ last_was_multiline = true;
+ else
+ last_was_multiline = false;
+
+ /* cleanup */
+
+ pfree (targetbuf.data);
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] reprise: pretty print viewdefs
A client has again raised with me the ugliness of pretty printed viewdefs. We looked at this a couple of years ago, but discussion went off into the weeds slightly, so I dropped it, but as requested I'm revisiting it. The problem can be simply seen in the screenshot here: http://developer.postgresql.org/~adunstan/view_before.png This is ugly, unreadable and unusable, IMNSHO. Calling it pretty is just laughable. The simple solution I originally proposed to put a line feed and some space before every target field in pretty print mode. This is a two line patch. The downsides are a) maybe not everyone will like the change and b) it will produce superfluous newlines, e.g. before CASE expressions. We can avoid the superfluous newlines at the cost of some code complexity. Whether it's worth it is a judgement call. If we don't want to do this unconditionally, we'd need either a new function which would make it available or a new flavor of pg_get_viewdef - if the latter I'm not really sure what the new signatures should be - oid/text | something not boolean, but what? Personally, I'd much rather just do it. Does anyone *really* like the present output? 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
