Re: [HACKERS] reprise: pretty print viewdefs

2012-01-16 Thread Andrew Dunstan



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

2012-01-13 Thread Andrew Dunstan



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

2012-01-12 Thread Hitoshi Harada
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

2011-12-27 Thread Andrew Dunstan



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

2011-12-25 Thread Andrew Dunstan



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

2011-12-24 Thread Greg Stark
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

2011-12-23 Thread Dimitri Fontaine
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

2011-12-22 Thread Robert Haas
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

2011-12-22 Thread Andrew Dunstan



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

2011-12-22 Thread Andrew Dunstan



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

2011-12-22 Thread Robert Haas
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

2011-12-22 Thread Tom Lane
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

2011-12-22 Thread Andrew Dunstan



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

2011-12-22 Thread Andrew Dunstan



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

2011-12-22 Thread Andrew Dunstan



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