Re: backup.sgml-cmd-v003.patch - Mailing list pgsql-hackers

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@mail.gmail.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.)

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.

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.)

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.

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.

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.

Regards,

Karl <kop@meme.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein