Re: pg_rewind in contrib

What is this define good for? I couldn't understand where it fits; is
it just a leftover?

+#define pageinfo_set_truncation(forkno, rnode, blkno) datapagemap_set_truncation(pagemap, forkno, rnode, blkno)

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

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. We didn't put pg_basebackup in contrib initially for instance.
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 agree with Andres that it would be better to avoid a second copy of
the xlogreader helper stuff. A #FRONTEND define in xlogreader.c seems
acceptable, and pg_rewind can have a wrapper function to add extra
functionality later, if necessary -- or we can add a flags argument,
currently unused, which could later be extended to indicate to read from
archive, or something.

Copyright lines need updated to 2015 (meh)

Our policy on header inclusion says that postgres_fe.h comes first, then
system includes, then other postgres headers. So at least this one is
wrong:

+++ b/contrib/pg_rewind/copy_fetch.c
@@ -0,0 +1,586 @@
[...]
+#include "postgres_fe.h"
+
+#include "catalog/catalog.h"
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "pg_rewind.h"
+#include "fetch.h"
+#include "filemap.h"
+#include "datapagemap.h"
+#include "util.h"

The reason for this IIRC is that system includes could modify behavior
of some calls in obscure platforms under obscure circumstances.

+ while ((xlde = readdir(xldir)) != NULL)
+ {

You need to reset errno here, see commit 6f03927fce038096f53ca67eeab9a.

+static int dstfd = -1;
+static char dstpath[MAXPGPATH] = "";
+
+void
+open_target_file(const char *path, bool trunc)
+{

I think these file-level static vars ought to be declared at top of
file, probably with more distinctive names.

+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?

+static bool
+endswith(const char *haystack, const char *needle)
+{
+ int needlelen = strlen(needle);
+ int haystacklen = strlen(haystack);
+
+ if (haystacklen < needlelen)
+ return false;
+
+ return strcmp(&haystack[haystacklen - needlelen], needle) == 0;
+}

We already have this function elsewhere.

+ if (type != FILE_TYPE_REGULAR && isRelDataFile(path))
+ {
+ fprintf(stderr, "data file in source \"%s\" is a directory.\n", path);
+ exit(1);
+ }

Here you have error messages ending with periods; but not in most other
places.

+/*
+ * 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.

+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?

Existing practice is that SQL keywords are uppercase:

+ sql =
+ "-- Create a recursive directory listing of the whole data directory\n"
+ "with recursive files (path, filename, size, isdir) as (\n"
+ " select '' as path, filename, size, isdir from\n"
+ " (select pg_ls_dir('.') as filename) as fn,\n"
+ " pg_stat_file(fn.filename) as this\n"
+ " union all\n"
+ " select parent.path || parent.filename || '/' as path,\n"
+ " fn, this.size, this.isdir\n"
+ " from files as parent,\n"
+ " pg_ls_dir(parent.path || parent.filename) as fn,\n"
+ " pg_stat_file(parent.path || parent.filename || '/' || fn) as this\n"
+ " where parent.isdir = 't'\n"
+ ")\n"
+ "-- Using the cte, fetch a listing of the all the files.\n"
+ "--\n"
+ "-- For tablespaces, use pg_tablespace_location() function to fetch\n"
+ "-- the link target (there is no backend function to get a symbolic\n"
+ "-- link's target in general, so if the admin has put any custom\n"
+ "-- symbolic links in the data directory, they won't be copied\n"
+ "-- correctly)\n"
+ "select path || filename, size, isdir,\n"
+ " pg_tablespace_location(pg_tablespace.oid) as link_target\n"
+ "from files\n"
+ "left outer join pg_tablespace on files.path = 'pg_tblspc/'\n"
+ " and oid::text = files.filename\n";

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)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services