pg_dump --pretty-print-views

Hi,

At the company I work for, we've been splitting dumps into separate
files and diffing them for a while now. By far the biggest problem we
had was with views: pg_dump by default dumps views on one line, in a
format which maximizes compatibility. Now this has several problems for
our use case:

1) The one-line equivalent of a 200-line view is completely impossible
to read.
2) If there's a difference between the two dumped view definitions,
it takes a long time to find where and what exactly it is.
3) For some reason some expressions are dumped differently depending
on how exactly they are written, cluttering the diff with false
positives.

While we can do the actual splitting of objects from a -Fc dump
relatively easily, we can't fix the view definitions after they've been
dumped. So I'm proposing a --pretty-print-views setting to pg_dump
(patch attached).

Any feedback is welcome.

Regards,
Marko Tiikkaja

On Thu, Jan 10, 2013 at 01:23:10PM +0100, Marko Tiikkaja wrote:
> Hi,
>
> At the company I work for, we've been splitting dumps into separate
> files and diffing them for a while now. By far the biggest problem
> we had was with views: pg_dump by default dumps views on one line,
> in a format which maximizes compatibility. Now this has several
> problems for our use case:
>
> 1) The one-line equivalent of a 200-line view is completely impossible
> to read.
> 2) If there's a difference between the two dumped view definitions,
> it takes a long time to find where and what exactly it is.
> 3) For some reason some expressions are dumped differently depending
> on how exactly they are written, cluttering the diff with false
> positives.
>
> While we can do the actual splitting of objects from a -Fc dump
> relatively easily, we can't fix the view definitions after they've
> been dumped. So I'm proposing a --pretty-print-views setting to
> pg_dump (patch attached).
>
> Any feedback is welcome.

Why not make this the new default? That way, new users will have the
benefit, and people with tools or processes that depend on the old
behavior can still have it.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

* David Fetter (david(at)fetter(dot)org) wrote:
> On Thu, Jan 10, 2013 at 01:23:10PM +0100, Marko Tiikkaja wrote:
> > Any feedback is welcome.
>
> Why not make this the new default? That way, new users will have the
> benefit, and people with tools or processes that depend on the old
> behavior can still have it.

I tend to agree with this, I've never really liked having the view def
all on one massive line.

Well, I'll caveat that with this- being able to grep -C2 and pull the
views and the view definitions has been nice on occation, but you can
pull independent objects out with pg_restore from a -Fc dump and, in
general, I think the advantage of having the view definition look
reasonable in the dump is more than being able to do tricks on views
(but not tables, etc anyway..) with grep.

Thanks,

Stephen

On 01/10/2013 07:23 AM, Marko Tiikkaja wrote:
> Hi,
>
> At the company I work for, we've been splitting dumps into separate
> files and diffing them for a while now. By far the biggest problem we
> had was with views: pg_dump by default dumps views on one line, in a
> format which maximizes compatibility. Now this has several problems
> for our use case:
>
> 1) The one-line equivalent of a 200-line view is completely impossible
> to read.
> 2) If there's a difference between the two dumped view definitions,
> it takes a long time to find where and what exactly it is.
> 3) For some reason some expressions are dumped differently depending
> on how exactly they are written, cluttering the diff with false
> positives.
>
> While we can do the actual splitting of objects from a -Fc dump
> relatively easily, we can't fix the view definitions after they've
> been dumped. So I'm proposing a --pretty-print-views setting to
> pg_dump (patch attached).
>
>

For versions >= 9.2 it would be better to allow passing in a
pretty-print value, like 80 or 0, instead of just passing 'true'. The
new line-wrapping that the integer argument triggers is much more
readable than the supposedly pretty value that 'true' provides.

cheers

andrew

On 1/10/13 3:22 PM, Andrew Dunstan wrote:
> For versions >= 9.2 it would be better to allow passing in a
> pretty-print value, like 80 or 0, instead of just passing 'true'. The
> new line-wrapping that the integer argument triggers is much more
> readable than the supposedly pretty value that 'true' provides.

Ooh, I had no idea we exposed that in SQL. I like this idea.

Regards,
Marko Tiikkaja

