Обсуждение: [HACKERS] [PATCH] Remove trailing whitespaces from documentation

Vladimir Rusinov <vrusinov@google.com> 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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Vladimir Rusinov <vrusinov@google.com> 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

Stephen Frost <sfrost@snowman.net> 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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> 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

* Vladimir Rusinov (vrusinov@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. <literal>on</> and <literal>off</> are also accepted, as
>          aliases for <literal>md5</> and <literal>plain</>, respectively.
>         </para>
> -
> +
>        </listitem>
>       </varlistentry>

> 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%';
>
>    <itemizedlist>
>      <listitem>
> -      <para>
> +      <para>
>          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.
>        </para>
>      </listitem>
>
> @@ -153,7 +153,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%';
>          <literal>FOR x IN query LOOP .. END LOOP</literal> 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.
>        </para>
>      </listitem>
>
> @@ -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.
>        </para>
>      </listitem>
>
> @@ -197,7 +197,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%';
>
>    <itemizedlist>
>      <listitem>
> -      <para>
> +      <para>
>          No background workers can be obtained because of the limitation that
>          the total number of background workers cannot exceed
>          <xref linkend="guc-max-worker-processes">.
> @@ -205,7 +205,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%';
>      </listitem>
>
>      <listitem>
> -      <para>
> +      <para>
>          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 <xref linkend="guc-max-parallel-workers">.
> @@ -213,7 +213,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%';
>      </listitem>
>
>      <listitem>
> -      <para>
> +      <para>
>          The client sends an Execute message with a non-zero fetch count.
>          See the discussion of the
>          <link linkend="protocol-flow-ext-query">extended query protocol</link>.
> @@ -228,7 +228,7 @@ EXPLAIN SELECT * FROM pgbench_accounts WHERE filler LIKE '%x%';
>      </listitem>
>
>      <listitem>
> -      <para>
> +      <para>
>          A prepared statement is executed using a <literal>CREATE TABLE .. AS
>          EXECUTE ..</literal> 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%';
>      </listitem>
>
>      <listitem>
> -      <para>
> +      <para>
>          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
>      <literal>PARALLEL RESTRICTED</literal>
> -    to ensure that they execute only in the leader.
> +    to ensure that they execute only in the leader.
>    </para>
>
>    <para>
> @@ -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 <literal>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 the scan of that table below a
>      <literal>Gather</> node.  In some cases, it would be
>      possible (and perhaps even efficient) to include the scan of that table in
>      the parallel portion of the query and defer the evaluation of the

> diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
> index 333b01d..8ea6624 100644
> --- a/doc/src/sgml/ref/alter_table.sgml
> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -1121,7 +1121,7 @@ ALTER TABLE [ IF EXISTS ] <replaceable class="PARAMETER">name</replaceable>
>      Also, because selecting from the parent also selects from its descendents,
>      a constraint on the parent cannot be marked valid unless it is also marked
>      valid for those descendents.  In all of these cases, <command>ALTER TABLE
> -    ONLY</command> will be rejected.
> +    ONLY</command> will be rejected.
>     </para>
>
>     <para>

> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
> index 00c984d..9339826 100644
> --- a/doc/src/sgml/ref/insert.sgml
> +++ b/doc/src/sgml/ref/insert.sgml
> @@ -526,7 +526,7 @@ INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</repl
>     updated by the command.
>    </para>
>   </refsect1>
> -
> +
>   <refsect1>
>    <title>Notes</title>

> diff --git a/doc/src/sgml/ref/prepare.sgml b/doc/src/sgml/ref/prepare.sgml
> index 8efd51a..fea2196 100644
> --- a/doc/src/sgml/ref/prepare.sgml
> +++ b/doc/src/sgml/ref/prepare.sgml
> @@ -73,7 +73,7 @@ PREPARE <replaceable class="PARAMETER">name</replaceable> [ ( <replaceable class
>     Prepared statements potentially have the largest performance advantage
>     when a single session is being used to execute a large number of similar
>     statements. The performance difference will be particularly
> -   significant if the statements are complex to plan or rewrite, e.g.
> +   significant if the statements are complex to plan or rewrite, e.g.
>     if the query involves a join of many tables or requires
>     the application of several rules. If the statement is relatively simple
>     to plan and rewrite but relatively expensive to execute, the

> diff --git a/doc/src/sgml/ref/reindexdb.sgml b/doc/src/sgml/ref/reindexdb.sgml
> --- a/doc/src/sgml/ref/reindexdb.sgml
> +++ b/doc/src/sgml/ref/reindexdb.sgml
> @@ -24,7 +24,7 @@ PostgreSQL documentation
>     <command>reindexdb</command>
>     <arg rep="repeat"><replaceable>connection-option</replaceable></arg>
>     <arg rep="repeat"><replaceable>option</replaceable></arg>
> -
> +
>     <arg choice="plain" rep="repeat">
>      <arg choice="opt">
>       <group choice="plain">

* Vladimir Rusinov (vrusinov@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

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

* Peter Eisentraut (peter.eisentraut@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

Stephen Frost <sfrost@snowman.net> writes:
> * Peter Eisentraut (peter.eisentraut@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/</</g to an example in order
to put it in the docs; why is stripping trailing whitespace a bigger
issue?
        regards, tom lane

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/</</g to an example in order
> 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

Tom,

* Tom Lane (tgl@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/</</g to an example in order
> 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/</</g,
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
</closeverbatim> 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

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/</</g to an example in order
> 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro,

* Alvaro Herrera (alvherre@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/</</g to an example in order
> > 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

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/</</g,
> 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
> </closeverbatim> 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

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