Re: pg_rewind in contrib

On 01/07/2015 06:19 PM, Alvaro Herrera wrote:

Fixed most of the issues you listed. More on a few remaining ones below.

> Please don't name files with generic names such as util.c/h.  It's
> annoying later; for instance when I want to open analyze.c I have to
> choose the one in parser or commands.  (pg_upgrade in particular is
> already a mess; let's not copy that.)

There was only one function in util.c, with only one caller, so I just
moved it to the calling file and removed util.c and util.h altogether.

There are few other files with fairly generic names: fetch.c, timeline.c
and (a new file I just added) logging.c. I agree it's annoying when
there are multiple files with same name, although I don't think it's
reasonable to totally avoid it either. We could call e.g. logging.c
pg_rewind_logging.c instead, but I'm not convinced that that is better.

> Is the final destiny of this still contrib?  There are several votes for
> putting it in src/bin/ right from the start, which sounds reasonable to
> me as well.

Seems that the majority opinion is to put this straight into src/bin.
Works for me, moved.

> If we do that, we will need more attention to translatability markers of
> user-visible error messages; if you want to continue using fprintf()
> then you will need _() around the literals, but it might be wise to use
> another function instead so that you can avoid them (say, have something
> like pg_upgrade's pg_fatal() which you can then list in nls.mk).
> ...
> Uh, I notice that you have _() in some places such as slurpFile().

I copy-pasted pg_fatal and pg_log from pg_upgrade, replaced all the
fprintf(stderr) calls with them, and created an nls.mk file. It's now
ready for translation.

I also removed the --verbose option and replaced it with --progress,
which shows a progress indicator similar to pg_basebackup's, and
--debug, which spews out the list of actions on each file and other
debugging output.

> +void
> +close_target_file(void)
> +{
> +    if (close(dstfd) != 0)
> +    {
> +        fprintf(stderr, "error closing destination file \"%s\": %s\n",
> +                dstpath, strerror(errno));
> +        exit(1);
> +    }
> +
> +    dstfd = -1;
> +    /* fsync? */
> +}
>
> Did you find an answer for the above question?

No. The question is, should pg_rewind fsync() every file that it
modifies? It would be a reasonable thing to do, to make sure that if you
crash immediately after running pg_rewind, the cluster is in a
consistent state. However, pg_basebackup for example doesn't do that.
initdb does, but that was added fairly recently.

I'm not thrilled about sprinkling fsync() calls everywhere that we touch
files. But I guess that would be the right thing to do. I'm planning to
do that as an add-on patch later, fixing also pg_basebackup and any
other utilities that need it.

> +/*
> + * Does it look like a relation data file?
> + */
> +static bool
> +isRelDataFile(const char *path)
>
> This doesn't consider forks ... why not?  If correct, probably it deserves a comment.

Added a comment. (Non-main forks are always copied in toto, so they are
not considered as relation data files for pg_rewind's purposes.)

> +struct filemap_t
> +{
> +    /*
> +     * New entries are accumulated to a linked list, in process_remote_file
> +     * and process_local_file.
> +     */
> +    file_entry_t *first;
> +    file_entry_t *last;
> +    int            nlist;
>
> Uh, this is like the seventh open-coded list implementation in frontend
> code.  Can't we have this using ilist for a change?

ilist is backend code. I'm not eager to move it to src/common. A linked
list is a trivial data structure, I don't think it's too bad to
re-invent that wheel.

In this case, it might make sense to get rid of the linked list
altogether. pg_rewind currently accumulates new entries in a list, and
turns that into a sorted array after all remote files have been
accumulated. But it could be rewritten to use an array to begin with.

> This FRONTEND game is pretty nasty -- why do you need it?  Can we fix
> the headers to avoid needing it?
>
> +/*-------------------------------------------------------------------------
> + *
> + * parsexlog.c
> + *      Functions for reading Write-Ahead-Log
> + *
> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1996-2008, Nippon Telegraph and Telephone Corporation
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + *-------------------------------------------------------------------------
> + */
> +
> +#define FRONTEND 1
> +#include "c.h"
> +#undef FRONTEND
> +#include "postgres.h"
> +
> +#include "pg_rewind.h"
> +#include "filemap.h"
> +
> +#include <unistd.h>
> +
> +#include "access/rmgr.h"
> +#include "access/xlog_internal.h"
> +#include "access/xlogreader.h"
> +#include "catalog/pg_control.h"
> +#include "catalog/storage_xlog.h"
> +#include "commands/dbcommands.h"
>
> (I think the answer is in dbcommands.h; we could split it to a new
> dbcommands_xlog.h)

Ah, nice, that was the only thing left that needed the FRONTEND hack. Done.

Here is a new version. There are now a fair amount of code changes
compared to the version on github, like the logging and progress
information, and a lot of comment changes. I also improved the
documentation.

This patch is also available at my git repository at
git://git.postgresql.org/git/users/heikki/postgres.git, branch
"pg_rewind". The commit history of that isn't very clean, so don't pay
too much attention to that.

- Heikki