Marko Tiikkaja <pgmail(at)joh(dot)to> writes:
> While we can do the actual splitting of objects from a -Fc dump
> relatively easily, we can't fix the view definitions after they've been
> dumped. So I'm proposing a --pretty-print-views setting to pg_dump
> (patch attached).

-1. The reason that pg_dump does not pretty-print things is that
it's unsafe; there is no real guarantee that the view will reload as
intended, because it's under-parenthesized. (Even if we were sure
it would reload safely into current code, which I'm not, what of
future versions that could have different operator precedences?)

I don't think we should offer a foot-gun option like this at all,
and as for making it the default, not bloody likely.

I think your schema-diffing needs would be better served by a tool
specifically directed at that problem; which pg_dump is not, but
I believe there are some out there.

regards, tom lane

On Thu, Jan 10, 2013 at 11:21:13AM -0500, Tom Lane wrote:
> Marko Tiikkaja <pgmail(at)joh(dot)to> writes:
> > While we can do the actual splitting of objects from a -Fc dump
> > relatively easily, we can't fix the view definitions after they've
> > been dumped. So I'm proposing a --pretty-print-views setting to
> > pg_dump (patch attached).
>
> -1. The reason that pg_dump does not pretty-print things is that
> it's unsafe; there is no real guarantee that the view will reload as
> intended, because it's under-parenthesized. (Even if we were sure
> it would reload safely into current code, which I'm not, what of
> future versions that could have different operator precedences?)

Under what circumstances do pretty-printed views not reload? It seems
to me that such circumstances would be pretty_print() bugs by
definition.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david(at)fetter(dot)org> writes:
> On Thu, Jan 10, 2013 at 11:21:13AM -0500, Tom Lane wrote:
>> -1. The reason that pg_dump does not pretty-print things is that
>> it's unsafe; there is no real guarantee that the view will reload as
>> intended, because it's under-parenthesized. (Even if we were sure
>> it would reload safely into current code, which I'm not, what of
>> future versions that could have different operator precedences?)

> Under what circumstances do pretty-printed views not reload? It seems
> to me that such circumstances would be pretty_print() bugs by
> definition.

It would not be a bug, particularly not in the case of a subsequent
release with different operator precedence. pg_dump's charter is to be
safe. Pretty-printing's charter is to look nice. These goals are not
compatible. If they were, we'd never have made a separate pretty
printing mode at all.

Now, we could consider changing the "safe" mode so that it tries to
provide nice whitespace/line breaks while not risking removal of
parentheses. But that would be a totally different patch, and I'm
not sure how much it would address Marko's desires anyway.

regards, tom lane

On 01/10/2013 12:09 PM, Tom Lane wrote:

> Now, we could consider changing the "safe" mode so that it tries to
> provide nice whitespace/line breaks while not risking removal of
> parentheses. But that would be a totally different patch, and I'm
> not sure how much it would address Marko's desires anyway.

I think there's a very good case for breaking the nexus between
PRETTYFLAG_PAREN and PRETTYFLAG_INDENT+line wrapping for views. Only
PRETTYFLAG_PAREN affects the safety issue. The others are just about
white space in safe places.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I think there's a very good case for breaking the nexus between
> PRETTYFLAG_PAREN and PRETTYFLAG_INDENT+line wrapping for views. Only
> PRETTYFLAG_PAREN affects the safety issue. The others are just about
> white space in safe places.

