Re: backup.sgml-cmd-v003.patch

On Sep 3, 2013, at 6:56 AM, Karl O. Pinc <kop(at)meme(dot)com> wrote:

> On 07/31/2013 12:08:12 PM, Ivan Lezhnjov IV wrote:
>
>> Patch filename: backup.sgml-cmd-v003.patch
>>
>> The third version of this patch takes into consideration feedback
>> received after original submission (it can be read starting from this
>> message http://www.postgresql.org/message-id/CA
>> +Tgmoaq-9D_mst113TdW=Ar8mpgBc+x6T61AzK3eMhww9gRcQ(at)mail(dot)gmail(dot)com)
>>
>> Essentially, it addresses the points that were raised in community
>> feedback and offers better worded statements that avoid implying that
>> some features are being deprecated when it isn't the case. We also
>> spent some more time polishing other details, like making adjustments
>> to the tone of the text so that it sounds more like a manual, and
>> less
>> like a blog post. More importantly, this chapter now makes it clear
>> that superuser privileges are not always required to perform a
>> successful backup because in practice as long as the role used to
>> make
>> a backup has sufficient read privileges on all of the objects a user
>> is interested in it's going to work just fine. We also mention and
>> show examples of usage for pg_restore and pigz alongside with gzip,
>> and probably something else too.
>
> Hi Ivan,
>
> I'm reviewing your patch. I did not read the entirety of the
> thread referenced above. I apologize if this causes problems.
>
> Attached is backup-sgml-cmnd-v003_1.patch to be applied on top of
> backup-sgml-cmnd-v003.patch and containing my edits. You will
> eventually want to produce a single patch (but see below).
> Meanwhile this might help you keep track of my changes.
>
> Attached also is your original v3 patch.
>
> ---
>
> Cleaned up and clarified here and there.
>
> The bit about OIDs being depreciated might properly belong in
> a separate patch. The same might be said about adding mention of pigz.
> If you submit these as separate patch file attachments
> they can always be applied in a single commit, but the reverse is
> more work for the committer. (Regardless, I see no reason to
> have separate commitfest entries or anything other than multiple
> attachments to the email that finalizes our discussion.)

Hello,

took me a while to get here, but a lot has been going on...

Okay, I'm new and I don't know why a single patch like this is more work for a commiter? Just so I understand and know.

>
> Minimally modified to note the existence of directory dumps. It may
> be that the utility/flexibility of directory dumps should also be
> mentioned.
>
> My thought is that the part beginning with "The options in detail
> are:" should not describe all the possibilities for the --format
> option, that being better left to the reference section. Likewise,
> this being prose, it might be best to describe all the options
> in-line, instead of presented as a list. I have left it as-is
> for you to improve as seen fit.

Agreed, it probably looks better as a sentence.

>
> I have frobbed your <programlisting> to adjust the indentation and
> line-wrap style. I submit it here for consideration in case this
> style is attractive. This is nothing but conceit. We should use the
> same style used elsewhere in the documentation. (I can't think
> offhand of a place to look for a line-wrapped shell example. If you
> can't find one I'll take a look and if neither of us finds one we'll
> then have choices.)

Looks good to me.

>
> I don't know that it's necessary to include pigz examples, because it
> sounds like pigz is a drop-in gzip replacement. I've left your
> examples in, in case you feel they are necessary.

We do. We believe it can encourage more people to consider using it. The way we see it, most people seem to be running mutlicore systems these days, yet many simply are not aware of pigz. We have been routinely informing our customers of pigz as an alternative to tried and tested gzip when helping optimize their configurations, and all of them without exception went for it. I guess everybody just likes to squeeze some extra juice for free.

>
> The existing text of the SQL Dump section could use some alteration to
> reduce redundancy and add clarity. I'm thinking specifically of
> mention of pg_restore as being required to restore custom format
> backups and of the default pg_dump output being not just "plain text"
> but being a collection of SQL commands. Yes, the latter is obvious
> upon reflection, psql being what it is, but I think it would be
> helpful to spell this out. Especially in the context of the current
> patch. There could well be other areas like this to be addressed.

I don't quite follow you here. I mean, I kinda understand what you mean in general, but when I look at the text I fail to see what you had in mind specifically.

For example, pg_restore is mentioned only 3 times in section 24.1. Each mention seems pretty essential to me. And the text flow is pretty natural.

Also, about plain text format being a collection of SQL commands. The very first paragraph of the section 24.1 reads "The idea behind this dump method is to generate a text file with SQL commands that, when fed back to the server, will recreate the database in the same state as it was at the time of the dump. PostgreSQL provides the utility program pg_dump for this purpose."

>
> Overall I have pretty good feelings about this patch. Backup and
> restore is complex and although this patch leaves a lot of issues
> unmentioned it seems a step in the right direction.
>

Thanks for a detailed response. I attached a patch file that builds on your corrections and introduces some of the edits discussed above.

Ivan