Re: Include WAL in base backup

Greetings,

* Magnus Hagander (magnus@hagander.net) wrote:
> For now, you need to set wal_keep_segments to make it work properly,
> but if you do the idea is that the tar file/stream generated in the
> base backup will include all the required WAL files.

Is there some reason to not ERROR outright if we're asked to provide WAL
and wal_keep_segments isn't set..?  I'd rather do that than only ERROR
when a particular WAL is missing..  That could lead to transient backup
errors that an inexperienced sysadmin or DBA might miss or ignore.
They'll notice if it doesn't work the first time they try it and spits
out a hint about wal_keep_segments.

> That means that
> you can start a postmaster directly against the directory extracted
> from the tarball, and you no longer need to set up archive logging to
> get a backup.

Like that part.

> I've got some refactoring I want to do around the
> SendBackupDirectory() function after this, but a review of the
> functionality first would be good. And obviously, documentation is
> still necessary.

mkay, I'm not going to try to make this ready for committer, but will
provide my comments on it overall.  Bit difficult to review when someone
else is reviewing the base patch too. :/

Here goes:

- I'm not a huge fan of the whole 'closetar' option, that feels really rather wrong to me.  Why not just open it and
closeit in perform_base_backup(), unconditionally?
 

- I wonder if you're not getting to a level where you shold be using a struct to pass the relevant information to
perform_base_backup()instead of adding more arguments on..  That's going to get unwieldy at some point.
 

- Why not initialize logid and logseg like so?:
int logid = startptr.xlogid;int logseg = startptr.xrecoff / XLogSegSize;
 Then use those in your elog?  Seems cleaner to me.

- A #define right in the middle of a while loop...?  Really?

- The grammar changes strike me as..  odd.  Typically, you would have an 'option' production that you can then have a
listof and then let each option be whatever the OR'd set of options is.  Wouldn't the current grammar require that you
putthe options in a specific order?  That'd blow.
 

@@ -687,7 +690,7 @@ BaseBackup()         * once since it can be relocated, and it will be checked before we do
*anything anyway.         */
 
-        if (basedir != NULL && i > 0)
+        if (basedir != NULL && !PQgetisnull(res, i, 1))            verify_dir_is_empty_or_create(PQgetvalue(res, i,
1));   }
 

- Should the 'i > 0' conditional above still be there..?

So, that's my review from just reading the source code and the thread..
Hope it's useful, sorry it's not more. :/
Thanks,
    Stephen