What I was actually thinking about was turning on indent/linewrapping
all the time (ie, no change on pg_dump's side, just hack ruleutils).
If we believe it's safe, why not just do it? It's the paren-removal
that pg_dump can't tolerate.

regards, tom lane

On 01/10/2013 12:35 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> I think there's a very good case for breaking the nexus between
>> PRETTYFLAG_PAREN and PRETTYFLAG_INDENT+line wrapping for views. Only
>> PRETTYFLAG_PAREN affects the safety issue. The others are just about
>> white space in safe places.
> What I was actually thinking about was turning on indent/linewrapping
> all the time (ie, no change on pg_dump's side, just hack ruleutils).
> If we believe it's safe, why not just do it? It's the paren-removal
> that pg_dump can't tolerate.
>
>

Works for me.

cheers

andrew

On Thu, Jan 10, 2013 at 11:07 PM, Andrew Dunstan <andrew(at)dunslane(dot)net>wrote:

>
> On 01/10/2013 12:35 PM, Tom Lane wrote:
>
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>
>>> I think there's a very good case for breaking the nexus between
>>> PRETTYFLAG_PAREN and PRETTYFLAG_INDENT+line wrapping for views. Only
>>> PRETTYFLAG_PAREN affects the safety issue. The others are just about
>>> white space in safe places.
>>>
>> What I was actually thinking about was turning on indent/linewrapping
>> all the time (ie, no change on pg_dump's side, just hack ruleutils).
>> If we believe it's safe, why not just do it? It's the paren-removal
>> that pg_dump can't tolerate.
>>
>>
>>
>
> Works for me.
>
>
Nice idea.

Marko, just hack ruleutils some thing like this:

diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 266cec5..a46f588 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -511,8 +511,9 @@ pg_get_viewdef(PG_FUNCTION_ARGS)
{
/* By OID */
Oid viewoid = PG_GETARG_OID(0);
+ int prettyFlags = PRETTYFLAG_INDENT;

- PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid, 0, -1)));
+ PG_RETURN_TEXT_P(string_to_text(pg_get_viewdef_worker(viewoid,
prettyFlags, WRAP_COLUMN_DEFAULT)));
}

Thanks

