Thread: Re: [COMMITTERS] pgsql: Move documentation of all recovery.conf option to a new chapter.

On Mon, 2010-02-22 at 11:47 +0000, Heikki Linnakangas wrote:
> Log Message:
> -----------
> Move documentation of all recovery.conf option to a new chapter.
> They used to be scattered between the "backup and restore" and "streaming
> replication" chapters.

It's just taken me 15 minutes to locate the settings for
primary_conninfo to better understand Stefan's recent post.

The commit referred to here is an extremely bad change.

If you intended this to be a heading within the "High Availability"
chapter then I would agree. This is what I thought you had done.

Having Streaming Rep described in HA and then describing the parameters
that make it work in a separate chapter is ridiculous. Plus, wherever
they are, they need cross references between them.

This is the last straw for me. The Streaming Rep docs are now truly
appalling. They were thin before, but tieing them in knots and splitting
them into disconnected pieces is just too much. If I can't find the damn
things, we've gone too far.

SR is a big feature and deserves proper docs.

-- Simon Riggs           www.2ndQuadrant.com

On Wed, Feb 24, 2010 at 8:24 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Mon, 2010-02-22 at 11:47 +0000, Heikki Linnakangas wrote:
>> Log Message:
>> -----------
>> Move documentation of all recovery.conf option to a new chapter.
>> They used to be scattered between the "backup and restore" and "streaming
>> replication" chapters.
>
> It's just taken me 15 minutes to locate the settings for
> primary_conninfo to better understand Stefan's recent post.

We would need to expose recovery configuration parameters as
indexterm, as well as GUC.

> The commit referred to here is an extremely bad change.
>
> If you intended this to be a heading within the "High Availability"
> chapter then I would agree. This is what I thought you had done.
>
> Having Streaming Rep described in HA and then describing the parameters
> that make it work in a separate chapter is ridiculous. Plus, wherever
> they are, they need cross references between them.
>
> This is the last straw for me. The Streaming Rep docs are now truly
> appalling. They were thin before, but tieing them in knots and splitting
> them into disconnected pieces is just too much. If I can't find the damn
> things, we've gone too far.
>
> SR is a big feature and deserves proper docs.

Yeah, we need to improve the SR document. So I began modifying it
in my git repository.
   * git://git.postgresql.org/git/users/fujii/postgres.git   * branch: replication-doc

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Simon Riggs wrote:
> On Mon, 2010-02-22 at 11:47 +0000, Heikki Linnakangas wrote:
>> Log Message:
>> -----------
>> Move documentation of all recovery.conf option to a new chapter.
>> They used to be scattered between the "backup and restore" and "streaming
>> replication" chapters.
> 
> It's just taken me 15 minutes to locate the settings for
> primary_conninfo to better understand Stefan's recent post.
> 
> The commit referred to here is an extremely bad change.
> 
> If you intended this to be a heading within the "High Availability"
> chapter then I would agree. This is what I thought you had done.

The idea was to have one chapter that describes all the options in
recovery.conf. Some of them are specific to streaming replication
(primary_conninfo), some are specific to PITR (recovery_target_*), some
are common (restore_command, restore_end_command). We need a reference
page to list them all.

> Having Streaming Rep described in HA and then describing the parameters
> that make it work in a separate chapter is ridiculous. Plus, wherever
> they are, they need cross references between them.

Yeah, there needs to be cross references. And the streaming rep docs
need to describe how to set the relevant settings, with examples.

> SR is a big feature and deserves proper docs.

Agreed.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

On Wed, 2010-02-24 at 08:30 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Mon, 2010-02-22 at 11:47 +0000, Heikki Linnakangas wrote:
> >> Log Message:
> >> -----------
> >> Move documentation of all recovery.conf option to a new chapter.
> >> They used to be scattered between the "backup and restore" and "streaming
> >> replication" chapters.
> > 
> > It's just taken me 15 minutes to locate the settings for
> > primary_conninfo to better understand Stefan's recent post.
> > 
> > The commit referred to here is an extremely bad change.
> > 
> > If you intended this to be a heading within the "High Availability"
> > chapter then I would agree. This is what I thought you had done.
> 
> The idea was to have one chapter that describes all the options in
> recovery.conf. Some of them are specific to streaming replication
> (primary_conninfo), some are specific to PITR (recovery_target_*), some
> are common (restore_command, restore_end_command). We need a reference
> page to list them all.

