Re: [PATCH] Add section headings to index types doc

Hi hackers,

Every time I have to look up what kinds of operations each index type is
suitable for, I get annoyed by the index types page being virtually
unskimmable due to not having headings for each index type.

Attached is a patch that adds <sect2> tags for each index type to make
it easier to see where the description of each one starts.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:

> Hi hackers,
>
> Every time I have to look up what kinds of operations each index type is
> suitable for, I get annoyed by the index types page being virtually
> unskimmable due to not having headings for each index type.
>
> Attached is a patch that adds <sect2> tags for each index type to make
> it easier to see where the description of each one starts.

Added to the next commitfest:

https://commitfest.postgresql.org/29/2665/

Also, for easier review, here's the `git diff -w` output, since the
<sect2> tags caused most of the page to have to be renidented.

Tangentially, does anyone know of a tool to strip whitespace changes
from an existing diff, as if it had been generated with `-w` in the
first place?

diff --git a/doc/src/sgml/indices.sgml b/doc/src/sgml/indices.sgml
index 28adaba72d..332d161547 100644
--- a/doc/src/sgml/indices.sgml
+++ b/doc/src/sgml/indices.sgml
@@ -122,6 +122,9 @@
B-tree indexes, which fit the most common situations.
</para>

+ <sect2 id="indexes-types-btree">
+ <title>B-tree</title>
+
<para>
<indexterm>
<primary>index</primary>
@@ -172,6 +175,10 @@
This is not always faster than a simple scan and sort, but it is
often helpful.
</para>
+ </sect2>
+
+ <sect2 id="indexes-types-hash">
+ <title>Hash</title>

<para>
<indexterm>
@@ -191,6 +198,10 @@
CREATE INDEX <replaceable>name</replaceable> ON <replaceable>table</replaceable> USING HASH (<replaceable>column</replaceable>);
</synopsis>
</para>
+ </sect2>
+
+ <sect2 id="indexes-type-gist">
+ <title>GiST</title>

<para>
<indexterm>
@@ -246,6 +257,10 @@
In <xref linkend="gist-builtin-opclasses-table"/>, operators that can be
used in this way are listed in the column <quote>Ordering Operators</quote>.
</para>
+ </sect2>
+
+ <sect2 id="indexes-type-spgist">
+ <title>SP-GiST</title>

<para>
<indexterm>
@@ -286,6 +301,10 @@
corresponding operator is specified in the <quote>Ordering Operators</quote>
column in <xref linkend="spgist-builtin-opclasses-table"/>.
</para>
+ </sect2>
+
+ <sect2 id="indexes-types-gin">
+ <title>GIN</title>

<para>
<indexterm>
@@ -327,6 +346,10 @@
classes are available in the <literal>contrib</literal> collection or as separate
projects. For more information see <xref linkend="gin"/>.
</para>
+ </sect2>
+
+ <sect2 id="indexes-types-brin">
+ <title>BRIN</title>

<para>
<indexterm>
@@ -360,6 +383,7 @@
documented in <xref linkend="brin-builtin-opclasses-table"/>.
For more information see <xref linkend="brin"/>.
</para>
+ </sect2>
</sect1>

- ilmari
--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

On Mon, Aug 3, 2020 at 1:32 PM Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>
wrote:

> ilmari(at)ilmari(dot)org (Dagfinn Ilmari Mannsåker) writes:
>
> > Hi hackers,
> >
> > Every time I have to look up what kinds of operations each index type is
> > suitable for, I get annoyed by the index types page being virtually
> > unskimmable due to not having headings for each index type.
> >
> > Attached is a patch that adds <sect2> tags for each index type to make
> > it easier to see where the description of each one starts.
>
> Added to the next commitfest:
>
> https://commitfest.postgresql.org/29/2665/
>
> Also, for easier review, here's the `git diff -w` output, since the
> <sect2> tags caused most of the page to have to be renidented.
>
> Tangentially, does anyone know of a tool to strip whitespace changes
> from an existing diff, as if it had been generated with `-w` in the
> first place?
>

I think you can do something like:

combinediff -w 0001-Add-section-headers-to-index-types-doc.patch /dev/null

(combinediff requires two diffs, but one can be /dev/null)

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

ilmari(at)ilmari(dot)org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Also, for easier review, here's the `git diff -w` output, since the
> <sect2> tags caused most of the page to have to be renidented.

TBH, I'd suggest just not being anal about whether the indentation
nesting is perfect ;-). There are certainly plenty of places in
the SGML files today where it is not. And for something like this,
I doubt the gain is worth the loss of "git blame" tracking and
possible back-patching hazards.