> cheers
>
> andrew
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/**mailpref/pgsql-hackers<http://www.postgresql.org/mailpref/pgsql-hackers>
>

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

On Fri, 18 Jan 2013 07:46:21 +0100, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> Nice idea.
>
> Marko, just hack ruleutils some thing like this:

Here's a patch attempting to do just that.

The actual code changes were trivial, the patch is mostly just regression
tests.

As for the docs, I wasn't entirely happy with what they say about
pg_get_viewdef(), but it didn't look like they needed to be changed by
this either.

Regards,
Marko Tiikkaja

Hi Marko,

I could not apply the patch with git apply, but able to apply it by patch
-p1 command.

However, will you please justify the changes done in "xml.out" ? I guess
they are not needed.
You might need to configure your sources with libxml.

Also, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. I will
keep that in code committors plate. Rest of the code changes looks good to
me.

Thanks

On Sun, Jan 27, 2013 at 6:23 PM, Marko Tiikkaja <pgmail(at)joh(dot)to> wrote:

> On Fri, 18 Jan 2013 07:46:21 +0100, Jeevan Chalke <
> jeevan(dot)chalke(at)enterprisedb(dot)**com <jeevan(dot)chalke(at)enterprisedb(dot)com>> wrote:
>
>> Nice idea.
>>
>> Marko, just hack ruleutils some thing like this:
>>
>
> Here's a patch attempting to do just that.
>
> The actual code changes were trivial, the patch is mostly just regression
> tests.
>
> As for the docs, I wasn't entirely happy with what they say about
> pg_get_viewdef(), but it didn't look like they needed to be changed by this
> either.
>
>
> Regards,
> Marko Tiikkaja

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

On 1/28/13 12:14 PM, Jeevan Chalke wrote:
> I could not apply the patch with git apply, but able to apply it by patch
> -p1 command.

IME that's normal for patches that went through filterdiff. I do: git
diff |filterdiff --format=context to re-format the patches to the
context diff preferred on the mailing list. Maybe if I somehow told git
to produce context diff it would work..

> However, will you please justify the changes done in "xml.out" ? I guess
> they are not needed.
> You might need to configure your sources with libxml.

If you look very carefully, the pretty-printing version adds one space
at the very beginning of the output. :-)

> Also, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. I will
> keep that in code committors plate. Rest of the code changes looks good to
> me.

Thanks for reviewing!

Regards,
Marko Tiikkaja

On 28/01/13 12:31, Marko Tiikkaja wrote:
> On 1/28/13 12:14 PM, Jeevan Chalke wrote:
>> I could not apply the patch with git apply, but able to apply it by patch
>> -p1 command.
>
> IME that's normal for patches that went through filterdiff. I do: git
> diff |filterdiff --format=context to re-format the patches to the
> context diff preferred on the mailing list. Maybe if I somehow told git
> to produce context diff it would work..

Try this, worked for me:

http://wiki.postgresql.org/wiki/Working_with_Git#Context_diffs_with_Git

Cheers,
Jan

Hi Marko,

On Mon, Jan 28, 2013 at 5:01 PM, Marko Tiikkaja <pgmail(at)joh(dot)to> wrote:

> On 1/28/13 12:14 PM, Jeevan Chalke wrote:
>
>> I could not apply the patch with git apply, but able to apply it by patch
>> -p1 command.
>>
>
> IME that's normal for patches that went through filterdiff. I do: git
> diff |filterdiff --format=context to re-format the patches to the context
> diff preferred on the mailing list. Maybe if I somehow told git to produce
> context diff it would work..
>
>
> However, will you please justify the changes done in "xml.out" ? I guess
>> they are not needed.
>> You might need to configure your sources with libxml.
>>
>
> If you look very carefully, the pretty-printing version adds one space at
> the very beginning of the output. :-)

That's fine. I am not at all pointing that to you. Have a look at this:

*** a/src/test/regress/expected/xml.out
--- b/src/test/regress/expected/xml.out
***************
*** 3,82 **** CREATE TABLE xmltest (
data xml
);
INSERT INTO xmltest VALUES (1, '<value>one</value>');
INSERT INTO xmltest VALUES (2, '<value>two</value>');
INSERT INTO xmltest VALUES (3, '<wrong');
! ERROR: invalid XML content
LINE 1: INSERT INTO xmltest VALUES (3, '<wrong');
^
! DETAIL: line 1: Couldn't find end of Start Tag wrong line 1
! <wrong
! ^

.
.
.
--- 3,84 ----
data xml
);
INSERT INTO xmltest VALUES (1, '<value>one</value>');
+ ERROR: unsupported XML feature
+ LINE 1: INSERT INTO xmltest VALUES (1, '<value>one</value>');
+ ^
+ DETAIL: This functionality requires the server to be built with libxml
support.
+ HINT: You need to rebuild PostgreSQL using --with-libxml.
INSERT INTO xmltest VALUES (2, '<value>two</value>');
+ ERROR: unsupported XML feature
+ LINE 1: INSERT INTO xmltest VALUES (2, '<value>two</value>');
+ ^
+ DETAIL: This functionality requires the server to be built with libxml
support.
+ HINT: You need to rebuild PostgreSQL using --with-libxml.

These changes are not at all required.
Follow the hint.

In other way, if I apply your patch and run make check I get regression
failure for xml.out.

Please check.

Thanks

>
>
> Also, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. I will
>> keep that in code committors plate. Rest of the code changes looks good to
>> me.
>>
>
> Thanks for reviewing!
>
>
> Regards,
> Marko Tiikkaja
>

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

On 1/29/13 10:18 AM, Jeevan Chalke wrote:
>
> That's fine. I am not at all pointing that to you. Have a look at this:

Ugh.. I'm sorry, I don't understand how this happened. I manually
looked through all the changes, but somehow this slipped through. Will
have a look this evening.

Regards,
Marko Tiikkaja

On Tue, 29 Jan 2013 10:18:51 +0100, Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> That's fine. I am not at all pointing that to you. Have a look at this:

Here's the third version of this patch, hopefully this time without any
problems. I looked through the patch and it looked OK, but I did that
last time too so I wouldn't trust myself on that one.

Regards,
Marko Tiikkaja

Hi Marko,

On Wed, Jan 30, 2013 at 2:07 AM, Marko Tiikkaja <pgmail(at)joh(dot)to> wrote:

> On Tue, 29 Jan 2013 10:18:51 +0100, Jeevan Chalke <
> jeevan(dot)chalke(at)enterprisedb(dot)**com <jeevan(dot)chalke(at)enterprisedb(dot)com>> wrote:
>
>> That's fine. I am not at all pointing that to you. Have a look at this:
>>
>
> Here's the third version of this patch, hopefully this time without any
> problems. I looked through the patch and it looked OK, but I did that last
> time too so I wouldn't trust myself on that one.
>

Looks good this time.

Will mark "Ready for committor"

However, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. In
my opinion we should put that by default but other people may object so I
will keep that in code committors plate.

Thanks

>
>
> Regards,
> Marko Tiikkaja

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

On 1/30/13 7:52 AM, Jeevan Chalke wrote:
> Looks good this time.
>
> Will mark "Ready for committor"

Thanks for reviewing it more carefully than I did! :-) And my apologies
for the confusion earlier.

> However, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. In
> my opinion we should put that by default but other people may object so I
> will keep that in code committors plate.

I have no opinion on this to one way or the other, so I'm fine with
waiting for someone else (possibly the committer) to decide this.

Regards,
Marko Tiikkaja

Marko Tiikkaja <pgmail(at)joh(dot)to> writes:
> On 1/30/13 7:52 AM, Jeevan Chalke wrote:
>> However, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. In
>> my opinion we should put that by default but other people may object so I
>> will keep that in code committors plate.

> I have no opinion on this to one way or the other, so I'm fine with
> waiting for someone else (possibly the committer) to decide this.

FWIW, I'd vote for not enabling that by default --- it's basically an
unwarranted assumption about how wide people's terminal windows are.

regards, tom lane

On 01/30/2013 09:58 AM, Tom Lane wrote:
> Marko Tiikkaja <pgmail(at)joh(dot)to> writes:
>> On 1/30/13 7:52 AM, Jeevan Chalke wrote:
>>> However, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. In
>>> my opinion we should put that by default but other people may object so I
>>> will keep that in code committors plate.
>> I have no opinion on this to one way or the other, so I'm fine with
>> waiting for someone else (possibly the committer) to decide this.
> FWIW, I'd vote for not enabling that by default --- it's basically an
> unwarranted assumption about how wide people's terminal windows are.
>
>

I'm not exactly sure what you're arguing for.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/30/2013 09:58 AM, Tom Lane wrote:
>> Marko Tiikkaja <pgmail(at)joh(dot)to> writes:
>>> On 1/30/13 7:52 AM, Jeevan Chalke wrote:
>>>> However, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. In
>>>> my opinion we should put that by default but other people may object so I
>>>> will keep that in code committors plate.

>> FWIW, I'd vote for not enabling that by default --- it's basically an
>> unwarranted assumption about how wide people's terminal windows are.

> I'm not exactly sure what you're arguing for.

Maybe I'm confused - I thought the alternative Jeevan was talking about
was that we enable PRETTY_INDENT processing by default, but not
wrapColumn processing.

regards, tom lane

On 01/30/2013 11:03 AM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 01/30/2013 09:58 AM, Tom Lane wrote:
>>> Marko Tiikkaja <pgmail(at)joh(dot)to> writes:
>>>> On 1/30/13 7:52 AM, Jeevan Chalke wrote:
>>>>> However, I am not sure about putting "WRAP_COLUMN_DEFAULT" by default. In
>>>>> my opinion we should put that by default but other people may object so I
>>>>> will keep that in code committors plate.
>>> FWIW, I'd vote for not enabling that by default --- it's basically an
>>> unwarranted assumption about how wide people's terminal windows are.
>> I'm not exactly sure what you're arguing for.
> Maybe I'm confused - I thought the alternative Jeevan was talking about
> was that we enable PRETTY_INDENT processing by default, but not
> wrapColumn processing.
>
>

Well, we could actually set the wrap value to 0, which would mean always
wrap. That wouldn't be making any assumption about the user's terminal
window size ;-)

Here are two examples, one of which is with exactly Marko's patch which
doesn't change the wrap value (i.e. leaves it at -1, meaning no wrap) ,
the other sets it at 0. The example is a view that is the same as
information_schema.columns.

Personally I find the wrapped case MUCH more readable. I guess anything
is an advance, but turning on PRETTY_INDENT without turning on some
level of target wrapping seems like a half-done job.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/30/2013 11:03 AM, Tom Lane wrote:
>>>> FWIW, I'd vote for not enabling that by default --- it's basically an
>>>> unwarranted assumption about how wide people's terminal windows are.

> Well, we could actually set the wrap value to 0, which would mean always
> wrap. That wouldn't be making any assumption about the user's terminal
> window size ;-)

I could live with that. I rather wonder actually if WRAP_COLUMN_DEFAULT
shouldn't be removed in favor of using 0 as the default. 80-column
windows may be our coding standard for Postgres, but that doesn't mean
they're not pretty passe elsewhere.

regards, tom lane

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Well, we could actually set the wrap value to 0, which would mean always
> wrap. That wouldn't be making any assumption about the user's terminal
> window size ;-)

+1

> Personally I find the wrapped case MUCH more readable. I guess anything is
> an advance, but turning on PRETTY_INDENT without turning on some level of
> target wrapping seems like a half-done job.

Minor gripe: the CASE WHEN THEN indenting is not ideal:

CASE
WHEN (a.attnotnull OR ((t.typtype = 'd'::"char") AND t.typnotnull)) THEN 'NO'::text
ELSE 'YES'::text
END)::information_schema.yes_or_no AS is_nullable,

I think the following is easier to read:

CASE
WHEN (a.attnotnull OR ((t.typtype = 'd'::"char") AND t.typnotnull))
THEN 'NO'::text
ELSE 'YES'::text
END)::information_schema.yes_or_no AS is_nullable,

But I realise it's not exactly what's being talked about in this thread…

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Thu, Jan 31, 2013 at 4:40 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>wrote:

> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > Well, we could actually set the wrap value to 0, which would mean always
> > wrap. That wouldn't be making any assumption about the user's terminal
> > window size ;-)
>
> +1
>

+1

After looking at both the SQL, I too like wrapped case.

>
> > Personally I find the wrapped case MUCH more readable. I guess anything
> is
> > an advance, but turning on PRETTY_INDENT without turning on some level of
> > target wrapping seems like a half-done job.
>
> Minor gripe: the CASE WHEN THEN indenting is not ideal:
>
> CASE
> WHEN (a.attnotnull OR ((t.typtype = 'd'::"char") AND t.typnotnull))
> THEN 'NO'::text
> ELSE 'YES'::text
> END)::information_schema.yes_or_no AS is_nullable,
>
> I think the following is easier to read:
>
> CASE
> WHEN (a.attnotnull OR ((t.typtype = 'd'::"char") AND t.typnotnull))
> THEN 'NO'::text
> ELSE 'YES'::text
> END)::information_schema.yes_or_no AS is_nullable,
>
> But I realise it's not exactly what's being talked about in this thread…
>
> Regards,
> --
> Dimitri Fontaine
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
>

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

"Marko Tiikkaja" <pgmail(at)joh(dot)to> writes:
> Here's the third version of this patch, hopefully this time without any
> problems. I looked through the patch and it looked OK, but I did that
> last time too so I wouldn't trust myself on that one.

Applied with corrections.

The xml expected output was still wrong - to do that part right, you
need to update xml.out with an xml-enabled build and xml_1.out with a
non-xml-enabled build.

Also, it seemed to me that the patch didn't go far enough, in that it
only touched pg_get_viewdef and not the sister functions. pg_dump would
certainly want pg_get_ruledef to have the same behavior, and in general
the documentation seems to me to be clear that all these functions have
similar pretty-print-vs-not behavior. As committed, the pretty_bool
argument only affects PRETTY_PAREN processing for all of them.

I also went ahead and set the default wrap column to zero rather than
the former 79, since it seemed clear that people like that behavior
better.

regards, tom lane

On 2/3/13 10:06 PM, Tom Lane wrote:
> Applied with corrections.

Thank you.

> The xml expected output was still wrong - to do that part right, you
> need to update xml.out with an xml-enabled build and xml_1.out with a
> non-xml-enabled build.

Ahh. That explains a lot.

Regards,
Marko Tiikkaja

On Sun, Feb 3, 2013 at 4:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "Marko Tiikkaja" <pgmail(at)joh(dot)to> writes:
> > Here's the third version of this patch, hopefully this time without any
> > problems. I looked through the patch and it looked OK, but I did that
> > last time too so I wouldn't trust myself on that one.
>
> Applied with corrections.
>
> The xml expected output was still wrong - to do that part right, you
> need to update xml.out with an xml-enabled build and xml_1.out with a
> non-xml-enabled build.
>
> Also, it seemed to me that the patch didn't go far enough, in that it
> only touched pg_get_viewdef and not the sister functions. pg_dump would
> certainly want pg_get_ruledef to have the same behavior, and in general
> the documentation seems to me to be clear that all these functions have
> similar pretty-print-vs-not behavior. As committed, the pretty_bool
> argument only affects PRETTY_PAREN processing for all of them.
>
> I also went ahead and set the default wrap column to zero rather than
> the former 79, since it seemed clear that people like that behavior
> better.
>
> regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Was this ever committed into core? Apologies, I'm not very familiar with
looking through the commit history of the source code and I don't see
anything about this option or pretty-print outputs in the pg_dump/restore
docs for 9.3. Had someone asking me about this feature for pg_extractor

https://github.com/omniti-labs/pg_extractor/issues/28

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com

On 4/29/14 4:29 PM, Keith Fiske wrote:
> Was this ever committed into core? Apologies, I'm not very familiar with
> looking through the commit history of the source code and I don't see
> anything about this option or pretty-print outputs in the pg_dump/restore
> docs for 9.3. Had someone asking me about this feature for pg_extractor

No, the idea was rejected.

Regards,
Marko Tiikkaja

Keith Fiske <keith(at)omniti(dot)com> writes:
> On Sun, Feb 3, 2013 at 4:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Applied with corrections.

> Was this ever committed into core? Apologies, I'm not very familiar with
> looking through the commit history of the source code and I don't see
> anything about this option or pretty-print outputs in the pg_dump/restore
> docs for 9.3. Had someone asking me about this feature for pg_extractor

Yeah, if I say "applied" that means I committed it. Here's the commit
log entry:

Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Branch: master Release: REL9_3_BR [62e666400] 2013-02-03 15:56:45 -0500

Perform line wrapping and indenting by default in ruleutils.c.

This patch changes pg_get_viewdef() and allied functions so that
PRETTY_INDENT processing is always enabled. Per discussion, only the
PRETTY_PAREN processing (that is, stripping of "unnecessary" parentheses)
poses any real forward-compatibility risk, so we may as well make dump
output look as nice as we safely can.

Also, set the default wrap length to zero (i.e, wrap after each SELECT
or FROM list item), since there's no very principled argument for the
former default of 80-column wrapping, and most people seem to agree this
way looks better.

Marko Tiikkaja, reviewed by Jeevan Chalke, further hacking by Tom Lane

As per the branch annotation, this is in 9.3 and up.

regards, tom lane

On Tue, Apr 29, 2014 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Keith Fiske <keith(at)omniti(dot)com> writes:
> > On Sun, Feb 3, 2013 at 4:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Applied with corrections.
>
> > Was this ever committed into core? Apologies, I'm not very familiar with
> > looking through the commit history of the source code and I don't see
> > anything about this option or pretty-print outputs in the pg_dump/restore
> > docs for 9.3. Had someone asking me about this feature for pg_extractor
>
> Yeah, if I say "applied" that means I committed it. Here's the commit
> log entry:
>
> Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> Branch: master Release: REL9_3_BR [62e666400] 2013-02-03 15:56:45 -0500
>
> Perform line wrapping and indenting by default in ruleutils.c.
>
> This patch changes pg_get_viewdef() and allied functions so that
> PRETTY_INDENT processing is always enabled. Per discussion, only the
> PRETTY_PAREN processing (that is, stripping of "unnecessary"
> parentheses)
> poses any real forward-compatibility risk, so we may as well make dump
> output look as nice as we safely can.
>
> Also, set the default wrap length to zero (i.e, wrap after each SELECT
> or FROM list item), since there's no very principled argument for the
> former default of 80-column wrapping, and most people seem to agree
> this
> way looks better.
>
> Marko Tiikkaja, reviewed by Jeevan Chalke, further hacking by Tom Lane
>
> As per the branch annotation, this is in 9.3 and up.
>
> regards, tom lane
>

Great! Thanks, Tom

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com

Huh, I had assumed this was old behaviour. I didn't realize this was
new with 9.3.

Considering the thread "pg_get_viewdefs() indentation considered
harmful" I'm beginning to think this was a regression. It results in
some dump files being unnecessarily large and the pg_dump consuming
too much memory and crashing.

Tom liked my suggestion of doing the indentation modulo 40. There are
plenty of other suggestions that can work too, giving up on
indentation after an indent of 40, switching to an indent distance of
1 if it's more than 10 levels deep, and so on. I think it doesn't
really matter which we choose but we have to do something. And given
this is new behaviour in 9.3 perhaps it should be backported too.

Greg Stark <stark(at)mit(dot)edu> writes:
> Huh, I had assumed this was old behaviour. I didn't realize this was
> new with 9.3.

> Considering the thread "pg_get_viewdefs() indentation considered
> harmful" I'm beginning to think this was a regression. It results in
> some dump files being unnecessarily large and the pg_dump consuming
> too much memory and crashing.

> Tom liked my suggestion of doing the indentation modulo 40. There are
> plenty of other suggestions that can work too, giving up on
> indentation after an indent of 40, switching to an indent distance of
> 1 if it's more than 10 levels deep, and so on. I think it doesn't
> really matter which we choose but we have to do something. And given
> this is new behaviour in 9.3 perhaps it should be backported too.

I'm still a bit skeptical about this being a catastrophic problem in
practice ... but anyway, I thought there were two somewhat different
proposals on the table in that thread:

1. Arrange for "x UNION y UNION z ..." to put all the UNION arms at
the same indentation level.

2. Change the indentation rules globally, in one or another fashion
as Greg mentions above, to prevent ruleutils from ever prepending
silly amounts of whitespace.

These are not mutually exclusive, and I think we should do both.
But it seems we're hung up on exactly which flavor of #2 to do.

I would argue that rules such as "reduce the indent step once we're too
far over" don't fix the problem, just postpone it. If we're taking this
complaint seriously at all then there needs to be a guaranteed maximum
indent distance, one way or another. So I'd go for either a modulo-N
rule or a hard limit, with some preference for the modulo-N approach.
Yeah, it does sound silly at first, but I think it'd preserve
readability better than just capping the indent.

regards, tom lane

I wrote:
> I'm still a bit skeptical about this being a catastrophic problem in
> practice ... but anyway, I thought there were two somewhat different
> proposals on the table in that thread:

> 1. Arrange for "x UNION y UNION z ..." to put all the UNION arms at
> the same indentation level.

> 2. Change the indentation rules globally, in one or another fashion
> as Greg mentions above, to prevent ruleutils from ever prepending
> silly amounts of whitespace.

> These are not mutually exclusive, and I think we should do both.

Here's a draft patch tackling point 1. This gets rid of a whole lot
of parenthesization, as well as indentation, for simple UNION lists.
You can see the results in the changed regression test outputs.

There are still a few oddities in the printout format, such as the
forced space between ( and SELECT at the start of a subquery;
but changing that would require touching more than just
get_setop_query(), and I'm not too sure what else would be affected,
so I let it be.

While I was testing this I noticed that there's something thoroughly
busted about the indentation of outer JOIN constructs, too --- you
can see this in some of the regression test queries that are touched
by this patch, where the JOINs are actually outdented to the left of
their FROM clause in the unpatched output. (They'd be outdented in
the new output, too, if negative indentation were possible...)
That seems like material for a separate patch though.

Comments?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Here's a draft patch tackling point 1. This gets rid of a whole lot
> of parenthesization, as well as indentation, for simple UNION lists.
> You can see the results in the changed regression test outputs.
[...]
> Comments?

+10000.

Thanks,

Stephen

I wrote:
> While I was testing this I noticed that there's something thoroughly
> busted about the indentation of outer JOIN constructs, too --- you
> can see this in some of the regression test queries that are touched
> by this patch, where the JOINs are actually outdented to the left of
> their FROM clause in the unpatched output. (They'd be outdented in
> the new output, too, if negative indentation were possible...)
> That seems like material for a separate patch though.

I poked into that too, and found that it seems to have been busted from
the very beginning. There's a unmatched subtraction of
PRETTYINDENT_JOIN_ON from the indentLevel, which I suppose must be an
accidental leftover from some previous version of the logic. And the code
is also trying to outdent JOIN clauses further than the corresponding
FROM, which seems unintuitive to me, even when the FROM is indented far
enough to make it possible (which it generally isn't in simple views).

Attached are two separate versions of a repair patch. The first one
causes JOIN clauses to be indented the same as their parent FROM.
The second one moves them over an additional 5 spaces. I find the
second layout more sensible, but it does result in significantly
more changes in the regression test outputs, as you can see.

Comments?

regards, tom lane

On Wed, Apr 30, 2014 at 6:33 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Here's a draft patch tackling point 1. This gets rid of a whole lot
>> of parenthesization, as well as indentation, for simple UNION lists.
>> You can see the results in the changed regression test outputs.
> [...]
>> Comments?
>
> +10000.
+1. Output is far easier to read.
--
Michael