Re: Include WAL in base backup

Greetings,

* Magnus Hagander (magnus(at)hagander(dot)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 close it 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 list of and then let each
option be whatever the OR'd set of options is. Wouldn't the current
grammar require that you put the 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