We need a section, not a new chapter, if at all. A chapter on its own
for this makes no sense at all. The new section should either be in the
Backup, HA or Server Config chapters.

Please revert the change to create a new chapter.

-- Simon Riggs           www.2ndQuadrant.com

On Wed, Feb 24, 2010 at 2:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Wed, 2010-02-24 at 08:30 +0200, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>> > On Mon, 2010-02-22 at 11:47 +0000, Heikki Linnakangas wrote:
>> >> Log Message:
>> >> -----------
>> >> Move documentation of all recovery.conf option to a new chapter.
>> >> They used to be scattered between the "backup and restore" and "streaming
>> >> replication" chapters.
>> >
>> > It's just taken me 15 minutes to locate the settings for
>> > primary_conninfo to better understand Stefan's recent post.
>> >
>> > The commit referred to here is an extremely bad change.
>> >
>> > If you intended this to be a heading within the "High Availability"
>> > chapter then I would agree. This is what I thought you had done.
>>
>> The idea was to have one chapter that describes all the options in
>> recovery.conf. Some of them are specific to streaming replication
>> (primary_conninfo), some are specific to PITR (recovery_target_*), some
>> are common (restore_command, restore_end_command). We need a reference
>> page to list them all.
>
> We need a section, not a new chapter, if at all. A chapter on its own
> for this makes no sense at all. The new section should either be in the
> Backup, HA or Server Config chapters.
>
> Please revert the change to create a new chapter.

Just to mention it, this change was discussed and apparently agreed to
5 days ago on hackers.  That's not to say that it necessarily
shouldn't be reverted, but I think that will take some discussion (and
more than 1 vote).

Looking at it, I think I do agree that this shouldn't be a whole
chapter by itself.  It's not nearly as high-level a concept as the
chapters that surround it.  But I do think it's good to list all the
recovery.conf settings in one central place.  The server config
chapter looks like the best fit to me on first glance, but I wonder
what others think?

...Robert

On Wed, 2010-02-24 at 03:11 -0500, Robert Haas wrote:

> But I do think it's good to list all the
> recovery.conf settings in one central place.

That part is fine, as long as we have links to it from necessary places.

The reason why these docs must be within a related chapter is that it
then shows up on the chapter TOC, allowing you to quickly answer the
"where were the docs for that?" question. If you go back up to the main
TOC there are way too many other chapters to be able to see which one
you should go to, even if you are used to the manual.

-- Simon Riggs           www.2ndQuadrant.com

On Wed, 2010-02-24 at 07:31 +0000, Simon Riggs wrote:

> Please revert the change to create a new chapter.

I will downgrade my request from a "revert" to the next rung down. Such
requests should be saved for the most serious matters.

Major change is necessary here; this change in particular is regrettably
a very bad one. Please reconsider and make substantial improvements.

-- Simon Riggs           www.2ndQuadrant.com

On Wed, Feb 24, 2010 at 5:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> Looking at it, I think I do agree that this shouldn't be a whole
> chapter by itself.  It's not nearly as high-level a concept as the
> chapters that surround it.  But I do think it's good to list all the
> recovery.conf settings in one central place.  The server config
> chapter looks like the best fit to me on first glance, but I wonder
> what others think?

Umm... how about the following layout (just merge the server config and
the recovery config)? Since the way to specify a parameter is different
from postgresql.conf and recovery.conf, the section "Server Parameters"
would be required in both.

18. Server Configuration
18.1. Server Parameters   18.1.1. Setting Parameters   18.1.2. File Locations   18.1.3. Connections and Authentication
<snip>   18.1.16. Short Options 
18.2. Recovery Parameters   18.2.1. Setting Parameters   18.2.2. Archive Recovery   18.2.3. Recovery Target   18.2.4.
StandbyServer 

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Simon Riggs wrote:
> Major change is necessary here; this change in particular is regrettably
> a very bad one. Please reconsider and make substantial improvements.

