Re: 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

On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstan <andrew(at)dunslane(dot)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

On 12/22/2011 12:18 PM, Robert Haas wrote:
> On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstan<andrew(at)dunslane(dot)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 K&R 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

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 Dunstan<andrew(at)dunslane(dot)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 K&R 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
C&P'd into the middle of the quoted text that shouldn't have. *sigh*

cheers

andrew

On Thu, Dec 22, 2011 at 12:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)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 K&R 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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstan <andrew(at)dunslane(dot)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

On 12/22/2011 01:05 PM, Tom Lane wrote:
> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Dec 19, 2011 at 1:51 PM, Andrew Dunstan<andrew(at)dunslane(dot)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

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

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

Robert Haas <robertmhaas(at)gmail(dot)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

On Thu, Dec 22, 2011 at 5:52 PM, Andrew Dunstan <andrew(at)dunslane(dot)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

On 12/24/2011 02:26 PM, Greg Stark wrote:
> On Thu, Dec 22, 2011 at 5:52 PM, Andrew Dunstan<andrew(at)dunslane(dot)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

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

On Tue, Dec 27, 2011 at 8:02 AM, Andrew Dunstan <andrew(at)dunslane(dot)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

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

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