I'm a compulsive neatnik when it comes to indentation of the
C code, but much less so about the SGML docs. YMMV of course.

regards, tom lane

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: tested, passed

a) I'm wondering if we should apply one more change to this page. The line-by-line listing of operators occupies much space. What do you think about an inline list (in a separate line) of the operators?

old source:
<simplelist>
<member><literal>&lt;</literal></member>
<member><literal>&lt;=</literal></member>
<member><literal>=</literal></member>
<member><literal>&gt;=</literal></member>
<member><literal>&gt;</literal></member>
</simplelist>

new source:
<synopsis>&lt; &nbsp; &lt;= &nbsp; = &nbsp; &gt;= &nbsp; &gt;</synopsis>
It looks nice in HTML as well as in PDF.

b) I'm in favor of the indentation of all affected lines as it is done in the patch.

The new status of this patch is: Waiting on Author

On Mon, Aug 10, 2020 at 12:52:17PM +0000, Jürgen Purtz wrote:
> The new status of this patch is: Waiting on Author

This has not been answered yet, so I have marked the patch as returned
with feedback.
--
Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:

> On Mon, Aug 10, 2020 at 12:52:17PM +0000, Jürgen Purtz wrote:
>> The new status of this patch is: Waiting on Author
>
> This has not been answered yet, so I have marked the patch as returned
> with feedback.

Updated patch attached, wich reformats the operator lists as requested
by Jürgen, and skips the reindentation as suggested by Tom.

The reindentation patch is attached separately, in case the committer
decides they want it properly indented after all.

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

On 30/09/2020 14:25, Dagfinn Ilmari Mannsåker wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>
>> On Mon, Aug 10, 2020 at 12:52:17PM +0000, Jürgen Purtz wrote:
>>> The new status of this patch is: Waiting on Author
>>
>> This has not been answered yet, so I have marked the patch as returned
>> with feedback.
>
> Updated patch attached, wich reformats the operator lists as requested
> by Jürgen, and skips the reindentation as suggested by Tom.

I wonder if "synopsis" is the right markup for the operator lists. I'm
not too familiar with SGML, but the closest similar list I could find is
this in create_operator.sgml:

> The operator name is a sequence of up to <symbol>NAMEDATALEN</symbol>-1
> (63 by default) characters from the following list:
> <literallayout>
> + - * / &lt; &gt; = ~ ! @ # % ^ &amp; | ` ?
> </literallayout>

Reading up on the meaning of "literallayout" at
https://tdg.docbook.org/tdg/4.5/literallayout.html, though, it doesn't
sound quite right either. Maybe "<simplelist type=horiz'>" ?

- Heikki

On 30.09.20 14:53, Heikki Linnakangas wrote:
> On 30/09/2020 14:25, Dagfinn Ilmari Mannsåker wrote:
>> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>>
>>> On Mon, Aug 10, 2020 at 12:52:17PM +0000, Jürgen Purtz wrote:
>>>> The new status of this patch is: Waiting on Author
>>>
>>> This has not been answered yet, so I have marked the patch as returned
>>> with feedback.
>>
>> Updated patch attached, wich reformats the operator lists as requested
>> by Jürgen, and skips the reindentation as suggested by Tom.
>
> I wonder if "synopsis" is the right markup for the operator lists. I'm
> not too familiar with SGML, but the closest similar list I could find
> is this in create_operator.sgml:
>
>>    The operator name is a sequence of up to
>> <symbol>NAMEDATALEN</symbol>-1
>>    (63 by default) characters from the following list:
>> <literallayout>
>> + - * / &lt; &gt; = ~ ! @ # % ^ &amp; | ` ?
>> </literallayout>
>
> Reading up on the meaning of "literallayout" at
> https://tdg.docbook.org/tdg/4.5/literallayout.html, though, it doesn't
> sound quite right either. Maybe "<simplelist type=horiz'>" ?
>
> - Heikki

<literallyout> loses the aqua background color (in comparison to the
existing documentation).

<simplelist type="horiz"
columns="5"><member><literal>&lt;</literal></member> ... is very chatty:
it needs the additional 'columns' attribute and the additional 'member'
element.

Therefor I am in favor of the <synopsis> solution as given in the last
patch of Dagfinn.

Playing around I found another solution, which also looks quite good.
The chapter uses operators within the text flow at different places. All
of them are embedded in a <literal> element (inline). Using this
<literal> element also for the index-specific operators, the reading of
the page gets easier and the rendering is consistent. But the drawback
is that these operator are no longer accentuated. Because they
'represents' the possible access methods per index-type, one can argue
that they should be shown in a special way, eg.: in a separate paragraph
as in Dagfin's patch. (I suppose that this was the original intention of
the huge number of line-breaks.) It would look like the following, but I
don't recommend to use it:

