Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
On 12/14/16 6:10 PM, Vladimir Rusinov wrote: > Either way, I've attached another version of my patch - this time it > avoids touching example psql output. Baby steps. > I'll let you decide on the way forward. I'm just happy to send some patches. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] [PATCH] Remove trailing whitespaces from documentation
On 12/14/16 5:12 PM, Stephen Frost wrote: > For my 2c, at least, because we're going to be constantly fighting with > the trailing whitespace in those examples. If you forget to s/ the docs aren't going to build and it's going to be extremely obvious > that you need to do something. Not that I'm actually happy about that- > I'd much rather tell the doc engine "copy this verbatim until you see a > tag" or whatever, and not have to go hack up psql > output at all, This already exists: git grep -w CDATA > That said, if we can make git complain about trailing whitespace in the > docs but not mind it in the regression test output, then at least most > will hopefully realize that they need to go through and strip out the > trailing whitespace before committing. That would be easy to do. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] [PATCH] Remove trailing whitespaces from documentation
On Wed, Dec 14, 2016 at 9:37 PM, Stephen Frost wrote: > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > > On 12/14/16 12:03 PM, Stephen Frost wrote: > > > If we do want to change that, perhaps we should also change psql to not > > > output the trailing whitespace in the first place..? > > > > Previous discussion: > > https://www.postgresql.org/message-id/flat/1285093687. > 5468.18.camel%40vanquo.pezone.net > > Thanks for that, but, frankly, it seems like most were in agreement that > we should go ahead and get rid of the trailing whitespace from psql's > output. The last couple emails on that thread hardly seems like a > critical issue (and I'm not sure why we couldn't eliminate that > whitespace and then programmatically forbid trailing whitespace > anyway..). > Thanks for the pointer, that's quite interesting. The real problem here, IMO, is the break in expected regression outputs. > The previous thread mainly discussed that in terms of its impact on > third-party tests using pg_regress, but for our own purposes it would be > just as nasty to need to adjust every single test case we back-patch for > the next five years. That's funny. Linked thread is from 2010. Here we are in 2016 arguing about it (we would've done by now). :) Looks like cost of having them around is not exactly 0. Either way, I've attached another version of my patch - this time it avoids touching example psql output. Baby steps. I'll let you decide on the way forward. I'm just happy to send some patches. Overall, regression tests that compare output of psql seems like a solution not without drawbacks. I absolutely see the appeal, but this practically freezes behavior of psql and makes e.g. server tests depend not only server behavior but also on piece of irrelevant client-only code. I could imagine a test system that is both has more-or-less human-readable expected.out files and does not depend on exact decorations added by psql. -- Vladimir Rusinov Storage SRE, Google Ireland Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland Registered in Dublin, Ireland Registration Number: 368047 From 80cf7ee7f71c95c89e0908512e41154daff74d3a Mon Sep 17 00:00:00 2001 From: Vladimir Rusinov Date: Wed, 14 Dec 2016 22:19:08 + Subject: [PATCH 1/1] Remove some of the trailing whitespaces from documentation. They are considered bad practice in many style guides and many editors configured to strip them on every save. Such editors will produce spurious diffs when editing the documentation. Signed-off-by: Vladimir Rusinov --- doc/src/sgml/config.sgml | 2 +- doc/src/sgml/parallel.sgml| 22 +++--- doc/src/sgml/ref/alter_table.sgml | 2 +- doc/src/sgml/ref/insert.sgml | 2 +- doc/src/sgml/ref/prepare.sgml | 2 +- doc/src/sgml/ref/reindexdb.sgml | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 0fc4e57..3b614b6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1181,7 +1181,7 @@ include_dir 'conf.d' it in plaintext. on and off are also accepted, as aliases for md5 and plain, respectively. - + diff --git a/doc/src/sgml/parallel.sgml b/doc/src/sgml/parallel.sgml index cf4c1c9..bca4886 100644 --- a/doc/src/sgml/parallel.sgml +++ b/doc/src/sgml/parallel.sgml @@ -134,12 +134,12 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%'; - + The query writes any data or locks any database rows. If a query contains a data-modifying operation either at the top level or within a CTE, no parallel plans for that query will be generated. This is a limitation of the current implementation which could be lifted in a -future release. +future release. @@ -153,7 +153,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%'; FOR x IN query LOOP .. END LOOP will never use a parallel plan, because the parallel query system is unable to verify that the code in the loop is safe to execute while parallel query is -active. +active. @@ -174,7 +174,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%'; query itself, that query will never use a parallel plan. This is a limitation of the current implementation, but it may not be desirable to remove this limitation, since it could result in a single query -using a very large number of processes. +using a very large number of processes. @@ -197,7 +197,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%'; - + No background workers can be obtained because of the limitation that the total number of background workers cannot exceed . @@ -205,7 +205,7 @@ EXPLAIN SELECT * FROM pgbench_
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Tom Lane wrote: > > > Thinking about this, I'm wondering what is the connection between > > what psql does and what should be in the SGML (or XML) docs, anyway. > > Nobody says boo when we have to do s/ > to put it in the docs; why is stripping trailing whitespace a bigger > > issue? > > Why do we need to put regular psql output verbatim in SGML/XML? One > idea is to add an output format to psql for docbook table markup, then > use that whenever we need to paste stuff into the docs. I do tend to like this idea. Though, if we want to talk about writing lots of new code, what I really like is the pgBackrest doc building system, which actually executes pgBackrest during the documentation build process to run the commands and get exactly what they produce included in the docs. There's been more than one example of *incorrect* hacking up of the example in the docs, to the point where the example doesn't actually run, just this year. Perhaps if the regression tests ran the examples and produced the output in a way which could be included in the docs... Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
Tom Lane wrote: > Thinking about this, I'm wondering what is the connection between > what psql does and what should be in the SGML (or XML) docs, anyway. > Nobody says boo when we have to do s/ to put it in the docs; why is stripping trailing whitespace a bigger > issue? Why do we need to put regular psql output verbatim in SGML/XML? One idea is to add an output format to psql for docbook table markup, then use that whenever we need to paste stuff into the docs. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] [PATCH] Remove trailing whitespaces from documentation
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > The real problem here, IMO, is the break in expected regression outputs. > The previous thread mainly discussed that in terms of its impact on > third-party tests using pg_regress, but for our own purposes it would be > just as nasty to need to adjust every single test case we back-patch for > the next five years. I agree, that'd be annoying. Having to rewrite the fix for dumping casts with pg_dump for every single major version we support because the patch from the prior version couldn't be applied has certainly been a rather annoying exercise. > Thinking about this, I'm wondering what is the connection between > what psql does and what should be in the SGML (or XML) docs, anyway. > Nobody says boo when we have to do s/ to put it in the docs; why is stripping trailing whitespace a bigger > issue? For my 2c, at least, because we're going to be constantly fighting with the trailing whitespace in those examples. If you forget to s/ tag" or whatever, and not have to go hack up psql output at all, which I contend is also rather tedious and annoying to do when writing new code and new tests. Is that worse than having to deal with back-patching regression test output? Who knows. That said, if we can make git complain about trailing whitespace in the docs but not mind it in the regression test output, then at least most will hopefully realize that they need to go through and strip out the trailing whitespace before committing. Maybe we can come up with a psql output mode of "make this work for the docs" or a tool to pipe arbitrary text through to have it "do the right thing" for including it in the docs. I'm still a bit split, as I do feel like it's 'bad' of psql to be emitting the trailing whitespace in the first place, even if it's been done that way forever and even though it'll be an annoyance if we change it. Ultimately, I'm alright with either way though, I just don't like the idea of removing all the whitespace from the docs without a way to minimize the chance that we end up adding it back due to copy/paste from psql output. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
On 12/14/16 4:51 PM, Tom Lane wrote: > Thinking about this, I'm wondering what is the connection between > what psql does and what should be in the SGML (or XML) docs, anyway. > Nobody says boo when we have to do s/ to put it in the docs; why is stripping trailing whitespace a bigger > issue? There is a previous discussion for that, too, equally inconclusive: https://www.postgresql.org/message-id/flat/1383707883.10722.5.camel%40vanquo.pezone.net -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] [PATCH] Remove trailing whitespaces from documentation
Stephen Frost writes: > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: >> Previous discussion: >> https://www.postgresql.org/message-id/flat/1285093687.5468.18.camel%40vanquo.pezone.net > Thanks for that, but, frankly, it seems like most were in agreement that > we should go ahead and get rid of the trailing whitespace from psql's > output. The last couple emails on that thread hardly seems like a > critical issue (and I'm not sure why we couldn't eliminate that > whitespace and then programmatically forbid trailing whitespace > anyway..). The real problem here, IMO, is the break in expected regression outputs. The previous thread mainly discussed that in terms of its impact on third-party tests using pg_regress, but for our own purposes it would be just as nasty to need to adjust every single test case we back-patch for the next five years. The way I read that thread is that people were left feeling that it was probably more work than it was worth. That's what I'm thinking, anyway. Thinking about this, I'm wondering what is the connection between what psql does and what should be in the SGML (or XML) docs, anyway. Nobody says boo when we have to do s/http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote: > On 12/14/16 12:03 PM, Stephen Frost wrote: > > If we do want to change that, perhaps we should also change psql to not > > output the trailing whitespace in the first place..? > > Previous discussion: > https://www.postgresql.org/message-id/flat/1285093687.5468.18.camel%40vanquo.pezone.net Thanks for that, but, frankly, it seems like most were in agreement that we should go ahead and get rid of the trailing whitespace from psql's output. The last couple emails on that thread hardly seems like a critical issue (and I'm not sure why we couldn't eliminate that whitespace and then programmatically forbid trailing whitespace anyway..). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
On 12/14/16 12:03 PM, Stephen Frost wrote: > If we do want to change that, perhaps we should also change psql to not > output the trailing whitespace in the first place..? Previous discussion: https://www.postgresql.org/message-id/flat/1285093687.5468.18.camel%40vanquo.pezone.net -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] [PATCH] Remove trailing whitespaces from documentation
* Vladimir Rusinov (vrusi...@google.com) wrote: > I'm not sure if it makes sense to merge just these, as it will not help > people with whitespace-eating editors. I think we've established that it's going to be quite a while before we will reach a point where whitespace-eating editors aren't a problem when it comes to working with our docs. That doesn't mean we shouldn't fix obviously incorrect trailing whitespace in the docs. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
On Wed, Dec 14, 2016 at 5:41 PM, Stephen Frost wrote: > > They are considered bad practice in many style guides and many editors > > configured to stip them on every save. > > > > Such editors will produce spurious diffs when editing the documentation. > > > > Therefore, I propose this patch. > > As mentioned down-thread, most of this is from psql output and I don't > know that we actually want to get rid of that whitespace. Ideally, we > could indicate that trailing whitespace should be preserved when > printing examples, either with the SGML or the XML format. > > The (relativly few) ones I include below do look like cases we should > probably fix and back-patch (to simplify later back-patching efforts). > Most of these only go back to 9.6 (parallel.sgml) anyway. I'm not sure if it makes sense to merge just these, as it will not help people with whitespace-eating editors. -- Vladimir Rusinov Storage SRE, Google Ireland Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland Registered in Dublin, Ireland Registration Number: 368047 smime.p7s Description: S/MIME Cryptographic Signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
* Vladimir Rusinov (vrusi...@google.com) wrote: > They are considered bad practice in many style guides and many editors > configured to stip them on every save. > > Such editors will produce spurious diffs when editing the documentation. > > Therefore, I propose this patch. As mentioned down-thread, most of this is from psql output and I don't know that we actually want to get rid of that whitespace. Ideally, we could indicate that trailing whitespace should be preserved when printing examples, either with the SGML or the XML format. The (relativly few) ones I include below do look like cases we should probably fix and back-patch (to simplify later back-patching efforts). Most of these only go back to 9.6 (parallel.sgml) anyway. Thanks! Stephen > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml > index 0fc4e57..3b614b6 100644 > --- a/doc/src/sgml/config.sgml > +++ b/doc/src/sgml/config.sgml > @@ -1181,7 +1181,7 @@ include_dir 'conf.d' > it in plaintext. on and off are also > accepted, as > aliases for md5 and plain, respectively. > > - > + > > > diff --git a/doc/src/sgml/parallel.sgml b/doc/src/sgml/parallel.sgml > @@ -134,12 +134,12 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler > LIKE '%x%'; > > > > - > + > The query writes any data or locks any database rows. If a query > contains a data-modifying operation either at the top level or within > a CTE, no parallel plans for that query will be generated. This is a > limitation of the current implementation which could be lifted in a > -future release. > +future release. > > > > @@ -153,7 +153,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > FOR x IN query LOOP .. END LOOP will never use a > parallel plan, because the parallel query system is unable to verify > that the code in the loop is safe to execute while parallel query is > -active. > +active. > > > > @@ -174,7 +174,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > query itself, that query will never use a parallel plan. This is a > limitation of the current implementation, but it may not be desirable > to remove this limitation, since it could result in a single query > -using a very large number of processes. > +using a very large number of processes. > > > > @@ -197,7 +197,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > > > > - > + > No background workers can be obtained because of the limitation that > the total number of background workers cannot exceed > . > @@ -205,7 +205,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > > > > - > + > No background workers can be obtained because of the limitation that > the total number of background workers launched for purposes of > parallel query cannot exceed linkend="guc-max-parallel-workers">. > @@ -213,7 +213,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > > > > - > + > The client sends an Execute message with a non-zero fetch count. > See the discussion of the > extended query > protocol. > @@ -228,7 +228,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > > > > - > + > A prepared statement is executed using a CREATE TABLE .. AS > EXECUTE .. statement. This construct converts what > otherwise > would have been a read-only operation into a read-write operation, > @@ -237,7 +237,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > > > > - > + > The transaction isolation level is serializable. This situation > does not normally arise, because parallel query plans are not > generated when the transaction isolation level is serializable. > @@ -467,7 +467,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > transaction. If you write a function which does this, and this behavior > difference is important to you, mark such functions as > PARALLEL RESTRICTED > -to ensure that they execute only in the leader. > +to ensure that they execute only in the leader. > > > > @@ -475,7 +475,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE > '%x%'; > parallel-restricted functions or aggregates involved in the query in > order to obtain a superior plan. So, for example, if a WHERE > clause applied to a particular table is parallel restricted, the query > -planner will not consider placing the scan of that table below a > +planner will not consider placing
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
On Wed, Dec 14, 2016 at 5:14 PM, Tom Lane wrote: > Stephen Frost writes: > > If we do want to change that, perhaps we should also change psql to not > > output the trailing whitespace in the first place..? > > Yeah, maybe. I seem to recall having looked at that a long time ago > and deciding that it wasn't worth the trouble, but the code involved > has probably been restructured since then, so maybe it'd be easier now. > That sounds like a good idea, I guess I can take another look. I could think of no reason to keep whitespaces there. Of course, that would also create a back-patch breakpoint for every > single regression test expected-file, which is doubling down on the > PITA factor in a rather major way. > It looks like tests (at least in master) ignore whitespace-only diffs. So we can handle trailing whitespaces in expected output files as a separate issue. One way to ease the pain is to remove trailing whitespaces in all supported branches via separate patches. -- Vladimir Rusinov Storage SRE, Google Ireland Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland Registered in Dublin, Ireland Registration Number: 368047 smime.p7s Description: S/MIME Cryptographic Signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > If we do want to change that, perhaps we should also change psql to not > > output the trailing whitespace in the first place..? > > Yeah, maybe. I seem to recall having looked at that a long time ago > and deciding that it wasn't worth the trouble, but the code involved > has probably been restructured since then, so maybe it'd be easier now. > > Of course, that would also create a back-patch breakpoint for every > single regression test expected-file, which is doubling down on the > PITA factor in a rather major way. I'm not convinced that any of this is worth the effort. Wouldn't it be possible, if we are moving the docs to XML, to mark the examples in the docs in a way similar to LaTeX's 'verbatim' and allow trailing whitespace there? We wouldn't be able to use git to complain about trailing whitespace in the docs then, I don't think, but perhaps we could detect invalid trailing whitespace when building the docs and complain then. I expect that most of us build the docs before we commit anything to them anyway. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
On Wed, Dec 14, 2016 at 4:50 PM, Tom Lane wrote: > Vladimir Rusinov writes: > > Therefore, I propose this patch. > > Right now is a really bad time to do that; what it will mostly accomplish > is to break back-patching of doc fixes for little benefit. > ack. As I said, it's a proposal and I'm not too attached to it. Glad it sparked some discussions though. That said, I don't fully buy this argument: diff is very simple. Merge conflicts during backporting will be trivial to fix, although a bit more annoying. > There is work afoot to convert the documentation to xml. If that > succeeds, it'd make sense to strip trailing spaces (and start enforcing > that in .gitattributes) when we do that, since it'll be a back-patch > breakpoint anyway. But right now the PITA factor outweighs the benefit. > What is the ETA for this work to be complete (or fail and be abandoned)? -- Vladimir Rusinov Storage SRE, Google Ireland Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland Registered in Dublin, Ireland Registration Number: 368047 smime.p7s Description: S/MIME Cryptographic Signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
Stephen Frost writes: > If we do want to change that, perhaps we should also change psql to not > output the trailing whitespace in the first place..? Yeah, maybe. I seem to recall having looked at that a long time ago and deciding that it wasn't worth the trouble, but the code involved has probably been restructured since then, so maybe it'd be easier now. Of course, that would also create a back-patch breakpoint for every single regression test expected-file, which is doubling down on the PITA factor in a rather major way. 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] [PATCH] Remove trailing whitespaces from documentation
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Vladimir Rusinov writes: > > Therefore, I propose this patch. > > Right now is a really bad time to do that; what it will mostly accomplish > is to break back-patching of doc fixes for little benefit. > > There is work afoot to convert the documentation to xml. If that > succeeds, it'd make sense to strip trailing spaces (and start enforcing > that in .gitattributes) when we do that, since it'll be a back-patch > breakpoint anyway. But right now the PITA factor outweighs the benefit. Almost all of that patch was just removing the whitespace from examples where the actual commands used produced output with trailing whitespace. I'm not entirely sure we want to change those cases given that's what we do, in fact, produce as output. In other words, the trailing whitespace there isn't just because someone mistakenly included it. If we do want to change that, perhaps we should also change psql to not output the trailing whitespace in the first place..? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Remove trailing whitespaces from documentation
Vladimir Rusinov writes: > Therefore, I propose this patch. Right now is a really bad time to do that; what it will mostly accomplish is to break back-patching of doc fixes for little benefit. There is work afoot to convert the documentation to xml. If that succeeds, it'd make sense to strip trailing spaces (and start enforcing that in .gitattributes) when we do that, since it'll be a back-patch breakpoint anyway. But right now the PITA factor outweighs the benefit. 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
