Re: [HACKERS] pg_dump --pretty-print-views
On Wed, Apr 30, 2014 at 6:33 AM, Stephen Frost wrote: > * Tom Lane (t...@sss.pgh.pa.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? > > +1. +1. Output is far easier to read. -- Michael -- 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] pg_dump --pretty-print-views
* Tom Lane (t...@sss.pgh.pa.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? +1. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] pg_dump --pretty-print-views
Greg Stark 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 -- 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] pg_dump --pretty-print-views
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. -- 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] pg_dump --pretty-print-views
On Tue, Apr 29, 2014 at 11:14 AM, Tom Lane wrote: > Keith Fiske writes: > > On Sun, Feb 3, 2013 at 4:06 PM, Tom Lane 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 > 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
Re: [HACKERS] pg_dump --pretty-print-views
Keith Fiske writes: > On Sun, Feb 3, 2013 at 4:06 PM, Tom Lane 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 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 -- 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] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
On Sun, Feb 3, 2013 at 4:06 PM, Tom Lane wrote: > "Marko Tiikkaja" 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@postgresql.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
Re: [HACKERS] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
"Marko Tiikkaja" 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump --pretty-print-views
On Thu, Jan 31, 2013 at 4:40 PM, Dimitri Fontaine wrote: > Andrew Dunstan 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.
Re: [HACKERS] pg_dump --pretty-print-views
Andrew Dunstan 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 -- 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] pg_dump --pretty-print-views
Andrew Dunstan 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 -- 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] pg_dump --pretty-print-views
On 01/30/2013 11:03 AM, Tom Lane wrote: Andrew Dunstan writes: On 01/30/2013 09:58 AM, Tom Lane wrote: Marko Tiikkaja 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 -- -- PostgreSQL database dump -- SET statement_timeout = 0; SET client_encoding = 'UTF8'; SET standard_conforming_strings = on; SET check_function_bodies = false; SET client_min_messages = warning; -- -- Name: plpgsql; Type: EXTENSION; Schema: -; Owner: -- CREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog; -- -- Name: EXTENSION plpgsql; Type: COMMENT; Schema: -; Owner: -- COMMENT ON EXTENSION plpgsql IS 'PL/pgSQL procedural language'; SET search_path = public, pg_catalog; -- -- Name: mycols; Type: VIEW; Schema: public; Owner: andrew -- CREATE VIEW mycols AS SELECT (current_database())::information_schema.sql_identifier AS table_catalog, (nc.nspname)::information_schema.sql_identifier AS table_schema, (c.relname)::information_schema.sql_identifier AS table_name, (a.attname)::information_schema.sql_identifier AS column_name, (a.attnum)::information_schema.cardinal_number AS ordinal_position, (pg_get_expr(ad.adbin, ad.adrelid))::information_schema.character_data AS column_default, ( 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, ( CASE WHEN (t.typtype = 'd'::"char") THEN CASE WHEN ((bt.typelem <> (0)::oid) AND (bt.typlen = (-1))) THEN 'ARRAY'::text WHEN (nbt.nspname = 'pg_catalog'::name) THEN format_type(t.typbasetype, NULL::integer) ELSE 'USER-DEFINED'::text END ELSE CASE WHEN ((t.typelem <> (0)::oid) AND (t.typlen = (-1))) THEN 'ARRAY'::text WHEN (nt.nspname = 'pg_catalog'::name) THEN format_type(a.atttypid, NULL::integer) ELSE 'USER-DEFINED'::text END END)::information_schema.character_data AS data_type, (information_schema._pg_char_max_length(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.cardinal_number AS character_maximum_length, (information_schema._pg_char_octet_length(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.cardinal_number AS character_octet_length, (information_schema._pg_numeric_precision(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.cardinal_number AS numeric_precision, (information_schema._pg_numeric_precision_radix(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.cardinal_number AS numeric_precision_radix, (information_schema._pg_numeric_scale(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.cardinal_number AS numeric_scale, (information_schema._pg_datetime_precision(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.cardinal_number AS datetime_precision, (information_schema._pg_interval_type(information_schema._pg_truetypid(a.*, t.*), information_schema._pg_truetypmod(a.*, t.*)))::information_schema.character_data AS interval_type, (NULL::integer)::information_schema.cardinal_number AS interval_precision, (NULL::character varying)::information_schema.sql_identifier AS character_set_catalog, (NULL::character varying)::information_schema.sql_identifier AS character_set_schema, (NULL::character varying)::information_schema.sql_identifier AS character_set_name, ( CASE WHEN (nco.nspname IS NOT NULL) THEN current_dat
Re: [HACKERS] pg_dump --pretty-print-views
Andrew Dunstan writes: > On 01/30/2013 09:58 AM, Tom Lane wrote: >> Marko Tiikkaja 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 -- 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] pg_dump --pretty-print-views
On 01/30/2013 09:58 AM, Tom Lane wrote: Marko Tiikkaja 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 -- 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] pg_dump --pretty-print-views
Marko Tiikkaja 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 -- 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] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
Hi Marko, On Wed, Jan 30, 2013 at 2:07 AM, Marko Tiikkaja wrote: > On Tue, 29 Jan 2013 10:18:51 +0100, Jeevan Chalke < > jeevan.chalke@enterprisedb.**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.
Re: [HACKERS] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
Hi Marko, On Mon, Jan 28, 2013 at 5:01 PM, 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.. > > > 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, 'one'); INSERT INTO xmltest VALUES (2, 'two'); INSERT INTO xmltest VALUES (3, 'one'); + ERROR: unsupported XML feature + LINE 1: INSERT INTO xmltest VALUES (1, 'one'); +^ + 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, 'two'); + ERROR: unsupported XML feature + LINE 1: INSERT INTO xmltest VALUES (2, 'two'); +^ + 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.
Re: [HACKERS] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
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 wrote: > On Fri, 18 Jan 2013 07:46:21 +0100, Jeevan Chalke < > jeevan.chalke@enterprisedb.**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.
Re: [HACKERS] pg_dump --pretty-print-views
On Thu, Jan 10, 2013 at 11:07 PM, Andrew Dunstan wrote:
>
> On 01/10/2013 12:35 PM, Tom Lane wrote:
>
>> Andrew Dunstan 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@postgresql.org)
> To make changes to your subscription:
> 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.
Re: [HACKERS] pg_dump --pretty-print-views
On 01/10/2013 12:35 PM, Tom Lane wrote: Andrew Dunstan 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 -- 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] pg_dump --pretty-print-views
Andrew Dunstan 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 -- 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] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
David Fetter 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 -- 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] pg_dump --pretty-print-views
On Thu, Jan 10, 2013 at 11:21:13AM -0500, Tom Lane wrote: > Marko Tiikkaja 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 http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.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 -- 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] pg_dump --pretty-print-views
Marko Tiikkaja 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 -- 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] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
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 -- 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] pg_dump --pretty-print-views
* David Fetter (da...@fetter.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 signature.asc Description: Digital signature
Re: [HACKERS] pg_dump --pretty-print-views
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 http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
***
*** 719,724 PostgreSQL documentation
--- 719,736
+ --pretty-print-views
+
+
+ Output views in a pretty-printed format. This makes
the
+ view definitions appearing in the dump easier to read for humans, but
+ at the same time makes them less likely to be interpreted in the exact
+ same way between different versions of PostgreSQL.
+
+
+
+
+
--quote-all-identifiers
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***
*** 137,142 static int dump_inserts = 0;
--- 137,143
static intcolumn_inserts = 0;
static intno_security_labels = 0;
static intno_unlogged_table_data = 0;
+ static intpretty_print_views = 0;
static intserializable_deferrable = 0;
***
*** 345,350 main(int argc, char **argv)
--- 346,352
{"inserts", no_argument, &dump_inserts, 1},
{"lock-wait-timeout", required_argument, NULL, 2},
{"no-tablespaces", no_argument, &outputNoTablespaces, 1},
+ {"pretty-print-views", no_argument, &pretty_print_views, 1},
{"quote-all-identifiers", no_argument, "e_all_identifiers,
1},
{"role", required_argument, NULL, 3},
{"section", required_argument, NULL, 5},
***
*** 606,611 main(int argc, char **argv)
--- 608,619
no_security_labels = 1;
/*
+* Pretty-printing views requires PostgreSQL 7.4 or later
+*/
+ if (pretty_print_views && fout->remoteVersion < 70400)
+ exit_horribly(NULL, "--pretty-print-views requires server
version 7.4 or later\n");
+
+ /*
* Start transaction-snapshot mode transaction to dump consistent data.
*/
ExecuteSqlStatement(fout, "BEGIN");
***
*** 840,845 help(const char *progname)
--- 848,856
printf(_(" --no-security-labels do not dump security label
assignments\n"));
printf(_(" --no-tablespaces do not dump tablespace
assignments\n"));
printf(_(" --no-unlogged-table-data do not dump unlogged table
data\n"));
+ printf(_(" --pretty-print-views\n"
+" dump views in a more
human-readable format (reduces\n"
+" compatibility of the
output dump)\n"));
printf(_(" --quote-all-identifiers quote all identifiers, even if
not key words\n"));
printf(_(" --section=SECTIONdump named section (pre-data,
data, or post-data)\n"));
printf(_(" --serializable-deferrablewait until the dump can run
without anomalies\n"));
***
*** 12387,12393 dumpTableSchema(Archive *fout, TableInfo *tbinfo)
reltypename = "VIEW";
/* Fetch the view definition */
! if (fout->remoteVersion >= 70300)
{
/* Beginning in 7.3, viewname is not unique; rely on
OID */
appendPQExpBuffer(query,
--- 12398,12411
reltypename = "VIEW";
/* Fetch the view definition */
! if (pretty_print_views)
! {
! /* Server must be 7.4 or later, so viewname is not unique; rely
on OID */
! appendPQExpBuffer(query,
! "SELECT
pg_catalog.pg_get_viewdef('%u'::pg_catalog.oid, TRUE) AS viewdef",
!
tbinfo->dobj.catId.oid);
! }
! else if (fout->remoteVersion >= 70300)
{
/* Beginning in 7.3, viewname is not unique; rely on
OID */