--

Jürgen Purtz

On Wed, Sep 30, 2020 at 4:25 AM Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>
wrote:

> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>
> > On Mon, Aug 10, 2020 at 12:52:17PM +0000, Jürgen Purtz wrote:
> >> The new status of this patch is: Waiting on Author
> >
> > This has not been answered yet, so I have marked the patch as returned
> > with feedback.
>
> Updated patch attached, wich reformats the operator lists as requested
> by Jürgen,

A couple of things:

One, I would place the equality operator for hash inside a standalone
synopsis just like all of the others.
Two, why is hash special in having its create index command provided here?
(I notice this isn't the fault of this patch but it stands out when
reviewing it)

I would suggest rewording hash to look more like the others and add a link
to the "CREATE INDEX" command from the chapter preamble.

and skips the reindentation as suggested by Tom.
>

Agreed
David J.

On 21.10.20 23:12, David G. Johnston wrote:
> On Wed, Sep 30, 2020 at 4:25 AM Dagfinn Ilmari Mannsåker
> <ilmari(at)ilmari(dot)org <mailto:ilmari(at)ilmari(dot)org>> wrote:
>
> Michael Paquier <michael(at)paquier(dot)xyz <mailto:michael(at)paquier(dot)xyz>>
> writes:
>
> > On Mon, Aug 10, 2020 at 12:52:17PM +0000, Jürgen Purtz wrote:
> >> The new status of this patch is: Waiting on Author
> >
> > This has not been answered yet, so I have marked the patch as
> returned
> > with feedback.
>
> Updated patch attached, wich reformats the operator lists as requested
> by Jürgen,
>
>
> A couple of things:
>
> One, I would place the equality operator for hash inside a standalone
> synopsis just like all of the others.
ok
> Two, why is hash special in having its create index command provided
> here? (I notice this isn't the fault of this patch but it stands out
> when reviewing it)
yes, this looks odd.
>
> I would suggest rewording hash to look more like the others
ok
> and add a link to the "CREATE INDEX" command from the chapter preamble.
is the link necessary?
>
> and skips the reindentation as suggested by Tom.
>
>
> Agreed
> David J.

--

J. Purtz

On Fri, Oct 23, 2020 at 3:18 AM Jürgen Purtz <juergen(at)purtz(dot)de> wrote:

> and add a link to the "CREATE INDEX" command from the chapter preamble.
>
> is the link necessary?
>

I suppose it would make more sense to add it to the previous section - the
introduction page. I do think having a link (or more than one) to CREATE
INDEX from the Indexes chapter is reader friendly. Having links to SQL
commands is never truly necessary - the reader knows a SQL command
reference exists and the name of the command allows them to find the
correct page.

David J.

On 23.10.20 18:08, David G. Johnston wrote:
> On Fri, Oct 23, 2020 at 3:18 AM Jürgen Purtz <juergen(at)purtz(dot)de
> <mailto:juergen(at)purtz(dot)de>> wrote:
>
>> and add a link to the "CREATE INDEX" command from the chapter
>> preamble.
> is the link necessary?
>
>
> I suppose it would make more sense to add it to the previous section -
> the introduction page.  I do think having a link (or more than one) to
> CREATE INDEX from the Indexes chapter is reader friendly.  Having
> links to SQL commands is never truly necessary - the reader knows a
> SQL command reference exists and the name of the command allows them
> to find the correct page.
>
> David J.
>
I'm afraid I haven't grasped everything of your intentions and
suggestions of your last two mails.

- equal operator in standalone paragraph: ok, integrated.

- shift "create index ... using HASH" to a different place: You suggest
shifting the statement or a link to the previous (sub-)chapter "11.1
Introduction"? But there is already a "create index" example. Please
read my suggestion/modification in the first paragraph of the "11.2
Index Types" page.

- "rewording hash": I don't know what is missing here. But I have added
a few words about the nature of this index type.

Attached are two patches: a) summary against master and b) standalone of
my current changes.

--

J. Purtz

=?UTF-8?Q?J=c3=bcrgen_Purtz?= <juergen(at)purtz(dot)de> writes:
> Attached are two patches: a) summary against master and b) standalone of
> my current changes.

This was a bit confused ... as submitted, the patch wanted to commit
a couple of patchfiles. I sorted it out I believe, and pushed with
a little additional fiddling of my own.

I did not commit the reindentation of existing text --- I don't think
it's worth adding "git blame" noise for.

regards, tom lane