Any suggestions?

Berkus suggested adding a new page to collect together reference
documentation of all the configuration files:

http://archives.postgresql.org/message-id/4B759C73.9050300@agliodbs.com

I actually tried doing that under the Reference book to see how it
looks, but I couldn't figure out the SGML required to do it. Could
someone else with better SGML skills take a look at it, please?

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

On Wed, 2010-02-24 at 19:30 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Major change is necessary here; this change in particular is regrettably
> > a very bad one. Please reconsider and make substantial improvements.
> 
> Any suggestions?

A new section inside the HA chapter would be useful, so this is on the
same level as "Streaming Replication". So just drop everything down one
level.

There are 3 use cases: PITR, file-based replication and streaming
replication. Having 3 separate sections would be less confusing since it
would allow focused examples. A final section could be the "master
reference" for defining those parameters. So having both separate and
combined sections may end up being less confusing.

-- Simon Riggs           www.2ndQuadrant.com

On Wed, 2010-02-24 at 18:59 +0900, Fujii Masao wrote:
> On Wed, Feb 24, 2010 at 5:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > Looking at it, I think I do agree that this shouldn't be a whole
> > chapter by itself.  It's not nearly as high-level a concept as the
> > chapters that surround it.  But I do think it's good to list all the
> > recovery.conf settings in one central place.  The server config
> > chapter looks like the best fit to me on first glance, but I wonder
> > what others think?
> 
> Umm... how about the following layout (just merge the server config and
> the recovery config)? Since the way to specify a parameter is different
> from postgresql.conf and recovery.conf, the section "Server Parameters"
> would be required in both.
> 
> 18. Server Configuration
> 18.1. Server Parameters
>     18.1.1. Setting Parameters
>     18.1.2. File Locations
>     18.1.3. Connections and Authentication
>     <snip>
>     18.1.16. Short Options
> 18.2. Recovery Parameters
>     18.2.1. Setting Parameters
>     18.2.2. Archive Recovery
>     18.2.3. Recovery Target
>     18.2.4. Standby Server

That would be OK too, but you'd need to add lots and lots of links to
ensure that each mention of the parameter goes to the definition.

-- Simon Riggs           www.2ndQuadrant.com

Simon Riggs wrote:
> A new section inside the HA chapter would be useful, so this is on the
> same level as "Streaming Replication". So just drop everything down one
> level.

The problem with that is that the recovery.conf options are also
relevant to plain-old recovery from an archive. And PITR. So it doesn't
really belong under HA either.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

On Wed, Feb 24, 2010 at 1:20 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Simon Riggs wrote:
>> A new section inside the HA chapter would be useful, so this is on the
>> same level as "Streaming Replication". So just drop everything down one
>> level.
>
> The problem with that is that the recovery.conf options are also
> relevant to plain-old recovery from an archive. And PITR. So it doesn't
> really belong under HA either.

Yeah, that was why I thought server configuration was better.

...Robert

On Wed, 2010-02-24 at 20:20 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > A new section inside the HA chapter would be useful, so this is on the
> > same level as "Streaming Replication". So just drop everything down one
> > level.
> 
> The problem with that is that the recovery.conf options are also
> relevant to plain-old recovery from an archive. And PITR. So it doesn't
> really belong under HA either.

Which is why, in the next paragraph, I proposed a way forwards...

-- Simon Riggs           www.2ndQuadrant.com

Simon Riggs wrote:
> Which is why, in the next paragraph, I proposed a way forwards...

Oh, sorry.

> There are 3 use cases: PITR, file-based replication and streaming
> replication. Having 3 separate sections would be less confusing since it
> would allow focused examples. A final section could be the "master
> reference" for defining those parameters. So having both separate and
> combined sections may end up being less confusing.

Isn't that pretty much what we have now, except that both file-based
replication and streaming replication are under the HA section? Can you
lay out a tree of the section titles you're suggesting?

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com